Added Sign-in-page: Sign-in with Github #170
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis change introduces GitHub OAuth authentication to the application. It adds backend support for GitHub OAuth, updates the user model, implements a new OAuth callback route and handler, and enhances the frontend with OAuth login UI and logic. Comprehensive documentation for GitHub OAuth setup is also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant GitHub
User->>Frontend: Clicks "Sign in with GitHub"
Frontend->>GitHub: Redirects to GitHub OAuth authorize URL
GitHub->>User: Prompts for authorization
User->>GitHub: Authorizes app
GitHub->>Frontend: Redirects with ?code=...
Frontend->>Frontend: GitHubCallback component extracts code
Frontend->>Backend: POST /github/callback { code }
Backend->>GitHub: Exchanges code for access token
GitHub-->>Backend: Returns access token
Backend->>GitHub: Fetches user profile & email
GitHub-->>Backend: Returns user data
Backend->>Backend: Create/update user in DB
Backend-->>Frontend: Returns user info / success
Frontend->>User: Displays status, redirects accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @kanchn-https for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
backend/models/User.js (1)
31-39: Consider using Mongoose's built-in timestamps optionMongoose provides a built-in
timestampsoption that automatically managescreatedAtandupdatedAtfields, eliminating the need for manual implementation.Remove the manual timestamp fields and the updatedAt pre-save hook (lines 56-60), then add the timestamps option to the schema:
const UserSchema = new mongoose.Schema({ // ... existing fields ... - // Timestamps - createdAt: { - type: Date, - default: Date.now, - }, - updatedAt: { - type: Date, - default: Date.now, - }, -}); +}, { + timestamps: true +});src/pages/GitHubCallback/GitHubCallback.tsx (1)
56-59: Avoid using 'any' type for errorsUse 'unknown' type for better type safety when handling errors.
-} catch (error: any) { +} catch (error) { setStatus('error'); - setMessage(error.message || 'Failed to authenticate with GitHub'); + setMessage(error instanceof Error ? error.message : 'Failed to authenticate with GitHub'); setTimeout(() => navigate('/login'), 3000);GITHUB_OAUTH_SETUP.md (2)
23-26: Add language specifiers to code blocksCode blocks should have language specifiers for proper syntax highlighting.
Line 23:
-``` +```envLine 29:
-``` +```envAlso applies to: 29-36
65-66: Format bare URLs as proper linksUse proper Markdown link syntax for better readability.
- - Frontend: http://localhost:5174 - - Backend: http://localhost:5000 + - Frontend: <http://localhost:5174> + - Backend: <http://localhost:5000> -1. Go to http://localhost:5174/login +1. Go to <http://localhost:5174/login>Also applies to: 70-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
GITHUB_OAUTH_SETUP.md(1 hunks)backend/models/User.js(2 hunks)backend/routes/auth.js(1 hunks)src/Routes/Router.tsx(2 hunks)src/context/AuthContext.tsx(1 hunks)src/pages/GitHubCallback/GitHubCallback.tsx(1 hunks)src/pages/Login/Login.tsx(7 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
GITHUB_OAUTH_SETUP.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
65-65: Bare URL used
(MD034, no-bare-urls)
66-66: Bare URL used
(MD034, no-bare-urls)
70-70: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (18)
src/Routes/Router.tsx (1)
9-9: LGTM!The GitHub OAuth callback route is correctly implemented and placed.
Also applies to: 19-19
backend/routes/auth.js (1)
110-125: LGTM!Session establishment and user data response are properly implemented.
src/pages/Login/Login.tsx (7)
6-6: LGTM!The new icon imports are correctly added and used appropriately throughout the component for the enhanced UI features.
19-20: LGTM!The new state variables are properly typed and serve clear purposes for the password visibility toggle and GitHub loading state management.
34-34: Good UX improvement!Clearing the message state before login attempts ensures old error messages don't persist, providing better user feedback.
109-121: LGTM!The GitHub sign-in button is well-implemented with proper loading states, disabled handling, theme support, and appropriate styling consistent with GitHub's branding guidelines.
123-133: LGTM!The divider implementation follows standard UI patterns for separating authentication methods and properly supports both light and dark themes.
155-155: LGTM!The password visibility toggle is well-implemented with proper state management, appropriate icons, good positioning, and consistent theming. This enhances user experience significantly.
Also applies to: 162-162, 168-178
184-196: LGTM!The enhanced submit button provides excellent user feedback with proper loading states, spinner animation, and icon usage. The implementation is clean and user-friendly.
src/context/AuthContext.tsx (9)
4-18: LGTM!The interface definitions are comprehensive and well-typed. The
Userinterface captures essential user data, andAuthContextTypeprovides a complete authentication API with proper TypeScript typing.
20-28: LGTM!The context creation and
useAuthhook follow React best practices with proper TypeScript typing and error handling to ensure the hook is used within the provider.
30-32: LGTM!The
AuthProviderPropsinterface is correctly defined with properReactNodetyping for the children prop.
34-38: LGTM!The component setup is well-structured with appropriate initial state values and proper environment variable handling with a sensible fallback.
40-56: LGTM!The
checkAuthStatusmethod is well-implemented with proper error handling, credential inclusion, and ensures the loading state is always updated in the finally block.
58-74: LGTM!The login method is well-implemented with proper credential handling, specific success message checking, state updates, and useful boolean return value for caller feedback.
76-88: Verify the intended signup flow.The signup method doesn't set the user state upon successful registration, unlike the login method. This means users will still appear as unauthenticated after signup.
Is this the intended behavior? Should users be automatically logged in after successful signup, or is a separate login required? If auto-login is intended, consider updating the user state similar to the login method.
90-100: LGTM!The logout method correctly clears the user state regardless of API response, ensuring the local authentication state is properly reset. The error handling is appropriate.
102-121: LGTM!The useEffect properly initializes authentication status on mount, and the context value object correctly implements the
AuthContextTypeinterface with all required properties and methods.
backend/routes/auth.js
Outdated
| // Set a random password since GitHub users don't have passwords | ||
| password: Math.random().toString(36).slice(-10) + Math.random().toString(36).slice(-10) | ||
| }); |
There was a problem hiding this comment.
Use cryptographically secure random password generation
Math.random() is not cryptographically secure. Use Node.js crypto module for generating secure random passwords.
+const crypto = require('crypto');
+
// ... in the user creation block ...
-password: Math.random().toString(36).slice(-10) + Math.random().toString(36).slice(-10)
+password: crypto.randomBytes(32).toString('hex')Consider making the password field optional in the schema for OAuth users to avoid storing unnecessary data.
🤖 Prompt for AI Agents
In backend/routes/auth.js around lines 98 to 100, replace the use of
Math.random() for password generation with the Node.js crypto module's secure
random byte generation to create a cryptographically secure password.
Additionally, update the user schema to make the password field optional for
OAuth users to avoid storing unnecessary password data.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/routes/auth.js (3)
40-53: Timeout implementation looks good.The addition of
AbortSignal.timeout(10000)addresses the previous concern about hanging requests. The timeout configuration is appropriate for OAuth token exchange.
55-64: Error handling improvements implemented correctly.The response status check before JSON parsing and generic error messages address the previous security and reliability concerns.
107-107: Security issue: Use cryptographically secure random password generation.
Math.random()is not cryptographically secure and should be replaced with Node.js crypto module for generating secure random passwords.Apply this diff to use secure random generation:
+const crypto = require('crypto'); + // ... in the user creation block ... -password: Math.random().toString(36).slice(-10) + Math.random().toString(36).slice(-10) +password: crypto.randomBytes(32).toString('hex')Consider making the password field optional in the User schema for OAuth users to avoid storing unnecessary data.
src/pages/Login/Login.tsx (2)
50-69: Excellent GitHub OAuth implementation.All previous concerns have been addressed:
- Environment variable validation prevents unsafe fallbacks
- Correct OAuth scope (
user:email) limits permissions appropriately- Proper error handling without dead code in finally block
- Clean redirect implementation
218-223: Proper React Router navigation implementation.The Link component usage correctly enables client-side navigation instead of full page reloads, addressing the previous concern about SPA navigation.
🧹 Nitpick comments (1)
backend/routes/auth.js (1)
100-116: Consider optimizing user creation/update logic.The current approach creates users with unnecessary password data for OAuth users. Consider making the password field optional or using a different approach for OAuth-only users.
if (!user) { // Create new user with GitHub data user = new User({ username: userData.login, email: primaryEmail, githubId: userData.id.toString(), githubUsername: userData.login, avatarUrl: userData.avatar_url, - // Set a random password since GitHub users don't have passwords - password: crypto.randomBytes(32).toString('hex') + // OAuth users don't need passwords + isOAuthUser: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/models/User.js(2 hunks)backend/routes/auth.js(1 hunks)src/pages/GitHubCallback/GitHubCallback.tsx(1 hunks)src/pages/Login/Login.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/models/User.js
- src/pages/GitHubCallback/GitHubCallback.tsx
🔇 Additional comments (10)
backend/routes/auth.js (4)
30-36: LGTM! Good input validation.The route correctly validates the required authorization code parameter and returns appropriate error responses.
69-93: Well-structured API calls with proper timeout handling.The GitHub API calls for user data and emails are properly implemented with timeouts and error handling. The email selection logic correctly prioritizes primary email with fallback to public email.
118-133: Session management implementation is correct.The use of
req.loginto establish the user session and the structured response with user data are properly implemented.
135-139: Good error handling and logging.The catch block properly logs errors and returns appropriate error responses without exposing sensitive information.
src/pages/Login/Login.tsx (6)
1-6: Good import organization and icon usage.The imports are well-organized and include the necessary React Router components and icons for the enhanced UI features.
19-20: Proper state management for new features.The addition of
showPasswordandisGitHubLoadingstate variables appropriately supports the new password visibility toggle and GitHub OAuth loading states.
113-124: Well-designed GitHub sign-in button.The button implementation includes proper loading states, accessibility considerations with disabled state, and follows GitHub's styling guidelines with the GitHub icon.
126-136: Excellent visual separation with divider.The divider between GitHub OAuth and email/password login provides clear visual separation and improves user experience. The theme-aware styling is consistent with the rest of the component.
158-181: Great password visibility toggle implementation.The password field enhancement with show/hide functionality improves user experience. The implementation correctly:
- Uses conditional input type based on state
- Provides proper button styling with hover effects
- Uses appropriate icons for visual feedback
- Adjusts padding to accommodate the toggle button
187-200: Enhanced submit button with loading states.The submit button improvements with spinner animation and arrow icon provide excellent user feedback during form submission.
|
@kanchn-https : cannot accept big changes in one commit.. Please split your commits into small changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/routes/auth.js (1)
96-117: Consider username preservation for existing users.The user creation/update logic is well-implemented with secure password generation. However, consider whether existing users should have their username preserved when linking GitHub accounts, as the current implementation overwrites the original username with the GitHub login.
If username preservation is desired:
} else { // Update existing user with GitHub info user.githubId = userData.id.toString(); // Convert to string - user.githubUsername = userData.login; + user.githubUsername = userData.login; + // Preserve original username unless it's empty + if (!user.username) { + user.username = userData.login; + } user.avatarUrl = userData.avatar_url; await user.save(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/routes/auth.js(2 hunks)
🔇 Additional comments (6)
backend/routes/auth.js (6)
4-4: LGTM! Good security practices.The crypto import for secure random password generation and the basic validation of the authorization code parameter follow security best practices.
Also applies to: 31-37
39-67: LGTM! Previous feedback addressed.The token exchange implementation properly handles timeouts, response status checking, and error handling as requested in previous reviews. The use of environment variables for sensitive data is appropriate.
69-82: LGTM! Consistent API handling.The user data fetching follows the same robust pattern as the token exchange with proper authentication, timeout handling, and status checking.
84-95: LGTM! Good fallback strategy for email handling.The email fetching logic properly handles the case where users may have multiple emails and appropriately falls back to the public email when a primary email isn't found.
119-134: LGTM! Proper session handling and user data response.The login logic correctly establishes the user session and returns appropriate user data without exposing sensitive information.
136-140: LGTM! Appropriate error handling.The error handling properly logs errors for debugging while returning generic error messages to prevent information leakage.
2 participants
#29 Related Issue
Description
Added GitHub OAuth authentication with enhanced login UI. Users can now sign in with their GitHub account or continue with email/password.
Features:
How Has This Been Tested?
Type of Change
Summary by CodeRabbit
New Features
Documentation
Bug Fixes