feat(security): force password change on first use (opt-in)#40669
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40669 +/- ##
==========================================
- Coverage 68.67% 64.29% -4.39%
==========================================
Files 1867 2659 +792
Lines 78198 144266 +66068
Branches 25075 33247 +8172
==========================================
+ Hits 53701 92749 +39048
- Misses 24497 49884 +25387
- Partials 0 1633 +1633
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds an opt-in "force password change on first use" lifecycle: a per-user password_must_change flag on UserAttribute, a before_request hook that redirects flagged users to the password-reset page, and a reset_password override that clears the flag. The feature is gated by a new ENABLE_FORCE_PASSWORD_CHANGE config flag (default off) and is marked as a draft pending end-to-end validation.
Changes:
- Adds
password_must_changeboolean column touser_attribute(model + Alembic migration) and helpers to set/clear/query the flag. - Registers a
before_requestenforcement hook with an endpoint-exemption list, and overridesSupersetSecurityManager.reset_passwordto clear the flag. - Introduces
ENABLE_FORCE_PASSWORD_CHANGEconfig and unit tests covering the exemption logic and flag query.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| superset/security/password_change.py | New module with flag helpers and the before_request enforcement hook. |
| superset/security/manager.py | Overrides reset_password to clear the forced-change flag. |
| superset/models/user_attributes.py | Adds the password_must_change boolean column. |
| superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py | Alembic migration adding the new column. |
| superset/initialization/init.py | Wires up the before_request enforcement hook. |
| superset/config.py | Adds the ENABLE_FORCE_PASSWORD_CHANGE config flag (default False). |
| tests/unit_tests/security/test_password_change.py | Unit tests for the exemption helper and password_change_required. |
rusackas
moved this from Needs Review
to Needs Follow-Up Work
in Superset Review Help Wanted
rusackas
added
maintain
rusackas
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
dosubot
Bot
added
authentication
|
| Language | Fuzzy before | Fuzzy after | New |
|---|---|---|---|
ar |
1136 | 1139 | +3 |
ca |
815 | 818 | +3 |
cs |
787 | 790 | +3 |
de |
997 | 1000 | +3 |
fi |
4823 | 4826 | +3 |
fr |
1014 | 1017 | +3 |
it |
1667 | 1670 | +3 |
ja |
322 | 325 | +3 |
ko |
1521 | 1524 | +3 |
lv |
295 | 298 | +3 |
mi |
807 | 810 | +3 |
nl |
1143 | 1146 | +3 |
pt |
1839 | 1842 | +3 |
ru |
293 | 296 | +3 |
sk |
774 | 777 | +3 |
sl |
963 | 966 | +3 |
th |
4823 | 4826 | +3 |
tr |
786 | 788 | +2 |
uk |
293 | 296 | +3 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (ar, ca, cs, de, fi, fr, it, ja, ko, lv, mi, nl, pt, ru, sk, sl, th, tr, uk):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
rusackas
moved this from Needs Follow-Up Work
to Needs Review
in Superset Review Help Wanted
Code Review Agent Run #7d258eActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
force-pushed
the
feat/force-password-change
branch
from
b52e30a to
4f67a03
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
rusackas
changed the title
Code Review Agent Run #8166cdActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
LGTM but #40676 also has a migration, so may need to rebase and update the down revision on whichever pr is merged second. |
Add an opt-in "must change password" lifecycle for accounts (typically created by an administrator): - New per-user flag password_must_change on the UserAttribute table (+ Alembic migration b7c9d1e2f3a4, default False). - superset/security/password_change.py: helpers to set/clear/query the flag and a before_request hook that, when ENABLE_FORCE_PASSWORD_CHANGE is enabled, redirects flagged users to the password-reset page. The hook exempts auth / password-reset / static / health endpoints to avoid a redirect loop, and is a no-op (zero overhead) when the feature is disabled (the default). - SupersetSecurityManager.reset_password override clears the flag on a successful reset (covers both self-service and admin reset, which both route through reset_password). - New config ENABLE_FORCE_PASSWORD_CHANGE (default False). Unit tests cover the endpoint-exemption logic (redirect-loop prevention) and the flag query helper. DRAFT: behavior-sensitive request middleware on the auth path. Needs end-to-end validation that the exemption list matches the actual FAB endpoint names (no redirect loop) and that the reset flow clears the flag, before merge. Wiring the flag to admin-create + secure-random initial passwords is a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Match exempt view classes exactly instead of substring matching so unrelated endpoints sharing a substring are not exempted from force-password-change enforcement. - Point the migration down_revision at the current alembic head. - Use @transaction() instead of manual db.session.commit(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address three codeant-ai review findings on the force-password-change feature: - Admin-initiated resets no longer clear the must-change flag. Only a self-service reset (acting user == target user) clears it, so an admin setting a temporary password preserves the change-at-next-login requirement. - _get_user_attribute fetches deterministically via order_by(id).first() instead of one_or_none(), so duplicate user_attribute rows (the table has no uniqueness on user_id) can't raise MultipleResultsFound / 500. - The enforcement redirect fallback now targets an exempt route (logout) and returns a 503 if no exempt target resolves, instead of redirecting to the non-exempt index and trapping flagged users in a 302 loop. Adds unit tests pinning each corrected behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
feat/force-password-change
branch
from
4d1427f to
1df7cea
Compare
|
Thanks @sadpandajoe! Already rebased onto current |
Master gained two sibling alembic heads (c8d2e3f4a5b6 and f7a1c93e0b21, both revising 31dae2559c05), so any branch adding a migration on top of just one of them fails 'superset db upgrade' with multiple heads. Make this PR's migration a merge point over both heads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Upsert-with-retry in set_password_must_change: the unique constraint on user_attribute.user_id means a concurrent insert can win between the read and the flush; catch IntegrityError in a nested transaction and update the winner's row (mirrors the session-invalidation upsert). - Derive the logout fallback from the registered auth view so non-DB auth backends (LDAP/OAuth/OID/remote-user) get a working exempt fallback, guarded so a non-exempt custom auth view can never become a loop target. - Refresh stale comments that predate the user_id unique constraint. - Typing/docstring nits: annotate migration upgrade/downgrade, test params and fixtures; add docstrings to the before-request hook and tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@sadpandajoe you called it... #40676 and #40695 both landed as siblings of |
github-project-automation
Bot
moved this from In Review
to Approved and/or Merged
in Superset Review Help Wanted
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Implements an opt-in force-password-change-on-first-use lifecycle (FINDING-016 / ASVS 6.4.1, CWE-262), for accounts an administrator provisions:
password_must_changeon theUserAttributetable (Superset's own per-user table — no change to FAB'sab_user), with Alembic migrationb7c9d1e2f3a4(defaultFalse,server_default false).superset/security/password_change.py— helpers to set / clear / query the flag, plus abefore_requesthook that (whenENABLE_FORCE_PASSWORD_CHANGEis on) redirects flagged users to the password-reset page. The hook exempts auth / password-reset / user-info / static / health endpoints to prevent a redirect loop, and is a no-op with zero per-request overhead when disabled (the default).SupersetSecurityManager.reset_passwordoverride clears the flag on a successful reset — covering both the self-service reset and the admin "Reset Password" action, which both route throughreset_password.ENABLE_FORCE_PASSWORD_CHANGE(defaultFalse).Slight trepidation....
before_requestredirect runs on the auth path; if the exemption list doesn't match the real FAB endpoint names, it could loop. The list is defensive (matchesauth/login/logout/resetmypassword/userinfoedit/static/healthsubstrings, and treats unknown/Noneendpoints as exempt), but needs validation against a running instance.set_password_must_change()hook for those flows to call.TESTING INSTRUCTIONS
Unit tests cover the endpoint-exemption logic (redirect-loop prevention) and the flag query helper. Before merge: flag a user, log in, confirm the redirect to reset, change the password, confirm the flag clears and normal access resumes.
ADDITIONAL INFORMATION
user_attribute.password_must_change; atomic, reversible)🤖 Generated with Claude Code