feature : external oauth#6292
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for external OAuth 2.0 authentication (specifically targeting Okta) by allowing Flowise to act as a resource server. It includes new database entities for managing integrations and federated accounts, CRUD controllers, and middleware to validate JWT access tokens. Feedback focuses on security and performance: a potential IDOR vulnerability was identified where workspaceId is sourced from the request body, and several performance optimizations were suggested, including reducing redundant database saves during authentication, eager loading related entities to avoid queries within loops, lowering OIDC discovery timeouts, and caching subscription-related metadata.
| const row = await externalOAuthIntegrationService.create(app.AppDataSource, { | ||
| name: body.name, | ||
| issuerUrl: body.issuerUrl, | ||
| audiences: body.audiences, | ||
| allowedClientIds: body.allowedClientIds ?? null, | ||
| permissionScopeMap: body.permissionScopeMap || {}, | ||
| organizationId: orgId, | ||
| workspaceId: body.workspaceId, | ||
| customPermissionsClaimName: body.customPermissionsClaimName, | ||
| enabled: body.enabled | ||
| }) |
There was a problem hiding this comment.
The workspaceId is being sourced from the request body. According to repository security rules, sensitive fields like workspaceId must be set on the server from a trusted source (such as the user's session or organization context) rather than being passed from the client to prevent IDOR vulnerabilities.
References
- Sensitive fields like workspaceId must be set on the server from a trusted source (e.g., user session), not from the client request body, to prevent IDOR vulnerabilities.
| if (existing) { | ||
| existing.email = params.email ?? existing.email | ||
| existing.userId = params.userId ?? existing.userId | ||
| existing.organizationId = params.organizationId | ||
| existing.workspaceId = params.workspaceId | ||
| await repo.save(existing) | ||
| return |
There was a problem hiding this comment.
Performing a database save operation on every successful authentication request will significantly impact API performance. Since most authentication events for the same user won't change the federated account details, you should check if the existing record actually needs updating (e.g., if the email has changed) before calling repo.save. Alternatively, consider throttling updates to a 'last login' timestamp.
| if (existing) { | |
| existing.email = params.email ?? existing.email | |
| existing.userId = params.userId ?? existing.userId | |
| existing.organizationId = params.organizationId | |
| existing.workspaceId = params.workspaceId | |
| await repo.save(existing) | |
| return | |
| if (existing) { | |
| const hasChanges = (params.email && existing.email !== params.email) || (params.userId && existing.userId !== params.userId); | |
| if (hasChanges) { | |
| existing.email = params.email ?? existing.email; | |
| existing.userId = params.userId ?? existing.userId; | |
| existing.organizationId = params.organizationId; | |
| existing.workspaceId = params.workspaceId; | |
| await repo.save(existing); | |
| } | |
| return; | |
| } |
| const workspace = await dataSource.getRepository(Workspace).findOne({ | ||
| where: { id: integration.workspaceId } | ||
| }) | ||
| if (!workspace || workspace.organizationId !== integration.organizationId) { | ||
| logger.warn(`[external-oauth]: Invalid workspace binding for integration ${integration.id}`) | ||
| continue | ||
| } | ||
| const org = await dataSource.getRepository(Organization).findOne({ | ||
| where: { id: integration.organizationId } | ||
| }) | ||
| if (!org) { | ||
| logger.warn(`[external-oauth]: Organization missing for integration ${integration.id}`) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Executing separate database queries for Workspace and Organization inside a loop on every authenticated request is inefficient. These entities should be eagerly loaded or joined during the initial findEnabledByIssuerUrl call to reduce the number of round-trips to the database.
|
|
||
| const url = wellKnownUrl(issuerUrl) | ||
| try { | ||
| const { data } = await axios.get<OidcMetadata>(url, { timeout: 10000 }) |
There was a problem hiding this comment.
A 10-second timeout for an OIDC discovery request is quite long for a blocking authentication middleware. If the external provider is slow or unresponsive, it will hang the client request. Consider reducing this timeout to a more aggressive value (e.g., 2-3 seconds) and ensure the failure is handled gracefully.
| const { data } = await axios.get<OidcMetadata>(url, { timeout: 10000 }) | |
| const { data } = await axios.get<OidcMetadata>(url, { timeout: 3000 }) |
| features = await identityManager.getFeaturesByPlan(subscriptionId) | ||
| } | ||
| const productId = await identityManager.getProductIdFromSubscription(subscriptionId) |
There was a problem hiding this comment.
The calls to getFeaturesByPlan and getProductIdFromSubscription are performed on every request. These values are typically static for a given organization/subscription and should be cached to avoid unnecessary overhead during the authentication process.
1 participant
No description provided.