codeant-ai-for-open-source[bot] commented on code in PR #40669:
URL: https://github.com/apache/superset/pull/40669#discussion_r3384799626
##########
superset/security/manager.py:
##########
@@ -635,6 +635,17 @@ def create_login_manager(self, app: Flask) -> LoginManager:
lm.request_loader(self.request_loader)
return lm
+ def reset_password(self, userid: int, password: str) -> None:
+ """Reset a user's password and clear any pending forced-change flag.
+
+ Covers both the self-service reset and the admin "Reset Password"
action,
+ which both route through this method.
+ """
+ super().reset_password(userid, password)
+ from superset.security.password_change import
clear_password_must_change
+
+ clear_password_must_change(userid)
Review Comment:
**Suggestion:** The forced-change flag is cleared unconditionally for every
password reset, including admin-initiated resets. That breaks the first-use
security lifecycle because an administrator reset can silently remove the
requirement for the target user to change their password at next login. Clear
the flag only for self-service resets (or explicitly re-set it for admin
resets) so admin resets do not bypass enforcement. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Forced-password-change-on-first-use bypassed after admin reset.
- ⚠️ Users may retain administrator-set temporary passwords indefinitely.
- ⚠️ Weakens compliance with ASVS forced-password-change controls.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the forced password change enforcement so the before-request hook
is active by
setting `ENABLE_FORCE_PASSWORD_CHANGE = True` and starting Superset, which
registers the
hook via `register_password_change_enforcement(self.superset_app)` in
`superset/initialization/__init__.py:17-19` (function
`register_request_handlers`).
2. Provision or select a user who has `password_must_change` set to `True` in
`UserAttribute` (column defined in `superset/models/user_attributes.py:48`)
– this can be
done through admin provisioning logic or directly via the helper
`set_password_must_change(user_id, True)` implemented in
`superset/security/password_change.py:77-91`.
3. As an administrator, trigger an admin-initiated password reset for that
user using the
built-in "Reset Password" action in the Flask‑AppBuilder security UI; per
the docstring in
`SupersetSecurityManager.reset_password` at
`superset/security/manager.py:639-643`, both
the self-service reset and this admin "Reset Password" action route through
`SupersetSecurityManager.reset_password`, which calls
`super().reset_password(userid,
password)` and then unconditionally executes
`clear_password_must_change(userid)` at
`superset/security/manager.py:644-647`, clearing the `password_must_change`
flag.
4. Have the affected user log in with the administrator-set password: the
before-request
handler `_enforce_password_change` registered in
`superset/security/password_change.py:116-129` obtains `g.user` and calls
`password_change_required(user)` at
`superset/security/password_change.py:68-74`, which
now returns `False` because `attr.password_must_change` has been cleared by
the admin
reset, so no redirect to `ResetMyPasswordView.this_form_get` is performed
and the user can
access normal application endpoints without ever being forced to change the
administrator-set password, thereby bypassing the intended "must change on
first use"
lifecycle.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1839830055fb490caf8a9f8308f978ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1839830055fb490caf8a9f8308f978ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 644:647
**Comment:**
*Security: The forced-change flag is cleared unconditionally for every
password reset, including admin-initiated resets. That breaks the first-use
security lifecycle because an administrator reset can silently remove the
requirement for the target user to change their password at next login. Clear
the flag only for self-service resets (or explicitly re-set it for admin
resets) so admin resets do not bypass enforcement.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40669&comment_hash=7620e3d632a8ad8428dde9f8793d73156b32b5d709e35f1b08fa66dd96f28fca&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40669&comment_hash=7620e3d632a8ad8428dde9f8793d73156b32b5d709e35f1b08fa66dd96f28fca&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
