password expiration policy - DO NOT MERGE by piyushabad88 · Pull Request #36 · facebookarchive/CommunityCellularManager

piyushabad88 · 2017-06-01T15:05:53Z

Please review password expiration policy implementation which include the below features:

Password Expiry Status pop-up comes to change user password for last seven days from PASSWORD_EXPIRED_DAY i.e 90 days configured.
New password rule (min 8 character length,combination of alphanumeric and special character)
Block user If user does not change password for last 90 days.

9muir

Looks pretty good at a high level. Nice job breaking out into a much smaller PR.

9muir · 2017-06-01T17:10:22Z

+
+@app.task(bind=True)
+def block_user(self):
+    """ Block  User if User password is not updated


Please make the comment reflect the fact that the number of days is configured in a setting.

comments incorporated in commit id 1703300.

9muir · 2017-06-01T17:10:54Z

+    """ Block  User if User password is not updated
+    for last 90 days
+    """
+    six_month_ago = (django.utils.timezone.now() -


six_month_ago is not a good name for a variable that might not represent a date exactly six months ago.

comments incorporated in commit id 1703300

9muir · 2017-06-01T17:14:23Z

+        return JsonResponse({'permissions': list(role_permission)})
+    return HttpResponseBadRequest()
+
+def validate_password_strength(value):


Would be good to have some unit tests to validate that this rejects 'weak' passwords, especially since the regex is kinda incomprehensible. It might be clearer if the code was structured as a conjunction of three simpler regex checks: one for a number, one for a letter, and one for a special character.

added test cases and made simpler regex checks for digit,alphabet,length and special character.
Updated in commit 1703300.

9muir · 2017-06-01T17:15:10Z

+
+
+@login_required(login_url='/login/')
+def role_default_permissions(request):


I don't think this function is logically part of this PR

yes, removed function.

9muir · 2017-06-01T17:17:53Z

+                next_url = request.POST['next']
+            if (today - user_profile.last_pwd_update).days >= \
+                    settings.ENDAGA['PASSSWORD_EXPIRED_LAST_SEVEN_DAYS']:
+                text = str(user) + ' , your account will be blocked in next '\


text should include words to the effect of '... unless you change your password.'

As a style point, I'd pretty much always use string formatting operator (%) rather than concatenating a bunch of strings.

comments incorporated in commit id 1703300.

9muir · 2017-06-01T17:18:18Z

    # For example to get a list of networks the user can view:
    # >>> get_objects_for_user(user_profile.user, 'view_network', klass=Network)
-    network = models.ForeignKey('Network', null=True, on_delete=models.SET_NULL)
+    network = models.ForeignKey('Network', null=True,


Please revert this accidental change.

facebook-github-bot · 2017-06-02T19:47:32Z

@aricent-ccm updated the pull request - view changes

removed spaces.

facebook-github-bot · 2017-06-05T07:19:11Z

@aricent-ccm updated the pull request - view changes

…tyCellularManager into sprint2_password_expiration_feature

…com/piyush-kr/CommunityCellularManager into sprint2_password_expiration_feature

kkroo

Looks great! Awesome test cases. Some small changes

kkroo · 2017-06-05T15:34:42Z

@@ -153,6 +153,19 @@ def __init__(self, *args, **kwargs):

 class ChangePasswordForm(PasswordChangeForm):


Built in validators here: https://docs.djangoproject.com/en/dev/topics/auth/passwords/#password-validation
Do you really need to override all of these fields to show a message? The validators should print their own policy text in a ValidationError on form submission.

Thanks,yes ,As django have four default validators and they having their own policy text.
But as per our requirement we don't need UserAttributeSimilarityValidator,CommonPasswordValidator. so only NumericPasswordValidator and MinimumLengthValidator we could use.But NumericPasswordValidator only check numeric ,it doesn't check alphanumeric and they don't have validator check for special character.For that regexValidator need to use.
For all this , made custom validator custom_password_validators, which validated min_length,alphanumeric and special character. And our custom policy provided as help text.
Removed override of all those fields , added custom validators to display our policy and validate password strength.

commitId:1f9e597e4ffae62e4e54c4447c02bc4e7f3a1232

kkroo · 2017-06-05T15:38:05Z

 </style>
+<html lang="en">
+<head>
+  <script src="https://code.jquery.com/jquery-1.12.4.js"></script>


We should use the same version of jquery across the project: 1.10.2

comments incorporated in commit id 1f9e597

kkroo · 2017-06-05T15:42:46Z

-        if 'next' in request.POST and request.POST['next']:
-            next_url = request.POST['next']
-        return redirect(next_url)
+        if user.is_active:


You need to check user is not None first in the case authenticate returns None

comments incorporated in commit id 1f9e597

kkroo · 2017-06-05T15:44:15Z

            return redirect("/dashboard/profile")
    return HttpResponseBadRequest()

+def validate_password_strength(password):


This is already built into Django. See my previous comment

removed and now validating fields in form itself.

kkroo · 2017-06-05T15:46:38Z

+        tags = 'password alert alert-danger'
+        messages.error(request, text, extra_tags=tags)
+        return redirect(redirect_url)
+    if request.POST['old_password'] == request.POST['new_password1']:


You should be doing this validation in the PasswordForm using the clean_new_password1 method. See this example: https://docs.djangoproject.com/en/1.8/_modules/django/contrib/auth/forms/#SetPasswordForm

kkroo · 2017-06-05T15:47:06Z

        tags = 'password alert alert-danger'
        messages.error(request, text, extra_tags=tags)
        return redirect(redirect_url)
+    if not validate_password_strength(request.POST['new_password1']):


This is builtin. See my comments

added custom validators which validate in form itself

facebook-github-bot · 2017-06-06T15:03:04Z

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

9muir

Not sure what's going on with the 'for' loops in the HTML templates

9muir · 2017-06-06T15:12:22Z

+        </h4>
+      </div>
+      <div class='modal-body'>
+        {% for message in messages %}


Is this really a loop? I assume we're only going to prompt a single user, the one that is logged in, to change their password.

thanks,yes, there is one message need to prompt.But as per django message framework, message it store in FallbackStorage is the default storage.As per its definition This class first uses CookieStorage for all messages, falling back to using SessionStorage for the messages that could not fit in a single cookie. Since it is uses SessionStorage, it also requires Django's contrib.sessions application.
so it seems need to iterate in for loop for displaying message in template.If we not iterate and directly shows messages, it show like
Hi <django.contrib.messages.storage.fallback. at 0x7f1b7cb75590> Please change your password.

9muir · 2017-06-06T15:13:53Z

+      <div class='modal-body'>
+        {% for message in messages %}
+        <p>
+          {% if message.text %} {% endif %} Hi {{ message }}


I don't understand what this {% if ... %} {% endif %} block is doing, it looks like nothing?

this is not required ,removed

9muir · 2017-06-06T15:14:31Z

+        {% for message in messages %}
+        <p>
+          {% if message.text %} {% endif %} Hi {{ message }}
+          {% endfor %}


This is a really weird place to put the {% endfor %}, but if you remove the 'for' construct it goes away anyway.

require to close for loop added close
,please see above comments regarding messages.storage.fallback.

9muir · 2017-06-06T15:16:39Z

+    <h4>Change Password</h4>
+    {% crispy change_pass_form %}
+
+    {% for message in messages %}


Another 'for' loop that I don't understand. If this really is a loop there should be a comment.

if we not give ,it's not show message.please see above comments regarding messages.storage.fallback.

facebook-github-bot · 2017-06-07T17:16:44Z

@aricent-ccm updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-06-07T17:56:19Z

@aricent-ccm updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-06-07T18:06:14Z

@aricent-ccm updated the pull request - view changes - changes since last import

password expiration policy

00837c8

facebook-github-bot added GH Review: review-needed CLA Signed labels Jun 1, 2017

piyushabad88 mentioned this pull request Jun 1, 2017

password expiration policy - DO NOT MERGE #34

Closed

9muir suggested changes Jun 1, 2017

View reviewed changes

review comments incorporated

1703300

Update test_user_password.py

1df4510

removed spaces.

piyushabad88 added 2 commits June 5, 2017 15:18

Merge branch 'master' of https://github.com/facebookincubator/Communi…

d807abc

…tyCellularManager into sprint2_password_expiration_feature

Merge branch 'sprint2_password_expiration_feature' of https://github.…

bf966a7

…com/piyush-kr/CommunityCellularManager into sprint2_password_expiration_feature

kkroo suggested changes Jun 5, 2017

View reviewed changes

facebook-github-bot added the Import Started label Jun 6, 2017

9muir suggested changes Jun 6, 2017

View reviewed changes

added custom password_validators and review comments incorporated

1f9e597

refactored code

094d531

missed to add file for custom_validators

a8319a1



		@login_required(login_url='/login/')
		def role_default_permissions(request):

		@@ -153,6 +153,19 @@ def __init__(self, args, *kwargs):

		class ChangePasswordForm(PasswordChangeForm):

Uh oh!

Conversation

piyushabad88 commented Jun 1, 2017

Uh oh!

9muir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 2, 2017

Uh oh!

facebook-github-bot commented Jun 5, 2017

Uh oh!

kkroo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 6, 2017

Uh oh!

9muir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment