codeant-ai-for-open-source[bot] commented on code in PR #41843:
URL: https://github.com/apache/superset/pull/41843#discussion_r3565421525
##########
superset/security/manager.py:
##########
@@ -1818,17 +1860,20 @@ def get_table_access_error_object(self, tables:
set["Table"]) -> SupersetError:
},
)
- def get_table_access_link( # pylint: disable=unused-argument
- self, tables: set["Table"]
- ) -> Optional[str]:
+ def get_table_access_link(self, tables: set["Table"]) -> Optional[str]:
"""
Return the access link for the denied SQL tables.
+ The configured ``PERMISSION_INSTRUCTIONS_LINK`` may template the denied
+ table names (and the current username) into the access URL.
+
:param tables: The set of denied SQL tables
:returns: The access URL
"""
- return get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
+ return _render_permission_instructions_link(
+ table_names=",".join(str(table) for table in tables),
Review Comment:
**Suggestion:** The access link builds `table_names` directly from a `set`,
which has non-deterministic iteration order across runs. This makes the
generated request-access URL unstable for the same denied tables (e.g.,
different parameter order each time), which can cause inconsistent UX and
brittle downstream processing; sort the tables before joining to produce a
deterministic link. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Request-access deep-link parameter order unstable across restarts.
- ⚠️ Error message table list order varies between identical denials.
- ⚠️ Downstream access-request forms may mis-handle unstable table_names.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure PERMISSION_INSTRUCTIONS_LINK in `superset/config.py:1930-1939`
to include
`{table_names}`, for example
`https://access.example.com/request?tables={table_names}`,
and start Superset with this configuration applied.
2. As a Gamma/viewer, issue a SQL Lab query that references multiple tables
you do not
have access to; the access check in
`SupersetSecurityManager.raise_for_access` (logic
shown in `superset/security/manager.py:3840-3959`) parses the SQL into
`Table` objects and
builds a `tables` set at line ~3854, then accumulates denied tables into the
`denied` set
at lines 47-65.
3. When `denied` is non-empty, `raise_for_access` executes the block at
`superset/security/manager.py:3906-3908`, raising
`SupersetSecurityException(self.get_table_access_error_object(denied))`;
`get_table_access_error_object` at `superset/security/manager.py:1846-1859`
constructs a
`SupersetError` with
`error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR`,
`extra["tables"] = [str(table) for table in tables]`, and `extra["link"] =
self.get_table_access_link(tables)`, where `tables` is the unordered
`denied` set.
4. `get_table_access_link` at `superset/security/manager.py:1863-1875` calls
`_render_permission_instructions_link(table_names=",".join(str(table) for
table in
tables))`, and the frontend `DatasourceSecurityAccessErrorMessage` in
`superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:44-53,71-78`
renders `extra.tables.join(', ')` and `extra.link`. Because `tables` and
`denied` are
Python sets, their iteration order (and thus the `table_names` URL parameter
and visible
table list) varies across interpreter runs, so restarting Superset can
produce different
table orderings for the same denial, resulting in non-deterministic
request-access URLs.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f20d7559ed84f2c8583bebeec14d1ce&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=9f20d7559ed84f2c8583bebeec14d1ce&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:** 1875:1875
**Comment:**
*Logic Error: The access link builds `table_names` directly from a
`set`, which has non-deterministic iteration order across runs. This makes the
generated request-access URL unstable for the same denied tables (e.g.,
different parameter order each time), which can cause inconsistent UX and
brittle downstream processing; sort the tables before joining to produce a
deterministic link.
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%2F41843&comment_hash=7bb854e24a11e49544609b5913c279f4ecfdf8ac2b3c8ca570fe2ffaa38fb3a8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=7bb854e24a11e49544609b5913c279f4ecfdf8ac2b3c8ca570fe2ffaa38fb3a8&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]
