425 add telegram login alongside discord authentication#426
425 add telegram login alongside discord authentication#426Behzad-rabiei merged 13 commits intomainfrom
Conversation
…identities schema
WalkthroughThis pull request introduces comprehensive support for Telegram authentication alongside Discord, enhancing the platform's multi-platform identity management. The changes span multiple files, including configuration, controllers, services, and interfaces, to implement a robust OAuth2 flow for Telegram. The implementation allows users to authenticate via Telegram, creates or retrieves user identities, and generates authentication tokens, while maintaining the existing Discord authentication infrastructure. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
src/services/platform.service.ts (1)
276-286: Enhance error handling and logging.While the error handling is good, consider these improvements:
- Include platform ID in success log
- Propagate error for better error tracking
try { await sagaService.createAndStartGuildSaga(platformId, { created: false, discordId: userDiscordId, message: IMPORT_COMPLETE_MESSAGE.trim(), useFallback: true, }); - logger.info(`Notification sent to Discord ID: ${userDiscordId}`); + logger.info({ platformId, userDiscordId }, 'Import completion notification sent'); } catch (error) { logger.error(error, `Failed to send notification to Discord ID: ${userDiscordId}`); + throw error; }src/controllers/platform.controller.ts (1)
388-404: Improve validation modularity and error messages.The validation function could be improved in several ways:
- Consider extracting platform-specific validation to separate functions
- Make error messages more specific about the current state
- Add TypeScript type guards
-const validatePlatformUpdate = (platform: IAuthAndPlatform['platform'], body: IAuthAndPlatform['body']) => { +const validatePlatformUpdate = (platform: IAuthAndPlatform['platform'], body: IAuthAndPlatform['body']): void => { if (platform.name !== PlatformNames.Discord) return; + validateDiscordPlatformUpdate(platform, body); +}; +const validateDiscordPlatformUpdate = ( + platform: IAuthAndPlatform['platform'], + body: IAuthAndPlatform['body'], +): void => { + const isUpdatingRestrictedFields = body.metadata?.selectedChannels || body.metadata?.period; + if (!isUpdatingRestrictedFields) return; + if (platform.metadata?.isInProgress && (body.metadata?.selectedChannels || body.metadata?.period)) { throw new ApiError( httpStatus.BAD_REQUEST, - 'Updating channels or date period is not allowed during server analysis.', + `Cannot update channels or date period: Server analysis is in progress (${platform.metadata?.analysisProgress || 0}%)`, ); } if (platform.metadata?.isFetchingInitialData && (body.metadata?.selectedChannels || body.metadata?.period)) { throw new ApiError( httpStatus.BAD_REQUEST, - 'Updating channels or date periods is not allowed during the initial fetching of the server.', + 'Cannot update channels or date period: Initial server data fetch is in progress', ); } };src/services/discord/auth.service.ts (2)
1-2: Consider using modern URL APIs instead ofquerystring.
Thequerystringmodule is considered legacy; you can simplify and future-proof the code by usingURLSearchParamsfrom Node.js.-import querystring from 'querystring'; +// Example alternative: +// import { URLSearchParams } from 'url';
10-11: Use an enum or constant dictionary for status codes.
Consolidating status codes into an enum can improve maintainability and readability.-const STATUS_CODE_SIGNIN = 1001; -const STATUS_CODE_LOGIN = 1002; +export enum OAuthStatusCode { + SIGNIN = 1001, + LOGIN = 1002, +}src/interfaces/Telegram.interface.ts (1)
1-10: Refine the index signature for strict type safety.
Using[key: string]: string | undefinedcan overshadow existing fields and inject potential type ambiguity. Consider storing unrecognized fields in a separate property or remove the index signature if not strictly necessary.-export interface TelegramCallbackParams { +// Optionally remove or refine the index signature for stricter type constraints. export interface TelegramCallbackParams { id: string; first_name?: string; last_name?: string; username?: string; photo_url?: string; auth_date: string; hash: string; - [key: string]: string | undefined; }src/services/telegram/auth.service.ts (1)
11-12: Consider moving status codes to a centralized constants file.Moving these status codes to a shared constants file would improve maintainability and prevent potential inconsistencies across the codebase.
src/controllers/auth.controller.ts (1)
35-35: Standardize error status codes across authentication providers.The error status codes differ between Discord (1003) and Telegram (1012) callbacks. Consider standardizing these codes or documenting the reason for the difference.
Also applies to: 52-52
src/utils/role.util.ts (2)
20-28: Consider platform-agnostic role checking.The early return when Discord identity is missing might prevent checking roles from other platforms (e.g., Telegram). This could lead to incomplete role assignment for users who only have Telegram authentication.
Consider refactoring to check roles across all supported platforms:
- if (!discordIdentity) { - return userRoles; - } + // Check roles for each supported platform + const platforms = [PlatformNames.Discord, PlatformNames.Telegram]; + for (const platform of platforms) { + const identity = userService.getIdentityByProvider(user.identities, platform); + if (identity) { + // Platform-specific role checking logic + } + }
Line range hint
43-63: Add support for Telegram roles.The role checking logic is tightly coupled to Discord-specific concepts (guild members, roles). With the addition of Telegram login, we need equivalent role checking for Telegram users.
Consider abstracting the platform-specific role checking:
interface PlatformRoleChecker { checkRoles(identity: IIdentity, community: HydratedDocument<ICommunity>): Promise<string[]>; } class DiscordRoleChecker implements PlatformRoleChecker { async checkRoles(identity: IIdentity, community: HydratedDocument<ICommunity>): Promise<string[]> { // Existing Discord role checking logic } } class TelegramRoleChecker implements PlatformRoleChecker { async checkRoles(identity: IIdentity, community: HydratedDocument<ICommunity>): Promise<string[]> { // New Telegram role checking logic } }src/services/user.service.ts (1)
160-162: Add type validation for provider parameter.The
getIdentityByProviderfunction should validate that the provider parameter is a validPlatformNamesvalue.const getIdentityByProvider = (identities: IIdentity[], provider: PlatformNames): IIdentity | undefined => { + if (!Object.values(PlatformNames).includes(provider)) { + throw new Error(`Invalid provider: ${provider}`); + } return identities.find((identity) => identity.provider === provider); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.json(2 hunks)src/app.ts(2 hunks)src/config/index.ts(2 hunks)src/controllers/auth.controller.ts(2 hunks)src/controllers/platform.controller.ts(3 hunks)src/interfaces/Telegram.interface.ts(1 hunks)src/interfaces/index.ts(1 hunks)src/public/index.html(1 hunks)src/routes/v1/auth.route.ts(1 hunks)src/services/discord/auth.service.ts(1 hunks)src/services/discord/index.ts(1 hunks)src/services/platform.service.ts(3 hunks)src/services/telegram/auth.service.ts(1 hunks)src/services/telegram/index.ts(2 hunks)src/services/user.service.ts(3 hunks)src/utils/role.util.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/public/index.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
🔇 Additional comments (16)
src/services/discord/auth.service.ts (2)
50-52: The default export is well-structured.
No further issues; it cleanly exposes the new OAuth callback method.
19-26: Add error handling for core service calls.
Calls toexchangeCodeandgetUserFromDiscordAPImay fail or return unexpected data, leading to uncaught exceptions. Consider wrapping them in try/catch to handle upstream errors gracefully.src/services/telegram/index.ts (1)
10-10: Placement ofauthServicelooks fine.
The export structure is coherent, ensuring the new service is readily accessible.src/interfaces/index.ts (1)
9-9: LGTM!The export statement follows the established pattern and integrates well with existing interface exports.
src/services/discord/index.ts (1)
5-5: LGTM! Verify auth service integration.The addition of authService aligns with the multi-provider authentication system.
Let's verify the auth service integration:
Also applies to: 12-12
✅ Verification successful
Auth service integration verified and properly implemented ✅
The auth service implementation is consistent across Discord and Telegram platforms, with proper separation of concerns:
- Platform-specific auth flows in respective services
- Shared core authentication logic in root service
- Consistent use of user and token services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent auth service usage across Discord and Telegram echo "Checking auth service implementation..." rg -A 5 "authService" src/services/Length of output: 1658
Script:
#!/bin/bash # Check auth service implementations echo "Discord auth.service:" ast-grep --pattern 'class $_Service { $$$ }' src/services/discord/auth.service.ts echo -e "\nTelegram auth.service:" ast-grep --pattern 'class $_Service { $$$ }' src/services/telegram/auth.service.tsLength of output: 286
Script:
#!/bin/bash # Find and check auth service files echo "Looking for auth.service files:" fd "auth.service.ts" src/ echo -e "\nChecking Discord auth.service content:" cat src/services/discord/auth.service.ts 2>/dev/null || echo "File not found" echo -e "\nChecking Telegram auth.service content:" cat src/services/telegram/auth.service.ts 2>/dev/null || echo "File not found" echo -e "\nChecking root auth.service content:" cat src/services/auth.service.ts 2>/dev/null || echo "File not found"Length of output: 8572
src/routes/v1/auth.route.ts (1)
12-12:⚠️ Potential issueAdd request validation for Telegram callback.
The Telegram callback route should include validation middleware to ensure request parameters are properly validated before processing.
Add validation similar to other auth routes:
-router.get('/telegram/authorize/callback', authController.telegramAuthorizeCallback); +router.get('/telegram/authorize/callback', validate(authValidation.telegramCallback), authController.telegramAuthorizeCallback);Let's verify the implementation of other OAuth callbacks:
src/app.ts (1)
49-49:⚠️ Potential issueReview security implications of static file serving.
Adding static file serving requires careful configuration to prevent exposure of sensitive files.
Verify no sensitive files in public directory:
Consider adding security measures:
-app.use(express.static(path.join(__dirname, 'public'))); +app.use(express.static(path.join(__dirname, 'public'), { + dotfiles: 'ignore', + etag: false, + index: false, + maxAge: '1d', + redirect: false, +}));✅ Verification successful
Static file serving setup is secure with room for optional hardening
The current implementation is secure as:
- Public directory contains only index.html
- Essential security headers are in place via helmet
- CORS is properly configured
Optional enhancement:
-app.use(express.static(path.join(__dirname, 'public'))); +app.use(express.static(path.join(__dirname, 'public'), { + dotfiles: 'ignore', + maxAge: '1d', + index: true +}));🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potentially sensitive files in public directory echo "Checking public directory contents..." find src/public -type f -name "*.json" -o -name "*.config.*" -o -name "*.env*" -o -name "*.key" -o -name "*.pem"Length of output: 199
Script:
#!/bin/bash # Check if public directory exists and list all files echo "Checking public directory structure..." if [ -d "src/public" ]; then find src/public -type f else echo "src/public directory does not exist" fi # Check express static configuration and security middleware echo -e "\nChecking express configuration..." rg -A 3 "express.static" src/ rg "helmet|cors|rate-limit" src/Length of output: 674
src/services/telegram/auth.service.ts (2)
69-71: LGTM!The export follows the established pattern used in other service modules.
47-48: Add type checking for userId.The userId should be validated to ensure it's a string and matches Telegram's ID format.
src/controllers/auth.controller.ts (1)
Line range hint
58-85: LGTM!The remaining functions and exports are unchanged and correctly maintained.
src/config/index.ts (3)
16-16: LGTM! Environment variable properly configured.The
TELEGRAM_BOT_TOKENenvironment variable is correctly defined with proper validation and description.
138-140: LGTM! Telegram configuration properly structured.The Telegram configuration is well-organized under the oAuth2 section, following the existing pattern used for other OAuth providers.
138-140: Verify if callback URI configuration is needed for Telegram.Unlike other OAuth providers in the configuration, the Telegram section doesn't include callback URI settings. While this might be intentional due to Telegram's different authentication flow, it's worth verifying that this is the expected behavior.
✅ Verification successful
Telegram's configuration is correctly implemented
The absence of a callback URI in the configuration is correct. Telegram uses a widget-based authentication flow where the callback URL is configured directly in the login widget (found in
src/public/index.html). The only configuration needed is the bot token, which is used for verifying the authentication data from Telegram.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Telegram-related callback URI usage in the codebase echo "Searching for Telegram callback URI references..." rg -i "telegram.*callback.*uri" echo "Searching for Telegram auth-related files..." fd -i telegram.*authLength of output: 266
Script:
#!/bin/bash # Search for Telegram-related authentication code echo "Searching for Telegram auth patterns..." rg -i "telegram.*auth|telegram.*login" -A 3 echo "Searching for Telegram Bot API usage..." rg -i "telegram.*bot" -A 3 echo "Searching for files with Telegram imports..." rg -i "import.*telegram|from.*telegram"Length of output: 4840
package.json (2)
57-57: Verify compatibility with updated mongoose package.The update from ^6.7.5 to ^6.13.8 includes several minor version bumps. While staying within version 6.x should maintain backward compatibility, it's important to verify there are no breaking changes that could affect the Telegram integration.
✅ Verification successful
✓ MongoDB driver and security updates, no breaking changes
The update from ^6.7.5 to ^6.13.8 is safe to proceed with as it:
- Contains important security fixes (CVE-2025-23061)
- Includes MongoDB driver updates for better stability
- Has no breaking changes that would affect the Telegram integration
- Maintains backward compatibility within the 6.x series
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check mongoose changelog for any breaking changes echo "Checking mongoose changelog between versions..." curl -s https://raw.githubusercontent.com/Automattic/mongoose/master/CHANGELOG.md | grep -A 10 "6.13.8" # Look for mongoose schema usage that might be affected echo "Searching for mongoose schema definitions..." rg "new mongoose\.Schema" -A 5Length of output: 662
Script:
#!/bin/bash # Fetch more changelog history and look for breaking changes between 6.7.5 and 6.13.8 echo "Checking extended mongoose changelog..." curl -s https://raw.githubusercontent.com/Automattic/mongoose/master/CHANGELOG.md | awk '/6\.7\.5/,/6\.13\.8/' | grep -i "breaking\|deprecat\|fix:\|feat:" # Search for mongoose-related code patterns echo -e "\nSearching for mongoose models and schemas..." rg "mongoose\.(model|Schema)" -A 5 # Look for mongoose connection and middleware echo -e "\nSearching for mongoose connections and middleware..." rg "mongoose\.(connect|connection|pre|post)" -A 3Length of output: 40764
31-31: Verify compatibility with updated @togethercrew.dev/db package.The update from ^3.0.79 to ^3.2.3 includes minor version changes. While this should be backward compatible, it's good practice to verify the changes.
✅ Verification successful
Update to @togethercrew.dev/db ^3.2.3 is safe to proceed.
The package maintains backward compatibility with consistent API usage patterns. Recent version updates (3.1.0, 3.2.0) and subsequent patches (3.2.1-3.2.3) show active maintenance without introducing breaking changes. Core dependencies remain stable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the changelog or release notes for @togethercrew.dev/db echo "Fetching package information..." npm view @togethercrew.dev/db versions --json | jq '.["`3.0.79`":]' # Look for any breaking changes in the codebase that might be affected echo "Searching for @togethercrew.dev/db usage..." rg "@togethercrew\.dev/db" -A 3 -B 3Length of output: 34581
Script:
#!/bin/bash # Fetch detailed package information including time and versions npm view @togethercrew.dev/db time --json npm view @togethercrew.dev/db@3.2.3 dependencies --json # Check DatabaseManager usage patterns echo "Checking DatabaseManager usage..." ast-grep --pattern 'DatabaseManager.$_($_)'Length of output: 3493
Script:
#!/bin/bash # Check for DatabaseManager method usage echo "Checking DatabaseManager method usage patterns..." rg "DatabaseManager\." -B 2 -A 2 # Check package.json in the db package for recent versions echo "Fetching recent version details..." npm view @togethercrew.dev/db@3.1.0 description --json npm view @togethercrew.dev/db@3.2.0 description --jsonLength of output: 20687
src/utils/role.util.ts (1)
39-41: Same early return issue as above.This is another instance where we're returning early if Discord identity is missing, potentially missing roles from other platforms.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/controllers/platform.controller.ts (1)
321-328:⚠️ Potential issueWrap Discord notification in try-catch block.
The notification call should be wrapped in try-catch to prevent notification failures from affecting the platform update.
Apply this diff to handle notification failures gracefully:
if (req.platform.name === PlatformNames.Discord) { const discordIdentity = userService.getIdentityByProvider(req.user.identities, PlatformNames.Discord); if (discordIdentity) { - await platformService.notifyDiscordUserImportComplete(req.platform.id, discordIdentity.id); + try { + await platformService.notifyDiscordUserImportComplete(req.platform.id, discordIdentity.id); + } catch (error) { + logger.error(error, 'Failed to send import completion notification'); + // Continue with platform update despite notification failure + } } }
🧹 Nitpick comments (2)
src/controllers/platform.controller.ts (2)
382-398: Enhance type safety and improve validation logic.
- Add return type annotation for better type safety
- Make error messages more specific about allowed operations
- Combine similar conditions to make the code more DRY
Apply this diff to improve the validation function:
-const validatePlatformUpdate = (platform: IAuthAndPlatform['platform'], body: IAuthAndPlatform['body']) => { +const validatePlatformUpdate = ( + platform: IAuthAndPlatform['platform'], + body: IAuthAndPlatform['body'] +): void => { if (platform.name !== PlatformNames.Discord) return; - if (platform.metadata?.isInProgress && (body.metadata?.selectedChannels || body.metadata?.period)) { - throw new ApiError( - httpStatus.BAD_REQUEST, - 'Updating channels or date period is not allowed during server analysis.', - ); - } - - if (platform.metadata?.isFetchingInitialData && (body.metadata?.selectedChannels || body.metadata?.period)) { + const isUpdatingRestrictedFields = body.metadata?.selectedChannels || body.metadata?.period; + const isServerBusy = platform.metadata?.isInProgress || platform.metadata?.isFetchingInitialData; + + if (isServerBusy && isUpdatingRestrictedFields) { + const operation = platform.metadata?.isInProgress ? 'server analysis' : 'initial data fetching'; + const allowedOperations = 'Only metadata updates unrelated to channels and date periods are allowed'; throw new ApiError( httpStatus.BAD_REQUEST, - 'Updating channels or date periods is not allowed during the initial fetching of the server.', + `Cannot update channels or date periods during ${operation}. ${allowedOperations}.` ); } };
Line range hint
1-398: Consider refactoring OAuth2 callback error handling.The file contains multiple OAuth2 callback handlers with similar error handling patterns. Consider:
- Creating an enum or constants for status codes
- Extracting common error handling logic into a shared utility function
Example implementation:
// types/constants.ts export enum CallbackStatus { DISCORD_SUCCESS = 1004, DISCORD_ERROR = 1005, TWITTER_SUCCESS = 1006, TWITTER_ERROR = 1007, // ... other status codes } // utils/callback.utils.ts export const handleCallbackError = ( res: Response, error: Error, platform: PlatformNames, errorCode: CallbackStatus ): void => { logger.error({ error }, `Failed in ${platform} connect callback`); const query = querystring.stringify({ statusCode: errorCode }); res.redirect(`${config.frontend.url}/callback?${query}`); };This would simplify the callback handlers and make the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app.ts(1 hunks)src/controllers/platform.controller.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/controllers/platform.controller.ts (1)
326-334:⚠️ Potential issueAdd error handling and fix potential race condition.
- The notification call should be wrapped in try-catch (as previously suggested).
- There's a potential race condition between sending the notification and updating the platform.
Consider this implementation:
const updatePlatform = catchAsync(async function (req: IAuthAndPlatform, res: Response) { validatePlatformUpdate(req.platform, req.body); + const platform = await platformService.updatePlatform(req.platform, req.body); if (req.platform.name === PlatformNames.Discord) { const discordIdentity = userService.getIdentityByProvider(req.user.identities, PlatformNames.Discord); if (discordIdentity) { + try { await platformService.notifyDiscordUserImportComplete(req.platform.id, discordIdentity.id); + } catch (error) { + logger.error(error, 'Failed to send import completion notification'); + // Continue despite notification failure + } } } - const platform = await platformService.updatePlatform(req.platform, req.body); res.send(platform); });
🧹 Nitpick comments (1)
src/controllers/platform.controller.ts (1)
387-403: Add JSDoc documentation and improve function design.
- The function lacks documentation explaining its purpose, parameters, and possible errors.
- The function could be more reusable by accepting metadata directly instead of the entire platform object.
Consider this improvement:
+/** + * Validates platform update request for Discord platform + * @param {IAuthAndPlatform['platform']} platform - The platform being updated + * @param {IAuthAndPlatform['body']} body - The update request body + * @throws {ApiError} When update is not allowed during analysis or initial data fetch + */ const validatePlatformUpdate = (platform: IAuthAndPlatform['platform'], body: IAuthAndPlatform['body']) => { if (platform.name !== PlatformNames.Discord) return; if (platform.metadata?.isInProgress && (body.metadata?.selectedChannels || body.metadata?.period)) { throw new ApiError( httpStatus.BAD_REQUEST, 'Updating channels or date period is not allowed during server analysis.', ); } if (platform.metadata?.isFetchingInitialData && (body.metadata?.selectedChannels || body.metadata?.period)) { throw new ApiError( httpStatus.BAD_REQUEST, 'Updating channels or date periods is not allowed during the initial fetching of the server.', ); } };Alternative design for better reusability:
/** * Validates Discord platform metadata updates * @param {boolean} isInProgress - Whether server analysis is in progress * @param {boolean} isFetchingInitialData - Whether initial data fetch is in progress * @param {object} updates - The proposed updates to channels or period * @throws {ApiError} When update is not allowed */ const validateDiscordMetadataUpdate = ( isInProgress: boolean, isFetchingInitialData: boolean, updates: { selectedChannels?: unknown; period?: unknown } ) => { const hasRestrictedUpdates = updates.selectedChannels || updates.period; if (isInProgress && hasRestrictedUpdates) { throw new ApiError( httpStatus.BAD_REQUEST, 'Updating channels or date period is not allowed during server analysis.', ); } if (isFetchingInitialData && hasRestrictedUpdates) { throw new ApiError( httpStatus.BAD_REQUEST, 'Updating channels or date periods is not allowed during the initial fetching of the server.', ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/platform.controller.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/migrations/db/1738072787796-update-user-documents-for-telegram-login.ts (2)
10-18: Consider more robust error handling & potential reconnection logic.
While this connection approach is straightforward, large migrations might benefit from retries or a more sophisticated error handling workflow to minimize downtime.
52-78: Use a more efficient bulk update rather than a per-document loop.
Iterating over all documents can be time-consuming for a large user base. If you only need to roll back, consider a single bulk operation (like a pipeline) to re-injectdiscordIdfromidentities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.migrate(1 hunks)src/migrations/db/1720532975152-add-isfetchingintialdata.ts(0 hunks)src/migrations/db/1738072787796-update-user-documents-for-telegram-login.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/migrations/db/1720532975152-add-isfetchingintialdata.ts
✅ Files skipped from review due to trivial changes (1)
- .migrate
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
🔇 Additional comments (2)
src/migrations/db/1738072787796-update-user-documents-for-telegram-login.ts (2)
1-2: Environment variable usage looks good.
No immediate issues found with loading environment variables from.env.
26-45: Preserving theidentities.
Currently, the pipeline removesidentitiesor in a separate field to avoid data loss.Do you want sample code to preserve the
identitiesfor future reference?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/user.service.ts (1)
154-162: Optimize the identity lookup.Consider using array destructuring for better readability and adding early returns for common cases:
const getIdentityByProvider = (identities: IIdentity[], provider: PlatformNames): IIdentity | undefined => { + if (!identities?.length) return undefined; + const [identity] = identities.filter((identity) => identity.provider === provider); + return identity; - return identities.find((identity) => identity.provider === provider); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/user.service.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
🔇 Additional comments (4)
src/services/user.service.ts (4)
2-4: LGTM! Appropriate types imported for identity management.The imports correctly include necessary mongoose types and platform-specific types from the shared package.
93-105: Add input validation for provider and userId.While the implementation is correct, it's missing important validations:
- Provider name validation
- Provider-specific ID format validation
const getUserByIdentity = async (provider: PlatformNames, userId: string): Promise<HydratedDocument<IUser> | null> => { + if (!Object.values(PlatformNames).includes(provider)) { + throw new ApiError(httpStatus.BAD_REQUEST, 'Invalid provider'); + } + + const isValidId = validateProviderId(provider, userId); + if (!isValidId) { + throw new ApiError(httpStatus.BAD_REQUEST, `Invalid ID format for provider ${provider}`); + } + return User.findOne({ identities: { $elemMatch: { provider, id: userId }, }, }); }; function validateProviderId(provider: PlatformNames, id: string): boolean { switch (provider) { case PlatformNames.Discord: return /^\d{17,19}$/.test(id); // Discord snowflake format case PlatformNames.Telegram: return /^\d+$/.test(id); // Telegram numeric ID format default: return false; } }
131-152: Add cross-user uniqueness check and input validation.The implementation needs:
- Cross-user uniqueness check for identities
- Provider name and ID format validation
const addIdentityToUser = async (userId: Types.ObjectId, identity: IIdentity): Promise<HydratedDocument<IUser>> => { + if (!Object.values(PlatformNames).includes(identity.provider)) { + throw new ApiError(httpStatus.BAD_REQUEST, 'Invalid provider'); + } + + const isValidId = validateProviderId(identity.provider, identity.id); + if (!isValidId) { + throw new ApiError(httpStatus.BAD_REQUEST, `Invalid ID format for provider ${identity.provider}`); + } + + // Check if identity exists for any other user + const existingUser = await getUserByIdentity(identity.provider, identity.id); + if (existingUser && !existingUser._id.equals(userId)) { + throw new ApiError(httpStatus.CONFLICT, 'Identity already linked to another user'); + } + const user = await User.findById(userId); if (!user) { throw new ApiError(httpStatus.NOT_FOUND, 'User not found'); }
172-175: LGTM! All new methods are properly exported.The export object correctly includes all the new identity management methods.
| /** | ||
| * Create a user with a specific identity | ||
| * @param {string} provider - The authentication provider (e.g., 'discord') | ||
| * @param {string} userId - The unique ID from the provider | ||
| * @param {Partial<IUser>} additionalData - Any additional user data | ||
| * @returns {Promise<HydratedDocument<IUser>>} | ||
| */ | ||
| const createUserWithIdentity = async ( | ||
| provider: PlatformNames, | ||
| userId: string, | ||
| additionalData: Partial<IUser> = {}, | ||
| ): Promise<HydratedDocument<IUser>> => { | ||
| const userBody: IUser = { | ||
| identities: [ | ||
| { | ||
| provider, | ||
| id: userId, | ||
| }, | ||
| ], | ||
| ...additionalData, | ||
| }; | ||
| return createUser(userBody); | ||
| }; |
There was a problem hiding this comment.
Add validation and uniqueness check before user creation.
The implementation should:
- Validate provider name and ID format (same as getUserByIdentity)
- Check if the identity already exists for another user
const createUserWithIdentity = async (
provider: PlatformNames,
userId: string,
additionalData: Partial<IUser> = {},
): Promise<HydratedDocument<IUser>> => {
+ if (!Object.values(PlatformNames).includes(provider)) {
+ throw new ApiError(httpStatus.BAD_REQUEST, 'Invalid provider');
+ }
+
+ const isValidId = validateProviderId(provider, userId);
+ if (!isValidId) {
+ throw new ApiError(httpStatus.BAD_REQUEST, `Invalid ID format for provider ${provider}`);
+ }
+
+ const existingUser = await getUserByIdentity(provider, userId);
+ if (existingUser) {
+ throw new ApiError(httpStatus.CONFLICT, 'Identity already exists');
+ }
+
const userBody: IUser = {
identities: [
{📝 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.
| /** | |
| * Create a user with a specific identity | |
| * @param {string} provider - The authentication provider (e.g., 'discord') | |
| * @param {string} userId - The unique ID from the provider | |
| * @param {Partial<IUser>} additionalData - Any additional user data | |
| * @returns {Promise<HydratedDocument<IUser>>} | |
| */ | |
| const createUserWithIdentity = async ( | |
| provider: PlatformNames, | |
| userId: string, | |
| additionalData: Partial<IUser> = {}, | |
| ): Promise<HydratedDocument<IUser>> => { | |
| const userBody: IUser = { | |
| identities: [ | |
| { | |
| provider, | |
| id: userId, | |
| }, | |
| ], | |
| ...additionalData, | |
| }; | |
| return createUser(userBody); | |
| }; | |
| /** | |
| * Create a user with a specific identity | |
| * @param {string} provider - The authentication provider (e.g., 'discord') | |
| * @param {string} userId - The unique ID from the provider | |
| * @param {Partial<IUser>} additionalData - Any additional user data | |
| * @returns {Promise<HydratedDocument<IUser>>} | |
| */ | |
| const createUserWithIdentity = async ( | |
| provider: PlatformNames, | |
| userId: string, | |
| additionalData: Partial<IUser> = {}, | |
| ): Promise<HydratedDocument<IUser>> => { | |
| if (!Object.values(PlatformNames).includes(provider)) { | |
| throw new ApiError(httpStatus.BAD_REQUEST, 'Invalid provider'); | |
| } | |
| const isValidId = validateProviderId(provider, userId); | |
| if (!isValidId) { | |
| throw new ApiError(httpStatus.BAD_REQUEST, `Invalid ID format for provider ${provider}`); | |
| } | |
| const existingUser = await getUserByIdentity(provider, userId); | |
| if (existingUser) { | |
| throw new ApiError(httpStatus.CONFLICT, 'Identity already exists'); | |
| } | |
| const userBody: IUser = { | |
| identities: [ | |
| { | |
| provider, | |
| id: userId, | |
| }, | |
| ], | |
| ...additionalData, | |
| }; | |
| return createUser(userBody); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/controllers/auth.controller.ts (2)
49-55: Enhance error handling with specific error types.The current error handling uses a generic status code (1003) for all errors. Consider differentiating between different types of failures (e.g., validation errors, network errors, authentication errors) for better debugging and user experience.
Example improvement:
logger.error({ err }, 'Failed to authorize Telegram account'); + let statusCode = 1003; // default error code + if (err instanceof ValidationError) { + statusCode = 1001; + } else if (err instanceof AuthenticationError) { + statusCode = 1002; + } const params = { - statusCode: 1003, + statusCode, };
22-56: Consider extracting common OAuth callback logic.Both Discord and Telegram callback handlers share similar patterns for error handling and redirection. Consider creating a common utility function to handle these shared aspects.
Example approach:
async function handleOAuthError(res: Response, err: Error, platform: string) { logger.error({ err }, `Failed to authorize ${platform} account`); const params = { statusCode: 1003, }; const query = querystring.stringify(params); res.redirect(`${config.frontend.url}/callback?${query}`); }This would reduce code duplication and make future OAuth provider additions easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/auth.controller.ts(2 hunks)src/services/telegram/auth.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/telegram/auth.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / lint / Lint
🔇 Additional comments (2)
src/controllers/auth.controller.ts (2)
8-9: LGTM! Import changes are well-structured.The new imports for Telegram-related functionality follow the existing import pattern and are appropriately organized.
44-44: Add runtime type validation for query parameters.The current type casting of query parameters to
TelegramCallbackParamsis unsafe.
| const code = req.query.code as string; | ||
| const returnedState = req.query.state as string; | ||
| const storedState = req.session.state; | ||
| const redirectUrl = await discordServices.authService.handleOAuthCallback({ | ||
| code, | ||
| config.oAuth2.discord.callbackURI.authorize, | ||
| ); | ||
| const discordUser = await discordServices.coreService.getUserFromDiscordAPI(discordOathCallback.access_token); | ||
| let user = await userService.getUserByFilter({ discordId: discordUser.id }); | ||
| state: returnedState, | ||
| storedState, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| user = await userService.createUser({ discordId: discordUser.id }); | ||
| statusCode = STATUS_CODE_SINGIN; | ||
| } | ||
| tokenService.saveDiscordOAuth2Tokens(user.id, discordOathCallback); | ||
| const tokens = await tokenService.generateAuthTokens(user); | ||
| res.redirect(redirectUrl); | ||
| } catch (err) { | ||
| logger.error({ err }, 'Failed to authorize Discord account'); | ||
| const params = { | ||
| statusCode: statusCode, | ||
| accessToken: tokens.access.token, | ||
| accessExp: tokens.access.expires.toString(), | ||
| refreshToken: tokens.refresh.token, | ||
| refreshExp: tokens.refresh.expires.toString(), | ||
| statusCode: 1003, | ||
| }; | ||
| const query = querystring.stringify(params); | ||
| res.redirect(`${config.frontend.url}/callback?` + query); | ||
| res.redirect(`${config.frontend.url}/callback?${query}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit state validation before proceeding with OAuth.
While the code handles the state parameter, it should explicitly validate that both states exist and match before proceeding with the OAuth callback.
Consider this improvement:
const code = req.query.code as string;
const returnedState = req.query.state as string;
const storedState = req.session.state;
+ if (!returnedState || !storedState || returnedState !== storedState) {
+ throw new Error('Invalid state parameter');
+ }
const redirectUrl = await discordServices.authService.handleOAuthCallback({📝 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 code = req.query.code as string; | |
| const returnedState = req.query.state as string; | |
| const storedState = req.session.state; | |
| const redirectUrl = await discordServices.authService.handleOAuthCallback({ | |
| code, | |
| config.oAuth2.discord.callbackURI.authorize, | |
| ); | |
| const discordUser = await discordServices.coreService.getUserFromDiscordAPI(discordOathCallback.access_token); | |
| let user = await userService.getUserByFilter({ discordId: discordUser.id }); | |
| state: returnedState, | |
| storedState, | |
| }); | |
| if (!user) { | |
| user = await userService.createUser({ discordId: discordUser.id }); | |
| statusCode = STATUS_CODE_SINGIN; | |
| } | |
| tokenService.saveDiscordOAuth2Tokens(user.id, discordOathCallback); | |
| const tokens = await tokenService.generateAuthTokens(user); | |
| res.redirect(redirectUrl); | |
| } catch (err) { | |
| logger.error({ err }, 'Failed to authorize Discord account'); | |
| const params = { | |
| statusCode: statusCode, | |
| accessToken: tokens.access.token, | |
| accessExp: tokens.access.expires.toString(), | |
| refreshToken: tokens.refresh.token, | |
| refreshExp: tokens.refresh.expires.toString(), | |
| statusCode: 1003, | |
| }; | |
| const query = querystring.stringify(params); | |
| res.redirect(`${config.frontend.url}/callback?` + query); | |
| res.redirect(`${config.frontend.url}/callback?${query}`); | |
| } | |
| }); | |
| const code = req.query.code as string; | |
| const returnedState = req.query.state as string; | |
| const storedState = req.session.state; | |
| if (!returnedState || !storedState || returnedState !== storedState) { | |
| throw new Error('Invalid state parameter'); | |
| } | |
| const redirectUrl = await discordServices.authService.handleOAuthCallback({ | |
| code, | |
| state: returnedState, | |
| storedState, | |
| }); | |
| res.redirect(redirectUrl); | |
| } catch (err) { | |
| logger.error({ err }, 'Failed to authorize Discord account'); | |
| const params = { | |
| statusCode: 1003, | |
| }; | |
| const query = querystring.stringify(params); | |
| res.redirect(`${config.frontend.url}/callback?${query}`); | |
| } | |
| }); |
1 participant
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Configuration
Bug Fixes
These changes primarily focus on expanding platform authentication options and improving overall system flexibility.