fix(rls): return a descriptive error for duplicate rule names - #42819
fix(rls): return a descriptive error for duplicate rule names#42819sadpandajoe wants to merge 4 commits into
Conversation
Creating or updating a Row Level Security rule with a name that already exists was only caught at DB flush as a SQLAlchemy IntegrityError, which surfaced to the client as an empty/opaque message (e.g. the toast "An error occurred while creating rowlevelsecuritys: ()"). Add an explicit name-uniqueness check to CreateRLSRuleCommand and UpdateRLSRuleCommand (excluding the rule being updated so an unchanged save is not rejected) that raises a translatable ValidationError before the DB write, and surface that ValidationError from the REST API as a 422 carrying the field message so the reason reaches the user. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42819 +/- ##
==========================================
- Coverage 66.37% 65.78% -0.59%
==========================================
Files 2857 2842 -15
Lines 161048 160694 -354
Branches 37046 36788 -258
==========================================
- Hits 106892 105711 -1181
- Misses 52141 52969 +828
+ Partials 2015 2014 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
authentication:row-level-security
There was a problem hiding this comment.
Pull request overview
Improves the Row Level Security (RLS) create/update API behavior by proactively validating rule-name uniqueness and returning a field-specific, translatable validation response (instead of a late DB IntegrityError with an empty message).
Changes:
- Add
RLSDAO.validate_uniqueness(name, rule_id=None)and call it fromCreateRLSRuleCommand.validate()/UpdateRLSRuleCommand.validate(). - Surface
marshmallow.ValidationError.messagesfrom the RLS RESTpost()/put()handlers as a422field error payload. - Add unit + integration coverage for duplicate-name create/rename and unchanged-name update.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
superset/daos/security.py |
Adds DAO-level name uniqueness helper for RLS rules (with optional self-exclusion on update). |
superset/commands/security/create.py |
Adds pre-write duplicate-name validation with translated field error. |
superset/commands/security/update.py |
Adds update-time duplicate-name validation excluding the current rule id. |
superset/row_level_security/api.py |
Returns 422 with structured field errors for command-raised ValidationError. |
tests/unit_tests/commands/security/rls_test.py |
Adds/adjusts unit tests to cover duplicate-name validation paths. |
tests/integration_tests/security/row_level_security_tests.py |
Adds integration assertions for descriptive duplicate-name responses and unchanged-name update success. |
Code Review Agent Run #ab6722Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| name = self._properties.get("name") | ||
| if name and not RLSDAO.validate_uniqueness(name): | ||
| raise ValidationError( | ||
| {"name": [_("A rule with this name already exists.")]} | ||
| ) |
There was a problem hiding this comment.
Suggestion: The duplicate-name check runs before datasource existence and access validation, so a caller who can invoke RLS creation but lacks access to the submitted datasource receives a duplicate-name response for an existing name and a forbidden response for a nonexistent name. This exposes global rule-name existence and changes the expected authorization response; perform datasource authorization before revealing whether the name is already used. The update command has the same ordering issue. [security]
Severity Level: Minor 🧹
- ⚠️ RLS users can enumerate globally used rule names.
- ⚠️ Unauthorized datasource requests reveal duplicate-name status.
- ⚠️ Update requests expose the same name-existence distinction.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/security/create.py
**Line:** 50:54
**Comment:**
*Security: The duplicate-name check runs before datasource existence and access validation, so a caller who can invoke RLS creation but lacks access to the submitted datasource receives a duplicate-name response for an existing name and a forbidden response for a nonexistent name. This exposes global rule-name existence and changes the expected authorization response; perform datasource authorization before revealing whether the name is already used. The update command has the same ordering issue.
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| query = db.session.query(RowLevelSecurityFilter).filter( | ||
| RowLevelSecurityFilter.name == name | ||
| ) | ||
| if rule_id is not None: | ||
| query = query.filter(RowLevelSecurityFilter.id != rule_id) | ||
| return not db.session.query(query.exists()).scalar() |
There was a problem hiding this comment.
Suggestion: The existence query is only a preflight check and is not atomic with the subsequent insert or update. Two concurrent requests can both observe the name as available, after which one write still raises the database unique-constraint error and the API returns the generic opaque SQLAlchemy message instead of the promised descriptive field error. Catch the unique-constraint violation and translate it to the same validation response, while retaining the database constraint as the authoritative guard. [race condition]
Severity Level: Minor 🧹
- ⚠️ Concurrent RLS creation can return opaque `422` errors.
- ⚠️ Concurrent renames can lose the descriptive validation message.
- ❌ No duplicate database rows are created due to the constraint.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/daos/security.py
**Line:** 36:41
**Comment:**
*Race Condition: The existence query is only a preflight check and is not atomic with the subsequent insert or update. Two concurrent requests can both observe the name as available, after which one write still raises the database unique-constraint error and the API returns the generic opaque SQLAlchemy message instead of the promised descriptive field error. Catch the unique-constraint violation and translate it to the same validation response, while retaining the database constraint as the authoritative guard.
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
SUMMARY
Creating (or renaming) a Row Level Security rule with a name that already exists was rejected only at DB flush time, by the
uniqueconstraint onRowLevelSecurityFilter.name. That surfaced as a SQLAlchemyIntegrityErrorwhose string form is empty, so the API returned422 {"message": "()"}and the UI rendered a toast with no reason in it:The user is told the save failed but not why, even though the cause is a simple, actionable name collision.
This adds an explicit name-uniqueness check to
CreateRLSRuleCommand.validate()andUpdateRLSRuleCommand.validate()that raises a translatableValidationErrorbefore the write, and surfaces it frompost()/put()as a422carrying the field message. The update path excludes the rule being edited, so saving a rule without renaming it is not treated as a collision.New
RLSDAO.validate_uniqueness(name, rule_id=None)mirrors the existing pattern inDatasetDAO/SemanticLayerDAO.After the change the response body is
{"message": {"name": ["A rule with this name already exists."]}}and the toast states the reason.Note the
ValidationErrorarm is deliberately ordered after the existingSubjectsNotFoundValidationError/DatasourceNotFoundValidationErrorarms, since those areValidationErrorsubclasses and must keep their own responses.RLSRuleNotFoundErroris aCommandException, so theput()404 path is unaffected.BEFORE/AFTER
Before:
before-primary-1440x900-default.mov
After:
after-primary-1440x900-default.mov
TESTING INSTRUCTIONS
my-rule(Regular, any dataset, any role, any clause). It saves.my-rule.Automated coverage added:
tests/unit_tests/commands/security/rls_test.py— create/update reject a duplicate name, allow a unique name, and allow an unchanged name (asserting the update check excludes the rule's own id).tests/integration_tests/security/row_level_security_tests.py— asserts the actual HTTP body for the create and rename cases, and that an unchanged-name save still returns 200.ADDITIONAL INFORMATION
Out of scope, pre-existing and unchanged by this PR: the frontend
createErrorHandlerformatting still pluralizes the resource label asrowlevelsecuritysand prefixes the field name, and the update path renders the serialized field object rather than a flattened sentence. Those live insuperset-frontend/src/views/CRUD/hooks.tsand affect every CRUD resource, so they are left for a separate change.