feat: implement table read permission checks for saved queries and add utility functions for query table resolution#1804
Merged
Conversation
…d utility functions for query table resolution
📝 WalkthroughWalkthroughThis PR adds table-level Cedar authorization enforcement for saved query execution and ad-hoc query testing. It introduces utilities to extract table names from SQL queries, enforces read permissions per table, integrates authorization into two use cases, and validates behavior with comprehensive E2E tests. ChangesTable-level Authorization for Queries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds table-level read permission enforcement for executing ad-hoc queries and saved queries by resolving referenced tables from SQL (when possible) and falling back to an “all tables must be readable” check when table resolution is indeterminate.
Changes:
- Introduces utilities to (a) resolve referenced tables from a SQL query and (b) enforce
table:readacross those tables (with an all-tables fallback). - Enforces table read checks in saved-query execution and
/query/testuse cases via Cedar authorization. - Adds AVA coverage for query-table resolution and SaaS e2e permission scenarios; updates a Mermaid diagram preview regex expectation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts | New SaaS e2e tests validating table read enforcement for /query/test and saved-query execution. |
| backend/test/ava-tests/non-saas-tests/non-saas-collect-query-tables.test.ts | Unit tests for SQL table collection behavior (including indeterminate cases). |
| backend/test/ava-tests/connection-diagram-preview.test.ts | Adjusts regex expectation for [NEW] marker formatting in Mermaid output. |
| backend/src/exceptions/text/messages.ts | Adds a dedicated “no read permission for table” message used by the new guard logic. |
| backend/src/entities/visualizations/panel/utils/collect-query-tables.util.ts | New utility to parse SQL and extract referenced tables (or return indeterminate with reason). |
| backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts | New utility enforcing table:read across resolved tables, with an all-tables fallback. |
| backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts | Applies table read enforcement before executing ad-hoc test queries. |
| backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts | Applies table read enforcement before executing saved queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+36
| if (NON_SQL_CONNECTION_TYPES.has(connectionType)) { | ||
| return { kind: 'indeterminate', reason: `non-SQL connection type "${connectionType}"` }; | ||
| } | ||
|
|
||
| const dialect = connectionTypeToParserDialect(connectionType); |
| tablesToCheck = collected.tables; | ||
| } else { | ||
| slackPostMessage( | ||
| `Saved-query permission check could not resolve referenced tables for connection ${connectionId} ` + |
Comment on lines
+43
to
+47
| for (const tableName of tablesToCheck) { | ||
| const allowed = await validateTableRead(tableName); | ||
| if (!allowed) { | ||
| throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(tableName)); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
backend/src/entities/visualizations/panel/utils/collect-query-tables.util.ts (1)
71-88: 💤 Low valueOptional: Consider consolidating parsing to avoid double work.
The query is parsed twice: once via
tableList()(line 42) and again viaastify()(line 73). You could extract both table references and CTE names from a single AST traversal. However, the current approach is clearer and the performance cost is negligible for authorization checks that run once per query execution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/visualizations/panel/utils/collect-query-tables.util.ts` around lines 71 - 88, The code parses the SQL twice (tableList() and collectCteNames() calling parser.astify()); to fix, consolidate parsing by parsing once and reusing the AST: modify collectCteNames or tableList to accept an AST (e.g., change collectCteNames signature to accept ast: any or add a new collectTablesAndCtes(parser: Parser, query: string, dialect: string) that calls parser.astify() once and traverses the AST to extract both table references and CTE names, updating callers to use the new API (reference functions collectCteNames, tableList and parser.astify) so you avoid duplicate parser.astify() calls.backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts (2)
43-48: ⚖️ Poor tradeoffSequential permission checks may cause latency for queries touching many tables.
Each table is validated sequentially via
await validateTableRead(tableName). For queries referencing many tables (e.g., large JOINs or the all-tables fallback), this could be slow. Consider parallelizing withPromise.allif the authorization service supports concurrent requests.♻️ Optional parallel validation
- for (const tableName of tablesToCheck) { - const allowed = await validateTableRead(tableName); - if (!allowed) { - throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(tableName)); - } - } + const results = await Promise.all( + tablesToCheck.map(async (tableName) => ({ + tableName, + allowed: await validateTableRead(tableName), + })), + ); + const denied = results.find((r) => !r.allowed); + if (denied) { + throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(denied.tableName)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts` around lines 43 - 48, The loop that calls validateTableRead sequentially should be converted to run validations in parallel: map tablesToCheck to an array of Promises by calling validateTableRead(tableName) for each table, await Promise.all on that array, then inspect results and if any validation returned falsy throw the existing ForbiddenException using Messages.NO_READ_PERMISSION_FOR_TABLE with the corresponding tableName; this keeps the same error behavior but avoids sequential awaits and reduces latency for large tablesToCheck collections.
36-39: 💤 Low valueConsider sanitizing or truncating the query in Slack messages.
The full query text is included in the Slack message. If queries may contain sensitive data (e.g., literals with PII), this could leak sensitive information to Slack. Consider truncating or omitting the query body, or redacting potential literals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts` around lines 36 - 39, The Slack alert currently includes the full query text via the variable query in the slackPostMessage call (alongside connectionId and collected.reason), which may leak sensitive literals; update the code to sanitize or truncate the query before sending — e.g., implement and call a helper like sanitizeQuery(query) or truncateQuery(query, maxLen) that strips/replaces literal values and/or limits length, then pass the sanitized value to slackPostMessage instead of the raw query; ensure the helper is used wherever the raw query was previously logged.backend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts (2)
45-53: 💤 Low valueError logging in
test.afterswallows the error silently.If cleanup fails, the error is logged but the test suite still appears to pass. Consider re-throwing or using
t.fail()if cleanup is critical, or at minimum ensure this doesn't mask issues in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts` around lines 45 - 53, The cleanup catch block in the test.after hook silences failures (in the block that calls Cacher.clearAllCache(), deletes process.env.CEDAR_AUTHORIZATION_ENABLED, and awaits app.close()), so change the handler to surface failures instead of swallowing them: after logging the error, re-throw it (or remove the try/catch so the thrown error fails the suite); ensure the test.after for saved-query-table-permissions-e2e.test.ts does not swallow exceptions from Cacher.clearAllCache or app.close so CI sees the failure.
19-21: 💤 Low valueMinor:
_testUtilsis declared but unused.The
_testUtilsvariable is retrieved from the module but never used in the tests. If not needed, consider removing it to reduce noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts` around lines 19 - 21, The _testUtils local is declared but never used; remove the unused declaration and any code that retrieves or assigns TestUtils from the testing module (the variable named _testUtils) to eliminate dead code—update the setup/teardown in the file where _testUtils is created (keep INestApplication app and currentTest intact) so no references to _testUtils remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts`:
- Around line 43-48: The loop that calls validateTableRead sequentially should
be converted to run validations in parallel: map tablesToCheck to an array of
Promises by calling validateTableRead(tableName) for each table, await
Promise.all on that array, then inspect results and if any validation returned
falsy throw the existing ForbiddenException using
Messages.NO_READ_PERMISSION_FOR_TABLE with the corresponding tableName; this
keeps the same error behavior but avoids sequential awaits and reduces latency
for large tablesToCheck collections.
- Around line 36-39: The Slack alert currently includes the full query text via
the variable query in the slackPostMessage call (alongside connectionId and
collected.reason), which may leak sensitive literals; update the code to
sanitize or truncate the query before sending — e.g., implement and call a
helper like sanitizeQuery(query) or truncateQuery(query, maxLen) that
strips/replaces literal values and/or limits length, then pass the sanitized
value to slackPostMessage instead of the raw query; ensure the helper is used
wherever the raw query was previously logged.
In
`@backend/src/entities/visualizations/panel/utils/collect-query-tables.util.ts`:
- Around line 71-88: The code parses the SQL twice (tableList() and
collectCteNames() calling parser.astify()); to fix, consolidate parsing by
parsing once and reusing the AST: modify collectCteNames or tableList to accept
an AST (e.g., change collectCteNames signature to accept ast: any or add a new
collectTablesAndCtes(parser: Parser, query: string, dialect: string) that calls
parser.astify() once and traverses the AST to extract both table references and
CTE names, updating callers to use the new API (reference functions
collectCteNames, tableList and parser.astify) so you avoid duplicate
parser.astify() calls.
In
`@backend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts`:
- Around line 45-53: The cleanup catch block in the test.after hook silences
failures (in the block that calls Cacher.clearAllCache(), deletes
process.env.CEDAR_AUTHORIZATION_ENABLED, and awaits app.close()), so change the
handler to surface failures instead of swallowing them: after logging the error,
re-throw it (or remove the try/catch so the thrown error fails the suite);
ensure the test.after for saved-query-table-permissions-e2e.test.ts does not
swallow exceptions from Cacher.clearAllCache or app.close so CI sees the
failure.
- Around line 19-21: The _testUtils local is declared but never used; remove the
unused declaration and any code that retrieves or assigns TestUtils from the
testing module (the variable named _testUtils) to eliminate dead code—update the
setup/teardown in the file where _testUtils is created (keep INestApplication
app and currentTest intact) so no references to _testUtils remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c709ef81-4823-4ed1-9d11-9a855556e80c
📒 Files selected for processing (8)
backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.tsbackend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.tsbackend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.tsbackend/src/entities/visualizations/panel/utils/collect-query-tables.util.tsbackend/src/exceptions/text/messages.tsbackend/test/ava-tests/connection-diagram-preview.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-collect-query-tables.test.tsbackend/test/ava-tests/saas-tests/saas-saved-query-table-permissions-e2e.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests