Skip to content

fix(auth): make an administrator-set password force a change at next login - #45702

Open
Dmitry-Kucher wants to merge 3 commits into
apache:masterfrom
Dmitry-Kucher:fix/admin-reset-forces-password-change
Open

Dmitry-Kucher wants to merge 3 commits into
apache:masterfrom
Dmitry-Kucher:fix/admin-reset-forces-password-change

Conversation

@Dmitry-Kucher

Copy link
Copy Markdown
Contributor

SUMMARY

ENABLE_FORCE_PASSWORD_CHANGE (#40669) redirects users flagged with password_must_change to their profile page until they choose a new password. Nothing in the tree ever raises that flag, though: set_password_must_change() has no caller, and #40669 left wiring it to administrator flows as a follow-up. Turning the setting on therefore enforces nothing.

This PR raises the flag in SupersetUserApi, which backs the Users list (and, since #44626, every administrator password reset):

  • pre_add (POST /api/v1/security/users/): a user created by an administrator starts on a password the administrator chose.
  • pre_update (PUT /api/v1/security/users/<pk>): an administrator setting another user's password hands out a temporary one. An administrator changing their own password has chosen it, so a pending change of theirs is cleared instead, as on /api/v1/me/.

Design notes:

  • Only while ENABLE_FORCE_PASSWORD_CHANGE is on. Otherwise enabling the setting later would retroactively lock out every account an administrator ever set a password for.
  • Only under AUTH_DB. With OAuth/LDAP/remote-user auth, the password set in the Users list is not the one users sign in with, so forcing a change would only lock pre-provisioned users out.
  • No early commit. set_password_must_change() is @transaction()-wrapped, so calling it from pre_add/pre_update would commit the half-applied user edit before FAB's own commit. The write goes through a new non-committing write_password_must_change() helper instead, so it rides FAB's commit, the same way invalidate_sessions_for_user and the subject sync already do there.
  • The self-service paths (PUT /api/v1/me/, password recovery) already clear the flag, so the lifecycle closes.

UPDATING.md and the ENABLE_FORCE_PASSWORD_CHANGE comment in config.py are updated.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (backend; the redirect UI already exists).

TESTING INSTRUCTIONS

pytest tests/unit_tests/security/test_admin_password_forces_change.py

Covers: admin create and admin password change raise the flag; self-edit doesn't (and clears a pending flag); non-password edit, setting off, and non-DB auth don't; and no commit is issued from pre_update. The tests call pre_add/pre_update directly with a stand-in acting user, as the neighbouring SupersetUserApi tests do; there is no unit-level HTTP harness for this API.

Manual: set ENABLE_FORCE_PASSWORD_CHANGE = True. As admin, set a new password for another user from Settings → List Users → Edit. Log in as that user: you're redirected to the profile page to choose a new password, and after changing it, normal access resumes.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: ENABLE_FORCE_PASSWORD_CHANGE (config, off by default)
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API

…login

ENABLE_FORCE_PASSWORD_CHANGE redirects users flagged with password_must_change
to their profile page until they choose a new password, but nothing in the
tree ever raised that flag: set_password_must_change() has no caller, and
setting on therefore enforced nothing.

Raise the flag in SupersetUserApi, which backs the Users list:

- pre_add: a user created by an administrator starts on a password the
  administrator chose.
- pre_update: an administrator setting another user's password hands out a
  temporary one. An administrator changing their own password has chosen it,
  so a pending change of theirs is cleared instead, as on /api/v1/me/.

Only while ENABLE_FORCE_PASSWORD_CHANGE is on, so enabling the setting later
does not retroactively lock out every account an administrator ever set a
password for, and only under AUTH_DB, where this is the password users sign in
with. The flag is written without committing, through a new
write_password_must_change() helper, so it rides the commit FAB issues right
after pre_add/pre_update instead of committing a half-applied user edit early
the way the @transaction-wrapped set_password_must_change() would.

Co-Authored-By: AIST code-assistant https://virtuman.com
@codeant-ai-for-open-source

codeant-ai-for-open-source Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

CodeAnt  CodeAnt PR Risk

🟢  Low Risk

  • The PR appears safe to merge; no actionable issues were identified in the review.

Assessed commit: b92eb7228288

@codecov

codecov Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.85%. Comparing base (ffdf7a4) to head (b92eb72).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #45702       +/-   ##
===========================================
+ Coverage   65.38%   84.85%   +19.46%     
===========================================
  Files        3080     3073        -7     
  Lines      209437   208108     -1329     
  Branches    49385    48752      -633     
===========================================
+ Hits       136950   176597    +39647     
+ Misses      69350    28131    -41219     
- Partials     3137     3380      +243     
Flag Coverage Δ
hive 34.19% <20.00%> (-0.01%) ⬇️
mysql 52.02% <40.00%> (-0.01%) ⬇️
postgres 52.03% <40.00%> (?)
presto 35.85% <20.00%> (-0.01%) ⬇️
python 87.96% <100.00%> (+35.73%) ⬆️
sqlite 51.78% <40.00%> (-0.01%) ⬇️
unit 82.91% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Oct 28, 2026
Resolve independent UPDATING.md additions by retaining both the administrator password-change note and upstream webhook/Impala proxy guidance.
@github-actions github-actions Bot added requires:rebase Requires rebasing on top of current master and removed requires:rebase Requires rebasing on top of current master labels Oct 29, 2026

@sadpandajoe sadpandajoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but will approve once conflicts are resolved

Retain all upstream upgrade notes and place the administrator password-change note at the end of Next to reduce repeated insertion conflicts. Security unit suite: 753 passed on a fresh SQLite database. Pre-commit passed for all PR files.
@github-actions github-actions Bot removed the requires:rebase Requires rebasing on top of current master label Nov 5, 2026
@Dmitry-Kucher

Copy link
Copy Markdown
Contributor Author

@sadpandajoe Conflicts are resolved and the branch is updated with master. All 753 security unit tests and pre-commit checks pass locally. Ready for another look — thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants