codeant-ai-for-open-source[bot] commented on code in PR #41076:
URL: https://github.com/apache/superset/pull/41076#discussion_r3500171841
##########
superset/datasets/api.py:
##########
@@ -440,17 +504,69 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(SqlaTable, pk)
+
try:
+ # Two commands, two commits, two Continuum transactions for an
+ # ``override_columns`` save — deliberately NOT merged into one
+ # transaction. A single-transaction design was attempted and
+ # reverted: ``DBEventLogger`` writes request logs through the
+ # SHARED scoped session and calls ``commit()`` /
+ # ``rollback()`` on it mid-request (superset/utils/log.py),
+ # so any save held uncommitted across a logged sub-action can
+ # be committed half-done (Postgres/MySQL) or rolled back
+ # entirely on a transient logger failure (SQLite's
+ # "database is locked"). Until the event logger gets its own
+ # session, per-command commit boundaries are the only shape
+ # whose failure modes are honest. Consequence the
+ # version-history UI must tolerate: one logical save can
+ # surface as two version transactions stamped the same second.
changed_model = UpdateDatasetCommand(pk, item,
override_columns).run()
+ # Capture the post-update identifiers BEFORE the refresh:
+ # RefreshDatasetCommand commits its own transaction, so reading
+ # afterwards would attribute the refresh's version to the
+ # user's update (and old→new would span two transactions).
+ new_info = current_entity_version_info(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ etag_version_uuid = new_info.version_uuid
if override_columns:
RefreshDatasetCommand(pk).run()
- response = self.response(200, id=changed_model.id, result=item)
+ # The ETag must reflect the entity's *current live* version,
+ # which after the refresh is the refresh's transaction —
+ # re-read it rather than reusing the pre-refresh uuid.
+ etag_version_uuid = current_entity_etag_uuid(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ response = self.response(
+ 200,
+ id=changed_model.id,
+ result=item,
+ old_version=old_info.version,
+ new_version=new_info.version,
+ old_transaction_id=old_info.transaction_id,
+ new_transaction_id=new_info.transaction_id,
+ old_version_uuid=old_info.version_uuid,
+ new_version_uuid=new_info.version_uuid,
+ )
+ set_version_etag(response, etag_version_uuid)
except DatasetNotFoundError:
response = self.response_404()
Review Comment:
**Suggestion:** The second command (`RefreshDatasetCommand`) re-runs
ownership validation after the update has already committed; if the update
changed owners and removed the caller, refresh can now fail with forbidden even
though the dataset update already succeeded. This yields a 403 response for a
request that actually mutated state. Run refresh without a second ownership
gate for this internal flow, or perform both actions under a consistent
authorization model before committing. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset update persists but responds forbidden to caller.
- ⚠️ UI may misreport failed save despite applied changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. As an existing owner of a dataset, issue `PUT
/api/v1/dataset/<pk>?override_columns=true` to `DatasetRestApi.put()` in
`superset/datasets/api.py:3-179` with a request body that changes the
`owners` field so
the caller will no longer be listed as an owner after the update.
2. Inside `UpdateDatasetCommand.validate()`
(`superset/commands/dataset/update.py:98-128`),
`security_manager.raise_for_ownership(self._model)` at lines 107-111 checks
ownership
against the pre-update state, authorises the caller, and the subsequent
`DatasetDAO.update()` call at lines 93-97 commits the new owners (removing
the caller) in
the first transaction.
3. After this commit, `DatasetRestApi.put()` computes `new_info =
current_entity_version_info(SqlaTable, changed_model.id,
changed_model.uuid)` at
`superset/datasets/api.py:552-555`; because `override_columns` is true, it
then calls
`RefreshDatasetCommand(pk).run()` at lines 556-557, whose `validate()` method
(`superset/commands/dataset/refresh.py:68-77`) re-fetches the dataset and
re-runs
`security_manager.raise_for_ownership(self._model)` against the now-updated
owners list.
4. Since the caller removed themselves from `owners`, this second ownership
check raises
`DatasetForbiddenError` (`superset/commands/dataset/exceptions.py:185-187`),
which is
propagated back to `DatasetRestApi.put()` and caught in the `except
DatasetForbiddenError`
block at `superset/datasets/api.py:159-160`, causing a 403 error response
even though the
dataset update from `UpdateDatasetCommand` has already been successfully
committed to the
database.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4d4a963e461f4dddbb10bff6ce41ae63&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=4d4a963e461f4dddbb10bff6ce41ae63&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/datasets/api.py
**Line:** 556:557
**Comment:**
*Logic Error: The second command (`RefreshDatasetCommand`) re-runs
ownership validation after the update has already committed; if the update
changed owners and removed the caller, refresh can now fail with forbidden even
though the dataset update already succeeded. This yields a 403 response for a
request that actually mutated state. Run refresh without a second ownership
gate for this internal flow, or perform both actions under a consistent
authorization model before committing.
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%2F41076&comment_hash=e5a3564866b260be3a8b316cbeb1044de2bb57d445282d05db44e5a288b255c3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=e5a3564866b260be3a8b316cbeb1044de2bb57d445282d05db44e5a288b255c3&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]
