You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/08/09 15:56:01 UTC
[PR] fix(security): harden account password-change and session-invalidation handling [superset]
rusackas opened a new pull request, #42934:
URL: https://github.com/apache/superset/pull/42934
### SUMMARY
A few related fixes to account password-change and session handling, bundled together since they touch overlapping code paths:
- `CurrentUserRestApi.update_me` now requires a `current_password` field (verified against the account's existing password) whenever a password change is requested, rather than accepting a new password with no proof of the old one. Skipped only for accounts that have no password set yet.
- Fixed a bug in the same handler where the plaintext password value could overwrite the just-computed hash before the record was persisted, since both lived in the same attributes dict passed to the DAO's update call.
- Password changes and resets (self-service and admin-initiated) now stamp the existing session-invalidation epoch for the account, so other active sessions stop working after a password change instead of remaining valid indefinitely.
- Added an explicit admin action (`DELETE /api/v1/security/users/<pk>/sessions`) to terminate a specific user's sessions without disabling their account — there wasn't previously a direct way to do this short of deactivating the user.
- `SupersetUserApi.post`/`put` used to commit the FAB user write, then sync the user's `Subject` row in a second, separate commit — so a failure in that second commit could leave an orphaned user with no matching `Subject` row. The sync now happens in `pre_add`/`pre_update`, which FAB calls before the commit its own `datamodel.add`/`edit` issues, so both writes ride the same transaction and roll back together on failure.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/views/test_current_user_api.py
pytest tests/unit_tests/security/test_password_change_session_invalidation.py
pytest tests/unit_tests/security/test_superset_user_api_subject_sync.py
```
Manual: as any user, `PUT /api/v1/me/` with a `password` and no `current_password` should now be rejected; with the correct `current_password` it should succeed and any other active session for that account should be invalidated. As an admin, `DELETE /api/v1/security/users/<id>/sessions` should end that user's sessions without changing their `active` status.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3802936888
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +198,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
<!-- Bito Reply -->
The explanation provided regarding the existing test coverage is reasonable. Since the new function is exercised through integration tests that interact with the database and simulate real-world usage, it effectively validates the function's behavior and its non-committing semantics in a production-like flow. No further action is required for this comment.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5261000322
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3802935716
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +198,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
It is covered, just not by name — `test_password_change_session_invalidation.py` exercises `invalidate_sessions_for_user` through its real callers (`security_manager.reset_password`, `CurrentUserRestApi.update_me`, the terminate-sessions admin action) against a real db session and asserts the epoch actually got stamped, which is closer to the real usage than a mock-based unit test would be.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5331241868
Rechecked the latest push — the admin-path epoch stamp (`SupersetUserApi.pre_update`) and the falsy-password drop in `CurrentUserRestApi.pre_update` both look good, and the `current_password` proof + no-password-yet exemption read cleanly. Nicely tested too.
One non-blocking behavior note: `_login_at` is only written at login and isn't refreshed after a self password change, so when a user changes their own password the epoch is stamped to `now` and the `before_request` hook then logs the actor out on the next request — not just their other sessions. That's fail-safe (it over-logs-out rather than under), so it's a UX call rather than a correctness issue; forcing re-login after a password change is a defensible posture. If you'd prefer to keep the acting session alive and only revoke the siblings, note that the epoch is `ceil(now)`, so the current session's `_login_at` would need to be re-stamped to at least the epoch value — a plain `utcnow()` would still compare as invalidated. Fine to leave for a follow-up.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3744381527
##########
superset/views/users/api.py:
##########
@@ -49,12 +50,36 @@ class CurrentUserRestApi(BaseSupersetApi):
def pre_update(self, item: User, data: Dict[str, Any]) -> None:
item.changed_on = datetime.now()
item.changed_by_fk = g.user.id
+ # Pop unconditionally: this key is only meaningful for verifying a
+ # password change below, and it isn't a real column on the user
+ # model -- it must never reach ``UserDAO.update``'s ``setattr`` loop.
+ current_password = data.pop("current_password", None)
if "password" in data and data["password"]:
+ # An account with no password set yet (e.g. provisioned via an
+ # external auth backend) has nothing to prove knowledge of; for
+ # every other account, the caller must confirm the existing
+ # password before it can be replaced.
+ proof_ok = current_password and check_password_hash(
+ item.password, current_password
+ )
Review Comment:
**Suggestion:** When an account has no password and the request nevertheless supplies a non-empty `current_password`, `check_password_hash` is called with `item.password` equal to `None` before the later `item.password` guard is evaluated. Werkzeug will fail while parsing the missing hash, producing an unhandled server error instead of allowing the first password to be set or returning validation feedback. Only call `check_password_hash` when `item.password` is present. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Passwordless users receive an unhandled error on valid password updates.
- ⚠️ First-password setup fails when clients send extra password-proof data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53f5eac8009443479b47df8fffbf0b1c&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=53f5eac8009443479b47df8fffbf0b1c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 62:64
**Comment:**
*Null Pointer: When an account has no password and the request nevertheless supplies a non-empty `current_password`, `check_password_hash` is called with `item.password` equal to `None` before the later `item.password` guard is evaluated. Werkzeug will fail while parsing the missing hash, producing an unhandled server error instead of allowing the first password to be set or returning validation feedback. Only call `check_password_hash` when `item.password` is present.
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%2F42934&comment_hash=ab46fd05d721c10cc6180e0e0389d21e01c16b29ea2620aca90a15c967e137b5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42934&comment_hash=ab46fd05d721c10cc6180e0e0389d21e01c16b29ea2620aca90a15c967e137b5&reaction=dislike'>👎</a>
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +187,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
**Suggestion:** The wrapper can regress the invalidation epoch when concurrent password changes or session-termination requests compute timestamps before acquiring the row lock. A transaction with an older `now` value can update the row after a transaction with a newer value commits, leaving the older epoch persisted; sessions created after the newer invalidation but before the older commit can then survive. Update the existing row using the maximum of its current value and the newly computed timestamp (or otherwise serialize/guard the write) so the epoch is monotonic. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Concurrent password changes can leave older sessions authenticated.
- ❌ Explicit admin session termination can be partially defeated.
- ⚠️ Affected sessions bypass logout until a later invalidation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=849d364fa266476598497d1ee58bfe6f&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=849d364fa266476598497d1ee58bfe6f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/session_invalidation.py
**Line:** 204:204
**Comment:**
*Race Condition: The wrapper can regress the invalidation epoch when concurrent password changes or session-termination requests compute timestamps before acquiring the row lock. A transaction with an older `now` value can update the row after a transaction with a newer value commits, leaving the older epoch persisted; sessions created after the newer invalidation but before the older commit can then survive. Update the existing row using the maximum of its current value and the newly computed timestamp (or otherwise serialize/guard the write) so the epoch is monotonic.
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%2F42934&comment_hash=2d3d170210ba481f12a9e01668e5402093da4755986446b206e9f159a635b9c4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42934&comment_hash=2d3d170210ba481f12a9e01668e5402093da4755986446b206e9f159a635b9c4&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5324914365
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5232410030
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5234163041
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5259514038
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f4cebc2dbc7fa83df917e1d3937bfb8c0dbf9a2b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a7b9f87d0359d0008a90112 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42934--superset-docs-preview.netlify.app](https://deploy-preview-42934--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6YSr62A06xuhYCTWKFhTR3XP3Fu-FmMIRT5gAKjCJxG7Fi78VWRsuOVV2OfsgaqPETrysJbKLe3hmLrWnwehlHBjbTjMijqSxrhv4fg) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42934--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3744945353
##########
superset/views/users/api.py:
##########
@@ -49,12 +50,36 @@ class CurrentUserRestApi(BaseSupersetApi):
def pre_update(self, item: User, data: Dict[str, Any]) -> None:
item.changed_on = datetime.now()
item.changed_by_fk = g.user.id
+ # Pop unconditionally: this key is only meaningful for verifying a
+ # password change below, and it isn't a real column on the user
+ # model -- it must never reach ``UserDAO.update``'s ``setattr`` loop.
+ current_password = data.pop("current_password", None)
if "password" in data and data["password"]:
+ # An account with no password set yet (e.g. provisioned via an
+ # external auth backend) has nothing to prove knowledge of; for
+ # every other account, the caller must confirm the existing
+ # password before it can be replaced.
+ proof_ok = current_password and check_password_hash(
+ item.password, current_password
+ )
Review Comment:
Right, `check_password_hash` was reached with `item.password is None` whenever `current_password` was truthy, before the `item.password` guard. Fixed by short-circuiting on `item.password` first.
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +187,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
Fair, the blind UPDATE could let a slower transaction with an older timestamp overwrite a newer committed epoch. Guarded the write to only apply when it advances (or initializes) the stored value.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5232427885
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42934?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `34.69388%` with `32 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.12%. Comparing base ([`3b164e4`](https://app.codecov.io/gh/apache/superset/commit/3b164e4270860ac07223d4df1a60ca7b56312362?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e44d5d4`](https://app.codecov.io/gh/apache/superset/commit/e44d5d4f75768b93cdfc9a34ac9f310d9397955e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42934?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 26.66% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/views/users/api.py](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&filepath=superset%2Fviews%2Fusers%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvdXNlcnMvYXBpLnB5) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/security/session\_invalidation.py](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&filepath=superset%2Fsecurity%2Fsession_invalidation.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvc2Vzc2lvbl9pbnZhbGlkYXRpb24ucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/views/users/schemas.py](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&filepath=superset%2Fviews%2Fusers%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvdXNlcnMvc2NoZW1hcy5weQ==) | 75.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (3b164e4) and HEAD (e44d5d4). Click for more details.
>
> <details><summary>HEAD has 131 uploads less than BASE</summary>
>
>| Flag | BASE (3b164e4) | HEAD (e44d5d4) |
>|------|------|------|
>|hive|17|1|
>|python|101|2|
>|presto|17|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42934 +/- ##
==========================================
- Coverage 66.41% 57.12% -9.29%
==========================================
Files 2858 2858
Lines 161446 161474 +28
Branches 37190 37188 -2
==========================================
- Hits 107222 92249 -14973
- Misses 52187 68369 +16182
+ Partials 2037 856 -1181
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.21% <34.69%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.18% <34.69%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.23% <34.69%> (-18.98%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42934/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42934?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3801933870
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +198,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for new public API</b></div>
<div id="fix">
The new public function `invalidate_sessions_for_user` (line 201) is called from at least two production callers (`security/manager.py:458`, `views/users/api.py:84`) but has no unit test coverage. Rule 11730 requires comprehensive unit tests for new tools; without them the wrapper's delegation and non-committing semantics could regress silently.
</div>
</div>
<small><i>Code Review Run #e841f8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n merged PR #42934:
URL: https://github.com/apache/superset/pull/42934
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): harden account password-change and session-invalidation handling [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #42934:
URL: https://github.com/apache/superset/pull/42934#issuecomment-5259494999
Thanks @sha174n, appreciate the close read. Fixed 1 and 2: `SupersetUserApi.pre_update` now stamps the epoch when `data.get("password")` is set, and a falsy `password` gets popped before it reaches `UserDAO.update`'s `setattr` loop.
On 3, I'm inclined to leave it as is. Most users don't have a `user_attribute` row until this (or another lazy attribute) creates one, so a pre-check `SELECT` would add a round trip to the common first-stamp case just to save one in the rarer already-fresh race. Feels like a wash.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org