feat: Update the minimum password length. #33373

feanil · 2023-09-28T19:17:45Z

This changes:

The default minimum length of passwords from 2 characters to 8 characters.
Updates the CMS password settings so that they pull the defaults from the LMS rather than have their own copy.

Operational Impact: None

If you have an existing password, this change along will not force you to update it. However if you reset your password or go to change it, you'll have to conform to the new guidelines.

If you would like to force people to update their password, you'll probably want to take a look at the password_policy plugin and its settings.

Make them resilient to the default changing where it makes sense.

pshiu

Thanks, @feanil!

cms/envs/common.py

lms/djangoapps/courseware/testutils.py

lms/envs/common.py

alangsto · 2023-10-12T20:02:05Z

cms/envs/common.py

@@ -121,6 +121,9 @@
    # Methods to derive settings
    _make_mako_template_dirs,
    _make_locale_paths,
+
+    # Password Validator Settings
+    AUTH_PASSWORD_VALIDATORS


Just curious about leaving this setting in if it has no assigned value - should we just remove it?

This pulls this setting from the LMS so the LMS and CMS have the same setting instead of having a copy over here.

feanil · 2023-10-11T15:55:48Z

cms/envs/common.py

@@ -121,6 +121,9 @@
    # Methods to derive settings
    _make_mako_template_dirs,
    _make_locale_paths,
+
+    # Password Validator Settings
+    AUTH_PASSWORD_VALIDATORS


This is a critical functional change. This will cause the CMS and LMS to have the same password policies by default.

Thanks @feanil! Should we include this in the PR description? It seems like a key piece of information that could be helpful to have at a glance.

Updated the description.

feanil · 2023-10-13T13:21:22Z

cms/envs/common.py

@@ -121,6 +121,9 @@
    # Methods to derive settings
    _make_mako_template_dirs,
    _make_locale_paths,
+
+    # Password Validator Settings
+    AUTH_PASSWORD_VALIDATORS


This pulls this setting from the LMS so the LMS and CMS have the same setting instead of having a copy over here.

lms/djangoapps/courseware/testutils.py

magajh · 2023-10-13T13:20:57Z

lms/djangoapps/mobile_api/testutils.py

@@ -50,7 +50,7 @@ def setUp(self):
            certificate_available_date=datetime.datetime.now(pytz.UTC)
        )
        self.user = UserFactory.create()
-        self.password = 'test'
+        self.password = 'Password1234'


Since this class inherits from ModuleStoreTestCase, could we use TEST_PASSWORD?

magajh · 2023-10-13T13:26:53Z

common/djangoapps/student/tests/factories.py

@@ -81,7 +81,7 @@ class Meta:
        model = User
        django_get_or_create = ('email', 'username')

-    _DEFAULT_PASSWORD = 'test'
+    _DEFAULT_PASSWORD = 'Password1234'


Is it possible to reuse the TEST_PASSWORD constant here? It might help in maintaining consistency

There are a few places, where I think making new accesses to the TEST_PASSWORD variable would result in weird coupling so I left them as is. I agree that we could make further improvements here but I think that what I have done so far is an improvement and good enough for now.

magajh · 2023-10-13T14:00:12Z

lms/djangoapps/survey/tests/test_utils.py

@@ -28,7 +28,7 @@ def setUp(self):
        self.client = Client()

        # Create two accounts
-        self.password = 'abc'
+        self.password = 'Password1234'


I think we can use the TEST_PASSWORD attribute here

magajh · 2023-10-13T14:02:07Z

lms/djangoapps/survey/tests/test_views.py

@@ -29,7 +29,7 @@ def setUp(self):
        self.client = Client()

        # Create two accounts
-        self.password = 'abc'
+        self.password = 'Password1234'


Since this class inherits from ModuleStoreTestCase, can we use the TEST_PASSWORD attribute?

magajh · 2023-10-13T14:10:17Z

