-
Notifications
You must be signed in to change notification settings - Fork 11.5k
feat: OAuth2 controller api v2 + refactor oAuth Trpc handlers #25989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: OAuth2 controller api v2 + refactor oAuth Trpc handlers #25989
Conversation
- Add new OAuth2 module under /auth with controller, service, repository, and DTOs - Implement skeleton endpoints: - GET /v2/auth/oauth2/clients/:clientId - Get OAuth client info - POST /v2/auth/oauth2/clients/:clientId/authorize - Generate authorization code - POST /v2/auth/oauth2/clients/:clientId/exchange - Exchange code for tokens - POST /v2/auth/oauth2/clients/:clientId/refresh - Refresh access token - Create input DTOs for authorize, exchange, and refresh operations - Create output DTOs for client info, authorization code, and tokens - Register OAuth2Module in endpoints.module.ts Co-Authored-By: morgan@cal.com <morgan@cal.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
…th2 controller - Add OAuthService export to platform-libraries - Refactor OAuth2Service to use OAuthService.validateClient() and OAuthService.verifyPKCE() - Remove duplicate local implementations of validateClient and verifyPKCE methods Co-Authored-By: morgan@cal.com <morgan@cal.com>
- Remove OAuthService export from platform-libraries (will be deprecated) - Reimplement validateClient and verifyPKCE methods locally in OAuth2Service - Use verifyCodeChallenge and generateSecret from platform-libraries directly Co-Authored-By: morgan@cal.com <morgan@cal.com>
… teams - Create AccessCodeRepository for access code Prisma operations - Add findTeamBySlugWithAdminRole method to TeamsRepository - Move generateAuthorizationCode, createTokens, verifyRefreshToken to OAuth2Service - Update OAuth2Repository to only contain OAuth client operations - Update OAuth2Module to import TeamsModule and provide AccessCodeRepository Addresses PR review comments to properly separate concerns Co-Authored-By: morgan@cal.com <morgan@cal.com>
- Implement createTokens method using jsonwebtoken to sign access and refresh tokens - Implement verifyRefreshToken method to verify JWT refresh tokens - Add ConfigService injection for CALENDSO_ENCRYPTION_KEY access - Import ConfigModule in OAuth2Module for dependency injection Based on logic from apps/web/app/api/auth/oauth/token/route.ts and apps/web/app/api/auth/oauth/refreshToken/route.ts Co-Authored-By: morgan@cal.com <morgan@cal.com>
…r OAuth2 - Add @UseGuards(ApiAuthGuard) to getClient endpoint for authentication - Add comprehensive e2e tests for all OAuth2 endpoints: - GET /v2/auth/oauth2/clients/:clientId - POST /v2/auth/oauth2/clients/:clientId/authorize - POST /v2/auth/oauth2/clients/:clientId/exchange - POST /v2/auth/oauth2/clients/:clientId/refresh - Tests cover happy paths and error cases (invalid client, invalid code, etc.) Co-Authored-By: morgan@cal.com <morgan@cal.com>
Co-Authored-By: morgan@cal.com <morgan@cal.com>
hbjORbj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff
This comment was marked as resolved.
This comment was marked as resolved.
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
supalarry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review:
| providers: [PrismaAccessCodeRepository, PrismaOAuthClientRepository, PrismaTeamRepository, OAuthService], | ||
| exports: [OAuthService], | ||
| }) | ||
| export class oAuthServiceModule {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the new code for oauth module, but we have the controllers related to platform oauth:
- 'apps/api/v2/src/modules/oauth-clients/controllers/oauth-clients/oauth-clients.controller.ts'
- 'apps/api/v2/src/modules/oauth-clients/controllers/oauth-flow/oauth-flow.controller.ts'
and then services and repositories they call all point to platform oauth clients.
Which is why I think we need to rename controllers above, their services and repositories with a "platform" prefix not only in file names but also class names with "Platform", because otherwise it will be confusing to find files and then code within them.
apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.e2e-spec.ts
Outdated
Show resolved
Hide resolved
apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.ts
Outdated
Show resolved
Hide resolved
apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts
Outdated
Show resolved
Hide resolved
|
|
||
| import { SUCCESS_STATUS, ERROR_STATUS } from "@calcom/platform-constants"; | ||
|
|
||
| export class OAuth2ClientDto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also return whether the oauth client is confidential or public under a new "type" property ?
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
- Remove unused GetOAuth2ClientInput class - Change API tag from 'Auth / OAuth2' to 'OAuth2' - Move OAuthClientFixture to separate fixture file (oauth2-client.repository.fixture.ts) - Add 'type' property to OAuth2ClientDto for confidential/public client type - Rename 'clientId' to 'id' in OAuth2ClientDto (using @expose mapping) - Clean up duplicate provider declarations in oauth2.module.ts - Update e2e tests to use new fixture and verify type property Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 44 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/oauth/services/OAuthService.ts">
<violation number="1" location="packages/features/oauth/services/OAuthService.ts:193">
P2: Property mismatch: errors are thrown with `{ reason: "..." }` but the handler reads `error.data?.cause`. This causes detailed error descriptions to be lost, always falling back to the generic OAuth error message.</violation>
</file>
<file name="packages/features/ee/teams/repositories/TeamRepository.ts">
<violation number="1" location="packages/features/ee/teams/repositories/TeamRepository.ts:609">
P2: Consider adding `accepted: true` to the membership filter. Without this check, users with pending admin invitations (who haven't accepted yet) could be treated as having admin access.</violation>
</file>
<file name="apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.ts">
<violation number="1" location="apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.ts:102">
P1: Potential open redirect: If `getClient` throws an unexpected error (e.g., database error), the code redirects to the unvalidated `body.redirectUri`. Per OAuth 2.0 spec, invalid/unverified redirect URIs should not receive redirects. Consider throwing an HTTP exception for all errors from `getClient` instead of redirecting to an unvalidated URI.</violation>
</file>
<file name="apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts">
<violation number="1" location="apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts:14">
P2: Swagger documentation will show `id` as the property name, but the actual JSON output will use `clientId` due to `@Expose({ name: "clientId" })`. Add `name: "clientId"` to the `@ApiProperty` decorator to align documentation with runtime behavior.</violation>
<violation number="2" location="apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts:56">
P2: Swagger documentation will show `type` as the property name, but the actual JSON output will use `clientType` due to `@Expose({ name: "clientType" })`. Add `name: "clientType"` to the `@ApiProperty` decorator to align documentation with runtime behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new HttpException(err.message, statusCode); | ||
| } | ||
| } | ||
| const errorRedirectUrl = this.oAuthService.buildErrorRedirectUrl(body.redirectUri, err, body.state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Potential open redirect: If getClient throws an unexpected error (e.g., database error), the code redirects to the unvalidated body.redirectUri. Per OAuth 2.0 spec, invalid/unverified redirect URIs should not receive redirects. Consider throwing an HTTP exception for all errors from getClient instead of redirecting to an unvalidated URI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.ts, line 102:
<comment>Potential open redirect: If `getClient` throws an unexpected error (e.g., database error), the code redirects to the unvalidated `body.redirectUri`. Per OAuth 2.0 spec, invalid/unverified redirect URIs should not receive redirects. Consider throwing an HTTP exception for all errors from `getClient` instead of redirecting to an unvalidated URI.</comment>
<file context>
@@ -0,0 +1,168 @@
+ throw new HttpException(err.message, statusCode);
+ }
+ }
+ const errorRedirectUrl = this.oAuthService.buildErrorRedirectUrl(body.redirectUri, err, body.state);
+ return res.redirect(303, errorRedirectUrl);
+ }
</file context>
| if (validOAuthErrors.includes(error.message)) { | ||
| return { | ||
| error: error.message, | ||
| errorDescription: (error.data?.cause as string | undefined) ?? error.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Property mismatch: errors are thrown with { reason: "..." } but the handler reads error.data?.cause. This causes detailed error descriptions to be lost, always falling back to the generic OAuth error message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/oauth/services/OAuthService.ts, line 193:
<comment>Property mismatch: errors are thrown with `{ reason: "..." }` but the handler reads `error.data?.cause`. This causes detailed error descriptions to be lost, always falling back to the generic OAuth error message.</comment>
<file context>
@@ -1,61 +1,454 @@
+ if (validOAuthErrors.includes(error.message)) {
+ return {
+ error: error.message,
+ errorDescription: (error.data?.cause as string | undefined) ?? error.message,
+ };
+ }
</file context>
| where: { | ||
| slug: teamSlug, | ||
| members: { | ||
| some: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Consider adding accepted: true to the membership filter. Without this check, users with pending admin invitations (who haven't accepted yet) could be treated as having admin access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/teams/repositories/TeamRepository.ts, line 609:
<comment>Consider adding `accepted: true` to the membership filter. Without this check, users with pending admin invitations (who haven't accepted yet) could be treated as having admin access.</comment>
<file context>
@@ -599,4 +599,21 @@ export class TeamRepository {
+ where: {
+ slug: teamSlug,
+ members: {
+ some: {
+ userId,
+ role: {
</file context>
| enum: OAuthClientType, | ||
| }) | ||
| @IsEnum(OAuthClientType) | ||
| @Expose({ name: "clientType" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Swagger documentation will show type as the property name, but the actual JSON output will use clientType due to @Expose({ name: "clientType" }). Add name: "clientType" to the @ApiProperty decorator to align documentation with runtime behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts, line 56:
<comment>Swagger documentation will show `type` as the property name, but the actual JSON output will use `clientType` due to `@Expose({ name: "clientType" })`. Add `name: "clientType"` to the `@ApiProperty` decorator to align documentation with runtime behavior.</comment>
<file context>
@@ -0,0 +1,74 @@
+ enum: OAuthClientType,
+ })
+ @IsEnum(OAuthClientType)
+ @Expose({ name: "clientType" })
+ type!: OAuthClientType;
+}
</file context>
| example: "clxxxxxxxxxxxxxxxx", | ||
| }) | ||
| @IsString() | ||
| @Expose({ name: "clientId" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Swagger documentation will show id as the property name, but the actual JSON output will use clientId due to @Expose({ name: "clientId" }). Add name: "clientId" to the @ApiProperty decorator to align documentation with runtime behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/modules/auth/oauth2/outputs/oauth2-client.output.ts, line 14:
<comment>Swagger documentation will show `id` as the property name, but the actual JSON output will use `clientId` due to `@Expose({ name: "clientId" })`. Add `name: "clientId"` to the `@ApiProperty` decorator to align documentation with runtime behavior.</comment>
<file context>
@@ -0,0 +1,74 @@
+ example: "clxxxxxxxxxxxxxxxx",
+ })
+ @IsString()
+ @Expose({ name: "clientId" })
+ id!: string;
+
</file context>
Fixes #26470
What does this PR do?
Adds a new OAuth2 controller for API v2 to support migrating OAuth endpoints from Next.js to the NestJS API. This includes fully functional implementations of client validation, PKCE verification, and JWT token generation/verification.
New endpoints created:
GET /v2/auth/oauth2/clients/:clientId- Returns OAuth client info (mirrorsgetClient.handler.ts)POST /v2/auth/oauth2/clients/:clientId/authorize- Generates authorization code and redirects (mirrorsgenerateAuthCode.handler.ts)POST /v2/auth/oauth2/clients/:clientId/exchange- Exchanges code for tokens (mirrorstoken/route.ts)POST /v2/auth/oauth2/clients/:clientId/refresh- Refreshes access token (mirrorsrefreshToken/route.ts)Files created:
apps/api/v2/src/modules/auth/oauth2/controllers/oauth2.controller.tspackages/features/oauth/services/OAuthService.ts(shared for tRPC and API v2)packages/features/oauth/repositories/OAuthClientRepository.ts,AccessCodeRepository.tsauthorize.input.ts,exchange.input.ts,refresh.input.tsoauth2-client.output.ts,oauth2-tokens.output.tsoauth2.module.tsoauth2.controller.e2e-spec.tspackages/features/oauth/di/(tokens, modules, container)Updates since last revision
Latest changes (PR comment fixes):
GetOAuth2ClientInputclass@ApiTags("Auth / OAuth2")to@ApiTags("OAuth2")for cleaner API organizationOAuthClientFixtureto separate fixture file (test/fixtures/repository/oauth2-client.repository.fixture.ts)typeproperty toOAuth2ClientDtoto expose confidential/public client typeclientIdtoidinOAuth2ClientDtooutput (using@Expose({ name: "clientId" })mapping)oauth2.module.tstypepropertyPrevious changes (tRPC handler fix):
generateAuthCode.handler.tsto check forErrorWithCodeinstead ofHttpError- the handler was still checking forHttpErrorin its catch block, butOAuthServicenow throwsErrorWithCode, causing error messages to be lost and replaced with "server_error"Earlier updates:
HttpErrorwithErrorWithCodethroughout OAuth files for consistent error handlingOAuthService.tsto throwErrorWithCodewith appropriateErrorCodeenum valuesredirectUria required input parameter (exact match validation)token_typefield name mismatch inDecodedRefreshTokeninterface@UseGuards(ApiAuthGuard)togetClientandauthorizeendpointsMandatory Tasks (DO NOT REMOVE)
How should this be tested?
TZ=UTC yarn jest --config ./jest-e2e.ts --testPathPattern="oauth2.controller.e2e-spec"inapps/api/v2TZ=UTC yarn vitest run packages/trpc/server/routers/viewer/oAuth/generateAuthCode.handler.test.tscodeandstateparamserroranderror_descriptionquery paramsgetOAuthService()frompackages/features/oauth/di/OAuthService.container.tsreturns a properly instantiated serviceChecklist
Human Review Checklist
getClient.handler.tsstill checks forHttpErrorbutOAuthServicenow throwsErrorWithCode- verify this doesn't cause error messages to be lost (may need same fix asgenerateAuthCode.handler.ts)getTokenmock approach (mocksnext-auth/jwtforApiAuthStrategy)redirectUriis required and exact match validation works correctlyconsole.logstatements remain in production codeas unknown as PrismaClienttype casting inprisma-access-code.repository.tsandprisma-oauth-client.repository.tsErrorWithCodeis properly exported from platform-libraries and error handling correctly mapsErrorCodeto HTTP status codes viagetHttpStatusCodemapErrorToOAuthErrorcorrectly handlesErrorWithCodeinstances (checkserror.codeinstead oferror.statusCode)@ApiExcludeController(true)correctly hides endpoints from Swagger docs@Expose({ name: "clientId" })correctly mapsidproperty toclientIdin JSON outputtypeproperty correctly exposesclientTypeenum value in OAuth2ClientDtoLink to Devin run: https://app.devin.ai/sessions/35eff6e6fcb340eea1ecaec74bf006f7
Requested by: Volnei Munhoz (volnei@cal.com) (@volnei)