SE-1496 conditionally re-enable user change password form in admin #21616

samuelallan72 · 2019-09-11T01:14:45Z

This PR enables setting/changing a user's password from the django admin, gated
behind a django setting FEATURES flag.

Using a django settings option because this affects entries to urls.py, which require a server restart to take effect. Using a waffle switch is misleading, because toggling it without restarting the server afterwards would leave the platform in an invalid state.

This behaviour was disabled across https://github.com/edx/edx-platform/pull/18970 and https://github.com/edx/edx-platform/pull/18972.

We want to re-enable it because our clients use this feature.

JIRA tickets: OSPR-3840

Dependencies: None

Sandbox URL:

Merge deadline: None

Testing instructions:

login to the admin on the lms as a superuser
navigate the admin change page for a user (eg /admin/auth/user/1/change/)
verify that there is no link to a password change form in the description
for the password field. (You should not see " but you can change the
password using this form.")
change the current url from .../change/ to .../password/ (eg.
/admin/auth/user/1/password/)
verify that a 404 page is displayed
repeat steps 2 - 5 on the cms
add/change the FEATURES: ENABLE_CHANGE_USER_PASSWORD_ADMIN flag to true in the lms config.
restart the cms and lms apps (reboot the server or run
/edx/bin/supervisorctl restart lms: cms: from a shell).
log in to the admin on the lms as a superuser
navigate the admin change page for a user (eg /admin/auth/user/1/change/)
verify that a link to a password change form ("but you can change the
password using this form") is visible.
click the form link and verify that the page loads correctly and the user
password can be changed
repeat the previous 4 steps from the cms admin.

Author notes and concerns:

Reviewers

@clemente
edX reviewer[s] TBD

Settings

EDXAPP_FEATURES:
  ENABLE_CHANGE_USER_PASSWORD_ADMIN: false

openedx-webhooks · 2019-09-11T01:14:54Z

Thanks for the pull request, @swalladge! I've created OSPR-3840 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

natabene · 2019-09-11T21:57:09Z

@swalladge Thank you for your contribution. Please let me know once all tests are green.

samuelallan72 · 2019-09-12T02:10:59Z

@natabene We're not sure what's happening with the tests; they appear to be failing for reasons unrelated to the changes in this PR.

We also have a question about the implementation for this that we'd appreciate an early look at: we initially implemented this using waffle switches, but then switched to a django setting in edx@c685617 (the rationale being that a django setting would be more appropriate because changes to this toggle require restarting the server or a redeploy). Which method would edX prefer?

Thanks.

samuelallan72 · 2019-09-12T02:17:36Z

It seems that tests are failing on installing configparser==4.0.1, which doesn't exist as a pypi release yet (latest at time of writing is 3.8.1 https://pypi.org/project/configparser/#history ). Strangely, this change was introduced in https://github.com/edx/edx-platform/pull/21609, and the tests run fine. ¯\_(ツ)_/¯

EDIT: configparser 4.0.x was pulled from pypi: jaraco/configparser#45 (comment)

EDIT2: opened https://github.com/edx/edx-platform/pull/21634 to fix this until upstream publishes a new release

EDIT3: a fix has been merged to master. will rebase this branch as soon as i get a chance. (done)

natabene · 2019-09-12T14:19:16Z

@marcotuts Can you give this an early look please?

nedbat · 2019-09-12T15:41:28Z

If/when this is merged, also merge the documentation change: https://github.com/edx/edx-documentation/pull/1842

clemente · 2019-09-16T15:48:30Z

👍

I tested this: test instructions. I didn't see the link until I enabled the flag. The form to change the password worked
I read through the code, it's clean
I checked for accessibility issues: none, and it's better like this
Includes documentation: yes, here

There's still the question of whether we want a feature flag or a Waffle switch, but I'll leave this to edX. Waffle switches are to be applied dynamically, without restart, but this new feature modifies URLs and requires a restart, so therefore a FEATURE flag seems better.

@swalladge could you check the sandbox and then ping Ned when it's ready?

samuelallan72 · 2019-09-17T02:33:25Z

@natabene CC @nedbat the sandbox is provisioned and this is ready for edX review. 🙂

natabene · 2019-09-17T15:32:36Z

@marcotuts This is awaiting your review.

samuelallan72 · 2019-10-08T05:43:59Z

@natabene Any updates on this? For planning purposes. 🙂

natabene · 2019-10-08T16:42:29Z

@swalladge No, no updates yet.

cocococosti · 2019-10-08T19:08:13Z

Hi! We tested this at eduNEXT and works great 👍 thank you

Applying patch from https://github.com/edx/edx-platform/pull/21616

marcotuts · 2020-02-20T16:26:48Z

Ok to move past product review for eng review, thanks!

natabene · 2020-02-20T16:29:26Z

@swalladge Moving to engineering review

ormsbee

I'm honestly not crazy about adding a way to explicitly bypass password policy like this (and adding a feature flag on top of that). But our handling of auth_user is idiosyncratic enough, and this use case is likely common enough where I'm okay with it. Doing a feature flag is definitely the right way to do this btw – even if it wasn't required at startup time to do urls.py manipulation.

Just one required change request, and then please do the normal squash + copying the context from your PR message to your commit message, and I'll merge. Thank you.

ormsbee · 2020-02-20T22:48:25Z

cms/urls.py

Please add this flag to common.py for both the lms and cms envs, defaulting to False. Please also follow the feature flag annotation format (example: https://github.com/edx/edx-platform/blob/master/lms/envs/common.py#L180-L191)

Thanks @ormsbee ! I have added the flag to the common.py files and tidied the commits.

This was previously disabled because changing another user's password is both not usually recommended and bypasses password policy. Here, we add a feature flag (`ENABLE_CHANGE_USER_PASSWORD_ADMIN`) to allow re-enabling this password change form. This allows continued use of this functionality by clients that require it.

ormsbee · 2020-02-21T14:49:25Z

jenkins run bokchoy

openedx-webhooks · 2020-02-21T16:03:10Z

@swalladge 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2020-02-21T17:04:18Z

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

edx-pipeline-bot · 2020-02-21T18:03:50Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

edx-pipeline-bot · 2020-02-21T20:30:23Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-status-bot · 2020-02-26T22:51:57Z

Your PR has finished running tests. There were no failures.

samuelallan72 requested a review from a team September 11, 2019 01:14

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 11, 2019

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Sep 11, 2019

samuelallan72 mentioned this pull request Sep 12, 2019

SE-1496 list change user password from admin flag openedx-unsupported/edx-documentation#1842

Merged

2 tasks

natabene requested a review from marcotuts September 12, 2019 14:19

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 12, 2019

samuelallan72 force-pushed the samuel/gate-change-password branch from c685617 to 3860741 Compare September 12, 2019 23:42

cocococosti referenced this pull request in eduNEXT/edunext-platform Oct 7, 2019

Applying patch from https://github.com/edx/edx-platform/pull/21616

7deba64

cocococosti mentioned this pull request Oct 7, 2019

Applying patch to enable password change from django admin eduNEXT/edunext-platform#335

Merged

jfavellar90 referenced this pull request in eduNEXT/edx-platform Nov 27, 2019

Applying patch from https://github.com/edx/edx-platform/pull/21616

56530c1

jfavellar90 mentioned this pull request Nov 28, 2019

Applying patch from https://github.com/edx/edx-platform/pull/21616 eduNEXT/edx-platform#166

Merged

jfavellar90 referenced this pull request in eduNEXT/edx-platform Nov 28, 2019

Merge pull request #166 from eduNEXT/Jhony/change_user_pass_via_admin

cfa6133

Applying patch from https://github.com/edx/edx-platform/pull/21616

openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels Feb 20, 2020

openedx-webhooks added engineering review and removed awaiting prioritization labels Feb 20, 2020

ormsbee requested changes Feb 21, 2020

View reviewed changes

samuelallan72 force-pushed the samuel/gate-change-password branch from 22e4597 to efa2d44 Compare February 21, 2020 00:52

samuelallan72 force-pushed the samuel/gate-change-password branch from efa2d44 to 39de23c Compare February 21, 2020 00:55

ormsbee approved these changes Feb 21, 2020

View reviewed changes

ormsbee merged commit 0d3d680 into openedx:master Feb 21, 2020

pomegranited deleted the samuel/gate-change-password branch February 21, 2020 21:36

samuelallan72 mentioned this pull request Feb 24, 2020

SE-1496 (backport) Add flag to enable password change form in admin open-craft/openedx-platform#210

Merged

1 task

openedx-webhooks added merged and removed engineering review labels Jan 21, 2021

SE-1496 conditionally re-enable user change password form in admin #21616

SE-1496 conditionally re-enable user change password form in admin #21616

Conversation

samuelallan72 commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Sep 11, 2019

Uh oh!

natabene commented Sep 11, 2019

Uh oh!

samuelallan72 commented Sep 12, 2019

Uh oh!

samuelallan72 commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Sep 12, 2019

Uh oh!

nedbat commented Sep 12, 2019

Uh oh!

clemente commented Sep 16, 2019

Uh oh!

samuelallan72 commented Sep 17, 2019

Uh oh!

natabene commented Sep 17, 2019

Uh oh!

samuelallan72 commented Oct 8, 2019

Uh oh!

natabene commented Oct 8, 2019

Uh oh!

cocococosti commented Oct 8, 2019

Uh oh!

marcotuts commented Feb 20, 2020

Uh oh!

natabene commented Feb 20, 2020

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

samuelallan72 Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Feb 21, 2020

Uh oh!

openedx-webhooks commented Feb 21, 2020

Uh oh!

edx-pipeline-bot commented Feb 21, 2020

Uh oh!

edx-pipeline-bot commented Feb 21, 2020

Uh oh!

edx-pipeline-bot commented Feb 21, 2020

Uh oh!

edx-status-bot commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

samuelallan72 commented Sep 11, 2019 •

edited

Loading

samuelallan72 commented Sep 12, 2019 •

edited

Loading