Conversation
- Add OAuth module with dynamic provider loading mechanism - Implement OAuth service with plugin and npm package loading support - Add OAuth types and base provider class for extensibility - Create user OAuth connection model for account linking - Add OAuth routes: providers list, login redirect, callback handling - Implement user synchronization logic (existing user binding, email matching, new user creation) - Add comprehensive unit tests for OAuth service and user login methods - Add end-to-end tests covering full OAuth flow and error scenarios - Include example OAuth provider implementation and documentation - Update environment configuration with OAuth settings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix unknown error type handling in OAuth service methods - Update controller method return types from Response to void - Add proper type guards for error instanceof Error checks - Ensure consistent error handling patterns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix cookie handling in e2e tests with proper array type checking - Update OAuth service test mocks to use correct environment variable names - Fix nullable field handling in OAuthUserInfo type definitions - Update test expectations to match actual OAuth service method signatures - Resolve private method access issues in users service tests
WalkthroughA comprehensive OAuth authentication system was integrated into the backend, including a new database model for user OAuth connections, a dynamic OAuth provider loading service, DTOs, controller routes, and extensive documentation. The system supports multiple OAuth providers, secure token handling, user synchronization, and is fully covered by unit and end-to-end tests. Documentation and sample environment variables were also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant UsersController
participant OAuthService
participant OAuthProvider
participant UsersService
participant DB
Frontend->>UsersController: GET /auth/oauth/providers
UsersController->>OAuthService: getProvidersConfig()
OAuthService-->>UsersController: providers list
UsersController-->>Frontend: providers list
Frontend->>UsersController: GET /auth/oauth/login/:providerId?state=...
UsersController->>OAuthService: generateAuthorizationUrl(providerId, state)
OAuthService->>OAuthProvider: getAuthorizationUrl(state)
OAuthProvider-->>OAuthService: auth URL
OAuthService-->>UsersController: auth URL
UsersController-->>Frontend: HTTP 302 Redirect to provider auth URL
Frontend->>OAuthProvider: User authenticates, provider redirects to callback
OAuthProvider-->>Frontend: Redirect to /auth/oauth/callback/:providerId?code=...&state=...
Frontend->>UsersController: GET /auth/oauth/callback/:providerId?code=...&state=...
UsersController->>OAuthService: handleCallback(providerId, code, state)
OAuthService->>OAuthProvider: handleCallback(code, state)
OAuthProvider-->>OAuthService: access_token
OAuthService->>OAuthProvider: getUserInfo(access_token)
OAuthProvider-->>OAuthService: userInfo
OAuthService-->>UsersController: userInfo
UsersController->>UsersService: loginWithOAuth(providerId, userInfo, ...)
UsersService->>DB: Find or create user, bind OAuth connection
DB-->>UsersService: user record
UsersService-->>UsersController: user DTO, session token
UsersController-->>Frontend: Redirect to frontend success page with token, user info
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #446 +/- ##
==========================================
- Coverage 88.28% 88.12% -0.17%
==========================================
Files 74 77 +3
Lines 3313 3629 +316
Branches 489 560 +71
==========================================
+ Hits 2925 3198 +273
- Misses 368 411 +43
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
src/auth/oauth/oauth.module.ts (1)
21-34: Well-designed dynamic module registration.The static
registermethod follows NestJS dynamic module patterns correctly:
- Accepts optional providers array
- Creates unique provider tokens to avoid conflicts
- Properly exports the OAuth service
- Imports necessary ConfigModule
Consider adding error boundary handling in the initialization phase:
async onModuleInit() { + try { await this.oauthService.initialize(); + } catch (error) { + // Log initialization errors appropriately + throw error; + } }src/users/DTO/oauth.dto.ts (1)
40-42: Appropriate DTO inheritance pattern.The OAuth user DTO correctly extends the base UserDto and adds the optional email field. The nullable email type properly handles cases where OAuth providers might not provide email information.
Consider adding JSDoc comments for better documentation:
+/** + * OAuth user DTO that extends UserDto with email field + * Email can be null if OAuth provider doesn't provide it + */ export class OAuthUserDto extends UserDto { email?: string | null; }src/auth/oauth/oauth.prisma (1)
6-11: Use English comments for consistency.Replace Chinese comments with English equivalents to maintain consistency across the codebase.
- providerId String @map("provider_id") // OAuth提供商ID，如 'ruc', 'google' - providerUserId String @map("provider_user_id") // 提供商侧用户唯一标识 + providerId String @map("provider_id") // OAuth provider ID, e.g., 'ruc', 'google' + providerUserId String @map("provider_user_id") // User's unique identifier from provider /// [rawProfileType] - rawProfile Json? @map("raw_profile") // 原始用户信息(JSON) - refreshToken String? @map("refresh_token") // 可选，OAuth长效令牌 - tokenExpires DateTime? @map("token_expires") // 可选，OAuth令牌过期时间 + rawProfile Json? @map("raw_profile") // Raw user info from provider (JSON) + refreshToken String? @map("refresh_token") // Optional, OAuth refresh token + tokenExpires DateTime? @map("token_expires") // Optional, OAuth token expiration timeplugins/oauth/example.js (3)
14-16: Use English comments for consistency.Replace Chinese comments with English equivalents.
- authorizationUrl: 'https://example.com/oauth/authorize', // 替换为实际的授权 URL - tokenUrl: 'https://example.com/oauth/token', // 替换为实际的 token URL - scope: ['read:user', 'user:email'], // 替换为实际需要的权限 + authorizationUrl: 'https://example.com/oauth/authorize', // Replace with actual authorization URL + tokenUrl: 'https://example.com/oauth/token', // Replace with actual token URL + scope: ['read:user', 'user:email'], // Replace with required scopes
58-62: Use optional chaining for safer access.Improve null safety by using optional chaining as suggested by static analysis.
- if (response.data && response.data.access_token) { - return response.data.access_token; + if (response.data?.access_token) { + return response.data.access_token;
70-70: Use English comments for URL placeholders.- const response = await axios.get('https://example.com/api/user', { // 替换为实际的用户信息 API + const response = await axios.get('https://example.com/api/user', { // Replace with actual user info APIAlso applies to: 79-79
prisma/schema.prisma (1)
202-207: Use English comments for consistency.- providerId String @map("provider_id") // OAuth提供商ID，如 'ruc', 'google' - providerUserId String @map("provider_user_id") // 提供商侧用户唯一标识 + providerId String @map("provider_id") // OAuth provider ID, e.g., 'ruc', 'google' + providerUserId String @map("provider_user_id") // User's unique identifier from provider /// [rawProfileType] - rawProfile Json? @map("raw_profile") // 原始用户信息(JSON) - refreshToken String? @map("refresh_token") // 可选，OAuth长效令牌 - tokenExpires DateTime? @map("token_expires") // 可选，OAuth令牌过期时间 + rawProfile Json? @map("raw_profile") // Raw user info from provider (JSON) + refreshToken String? @map("refresh_token") // Optional, OAuth refresh token + tokenExpires DateTime? @map("token_expires") // Optional, OAuth token expiration timesrc/users/users.service.ts (1)
741-763: Consider using optional chaining for cleaner code.The method correctly handles placeholder emails for OAuth users. However, the conditional check can be simplified.
Apply this diff to use optional chaining:
- // 检查是否是占位符email，如果是则返回null - const email = - user.email && user.email.endsWith('@placeholder.internal') - ? null - : user.email; + // 检查是否是占位符email，如果是则返回null + const email = user.email?.endsWith('@placeholder.internal') + ? null + : user.email;src/auth/oauth/oauth.service.ts (1)
197-221: Consider making the npm package namespace configurable.The npm package namespace is currently hardcoded to
@sageseekersociety/cheese-auth-. For better flexibility across different deployments or organizations, consider making this configurable.Add a configuration option for the npm namespace:
private async tryLoadFromNpm( providerId: string, config: OAuthProviderConfig, ): Promise<OAuthProvider | null> { - const packageName = `@sageseekersociety/cheese-auth-${providerId}-oauth-provider`; + const npmNamespace = this.configService.get<string>('OAUTH_NPM_NAMESPACE') || '@sageseekersociety/cheese-auth'; + const packageName = `${npmNamespace}-${providerId}-oauth-provider`;test/user.e2e-spec.ts (1)
1394-1416: Remove debug console.log statements from tests.Debug logging statements should be removed before merging to keep test output clean.
Remove the console.log statements:
- // Add debug logging to verify email matching - console.log(`Test: regularUser email = "${regularUser.email}"`); - console.log(`Test: regularUser userId = ${regularUser.userId}`); - // Override OAuth mocks to return the regularUser's email const oauthService = app.get(OAuthService); if (oauthService) { jest .spyOn(oauthService, 'handleCallback') .mockResolvedValue('mock_access_token_existing'); jest.spyOn(oauthService, 'getUserInfo').mockImplementation(async () => { const userInfo = { id: `oauth-binding-test-${Date.now()}`, // Use unique ID to avoid conflicts email: regularUser.email, // Use the exact email from regularUser name: 'OAuth Existing User', username: 'oauthexisting', preferredUsername: 'oauthexisting', }; - console.log( - `Mock: OAuth getUserInfo returning email = "${userInfo.email}"`, - ); return userInfo; }); }src/users/users.controller.ts (1)
1261-1348: Well-implemented OAuth callback handlerThe OAuth callback implementation is comprehensive with proper error handling, logging, and security considerations. The cookie configuration with
sameSite: 'lax'is appropriate for OAuth flows.Consider documenting that the JWT token is passed via URL query parameter (line 1318) for security-conscious deployments. While this is a common pattern for SPAs, some organizations may prefer alternative approaches like temporary server-side storage with a one-time code exchange.
src/auth/oauth/oauth.types.ts (1)
1-6: Update author attributionPlease replace "Claude Assistant" with the actual contributor's name and email.
-* Author(s): -* Claude Assistant +* Author(s): +* Your Name <your.email@example.com>docs/oauth.md (1)
1-5: Clean up informal introductionThe beginning of the documentation reads like conversation notes rather than formal documentation. Consider removing or rewriting the informal introduction.
-明白了。我将研究如何将cheese-auth仓库中的OAuth认证功能（特别是你们学校的OAuth登录支持）作为可选的登录方式整合回cheese-backend仓库中，并确保其不会破坏现有的认证逻辑。 - -我会仔细分析cheese-auth中OAuth相关的模块、依赖和接口，评估其如何在cheese-backend中以模块化方式集成，包括必要的中间件、配置、用户信息同步等部分。完成后会整理成一份清晰的迁移方案供你参考。 - - +# OAuth Integration Guide for Cheese-Backend + +This document describes how to integrate OAuth authentication functionality from the cheese-auth repository into cheese-backend as an optional login method, while preserving existing authentication flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/oauth-implementation-guide.md(1 hunks)docs/oauth.md(1 hunks)plugins/oauth/example.js(1 hunks)prisma/schema.prisma(2 hunks)sample.env(1 hunks)src/auth/auth.module.ts(2 hunks)src/auth/oauth/oauth.module.ts(1 hunks)src/auth/oauth/oauth.prisma(1 hunks)src/auth/oauth/oauth.service.spec.ts(1 hunks)src/auth/oauth/oauth.service.ts(1 hunks)src/auth/oauth/oauth.types.ts(1 hunks)src/users/DTO/oauth.dto.ts(1 hunks)src/users/users.controller.ts(6 hunks)src/users/users.prisma(2 hunks)src/users/users.service.spec.ts(1 hunks)src/users/users.service.ts(4 hunks)test/user.e2e-spec.ts(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/oauth/example.js
[error] 59-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/users/users.service.ts
[error] 759-764: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1707-1708: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
docs/oauth.md
[uncategorized] ~91-~91: 您的意思是“"不"建”？
Context: ...录存在且用户存在，但发现用户缺少用户档案（profile）（理论上不应发生），则补建默认档案以保持数据一致性。 2. 按邮箱匹配现有用户： 如果没有现成...
(BU)
[uncategorized] ~95-~95: 您的意思是“"账"号”？
Context: ...，update 分支会更新绑定到当前用户ID。这一操作将日志记录输出，表示发生了帐号关联。 * 之后复用找到的本地用户记录，跳至步骤4。 3. **创建新...
(ZHANG7_ZHANG8)
[uncategorized] ~100-~100: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...ID}`。规范化该用户名：去除特殊字符、转为小写，并确保长度在合适范围内（如不足4字符则补前缀，超长则截断）。然后检查是否与现有用户重名，若是则在末尾追加递增数字后缀...
(wa5)
[uncategorized] ~100-~100: 您的意思是“"不"前缀”？
Context: ...。规范化该用户名：去除特殊字符、转为小写，并确保长度在合适范围内（如不足4字符则补前缀，超长则截断）。然后检查是否与现有用户重名，若是则在末尾追加递增数字后缀确保...
(BU)
[uncategorized] ~134-~134: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：相同"地"会话
Context: ...in 方法所做的记录和 token 发放逻辑），将 OAuth 登录结果接入相同的会话管理流程，实现统一的用户体验。 ## 模块集成与兼容性 有了上述组件，实...
(wb4)
[uncategorized] ~147-~147: 您的意思是“"既"是”吗？
Context: ...sController 构造函数即可通过 DI 拿到它。Cheese-Auth 即是在 UsersController 中注入了 OAuthService，因此我...
(JI11_JI2)
[uncategorized] ~147-~147: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：相同"地"依赖
Context: ...ersController 中注入了 OAuthService，因此我们保持相同的依赖关系即可。 * 路由集成： 将上文列出的 `/users/auth...
(wb4)
[uncategorized] ~155-~155: 您的意思是“"不"验证”？
Context: ...ontroller 增加调用它的入口。因此，原有邮箱验证、密码重置、TOTP 两步验证等功能都与 OAuth 登录互不冲突，各自按需执行。 * **前端配合：...
(BU)
[uncategorized] ~187-~187: 能愿动词不能成为‘把’字句、‘被’字句的谓语动词。应该是:"便于把……实现"。
Context: ...现作为私有文件部署（通过文档指导运维放置），而不直接提交代码。当然，这需要权衡：把实现代码纳入仓库便于版本管理，但需确保不包含敏感信息（比如具体的学校OAuth端点可能是公开协议，无...
(wa3)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: build-and-push-image
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (16, 8.0.0, 22.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.12.2, 20.x)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (latest, 8.12.2, 22.x)
🔇 Additional comments (15)
sample.env (1)
90-108: Comprehensive OAuth configuration setup.The OAuth environment variables are well-structured and cover all necessary aspects:
- Provider management (enabled providers, plugin paths)
- Security settings (npm loading control)
- Provider credentials with clear naming convention
- Frontend integration paths
All sensitive values are appropriately commented out for a sample file.
src/auth/auth.module.ts (3)
16-16: Proper OAuth module integration.The import statement correctly brings in the OAuth module for integration.
34-34: Dynamic OAuth module registration.Using
OAuthModule.register()follows NestJS dynamic module patterns and allows for flexible OAuth provider configuration.
38-38: OAuth module properly exported.Exporting the OAuth module makes it available to other modules that import AuthModule, following good architectural practices.
src/users/users.prisma (2)
12-12: Correct import for OAuth connection model.The import statement properly references the OAuth connection model from the auth module.
58-58: Proper one-to-many OAuth relationship.The
oauthConnectionsfield correctly establishes a one-to-many relationship between users and their OAuth connections, allowing multiple OAuth providers per user.src/auth/oauth/oauth.module.ts (1)
14-19: Proper lifecycle management for OAuth initialization.The
OnModuleInitimplementation ensures OAuth service is properly initialized when the module starts. The async initialization allows for proper setup of OAuth providers.src/users/DTO/oauth.dto.ts (2)
12-20: Well-structured OAuth providers response DTO.The DTO properly extends the base response pattern and defines the expected structure for OAuth provider information including id, name, and scope arrays.
22-37: Comprehensive OAuth callback validation.The DTO correctly handles OAuth 2.0 standard callback parameters:
- Required
codeparameter for authorization code flow- Optional
statefor CSRF protection- Optional error parameters for error handling
All fields are properly validated with class-validator decorators.
src/users/users.service.ts (2)
44-44: LGTM! OAuth type imports added correctly.The OAuth-related type imports are properly organized and follow the existing import structure.
Also applies to: 54-54
1858-1945: Well-designed OAuth helper methods!The helper methods provide robust support for OAuth user creation:
- Smart username generation with multiple fallbacks
- Proper uniqueness handling with counter suffixes
- Secure random password generation
- Default profile creation for edge cases
Great implementation!
docs/oauth-implementation-guide.md (1)
1-286: Excellent OAuth implementation documentation!The guide provides comprehensive coverage of:
- Clear architectural overview
- Step-by-step implementation instructions
- Security best practices
- Troubleshooting guidance
The documentation will be invaluable for developers implementing new OAuth providers.
src/auth/oauth/oauth.service.ts (1)
8-332: Excellent OAuth service implementation!The service demonstrates:
- Robust security with path traversal protection
- Comprehensive error handling and logging
- Clean modular design for provider management
- Proper async initialization pattern
Great architectural design for extensible OAuth support!
test/user.e2e-spec.ts (2)
16-18: Good OAuth test setup!The OAuth-related imports and environment variable configuration before module initialization ensure proper test isolation and configuration.
Also applies to: 225-229
1212-1545: Comprehensive OAuth e2e test coverage!The OAuth test suite thoroughly covers:
- All OAuth endpoints and flows
- Success and error scenarios
- User binding and creation logic
- Token validation and cookie handling
Excellent test implementation that ensures the OAuth feature works correctly end-to-end!
plugins/oauth/example.js
Outdated
| const response = await axios.post(this.config.tokenUrl, { | ||
| client_id: this.config.clientId, | ||
| client_secret: this.config.clientSecret, | ||
| code, | ||
| grant_type: 'authorization_code', | ||
| redirect_uri: this.config.redirectUrl, | ||
| }, { | ||
| headers: { | ||
| 'Accept': 'application/json', | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Fix Content-Type header to match data format.
The code sends JSON data but sets Content-Type to application/x-www-form-urlencoded. This mismatch could cause OAuth providers to reject the request.
- const response = await axios.post(this.config.tokenUrl, {
- client_id: this.config.clientId,
- client_secret: this.config.clientSecret,
- code,
- grant_type: 'authorization_code',
- redirect_uri: this.config.redirectUrl,
- }, {
- headers: {
- 'Accept': 'application/json',
- 'Content-Type': 'application/x-www-form-urlencoded',
- },
- });
+ const params = new URLSearchParams({
+ client_id: this.config.clientId,
+ client_secret: this.config.clientSecret,
+ code,
+ grant_type: 'authorization_code',
+ redirect_uri: this.config.redirectUrl,
+ });
+
+ const response = await axios.post(this.config.tokenUrl, params.toString(), {
+ headers: {
+ 'Accept': 'application/json',
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
+ });📝 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.
| const response = await axios.post(this.config.tokenUrl, { | |
| client_id: this.config.clientId, | |
| client_secret: this.config.clientSecret, | |
| code, | |
| grant_type: 'authorization_code', | |
| redirect_uri: this.config.redirectUrl, | |
| }, { | |
| headers: { | |
| 'Accept': 'application/json', | |
| 'Content-Type': 'application/x-www-form-urlencoded', | |
| }, | |
| }); | |
| const params = new URLSearchParams({ | |
| client_id: this.config.clientId, | |
| client_secret: this.config.clientSecret, | |
| code, | |
| grant_type: 'authorization_code', | |
| redirect_uri: this.config.redirectUrl, | |
| }); | |
| const response = await axios.post(this.config.tokenUrl, params.toString(), { | |
| headers: { | |
| 'Accept': 'application/json', | |
| 'Content-Type': 'application/x-www-form-urlencoded', | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In plugins/oauth/example.js around lines 45 to 56, the Content-Type header is
set to 'application/x-www-form-urlencoded' while the data is sent as JSON,
causing a mismatch. To fix this, either change the Content-Type header to
'application/json' to match the JSON data format or convert the data to
URL-encoded format to match the existing header. Ensure the header and data
format are consistent for the OAuth provider to accept the request.
| ```typescript | ||
| // plugins/oauth/your-provider.ts | ||
| import axios from 'axios'; | ||
| import { BaseOAuthProvider, OAuthProviderConfig, OAuthUserInfo } from '../../src/auth/oauth/oauth.types'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the import path for TypeScript providers.
The import path assumes the provider is located relative to the src directory, which may not be accurate for plugins.
Consider updating the documentation to clarify the correct import approach:
-import { BaseOAuthProvider, OAuthProviderConfig, OAuthUserInfo } from '../../src/auth/oauth/oauth.types';
+// Note: Adjust the import path based on your provider location
+// For plugins in plugins/oauth/, you may need to use a different path or
+// have these types available as a separate package
+import { BaseOAuthProvider, OAuthProviderConfig, OAuthUserInfo } from '@your-app/oauth-types';📝 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.
| import { BaseOAuthProvider, OAuthProviderConfig, OAuthUserInfo } from '../../src/auth/oauth/oauth.types'; | |
| // Note: Adjust the import path based on your provider location | |
| // For plugins in plugins/oauth/, you may need to use a different path or | |
| // have these types available as a separate package | |
| import { BaseOAuthProvider, OAuthProviderConfig, OAuthUserInfo } from '@your-app/oauth-types'; |
🤖 Prompt for AI Agents
In docs/oauth-implementation-guide.md at line 129, clarify the import path for
TypeScript providers by explaining that the relative import from
'../../src/auth/oauth/oauth.types' assumes the provider is within the src
directory, which may not be correct for plugins. Update the documentation to
guide users on adjusting the import path based on their project structure or
plugin location to avoid confusion.
| async handleCallback(code, state) { | ||
| const response = await axios.post(this.config.tokenUrl, { | ||
| client_id: this.config.clientId, | ||
| client_secret: this.config.clientSecret, | ||
| code, | ||
| grant_type: 'authorization_code', | ||
| redirect_uri: this.config.redirectUrl, | ||
| }); | ||
|
|
||
| return response.data.access_token; | ||
| } | ||
|
|
||
| async getUserInfo(accessToken) { | ||
| const response = await axios.get('https://your-provider.com/api/user', { | ||
| headers: { 'Authorization': `Bearer ${accessToken}` }, | ||
| }); | ||
|
|
||
| const userData = response.data; | ||
| return { | ||
| id: userData.id.toString(), | ||
| email: userData.email, | ||
| name: userData.name, | ||
| username: userData.username, | ||
| preferredUsername: userData.preferred_username, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the provider example.
The example provider implementation should demonstrate proper error handling for production use.
Consider wrapping axios calls in try-catch blocks:
async handleCallback(code, state) {
+ try {
const response = await axios.post(this.config.tokenUrl, {
client_id: this.config.clientId,
client_secret: this.config.clientSecret,
code,
grant_type: 'authorization_code',
redirect_uri: this.config.redirectUrl,
});
return response.data.access_token;
+ } catch (error) {
+ throw new Error(`Failed to exchange code for token: ${error.message}`);
+ }
}
async getUserInfo(accessToken) {
+ try {
const response = await axios.get('https://your-provider.com/api/user', {
headers: { 'Authorization': `Bearer ${accessToken}` },
});
const userData = response.data;
return {
id: userData.id.toString(),
email: userData.email,
name: userData.name,
username: userData.username,
preferredUsername: userData.preferred_username,
};
+ } catch (error) {
+ throw new Error(`Failed to fetch user info: ${error.message}`);
+ }
}📝 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.
| async handleCallback(code, state) { | |
| const response = await axios.post(this.config.tokenUrl, { | |
| client_id: this.config.clientId, | |
| client_secret: this.config.clientSecret, | |
| code, | |
| grant_type: 'authorization_code', | |
| redirect_uri: this.config.redirectUrl, | |
| }); | |
| return response.data.access_token; | |
| } | |
| async getUserInfo(accessToken) { | |
| const response = await axios.get('https://your-provider.com/api/user', { | |
| headers: { 'Authorization': `Bearer ${accessToken}` }, | |
| }); | |
| const userData = response.data; | |
| return { | |
| id: userData.id.toString(), | |
| email: userData.email, | |
| name: userData.name, | |
| username: userData.username, | |
| preferredUsername: userData.preferred_username, | |
| }; | |
| } | |
| async handleCallback(code, state) { | |
| try { | |
| const response = await axios.post(this.config.tokenUrl, { | |
| client_id: this.config.clientId, | |
| client_secret: this.config.clientSecret, | |
| code, | |
| grant_type: 'authorization_code', | |
| redirect_uri: this.config.redirectUrl, | |
| }); | |
| return response.data.access_token; | |
| } catch (error) { | |
| throw new Error(`Failed to exchange code for token: ${error.message}`); | |
| } | |
| } | |
| async getUserInfo(accessToken) { | |
| try { | |
| const response = await axios.get('https://your-provider.com/api/user', { | |
| headers: { 'Authorization': `Bearer ${accessToken}` }, | |
| }); | |
| const userData = response.data; | |
| return { | |
| id: userData.id.toString(), | |
| email: userData.email, | |
| name: userData.name, | |
| username: userData.username, | |
| preferredUsername: userData.preferred_username, | |
| }; | |
| } catch (error) { | |
| throw new Error(`Failed to fetch user info: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In docs/oauth-implementation-guide.md around lines 89 to 114, the axios calls in
handleCallback and getUserInfo lack error handling. Wrap each axios call in a
try-catch block to catch and handle errors gracefully. In the catch block, log
or throw a meaningful error message to ensure the provider implementation
demonstrates proper error handling for production use.
| @Res() res?: Response, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
Remove optional modifier from Response parameter
The res parameter should not be optional when using @Res() decorator. NestJS always provides the Response object when this decorator is used.
- @Res() res?: Response,
+ @Res() res: Response,Also update line 1246 to remove the non-null assertion:
- res!.redirect(authUrl);
+ res.redirect(authUrl);And line 1254:
- res!.redirect(errorUrl);
+ res.redirect(errorUrl);📝 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.
| @Res() res?: Response, | |
| ): Promise<void> { | |
| @Get('auth/oauth/login/:providerId') | |
| @NoAuth() | |
| async oauthLogin( | |
| @Req() req: Request, | |
| - @Res() res?: Response, | |
| + @Res() res: Response, | |
| ): Promise<void> { | |
| const { providerId } = req.params; | |
| let authUrl: string; | |
| try { | |
| authUrl = this.oauthService.getAuthorizationUrl(providerId); | |
| - res!.redirect(authUrl); | |
| + res.redirect(authUrl); | |
| } catch (err) { | |
| const errorUrl = this.oauthService.getErrorRedirectUrl(providerId, err); | |
| - res!.redirect(errorUrl); | |
| + res.redirect(errorUrl); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/users/users.controller.ts around lines 1238 to 1239, remove the optional
modifier from the `res` parameter so it is declared as `@Res() res: Response`
without the question mark. Then, on line 1246, remove the non-null assertion
operator (!) when accessing `res`. Also, update line 1254 to remove any non-null
assertions related to `res`. This ensures the Response object is treated as
always present, consistent with NestJS behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/users/users.service.spec.ts (1)
4-6: Remove AI assistant attribution.Remove the author attribution referencing an AI assistant.
* Author(s): - * HuanCheng65 + * [Your name or team]src/auth/oauth/oauth.service.spec.ts (1)
4-6: Remove AI assistant attribution.Remove the author attribution referencing an AI assistant.
* Author(s): - * HuanCheng65 + * [Your name or team]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prisma/migrations/migration_lock.tomlis excluded by!**/*.toml
📒 Files selected for processing (10)
.gitignore(1 hunks)prisma/migrations/20250625052726_add_oauth_tables/migration.sql(1 hunks)src/auth/oauth/oauth.module.spec.ts(1 hunks)src/auth/oauth/oauth.module.ts(1 hunks)src/auth/oauth/oauth.service.spec.ts(1 hunks)src/auth/oauth/oauth.service.ts(1 hunks)src/auth/oauth/oauth.types.spec.ts(1 hunks)src/auth/oauth/oauth.types.ts(1 hunks)src/users/DTO/oauth.dto.ts(1 hunks)src/users/users.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- prisma/migrations/20250625052726_add_oauth_tables/migration.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- src/auth/oauth/oauth.module.ts
- src/users/DTO/oauth.dto.ts
- src/auth/oauth/oauth.service.ts
- src/auth/oauth/oauth.types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth/oauth/oauth.types.spec.ts
[error] 17-19: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: test (16, 8.0.0, 22.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.12.2, 20.x)
🔇 Additional comments (5)
src/auth/oauth/oauth.module.spec.ts (1)
13-194: Well-structured and comprehensive test coverage!The test suite thoroughly covers the OAuthModule functionality including dynamic registration, lifecycle hooks, and various edge cases. The mocking strategy is appropriate and the test organization follows best practices.
src/auth/oauth/oauth.types.spec.ts (1)
42-245: Excellent test coverage for OAuth types!The tests comprehensively cover:
- Authorization URL generation with various parameter combinations
- Proper URL encoding and special character handling
- Error class functionality and inheritance
- Edge cases like empty scopes
The test organization and naming conventions are clear and follow best practices.
src/users/users.service.spec.ts (1)
131-731: Outstanding test coverage for OAuth login functionality!This test suite is exemplary with:
- Comprehensive scenario coverage (13+ test cases)
- Proper edge case handling (deleted users, missing data, special characters)
- Transaction and error handling verification
- Security considerations (XSS prevention, data sanitization)
- Clear test organization and descriptive test names
The mocking strategy effectively isolates the service under test while verifying the correct interactions with dependencies.
src/auth/oauth/oauth.service.spec.ts (2)
764-859: Excellent security validation tests!The security test suite demonstrates thorough attention to security concerns:
- Path traversal prevention in plugin loading
- XSS attempt filtering in provider IDs
- Command injection prevention
- Proper validation and sanitization of user inputs
This security-first approach is commendable and ensures the OAuth implementation is resilient against common attack vectors.
67-701: Comprehensive test coverage for OAuthService!The test suite thoroughly covers:
- All service methods with both success and error scenarios
- Dynamic provider loading from plugins and NPM packages
- Configuration validation and error handling
- Edge cases like multiple initialization attempts
- Proper isolation with filesystem and module mocking
The test organization follows a clear structure making it easy to understand and maintain.
| constructor(config: OAuthProviderConfig) { | ||
| super(config); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unnecessary constructor.
The constructor only calls super(config) without any additional logic. This is automatically inherited from the parent class.
Apply this diff to remove the unnecessary constructor:
class TestOAuthProvider extends BaseOAuthProvider {
- constructor(config: OAuthProviderConfig) {
- super(config);
- }
-
async handleCallback(code: string, state?: string): Promise<string> {📝 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.
| constructor(config: OAuthProviderConfig) { | |
| super(config); | |
| } | |
| class TestOAuthProvider extends BaseOAuthProvider { | |
| async handleCallback(code: string, state?: string): Promise<string> { | |
| // ... existing implementation ... | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-19: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🤖 Prompt for AI Agents
In src/auth/oauth/oauth.types.spec.ts around lines 17 to 19, the constructor is
redundant as it only calls super(config) without adding any logic. Remove the
entire constructor method to rely on the parent class's constructor inheritance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/auth/oauth/oauth.service.spec.ts (2)
256-464: Thorough testing of core OAuth operations.The tests for
getProvider,generateAuthorizationUrl,handleCallback, andgetUserInfoproperly cover both success and error scenarios. The consistent error handling for nonexistent providers is well-implemented.Consider refactoring duplicate setup code.
There's significant duplication in the beforeEach blocks across these test suites. Consider extracting common mock setup into helper functions.
880-892: Address static analysis suggestion carefully.The static analysis tool suggests using optional chaining on line 887, but the current explicit null checking is actually safer in this context:
if (pathStr && pathStr.includes('test')) {This pattern correctly checks for both null/undefined and ensures
pathStris truthy before calling methods on it. Optional chaining would bepathStr?.includes('test')but this would returnundefinedinstead offalsefor null/undefined values, which could change the conditional logic.The current code is correct and safer than the suggested optional chaining in this specific case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yamltsconfig.build.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
.gitignore(1 hunks)src/auth/oauth/oauth.service.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth/oauth/oauth.service.spec.ts
[error] 887-887: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (latest, 8.12.2, 20.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (16, 8.0.0, 22.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (16, 8.0.0, 22.x)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (latest, 8.12.2, 20.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
src/auth/oauth/oauth.service.spec.ts (6)
1-23: Well-structured test file setup.The file header, imports, and initial mocking setup are properly organized. The fs module mocking is correctly placed before the import to ensure proper test isolation.
24-65: Excellent mock provider implementation.The MockOAuthProvider properly implements the OAuthProvider interface with realistic OAuth flow simulation. The conditional logic for valid/invalid codes and tokens provides good test coverage scenarios.
72-131: Comprehensive test setup with proper isolation.The test module setup correctly mocks all dependencies and the cleanup in
afterEachensures proper test isolation by resetting service state. This is essential for preventing test interference in a stateful service.
132-254: Comprehensive initialization test coverage.The initialization tests thoroughly cover various scenarios including error handling, security validation, and edge cases. The provider ID validation tests are particularly important for preventing security vulnerabilities.
764-908: Excellent security validation coverage.The security tests comprehensively cover path traversal, XSS, and injection attack prevention. These tests are crucial for a service that dynamically loads OAuth providers from external files and npm packages. The validation of provider IDs and plugin paths helps prevent security vulnerabilities.
1406-1591: Comprehensive edge case and configuration handling.The remaining tests provide excellent coverage of configuration parsing, error handling, and internal method testing. The tests for non-Error objects being thrown and whitespace handling in configuration are particularly valuable for production robustness.
Excellent overall test suite.
This test file demonstrates exceptional testing practices with comprehensive coverage of functionality, security, error handling, and edge cases. The OAuth service is thoroughly validated across all scenarios.
1 participant
No description provided.