codeant-ai-for-open-source[bot] commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3673762516
##########
tests/unit_tests/subjects/test_utils.py:
##########
@@ -143,6 +146,187 @@ def
test_get_or_create_role_subject_missing_role_returns_none() -> None:
mock_sync.assert_not_called()
+# --------------------------------------------------------------------------
+# get_or_create_group_subject / get_user_group_subjects backfill
+# --------------------------------------------------------------------------
+
+
+@patch("superset.subjects.utils.get_group_subject")
+def test_get_or_create_group_subject_returns_existing(
+ mock_get_group_subject: MagicMock,
+) -> None:
+ """An already-synced group resolves to its Subject with no sync."""
+ existing = _make_subject(10, SubjectType.GROUP)
+ mock_get_group_subject.return_value = existing
+
+ assert get_or_create_group_subject(5) is existing
+ mock_get_group_subject.assert_called_once_with(5)
+
+
+def test_get_or_create_group_subject_syncs_unsynced_group() -> None:
+ """A group that exists but has no Subject row yet is synced on demand
+ rather than treated as nonexistent."""
+ created = _make_subject(11, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 5
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, created],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject") as mock_sync,
+ ):
+ mock_db.session.get.return_value = group
+
+ assert get_or_create_group_subject(5) is created
+
+ mock_sync.assert_called_once_with(group)
+ # The insert is wrapped in a SAVEPOINT so a concurrent conflict can't
+ # poison the surrounding transaction.
+ mock_db.session.begin_nested.assert_called_once()
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_handles_concurrent_insert() -> None:
+ """A concurrent backfill (IntegrityError in the savepoint) is swallowed and
+ the row the other request wrote is reloaded, keeping the transaction
usable."""
+ from sqlalchemy.exc import IntegrityError
+
+ winner = _make_subject(12, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, winner],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("duplicate group_id", None, Exception())
+ )
+
+ assert get_or_create_group_subject(7) is winner
+
+ # First lookup misses, the reload after the conflict returns the winner.
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_reraises_unrelated_integrity_error() ->
None:
+ """An IntegrityError that left no matching row isn't the expected
unique-key
+ race, so it propagates instead of silently dropping the group."""
+ from sqlalchemy.exc import IntegrityError
+
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch("superset.subjects.utils.get_group_subject", return_value=None),
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("unrelated failure", None, Exception())
+ )
+
+ with pytest.raises(IntegrityError):
+ get_or_create_group_subject(7)
Review Comment:
**Suggestion:** The implementation catches every `IntegrityError` from the
savepoint and then returns the result of `get_group_subject`, so an unrelated
integrity failure with no matching row returns `None` instead of raising. This
assertion therefore fails deterministically; either narrow the production
exception handling to the expected uniqueness conflict or change the test to
match the intended contract. [error handling]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unrelated database failures are silently converted into missing group
Subjects.
- ⚠️ Creator-group viewer assignment can silently omit group access.
- ⚠️ Transaction integrity errors become difficult to diagnose.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b33e2c5f18d34158b13fa331816806fb&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=b33e2c5f18d34158b13fa331816806fb&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:** tests/unit_tests/subjects/test_utils.py
**Line:** 238:239
**Comment:**
*Error Handling: The implementation catches every `IntegrityError` from
the savepoint and then returns the result of `get_group_subject`, so an
unrelated integrity failure with no matching row returns `None` instead of
raising. This assertion therefore fails deterministically; either narrow the
production exception handling to the expected uniqueness conflict or change the
test to match the intended contract.
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%2F42472&comment_hash=0ad6b7f53c66a9f72e6fe47dc7dcc4e8645ff141cb1af392d7dfedd78e273246&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=0ad6b7f53c66a9f72e6fe47dc7dcc4e8645ff141cb1af392d7dfedd78e273246&reaction=dislike'>👎</a>
##########
tests/unit_tests/subjects/test_utils.py:
##########
@@ -143,6 +146,187 @@ def
test_get_or_create_role_subject_missing_role_returns_none() -> None:
mock_sync.assert_not_called()
+# --------------------------------------------------------------------------
+# get_or_create_group_subject / get_user_group_subjects backfill
+# --------------------------------------------------------------------------
+
+
+@patch("superset.subjects.utils.get_group_subject")
+def test_get_or_create_group_subject_returns_existing(
+ mock_get_group_subject: MagicMock,
+) -> None:
+ """An already-synced group resolves to its Subject with no sync."""
+ existing = _make_subject(10, SubjectType.GROUP)
+ mock_get_group_subject.return_value = existing
+
+ assert get_or_create_group_subject(5) is existing
+ mock_get_group_subject.assert_called_once_with(5)
+
+
+def test_get_or_create_group_subject_syncs_unsynced_group() -> None:
+ """A group that exists but has no Subject row yet is synced on demand
+ rather than treated as nonexistent."""
+ created = _make_subject(11, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 5
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, created],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject") as mock_sync,
+ ):
+ mock_db.session.get.return_value = group
+
+ assert get_or_create_group_subject(5) is created
+
+ mock_sync.assert_called_once_with(group)
+ # The insert is wrapped in a SAVEPOINT so a concurrent conflict can't
+ # poison the surrounding transaction.
+ mock_db.session.begin_nested.assert_called_once()
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_handles_concurrent_insert() -> None:
+ """A concurrent backfill (IntegrityError in the savepoint) is swallowed and
+ the row the other request wrote is reloaded, keeping the transaction
usable."""
+ from sqlalchemy.exc import IntegrityError
+
+ winner = _make_subject(12, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, winner],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("duplicate group_id", None, Exception())
+ )
+
+ assert get_or_create_group_subject(7) is winner
+
+ # First lookup misses, the reload after the conflict returns the winner.
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_reraises_unrelated_integrity_error() ->
None:
+ """An IntegrityError that left no matching row isn't the expected
unique-key
+ race, so it propagates instead of silently dropping the group."""
+ from sqlalchemy.exc import IntegrityError
+
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch("superset.subjects.utils.get_group_subject", return_value=None),
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("unrelated failure", None, Exception())
Review Comment:
**Suggestion:** This does not simulate a concurrent insert during
synchronization: raising from `__enter__` aborts before
`sync_group_subject(group)` executes, so the test never verifies that an
`IntegrityError` raised by the insert is handled or that the savepoint protects
the surrounding transaction. Configure the synchronization mock to raise inside
the context (and assert it was called) or use a real conflicting insert. [code
quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Concurrency test does not exercise the insert path.
- ⚠️ Regressions in `sync_group_subject()` handling can pass unnoticed.
- ⚠️ Savepoint transaction behavior remains unverified.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=679ab3c664a444618b86f1539b55ce36&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=679ab3c664a444618b86f1539b55ce36&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:** tests/unit_tests/subjects/test_utils.py
**Line:** 234:235
**Comment:**
*Code Quality: This does not simulate a concurrent insert during
synchronization: raising from `__enter__` aborts before
`sync_group_subject(group)` executes, so the test never verifies that an
`IntegrityError` raised by the insert is handled or that the savepoint protects
the surrounding transaction. Configure the synchronization mock to raise inside
the context (and assert it was called) or use a real conflicting insert.
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%2F42472&comment_hash=792d4f844254509dc084be7bf862f6245cf55b149fd1a297c3dc6b7c581f6528&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=792d4f844254509dc084be7bf862f6245cf55b149fd1a297c3dc6b7c581f6528&reaction=dislike'>👎</a>
##########
superset/subjects/utils.py:
##########
@@ -79,6 +80,117 @@ def get_user_subject_ids_subquery(user_id: int) ->
CompoundSelect:
return union_all(user_subj, role_subj, group_subj, group_role_subj)
+def get_user_group_subject_ids_subquery(user_id: int) -> Select:
+ """Return a Select of GROUP-type Subject IDs for a user's groups.
+
+ Unlike :func:`get_user_subject_ids_subquery` this covers group membership
+ only — it excludes the user's own subject and any roles.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ return (
+ select(Subject.id)
+ .join(assoc_user_group, Subject.group_id ==
assoc_user_group.c.group_id)
+ .where(
+ assoc_user_group.c.user_id == user_id,
+ Subject.type == SubjectType.GROUP,
+ )
+ )
+
+
+def get_user_group_subjects(user_id: int) -> list[Subject]:
+ """Return the GROUP-type Subjects for every group a user belongs to.
+
+ Any group whose Subject row is missing is backfilled on demand (see
+ :func:`get_or_create_group_subject`) rather than skipped: a single viewer
+ disables an asset's datasource-access fallback, so a partial viewer set
+ would silently lock out the members of an unsynced group.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ subjects = (
+ db.session.query(Subject)
+ .filter(Subject.id.in_(get_user_group_subject_ids_subquery(user_id)))
+ .all()
+ )
+ member_group_ids = {
+ row[0]
+ for row in db.session.execute(
+ select(assoc_user_group.c.group_id).where(
+ assoc_user_group.c.user_id == user_id
+ )
+ )
+ }
+ for group_id in member_group_ids - {subject.group_id for subject in
subjects}:
+ if subject := get_or_create_group_subject(group_id):
+ subjects.append(subject)
+ return subjects
+
+
+def _assigns_creator_groups_as_viewers() -> bool:
+ from superset import is_feature_enabled
+
+ return has_app_context() and
is_feature_enabled("ASSIGN_CREATOR_GROUPS_AS_VIEWERS")
Review Comment:
**Suggestion:** The creator-group default is gated only by
`ASSIGN_CREATOR_GROUPS_AS_VIEWERS`, not by `ENABLE_VIEWERS`. When the
assignment flag is enabled while viewer access is disabled, new dashboards and
charts still persist non-empty viewer relationships; the access filters treat
any non-empty viewer relationship as restrictive, so those assets lose
datasource-permission fallback even though viewer access is disabled. Require
viewer access to be enabled before applying these defaults, or make the access
filters ignore viewer relationships when `ENABLE_VIEWERS` is false. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ New dashboards and charts can deny datasource-authorized users.
- ❌ Viewer-disabled deployments may unexpectedly enforce group restrictions.
- ⚠️ Dashboard and chart listing fallback becomes inconsistent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=61d63fea168040a5bb11395b0feb0cdd&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=61d63fea168040a5bb11395b0feb0cdd&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/subjects/utils.py
**Line:** 130:133
**Comment:**
*Security: The creator-group default is gated only by
`ASSIGN_CREATOR_GROUPS_AS_VIEWERS`, not by `ENABLE_VIEWERS`. When the
assignment flag is enabled while viewer access is disabled, new dashboards and
charts still persist non-empty viewer relationships; the access filters treat
any non-empty viewer relationship as restrictive, so those assets lose
datasource-permission fallback even though viewer access is disabled. Require
viewer access to be enabled before applying these defaults, or make the access
filters ignore viewer relationships when `ENABLE_VIEWERS` is false.
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%2F42472&comment_hash=0514d133073bb48307671a08ceb33926a5ae239844e0f25a7432dd1a95d37fb4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=0514d133073bb48307671a08ceb33926a5ae239844e0f25a7432dd1a95d37fb4&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]
