codeant-ai-for-open-source[bot] commented on code in PR #39469:
URL: https://github.com/apache/superset/pull/39469#discussion_r3474424037
##########
superset/views/users/api.py:
##########
@@ -163,6 +302,132 @@ def update_me(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/password", methods=["PUT"])
+ @protect()
+ @permission_name("write")
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.put_password",
+ log_to_statsd=False,
+ )
+ @requires_json
+ @_rate_limit_me_password_change
+ def update_my_password(self) -> Response:
Review Comment:
**Suggestion:** This new password-change route is not integrated with the
forced-password-change enforcement allowlist, so users flagged with
`password_must_change` will be redirected by the global before-request hook
instead of reaching this API. That breaks the intended self-service flow
exactly for first-login users who must change their password. Add this endpoint
to the exempt password-change routes (or equivalent bypass logic) so flagged
users can call it. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Forced-change users blocked from /api/v1/me/password endpoint.
- ⚠️ First-login self-service password change flow broken.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Application startup registers the forced-password-change before-request
hook via
`register_request_handlers()` in `superset/initialization/__init__.py:6-16`,
which calls
`register_password_change_enforcement(self.superset_app)` from
`superset/security/password_change.py:148-156`.
2. The enforcement hook `_enforce_password_change()` in
`superset/security/password_change.py:155-175` checks
`_is_exempt_endpoint(request.endpoint)` (lines 133-145) against
`_EXEMPT_VIEW_CLASSES` and
`_EXEMPT_ENDPOINTS` (lines 51-66). `CurrentUserRestApi` is not listed in
`_EXEMPT_VIEW_CLASSES`, and its endpoints (e.g.
`CurrentUserRestApi.update_my_password`)
are not in `_EXEMPT_ENDPOINTS`.
3. A user is provisioned with `password_must_change = True` (column defined
in
`superset/models/user_attributes.py:60-64` and set via
`set_password_must_change()` in
`superset/security/password_change.py:95-123`). With
`ENABLE_FORCE_PASSWORD_CHANGE`
enabled (checked at `password_change.py:163-164`),
`password_change_required(user)`
returns True (lines 86-92).
4. The same authenticated user calls `PUT /api/v1/me/password`, which is
routed to
`CurrentUserRestApi.update_my_password` defined at
`superset/views/users/api.py:305-235`.
Before the view runs, `_enforce_password_change()` sees `request.endpoint`
for this
method, determines it is not exempt (per step 2),
`password_change_required(user)` returns
True, and the hook returns a redirect to the reset form or logout (lines
176-203) instead
of `None`. As a result, the request never reaches `update_my_password()`, so
forced-change
users cannot use this API to change their password.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=983b7b1979854c91839c7dba4ccc2932&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=983b7b1979854c91839c7dba4ccc2932&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/views/users/api.py
**Line:** 305:316
**Comment:**
*Api Mismatch: This new password-change route is not integrated with
the forced-password-change enforcement allowlist, so users flagged with
`password_must_change` will be redirected by the global before-request hook
instead of reaching this API. That breaks the intended self-service flow
exactly for first-login users who must change their password. Add this endpoint
to the exempt password-change routes (or equivalent bypass logic) so flagged
users can call it.
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%2F39469&comment_hash=b6e978e9a3a0c7fef3e05e8ef9f9ff46c5c94e371873e10114291c0a61875f5e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b6e978e9a3a0c7fef3e05e8ef9f9ff46c5c94e371873e10114291c0a61875f5e&reaction=dislike'>👎</a>
##########
superset/views/users/api.py:
##########
@@ -163,6 +302,132 @@ def update_me(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/password", methods=["PUT"])
+ @protect()
+ @permission_name("write")
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.put_password",
+ log_to_statsd=False,
+ )
+ @requires_json
+ @_rate_limit_me_password_change
+ def update_my_password(self) -> Response:
+ """Update the current user's password (AUTH_DB only)
+ ---
+ put:
+ summary: Update the current user's password
+ description: >-
+ Changes the authenticated user's password when ``AUTH_TYPE`` is
``AUTH_DB``.
+ Requires the current password and a new password that satisfies
+ ``AUTH_DB_CONFIG`` policy.
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/CurrentUserPasswordPutSchema'
+ responses:
+ 200:
+ description: Password updated successfully
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref: '#/components/schemas/UserResponseSchema'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if app.config.get("AUTH_TYPE") != AUTH_DB:
+ return self.response_400(
+ message=(
+ "Password change is only available when AUTH_TYPE is
AUTH_DB."
+ ),
+ )
+
+ try:
+ body = _load_password_change_body(
+ self.current_user_password_put_schema,
+ request.json,
+ )
+ user_db = db.session.get(User, g.user.id)
+ if user_db is None:
+ return self.response_404()
+
+ old_hash = user_db.password
+ if not verify_auth_db_password(old_hash, body["current_password"]):
+ return self.response_400(message="Incorrect current password.")
+
+ new_hash = hash_auth_db_password(body["new_password"])
+ except ValidationError as error:
+ return self.response_400(message=error.messages)
+
+ try:
+ user_after = _commit_user_password_change(
+ self,
+ g.user.id,
+ old_hash,
+ new_hash,
+ )
+ except PasswordChangeConflictError:
+ return self.response_400(
+ message=(
+ "Unable to update password. Your password may have been "
+ "changed elsewhere; please try again."
+ ),
+ )
+ except SQLAlchemyError:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ logger.exception("Failed to commit password change")
+ return self.response_500(
+ message="Unable to update password. Please try again.",
+ )
+
+ _reestablish_login_session(user_after)
+ return self.response(200, result=user_response_schema.dump(user_after))
Review Comment:
**Suggestion:** The successful self-service password change path does not
clear the `password_must_change` flag, unlike the existing self-service reset
flow. If that flag is set, users can remain stuck in forced-change enforcement
even after changing their password. Clear the forced-change attribute after a
successful commit in this endpoint to keep behavior consistent with other
self-service password-change paths. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Users may remain stuck in forced-change enforcement.
- ⚠️ Self-service password-change behavior diverges from reset-password flow.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Existing self-service password reset flows call
`SupersetSecurityManager.reset_password()` in
`superset/security/manager.py:945-35`, which
after `super().reset_password(userid, password)` computes `is_self_service`
(lines 22-27)
and, when true, invokes `clear_password_must_change(int(userid))` from
`superset/security/password_change.py:126-130`, setting
`UserAttribute.password_must_change = False` (column defined in
`superset/models/user_attributes.py:60-64`).
2. The new API `CurrentUserRestApi.update_my_password` is implemented in
`superset/views/users/api.py:146-235`. After validating the current and new
passwords
(lines 196-210), it hashes the new value and calls
`_commit_user_password_change(...)` at
lines 213-219 to update `User.password` and session stamps, but does not call
`reset_password()` or `clear_password_must_change()`.
3. When an authenticated user who currently has `password_must_change =
True` (set via
`set_password_must_change()` in
`superset/security/password_change.py:95-123`)
successfully calls `PUT /api/v1/me/password`, the code block at
`superset/views/users/api.py:373-378` updates their password, then
`_reestablish_login_session(user_after)` at line 393 refreshes their session
and the API
returns 200 at line 394. At no point in this path is
`clear_password_must_change()`
invoked, so the flag in `UserAttribute` remains True.
4. Later, when `ENABLE_FORCE_PASSWORD_CHANGE` is (or becomes) enabled, the
before-request
hook `_enforce_password_change()` in
`superset/security/password_change.py:155-175` calls
`password_change_required(user)` (lines 86-92), which still reads
`attr.password_must_change` as True. The user is therefore treated as still
needing a
forced password change and is redirected on non-exempt endpoints, even
though they already
updated their password via the self-service API, leaving account state
inconsistent
compared to the established `reset_password` self-service behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c8613f80d37b4db5b243a967e65e8374&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c8613f80d37b4db5b243a967e65e8374&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/views/users/api.py
**Line:** 373:394
**Comment:**
*Incomplete Implementation: The successful self-service password change
path does not clear the `password_must_change` flag, unlike the existing
self-service reset flow. If that flag is set, users can remain stuck in
forced-change enforcement even after changing their password. Clear the
forced-change attribute after a successful commit in this endpoint to keep
behavior consistent with other self-service password-change paths.
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%2F39469&comment_hash=ddb8da83131de34c58fe83cea3b6ee989657ad1268f92c963bd5347a503a95fa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=ddb8da83131de34c58fe83cea3b6ee989657ad1268f92c963bd5347a503a95fa&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]
