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:25 UTC
[PR] fix: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
rusackas opened a new pull request, #42938:
URL: https://github.com/apache/superset/pull/42938
### SUMMARY
Two unrelated identity/cache-key robustness fixes:
- `sync_database_permissions_task` resolved its acting identity from a username string at execution time. Since that resolution happens after the Celery enqueue/execute boundary, a username change in between (a rename, or reassignment of a freed-up name) could change which user's account the async sync runs under — including for OAuth2 token lookup and `impersonate_user` connections, both of which key off the bound `g.user`. The user id is now resolved once at enqueue time and carried across the Celery boundary instead, matching the convention already used by other identity-carrying tasks in this codebase (e.g. `export_dashboard_excel`).
- `collect_rls_predicates_for_sql` feeds `SqlaTable.get_extra_cache_keys` for virtual datasets, folding each user's RLS predicates into the dataset's cache key so users with different RLS scopes get different cache entries. On a SQL-parse failure it fell back to an empty list — a fixed contribution regardless of who's asking, collapsing what should be distinct cache-key contributions for differently-scoped users into one. It now returns a marker derived from the current user's id instead, so a parse failure still varies the cache key per user. This only touches the cache-key helper; RLS predicate enforcement on the actual query goes through a separate, untouched path (`get_sqla_row_level_filters`).
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/commands/databases/sync_permissions_identity_test.py
pytest tests/unit_tests/commands/databases/
pytest tests/unit_tests/utils/rls_test.py
```
### 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
- [ ] 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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42938:
URL: https://github.com/apache/superset/pull/42938#issuecomment-5232431842
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42938?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 `16.66667%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.13%. 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 ([`2f3d8b8`](https://app.codecov.io/gh/apache/superset/commit/2f3d8b821b26d98dde6262822da4c838d450d3b4?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/42938?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/commands/database/sync\_permissions.py](https://app.codecov.io/gh/apache/superset/pull/42938?src=pr&el=tree&filepath=superset%2Fcommands%2Fdatabase%2Fsync_permissions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YWJhc2Uvc3luY19wZXJtaXNzaW9ucy5weQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/rls.py](https://app.codecov.io/gh/apache/superset/pull/42938?src=pr&el=tree&filepath=superset%2Futils%2Frls.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcmxzLnB5) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42938?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 (2f3d8b8). Click for more details.
>
> <details><summary>HEAD has 131 uploads less than BASE</summary>
>
>| Flag | BASE (3b164e4) | HEAD (2f3d8b8) |
>|------|------|------|
>|hive|17|1|
>|python|101|2|
>|presto|17|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42938 +/- ##
==========================================
- Coverage 66.41% 57.13% -9.29%
==========================================
Files 2858 2858
Lines 161446 161449 +3
Branches 37190 37190
==========================================
- Hits 107222 92236 -14986
- Misses 52187 68357 +16170
+ Partials 2037 856 -1181
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42938/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/42938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.21% <16.66%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42938/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/42938/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/42938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.17% <16.66%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.23% <16.66%> (-18.99%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42938/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/42938/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/42938?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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42938:
URL: https://github.com/apache/superset/pull/42938#issuecomment-5232411723
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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42938:
URL: https://github.com/apache/superset/pull/42938#discussion_r3744964232
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to replace the generic `Exception` catch with specific exception types is a standard practice to avoid masking unexpected errors. However, if the intention is to catch all potential parsing errors from the `sqlglot` library to ensure the fallback mechanism always triggers, keeping the broad `Exception` catch is acceptable. You may choose to ignore this suggestion if the current implementation is necessary for the required robustness.
**superset/utils/rls.py**
```
except Exception:
# If we can't parse the SQL, we can't tell which (if any) RLS
# predicates would apply, so we can't contribute a meaningful cache
# key component. Returning an empty list here would make every
# user's failure collapse onto the same (missing) contribution,
# which is unsafe when different users have different RLS scopes on
# the underlying tables. Fall back to a per-user marker instead, so
# the cache key still varies by user even though we don't know the
# actual predicates.
return [
f"rls-predicate-parse-failed-for-user-{get_user_id()}"
]
```
--
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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #42938:
URL: https://github.com/apache/superset/pull/42938#issuecomment-5259891275
@sha174n thanks for the thorough look. Added a note about the rolling-deploy window to the description.
--
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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [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 #42938:
URL: https://github.com/apache/superset/pull/42938#discussion_r3744384588
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
**Suggestion:** The fallback is not actually isolated for guest-token requests: `get_user_id()` returns `None` for `GuestUser`, while guest RLS rules are derived from the token's `rls_rules` and can differ between tokens. Consequently, parse failures for different guest scopes all contribute `rls-predicate-parse-failed-for-user-None`, allowing their virtual-dataset cache entries to collide. Use a stable identifier derived from the guest token or its RLS scope instead of only the database user id. [cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest RLS cache contributions collide during parse failures.
- ❌ Virtual-dataset results may cross guest scopes.
- ⚠️ Embedded dashboards are affected when SQL parsing fails.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2dd4b582e7684e00a75175f5f7b70ec0&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=2dd4b582e7684e00a75175f5f7b70ec0&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/utils/rls.py
**Line:** 193:193
**Comment:**
*Cache: The fallback is not actually isolated for guest-token requests: `get_user_id()` returns `None` for `GuestUser`, while guest RLS rules are derived from the token's `rls_rules` and can differ between tokens. Consequently, parse failures for different guest scopes all contribute `rls-predicate-parse-failed-for-user-None`, allowing their virtual-dataset cache entries to collide. Use a stable identifier derived from the guest token or its RLS scope instead of only the database user id.
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%2F42938&comment_hash=d44dac41a455f344409bba717e3a1294f7c313812dafa9cc631f40d3d3de1c07&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42938&comment_hash=d44dac41a455f344409bba717e3a1294f7c313812dafa9cc631f40d3d3de1c07&reaction=dislike'>👎</a>
##########
superset/commands/database/sync_permissions.py:
##########
@@ -313,14 +318,14 @@ def _rename_database_in_permissions(
@celery_app.task(name="sync_database_permissions", soft_time_limit=600)
def sync_database_permissions_task(
- database_id: int, username: str, old_db_connection_name: str
+ database_id: int, user_id: int, old_db_connection_name: str
Review Comment:
**Suggestion:** The Celery task's second argument changed from the queued username value to a user id without preserving compatibility with messages already published under the previous contract. During a rolling deployment, a worker running this version can consume an old three-argument message, pass the username string to `get_user_by_id()`, and then swallow `UserNotFoundInSessionError`, leaving that permission sync undone. Version the task name or accept and translate the legacy payload while old messages may remain in the broker. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Rolling deployments can strand queued permission synchronizations.
- ❌ Database permissions remain stale after failed task consumption.
- ⚠️ The task logs failure without retrying the incompatible message.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b331ec979b84693af35a230d328b2ed&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=7b331ec979b84693af35a230d328b2ed&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/commands/database/sync_permissions.py
**Line:** 320:321
**Comment:**
*Api Mismatch: The Celery task's second argument changed from the queued username value to a user id without preserving compatibility with messages already published under the previous contract. During a rolling deployment, a worker running this version can consume an old three-argument message, pass the username string to `get_user_by_id()`, and then swallow `UserNotFoundInSessionError`, leaving that permission sync undone. Version the task name or accept and translate the legacy payload while old messages may remain in the broker.
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%2F42938&comment_hash=d6a8a6f9fed9af61f4a4c4068f66cc0e667b4eef8582d2196464489461863bc7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42938&comment_hash=d6a8a6f9fed9af61f4a4c4068f66cc0e667b4eef8582d2196464489461863bc7&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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42938:
URL: https://github.com/apache/superset/pull/42938#issuecomment-5234170458
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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42938:
URL: https://github.com/apache/superset/pull/42938
--
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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42938:
URL: https://github.com/apache/superset/pull/42938#discussion_r3744395053
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Replace blind Exception catch clause</b></div>
<div id="fix">
Catching generic `Exception` on line 184 is a blind except that masks all errors. Replace with specific exception types that can reasonably occur during SQL parsing.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
)
except Exception:
# If we can't parse the SQL, we can't tell which (if any) RLS
# predicates would apply, so we can't contribute a meaningful cache
# key component. Returning an empty list here would make every
# user's failure collapse onto the same (missing) contribution,
# which is unsafe when different users have different RLS scopes on
# the underlying tables. Fall back to a per-user marker instead, so
# the cache key still varies by user even though we don't know the
# actual predicates.
return [
f"rls-predicate-parse-failed-for-user-{get_user_id()}"
]
````
</div>
</details>
</div>
<small><i>Code Review Run #e27c3a</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: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42938:
URL: https://github.com/apache/superset/pull/42938#discussion_r3744963384
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
Good catch, `get_user_id()` returns `None` for guest users so different guest tokens were colliding on the same sentinel. Now hashing the guest token's own `rls_rules` instead so distinct scopes stay separate.
##########
superset/commands/database/sync_permissions.py:
##########
@@ -313,14 +318,14 @@ def _rename_database_in_permissions(
@celery_app.task(name="sync_database_permissions", soft_time_limit=600)
def sync_database_permissions_task(
- database_id: int, username: str, old_db_connection_name: str
+ database_id: int, user_id: int, old_db_connection_name: str
Review Comment:
This only bites during the exact rolling-deploy window in async mode, and the failure is just a skipped sync that reruns cleanly on the next update, not corrupted state. Don't think that's worth versioning the task for.
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
The suggested diff doesn't actually narrow the catch, it's the same `except Exception`. Keeping it broad here is intentional, sqlglot can raise all sorts of exception types on malformed SQL and the fallback needs to fire regardless.
--
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