feat(agents-microservice): migrate AI request endpoints to agents microservice#1841
Merged
Conversation
…roservice - Marked existing AI request endpoints in UserAIRequestsControllerV2 as deprecated, indicating they have moved to the agents microservice. - Introduced new AgentsController to handle AI-related requests, including user token validation, table structure retrieval, and execution of AI queries. - Implemented use cases for validating user tokens, table AI requests, connection edits, and executing raw queries and aggregation pipelines. - Added data structures and DTOs for handling requests and responses in the agents microservice. - Established utility functions for setting up AI connections and asserting user permissions on tables. - Integrated Sentry for error tracking and added streaming capabilities for settings creation.
📝 WalkthroughWalkthroughThis PR introduces a new agents microservice for the application with eight endpoints supporting AI-driven database operations. The implementation includes JWT token validation, permission checking via Cedar, SQL/aggregation query execution, table structure retrieval, and AI-assisted settings scanning with Server-Sent Events streaming. Existing AI endpoints are marked as deprecated and transitioning to this new microservice. ChangesAgents Microservice
Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentsController
participant AuthUseCase as Auth Use Case
participant DataUseCase as Data Query Use Case
participant setupAI as setupAiConnection
participant CedarPermissions
participant DAO as Data Access Object
Client->>AgentsController: POST /internal/agents/auth/validate-user-token
AgentsController->>AuthUseCase: execute(token)
AuthUseCase->>AuthUseCase: verify JWT, check logout, load user
AuthUseCase-->>AgentsController: ValidatedUserTokenRO
AgentsController-->>Client: 200 {sub, email, exp, iat}
Client->>AgentsController: POST /internal/agents/ai/data/:connectionId/table-structure
AgentsController->>DataUseCase: execute(connectionId, userId, tableName, masterPassword)
DataUseCase->>setupAI: decrypt & validate connection
setupAI-->>DataUseCase: AiConnectionSetup (DAO, userEmail)
DataUseCase->>CedarPermissions: improvedCheckTableRead(userId, connectionId, tableName)
CedarPermissions-->>DataUseCase: permission allowed
DataUseCase->>DAO: getTableStructure(tableName, userEmail)
DAO-->>DataUseCase: structure record with FK relations
DataUseCase->>CedarPermissions: check related table permissions
CedarPermissions-->>DataUseCase: permission results per related table
DataUseCase-->>AgentsController: Record<string, unknown>
AgentsController-->>Client: 200 {schema, tables, relationships}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
This PR migrates AI-related request/validation and data-execution endpoints into a new agents-microservice module while keeping legacy AI controllers in place (marked deprecated) until traffic is switched over.
Changes:
- Added an
AgentsController+AgentsModuleexposing internal endpoints for JWT validation, permission checks, connection context, table structure, raw SQL execution, Mongo aggregation execution, and streaming settings scan. - Introduced agents-microservice use cases, DTOs, and response/data structures to support the new endpoints.
- Marked existing AI endpoints/controllers as deprecated and registered the new module in
app.module.ts.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts | Shared helpers for AI connection setup and permission-aware metadata retrieval. |
| backend/src/microservices/agents-microservice/use-cases/validate-user-token.use.case.ts | Validates end-user JWTs (logout check, user existence/suspension, 2FA scope). |
| backend/src/microservices/agents-microservice/use-cases/validate-table-ai-request.use.case.ts | Cedar authorization check for table AI request action. |
| backend/src/microservices/agents-microservice/use-cases/validate-connection-edit.use.case.ts | Cedar authorization check for connection edit action. |
| backend/src/microservices/agents-microservice/use-cases/scan-and-create-settings.use.case.ts | Streaming wrapper around shared AI scan/settings creation job. |
| backend/src/microservices/agents-microservice/use-cases/get-ai-table-structure.use.case.ts | Retrieves permission-aware table structure and related table structures. |
| backend/src/microservices/agents-microservice/use-cases/get-ai-connection-context.use.case.ts | Exposes connection context (type/schema/isMongoDb/userEmail) for agents. |
| backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts | Validates and executes read-only SQL with table-level permission checks. |
| backend/src/microservices/agents-microservice/use-cases/execute-ai-aggregation-pipeline.use.case.ts | Validates and executes read-only Mongo aggregation pipeline with permission checks. |
| backend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.ts | Defines interfaces for the new agents use cases. |
| backend/src/microservices/agents-microservice/dto/agents-auth.dtos.ts | DTOs for internal auth/permission validation endpoints. |
| backend/src/microservices/agents-microservice/dto/agents-ai-data.dtos.ts | DTOs for AI data endpoints (context/structure/query execution/settings scan). |
| backend/src/microservices/agents-microservice/data-structures/agents.ds.ts | Request data structures for agents use cases. |
| backend/src/microservices/agents-microservice/data-structures/agents-responses.ds.ts | Response objects for agents endpoints. |
| backend/src/microservices/agents-microservice/agents.module.ts | Registers controller, use-case providers, and SaaSAuth middleware. |
| backend/src/microservices/agents-microservice/agents.controller.ts | Adds internal routes under internal/agents for agents microservice. |
| backend/src/entities/ai/user-ai-requests-v2.controller.ts | Marks existing AI endpoints as deprecated (moved to agents microservice). |
| backend/src/entities/ai/ai-conversation-history/user-ai-chat.controller.ts | Marks AI chat endpoints as deprecated (moved to agents microservice). |
| backend/src/common/data-injection.tokens.ts | Adds new UseCaseType tokens for agents microservice use cases. |
| backend/src/app.module.ts | Registers the new AgentsModule in the application module imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+32
| const foundConnection = await dbContext.connectionRepository.findAndDecryptConnection( | ||
| connectionId, | ||
| masterPassword as string, | ||
| ); |
Comment on lines
+35
to
+38
| const connection = await this._dbContext.connectionRepository.findAndDecryptConnection( | ||
| connectionId, | ||
| masterPassword as string, | ||
| ); |
| @@ -0,0 +1,38 @@ | |||
| import { ApiProperty } from '@nestjs/swagger'; | |||
| import { IsNotEmpty, IsString } from 'class-validator'; | |||
Comment on lines
+11
to
+20
| export class ValidateTableAiRequestDto { | ||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| userId: string; | ||
|
|
||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| connectionId: string; |
Comment on lines
+28
to
+38
| export class ValidateConnectionEditDto { | ||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| userId: string; | ||
|
|
||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| connectionId: string; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/src/microservices/agents-microservice/data-structures/agents.ds.ts (1)
34-36: ⚖️ Poor tradeoffConsider decoupling the Response object from domain data structures.
Including the Express
Responseobject directly in this data structure couples the domain layer to HTTP infrastructure. While pragmatic for streaming, consider using a generic stream/writer interface instead to improve testability and architectural purity.For example:
interface ProgressWriter { write(chunk: unknown): void; end(): void; }Then adapt Express Response to this interface at the controller boundary.
🤖 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/microservices/agents-microservice/data-structures/agents.ds.ts` around lines 34 - 36, ScanAndCreateSettingsDs currently embeds the Express Response object, coupling the domain to HTTP; replace that dependency by introducing a generic writer interface (e.g., ProgressWriter with write/end methods) and change ScanAndCreateSettingsDs to accept that interface instead of Response; adapt controllers/handlers at the boundary to wrap/convert Express Response into the ProgressWriter implementation and update any usages of ScanAndCreateSettingsDs and AiDataRequestDs consumers to call write/end on the new interface.
🤖 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.
Inline comments:
In `@backend/src/microservices/agents-microservice/dto/agents-ai-data.dtos.ts`:
- Around line 11-14: The masterPassword field is declared as `string | null` but
validated with `@IsString()` and `@IsOptional()`, causing null to fail
validation; change the DTO so `masterPassword` is optional and not nullable
(e.g., `masterPassword?: string` or `string | undefined`) and keep
`@IsOptional()` + `@IsString()` so null is no longer accepted; update the
corresponding data definition in agents.ds.ts to match the non-nullable/optional
type and verify callers such as `setupAiConnection` (which casts to string)
still work without changing runtime casting.
In
`@backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts`:
- Around line 37-53: Replace the fragile string check in isValidSQLQuery with an
AST-based validator: parse the incoming query (before calling
assertUserCanReadQueryTables, before wrapQueryWithLimit and executeRawQuery)
using a SQL parser and validate that the AST contains only SELECT statements,
column references, literals, allowed operators and explicit table-qualified
column expressions; reject any function calls, subqueries in select-list,
system/UDR calls or non-column expressions unless the function is explicitly
whitelisted per engine (use ConnectionTypesEnum to pick the allowlist). Update
the validation call site in execute-ai-raw-query.use.case.ts to run the new AST
validator and return a BadRequestException on disallowed constructs, and augment
or replace the current assertUserCanReadQueryTables usage so it still authorizes
referenced table names (from the AST) but does not assume the SELECT list is
safe. Ensure whitelists are configurable per engine and documented where
isValidSQLQuery (or its replacement) and assertUserCanReadQueryTables are
implemented.
In
`@backend/src/microservices/agents-microservice/use-cases/get-ai-connection-context.use.case.ts`:
- Around line 22-38: The implementation in get-ai-connection-context.use.case.ts
returns connection metadata immediately after setupAiConnection(...) without
verifying the caller's permissions; update the implementation method to call the
existing connection/table authorization flow (reuse the project's connection
authorization helper used elsewhere — e.g., the connection/table auth check
invoked in agents flows) after obtaining foundConnection and before constructing
AiConnectionContextRO, and if the authorization check fails, throw the
appropriate access-denied error; ensure you reference setupAiConnection(...) and
the same authorization function used for table/query execution so only
authorized userId can receive type/schema/isMongoDb/userEmail.
In
`@backend/src/microservices/agents-microservice/use-cases/validate-user-token.use.case.ts`:
- Around line 31-75: The current catch block is too broad and converts
repository failures into 401s; narrow the try/catch to only JWT verification and
claim parsing (the jwt.verify call and extracting data.id/scope/email/exp/iat)
so only token parsing errors are caught and normalized to UnauthorizedException,
then move the user lookup and related checks (calls to
this._dbContext.userRepository.findOneUserById, foundUser.suspended check, and
the addedScope handling that can throw TwoFaRequiredException) outside that try
so DB errors bubble as 5xx (or explicitly wrap repository errors in a 5xx
HttpException), and keep the existing logOutRepository.isLoggedOut usage as-is
outside the JWT parsing catch.
In
`@backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts`:
- Around line 110-152: The returned raw metadata arrays tableForeignKeys and
referencedTableNamesAndColumns must be filtered to remove entries for which the
caller lacks read permission; for each entry in referencedTableNamesAndColumns
and tableForeignKeys call cedarPermissions.improvedCheckTableRead(userId,
foundConnection.id, referenced_table_name or table.table_name) (same check used
when building referencedTablesStructures and foreignTablesStructures) and only
include items that pass the permission check before returning them, so the
response exposes only permission-aware related-table names/columns alongside
referencedTablesStructures and foreignTablesStructures.
---
Nitpick comments:
In `@backend/src/microservices/agents-microservice/data-structures/agents.ds.ts`:
- Around line 34-36: ScanAndCreateSettingsDs currently embeds the Express
Response object, coupling the domain to HTTP; replace that dependency by
introducing a generic writer interface (e.g., ProgressWriter with write/end
methods) and change ScanAndCreateSettingsDs to accept that interface instead of
Response; adapt controllers/handlers at the boundary to wrap/convert Express
Response into the ProgressWriter implementation and update any usages of
ScanAndCreateSettingsDs and AiDataRequestDs consumers to call write/end on the
new interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 573352c8-2005-4d34-864b-e8ad7123c1db
📒 Files selected for processing (20)
backend/src/app.module.tsbackend/src/common/data-injection.tokens.tsbackend/src/entities/ai/ai-conversation-history/user-ai-chat.controller.tsbackend/src/entities/ai/user-ai-requests-v2.controller.tsbackend/src/microservices/agents-microservice/agents.controller.tsbackend/src/microservices/agents-microservice/agents.module.tsbackend/src/microservices/agents-microservice/data-structures/agents-responses.ds.tsbackend/src/microservices/agents-microservice/data-structures/agents.ds.tsbackend/src/microservices/agents-microservice/dto/agents-ai-data.dtos.tsbackend/src/microservices/agents-microservice/dto/agents-auth.dtos.tsbackend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.tsbackend/src/microservices/agents-microservice/use-cases/execute-ai-aggregation-pipeline.use.case.tsbackend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.tsbackend/src/microservices/agents-microservice/use-cases/get-ai-connection-context.use.case.tsbackend/src/microservices/agents-microservice/use-cases/get-ai-table-structure.use.case.tsbackend/src/microservices/agents-microservice/use-cases/scan-and-create-settings.use.case.tsbackend/src/microservices/agents-microservice/use-cases/validate-connection-edit.use.case.tsbackend/src/microservices/agents-microservice/use-cases/validate-table-ai-request.use.case.tsbackend/src/microservices/agents-microservice/use-cases/validate-user-token.use.case.tsbackend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts
Comment on lines
+11
to
+14
| @ApiPropertyOptional() | ||
| @IsOptional() | ||
| @IsString() | ||
| masterPassword?: string | null; |
There was a problem hiding this comment.
Fix validation logic for nullable masterPassword.
The masterPassword field is typed as string | null but decorated with @IsString() and @IsOptional(). When a client explicitly sends { "masterPassword": null }, class-validator's @IsString() will reject the null value because @IsOptional() only skips validation for undefined, not null.
Either:
- Change the type to
string | undefined(removenullfrom the union), or - Add
@ValidateIf((o) => o.masterPassword !== null)before@IsString()to explicitly allownull.
Given the downstream usage in setupAiConnection which casts to string and the optional nature, option 1 is cleaner.
🔧 Proposed fix
`@ApiPropertyOptional`()
`@IsOptional`()
`@IsString`()
- masterPassword?: string | null;
+ masterPassword?: string;And update the corresponding data structure in agents.ds.ts:
- masterPassword: string | null;
+ masterPassword?: string;Then in the controller, change:
- masterPassword: body.masterPassword ?? null
+ masterPassword: body.masterPassword📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @ApiPropertyOptional() | |
| @IsOptional() | |
| @IsString() | |
| masterPassword?: string | null; | |
| `@ApiPropertyOptional`() | |
| `@IsOptional`() | |
| `@IsString`() | |
| masterPassword?: string; |
🤖 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/microservices/agents-microservice/dto/agents-ai-data.dtos.ts`
around lines 11 - 14, The masterPassword field is declared as `string | null`
but validated with `@IsString()` and `@IsOptional()`, causing null to fail
validation; change the DTO so `masterPassword` is optional and not nullable
(e.g., `masterPassword?: string` or `string | undefined`) and keep
`@IsOptional()` + `@IsString()` so null is no longer accepted; update the
corresponding data definition in agents.ds.ts to match the non-nullable/optional
type and verify callers such as `setupAiConnection` (which casts to string)
still work without changing runtime casting.
Comment on lines
+37
to
+53
| if (!isValidSQLQuery(query)) { | ||
| throw new BadRequestException( | ||
| 'Invalid SQL query. Please ensure it is a read-only SELECT statement without any forbidden keywords.', | ||
| ); | ||
| } | ||
|
|
||
| await assertUserCanReadQueryTables({ | ||
| query, | ||
| connectionType: foundConnection.type as ConnectionTypesEnum, | ||
| connectionId: foundConnection.id, | ||
| validateTableRead: (referencedTableName) => | ||
| this.cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, referencedTableName), | ||
| listAllTableNames: async () => (await dataAccessObject.getTablesFromDB()).map((table) => table.tableName), | ||
| }); | ||
|
|
||
| const wrappedQuery = wrapQueryWithLimit(query, foundConnection.type as ConnectionTypesEnum); | ||
| const queryResult = await dataAccessObject.executeRawQuery(wrappedQuery, tableName, userEmail); |
There was a problem hiding this comment.
The read-only gate still allows sensitive DB-side function calls.
isValidSQLQuery(...) only blocks a small keyword list and checks for SELECT ... FROM. Payloads such as SELECT pg_read_file('/etc/passwd') FROM some_allowed_table still satisfy that filter, and assertUserCanReadQueryTables(...) only authorizes table names, not the expressions being executed. That leaves this endpoint able to exfiltrate server- or DB-side secrets through a nominally read-only query.
Please move this endpoint off the string-based validator and onto a parser/allowlist that rejects function calls and other non-column expressions unless they are explicitly whitelisted per engine.
🤖 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/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts`
around lines 37 - 53, Replace the fragile string check in isValidSQLQuery with
an AST-based validator: parse the incoming query (before calling
assertUserCanReadQueryTables, before wrapQueryWithLimit and executeRawQuery)
using a SQL parser and validate that the AST contains only SELECT statements,
column references, literals, allowed operators and explicit table-qualified
column expressions; reject any function calls, subqueries in select-list,
system/UDR calls or non-column expressions unless the function is explicitly
whitelisted per engine (use ConnectionTypesEnum to pick the allowlist). Update
the validation call site in execute-ai-raw-query.use.case.ts to run the new AST
validator and return a BadRequestException on disallowed constructs, and augment
or replace the current assertUserCanReadQueryTables usage so it still authorizes
referenced table names (from the AST) but does not assume the SELECT list is
safe. Ensure whitelists are configurable per engine and documented where
isValidSQLQuery (or its replacement) and assertUserCanReadQueryTables are
implemented.
Comment on lines
+22
to
+38
| protected async implementation(inputData: AiDataRequestDs): Promise<AiConnectionContextRO> { | ||
| const { connectionId, userId, masterPassword } = inputData; | ||
|
|
||
| const { foundConnection, isMongoDb, userEmail } = await setupAiConnection( | ||
| this._dbContext, | ||
| connectionId, | ||
| masterPassword, | ||
| userId, | ||
| ); | ||
|
|
||
| return { | ||
| connectionId: foundConnection.id, | ||
| type: foundConnection.type as string, | ||
| schema: foundConnection.schema ?? null, | ||
| isMongoDb, | ||
| userEmail, | ||
| }; |
There was a problem hiding this comment.
Authorize the caller before returning connection context.
This flow returns connection metadata immediately after setupAiConnection() and never checks whether userId can access the target connection. In the supplied agents flows, table structure and query execution are permission-gated, but this endpoint lets an internal caller enumerate type/schema for any valid connectionId. Please reuse the existing connection/table authorization path before building the response.
🤖 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/microservices/agents-microservice/use-cases/get-ai-connection-context.use.case.ts`
around lines 22 - 38, The implementation in
get-ai-connection-context.use.case.ts returns connection metadata immediately
after setupAiConnection(...) without verifying the caller's permissions; update
the implementation method to call the existing connection/table authorization
flow (reuse the project's connection authorization helper used elsewhere — e.g.,
the connection/table auth check invoked in agents flows) after obtaining
foundConnection and before constructing AiConnectionContextRO, and if the
authorization check fails, throw the appropriate access-denied error; ensure you
reference setupAiConnection(...) and the same authorization function used for
table/query execution so only authorized userId can receive
type/schema/isMongoDb/userEmail.
Comment on lines
+31
to
+75
| const isLoggedOut = await this._dbContext.logOutRepository.isLoggedOut(token); | ||
| if (isLoggedOut) { | ||
| throw new UnauthorizedException('Token is invalid'); | ||
| } | ||
|
|
||
| try { | ||
| const jwtSecret = appConfig.auth.jwtSecret; | ||
| if (!jwtSecret) { | ||
| throw new UnauthorizedException('JWT verification failed'); | ||
| } | ||
| const data = jwt.verify(token, jwtSecret) as jwt.JwtPayload; | ||
| const userId = data.id; | ||
|
|
||
| if (!userId) { | ||
| throw new UnauthorizedException('JWT verification failed'); | ||
| } | ||
|
|
||
| const foundUser = await this._dbContext.userRepository.findOneUserById(userId); | ||
| if (!foundUser) { | ||
| throw new UnauthorizedException('JWT verification failed'); | ||
| } | ||
|
|
||
| if (foundUser.suspended) { | ||
| throw new UnauthorizedException(Messages.ACCOUNT_SUSPENDED); | ||
| } | ||
|
|
||
| const addedScope: Array<JwtScopesEnum> = data.scope; | ||
| if (addedScope && addedScope.length > 0) { | ||
| if (addedScope.includes(JwtScopesEnum.TWO_FA_ENABLE)) { | ||
| throw new TwoFaRequiredException(); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| sub: userId, | ||
| email: data.email ?? null, | ||
| exp: data.exp ?? null, | ||
| iat: data.iat ?? null, | ||
| }; | ||
| } catch (e) { | ||
| Sentry.captureException(e); | ||
| if (e instanceof HttpException) { | ||
| throw e; | ||
| } | ||
| throw new UnauthorizedException(Messages.AUTHORIZATION_REJECTED); |
There was a problem hiding this comment.
Don't collapse repository failures into 401 responses.
Lines 31-75 put userRepository.findOneUserById(...) inside the same catch that normalizes token failures to UnauthorizedException(Messages.AUTHORIZATION_REJECTED), while logOutRepository.isLoggedOut(...) sits outside that catch entirely. A database outage on the user lookup path is therefore reported as an auth rejection instead of a 5xx, and the logout-check path follows a different error path again. Narrow the catch to JWT verification / claim parsing and let repository failures bubble or be wrapped explicitly as server errors.
Suggested direction
- try {
- const jwtSecret = appConfig.auth.jwtSecret;
- if (!jwtSecret) {
- throw new UnauthorizedException('JWT verification failed');
- }
- const data = jwt.verify(token, jwtSecret) as jwt.JwtPayload;
- const userId = data.id;
+ const jwtSecret = appConfig.auth.jwtSecret;
+ if (!jwtSecret) {
+ throw new UnauthorizedException('JWT verification failed');
+ }
+
+ let data: jwt.JwtPayload;
+ try {
+ data = jwt.verify(token, jwtSecret) as jwt.JwtPayload;
+ } catch (e) {
+ Sentry.captureException(e);
+ throw new UnauthorizedException(Messages.AUTHORIZATION_REJECTED);
+ }
+
+ const userId = data.id;
- if (!userId) {
- throw new UnauthorizedException('JWT verification failed');
- }
+ if (!userId) {
+ throw new UnauthorizedException('JWT verification failed');
+ }
- const foundUser = await this._dbContext.userRepository.findOneUserById(userId);
- if (!foundUser) {
- throw new UnauthorizedException('JWT verification failed');
- }
+ const foundUser = await this._dbContext.userRepository.findOneUserById(userId);
+ if (!foundUser) {
+ throw new UnauthorizedException('JWT verification failed');
+ }
-
- ...
- } catch (e) {
- Sentry.captureException(e);
- if (e instanceof HttpException) {
- throw e;
- }
- throw new UnauthorizedException(Messages.AUTHORIZATION_REJECTED);
- }As per coding guidelines, "Ensure all error handling is explicit - use try/catch blocks appropriately".
🤖 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/microservices/agents-microservice/use-cases/validate-user-token.use.case.ts`
around lines 31 - 75, The current catch block is too broad and converts
repository failures into 401s; narrow the try/catch to only JWT verification and
claim parsing (the jwt.verify call and extracting data.id/scope/email/exp/iat)
so only token parsing errors are caught and normalized to UnauthorizedException,
then move the user lookup and related checks (calls to
this._dbContext.userRepository.findOneUserById, foundUser.suspended check, and
the addedScope handling that can throw TwoFaRequiredException) outside that try
so DB errors bubble as 5xx (or explicitly wrap repository errors in a 5xx
HttpException), and keep the existing logOutRepository.isLoggedOut usage as-is
outside the JWT parsing catch.
Source: Coding guidelines
Comment on lines
+110
to
+152
| const [tableStructure, tableForeignKeys, referencedTableNamesAndColumns] = await Promise.all([ | ||
| dao.getTableStructure(tableName, userEmail), | ||
| dao.getTableForeignKeys(tableName, userEmail), | ||
| dao.getReferencedTableNamesAndColumns(tableName, userEmail), | ||
| ]); | ||
|
|
||
| const referencedTablesStructures = []; | ||
| const structurePromises = referencedTableNamesAndColumns.flatMap((referencedTable) => | ||
| referencedTable.referenced_by.map(async (table) => { | ||
| const canRead = await cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, table.table_name); | ||
| if (!canRead) { | ||
| return null; | ||
| } | ||
| const structure = await dao.getTableStructure(table.table_name, userEmail); | ||
| return { tableName: table.table_name, structure }; | ||
| }), | ||
| ); | ||
| referencedTablesStructures.push(...(await Promise.all(structurePromises)).filter((item) => item !== null)); | ||
|
|
||
| const foreignTablesStructures = []; | ||
| const foreignTablesStructurePromises = tableForeignKeys.map(async (foreignKey) => { | ||
| const canRead = await cedarPermissions.improvedCheckTableRead( | ||
| userId, | ||
| foundConnection.id, | ||
| foreignKey.referenced_table_name, | ||
| ); | ||
| if (!canRead) { | ||
| return null; | ||
| } | ||
| const structure = await dao.getTableStructure(foreignKey.referenced_table_name, userEmail); | ||
| return { tableName: foreignKey.referenced_table_name, structure }; | ||
| }); | ||
| foreignTablesStructures.push(...(await Promise.all(foreignTablesStructurePromises)).filter((item) => item !== null)); | ||
|
|
||
| return { | ||
| tableStructure, | ||
| tableName, | ||
| schema: foundConnection.schema || null, | ||
| tableForeignKeys, | ||
| referencedTableNamesAndColumns, | ||
| referencedTablesStructures, | ||
| foreignTablesStructures, | ||
| }; |
There was a problem hiding this comment.
Filter unauthorized related-table metadata out of the response.
The permission checks here only gate referencedTablesStructures and foreignTablesStructures. The raw tableForeignKeys and referencedTableNamesAndColumns objects are still returned unchanged on Lines 148-149, so a user who can read tableName but not a related table still learns that table’s name and relationship/column metadata. That breaks the “permission-aware” contract of this endpoint.
Suggested direction
- const [tableStructure, tableForeignKeys, referencedTableNamesAndColumns] = await Promise.all([
+ const [tableStructure, rawTableForeignKeys, rawReferencedTableNamesAndColumns] = await Promise.all([
dao.getTableStructure(tableName, userEmail),
dao.getTableForeignKeys(tableName, userEmail),
dao.getReferencedTableNamesAndColumns(tableName, userEmail),
]);
+
+ const tableForeignKeys = (
+ await Promise.all(
+ rawTableForeignKeys.map(async (foreignKey) =>
+ (await cedarPermissions.improvedCheckTableRead(
+ userId,
+ foundConnection.id,
+ foreignKey.referenced_table_name,
+ ))
+ ? foreignKey
+ : null,
+ ),
+ )
+ ).filter((foreignKey) => foreignKey !== null);
+
+ const referencedTableNamesAndColumns = (
+ await Promise.all(
+ rawReferencedTableNamesAndColumns.map(async (entry) => {
+ const referencedBy = (
+ await Promise.all(
+ entry.referenced_by.map(async (table) =>
+ (await cedarPermissions.improvedCheckTableRead(
+ userId,
+ foundConnection.id,
+ table.table_name,
+ ))
+ ? table
+ : null,
+ ),
+ )
+ ).filter((table) => table !== null);
+
+ return referencedBy.length > 0 ? { ...entry, referenced_by: referencedBy } : null;
+ }),
+ )
+ ).filter((entry) => entry !== null);🤖 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/microservices/agents-microservice/utils/ai-data-access.helpers.ts`
around lines 110 - 152, The returned raw metadata arrays tableForeignKeys and
referencedTableNamesAndColumns must be filtered to remove entries for which the
caller lacks read permission; for each entry in referencedTableNamesAndColumns
and tableForeignKeys call cedarPermissions.improvedCheckTableRead(userId,
foundConnection.id, referenced_table_name or table.table_name) (same check used
when building referencedTablesStructures and foreignTablesStructures) and only
include items that pass the permission check before returning them, so the
response exposes only permission-aware related-table names/columns alongside
referencedTablesStructures and foreignTablesStructures.
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
Release Notes
New Features
Deprecations