openedx/tests/xblock_integration/test_crowdsource_hinter.py

@@ -21,8 +21,8 @@ class TestCrowdsourceHinter(SharedModuleStoreTestCase, LoginEnrollmentTestCase):
    Create the test environment with the crowdsourcehinter xblock.
    """
    STUDENTS = [
-        {'email': 'view@test.com', 'password': 'foo'},
-        {'email': 'view2@test.com', 'password': 'foo'}
+        {'email': 'view@test.com', 'password': 'Password1234'},


Maybe we could reuse the TEST_PASSWORD attribute here for consistency?

magajh · 2023-10-13T14:21:05Z

lms/djangoapps/course_blocks/transformers/tests/helpers.py

@@ -253,7 +253,7 @@ def setUp(self):
                    parent_block.children.append(self.xblock_keys[i])
                    update_block(parent_block)

-        self.password = 'test'
+        self.password = 'Password1234'


same here, I believe we can use TEST_PASSWORD

magajh · 2023-10-13T14:21:41Z

lms/djangoapps/course_wiki/tests/tests.py

@@ -24,7 +24,7 @@ def setUp(self):
        # Create two accounts
        self.student = 'view@test.com'
        self.instructor = 'view2@test.com'
-        self.password = 'foo'
+        self.password = 'Password1234'


Since this class inherits from ModuleStoreTestCase, could we use the TEST_PASSWORD attribute?

magajh · 2023-10-13T14:23:07Z

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -154,7 +154,7 @@ def setUp(self):
        # create a test student
        self.course = CourseFactory.create(display_name=self.COURSE_NAME, number=self.COURSE_SLUG)
        self.student = 'view@test.com'
-        self.password = 'foo'
+        self.password = 'Password1234'


Since this class inherits from ModuleStoreTestCase, can we use TEST_PASSWORD?

magajh · 2023-10-13T14:26:29Z

lms/djangoapps/grades/rest_api/v1/tests/test_views.py

@@ -548,7 +548,7 @@ class CourseSubmissionHistoryWithDataTest(TestSubmittingProblems):
    def setUp(self):
        super().setUp()
        self.namespaced_url = 'grades_api:v1:submission_history'
-        self.password = 'test'
+        self.password = 'Password1234'


This test case inherits from TestSubmittingProblems, so I believe we can use the TEST_PASSWORDattribute

magajh · 2023-10-13T15:01:20Z

lms/djangoapps/verify_student/tests/test_views.py

@@ -122,8 +122,8 @@ def test_start_new_verification(self):
        Test the case where the user has no pending `PhotoVerificationAttempts`,


I think it would be better to define 'Password1234' as a single variable and reuse it throughout this file. If not, maybe we can define the attribute in the setup classes for each necessary test case to reduce repetition.

magajh · 2023-10-13T15:18:09Z

openedx/tests/xblock_integration/xblock_testcase.py

+        {'email': 'alice@test.edx.org', 'password': 'Password1234'},
+        {'email': 'bob@test.edx.org', 'password': 'Password1234'},
+        {'email': 'eve@test.edx.org', 'password': 'Password1234'},


How about introducing a variable here to avoid repetition?

Given how localized all the values are I don't think it's super valuable in this case.

magajh · 2023-10-13T15:54:17Z

Hey @feanil, should we update the password in this mixins file for tests as well? https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/course_api/tests/mixins.py#L11. Do you think it's necessary, or is it fine to leave it as is? I'm wondering why the tests that are using the mixin aren't affected.

feanil · 2023-10-16T15:37:32Z

Hey @feanil, should we update the password in this mixins file for tests as well? https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/course_api/tests/mixins.py#L11. Do you think it's necessary, or is it fine to leave it as is? I'm wondering why the tests that are using the mixin aren't affected.

Good catch @magajh it looks like the reason this doesn't fail is because the API endpoints that are being tested don't require authentication so the fact that auth failed doesn't matter. I've updated the password anyway so that it's less confusing.

Some of the places where we had explicit copies of the password were not necessary so we referece the exsting TEST_PASSWORD variable where possible.

magajh

LGTM!

edx-pipeline-bot · 2023-10-17T15:13:31Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-10-17T15:34:56Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-10-17T15:34:56Z

2U Release Notice: This PR has been deployed to the edX production environment.

feanil force-pushed the feanil/update_password_length_default branch 7 times, most recently from 8d02bfe to 6ccca3e Compare September 29, 2023 17:55

feanil added 2 commits October 2, 2023 14:10

feat: Update the minimum password length.

6831451

test: Fix validation tests.

ae42b09

Make them resilient to the default changing where it makes sense.

feanil force-pushed the feanil/update_password_length_default branch 21 times, most recently from 35318f1 to 6d851c4 Compare October 5, 2023 18:18

feanil force-pushed the feanil/update_password_length_default branch 3 times, most recently from ba66223 to 3b5c32b Compare October 10, 2023 20:01

test: Update more tests that had short passwords.

1e2ea85

feanil force-pushed the feanil/update_password_length_default branch from 3b5c32b to 1e2ea85 Compare October 10, 2023 20:36

test: Update to an even longer password.

64e91d4

feanil force-pushed the feanil/update_password_length_default branch from 65263d4 to 64e91d4 Compare October 12, 2023 14:32

feanil marked this pull request as ready for review October 12, 2023 19:30

feanil requested review from pshiu, magajh and alangsto October 12, 2023 19:31

pshiu approved these changes Oct 12, 2023

View reviewed changes

cms/envs/common.py Show resolved Hide resolved

lms/djangoapps/courseware/testutils.py Show resolved Hide resolved

lms/envs/common.py Show resolved Hide resolved

alangsto approved these changes Oct 13, 2023

View reviewed changes

feanil commented Oct 13, 2023

View reviewed changes

magajh reviewed Oct 13, 2023

View reviewed changes

test: Update to reuse variables in more places.

e3851ab

Some of the places where we had explicit copies of the password were not necessary so we referece the exsting TEST_PASSWORD variable where possible.

feanil force-pushed the feanil/update_password_length_default branch from 9dba83b to e3851ab Compare October 16, 2023 16:33

magajh approved these changes Oct 16, 2023

View reviewed changes

feanil merged commit 7202c22 into master Oct 17, 2023
62 checks passed

feanil deleted the feanil/update_password_length_default branch October 17, 2023 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update the minimum password length. #33373

feat: Update the minimum password length. #33373

feanil commented Sep 28, 2023 •

edited

pshiu left a comment

alangsto Oct 12, 2023

feanil Oct 13, 2023

feanil Oct 11, 2023

magajh Oct 13, 2023

feanil Oct 16, 2023

feanil Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

feanil Oct 16, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023 •

edited

magajh Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

magajh Oct 13, 2023

feanil Oct 16, 2023

magajh commented Oct 13, 2023

feanil commented Oct 16, 2023

magajh left a comment •

edited

edx-pipeline-bot commented Oct 17, 2023

edx-pipeline-bot commented Oct 17, 2023

edx-pipeline-bot commented Oct 17, 2023

		@@ -122,8 +122,8 @@ def test_start_new_verification(self):
		Test the case where the user has no pending `PhotoVerificationAttempts`,

feat: Update the minimum password length. #33373

feat: Update the minimum password length. #33373

Conversation

feanil commented Sep 28, 2023 • edited

pshiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magajh Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magajh commented Oct 13, 2023

feanil commented Oct 16, 2023

magajh left a comment • edited

Choose a reason for hiding this comment

edx-pipeline-bot commented Oct 17, 2023

edx-pipeline-bot commented Oct 17, 2023

edx-pipeline-bot commented Oct 17, 2023

feanil commented Sep 28, 2023 •

edited

magajh Oct 13, 2023 •

edited

magajh left a comment •

edited