Fix: authentication logic improved for saving only verified users.#153
Fix: authentication logic improved for saving only verified users.#153bhavik-mangla merged 4 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/controllers/auth.go (1)
594-613: Consider using a separate secret for signup tokens.Using the same JWT secret for both signup and authentication tokens works due to the
typeclaim differentiation, but using separate secrets would provide better isolation between token types and reduce blast radius if one secret is compromised.frontend/src/Pages/Authentication/forms.tsx (1)
204-223: UnusedThe
OTPVerificationFormbut is no longer used sinceverifyEmailnow only takes the OTP code. Consider removing it from the interface if it's not needed elsewhere.Also, the
handleSubmitfunction lacks a try-catch block. IfverifyEmailthrows an error (viahandleErrorin authContext),handleOtpVerified()won't be called, but the unhandled rejection could cause issues.🔎 Proposed fix
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); - await verifyEmail(otp); - handleOtpVerified(); + try { + await verifyEmail(otp); + handleOtpVerified(); + } catch { + // Error is already handled by authContext + } };And if
interface OTPVerificationFormProps { - email: string; handleOtpVerified: () => void; } -export const OTPVerificationForm: React.FC<OTPVerificationFormProps> = ({ email, handleOtpVerified }) => { +export const OTPVerificationForm: React.FC<OTPVerificationFormProps> = ({ handleOtpVerified }) => {frontend/src/context/authContext.tsx (1)
192-253: LGTM with a minor suggestion.The verification flow correctly retrieves the signup token, sends it for validation, and handles the successful response by storing the auth token and user data.
Consider also clearing the
signupTokenfrom localStorage when the backend returns an "expired token" error, so users aren't stuck with a stale token:🔎 Optional enhancement
if (!response.ok) { const data = await response.json(); + // Clear stale token if expired + if (data.error?.includes('expired')) { + localStorage.removeItem('signupToken'); + } throw new Error(data.error || 'Verification failed'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/controllers/auth.gobackend/structs/auth.gobackend/utils/email.gofrontend/src/Pages/Authentication/forms.tsxfrontend/src/context/authContext.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/context/authContext.tsx (2)
backend/models/user.go (1)
User(10-35)frontend/src/types/user.ts (1)
User(1-21)
backend/controllers/auth.go (2)
backend/db/db.go (1)
MongoDatabase(18-18)backend/utils/auth.go (2)
ExtractNameFromEmail(44-51)Claims(68-73)
🔇 Additional comments (6)
backend/utils/email.go (1)
44-46: LGTM!The updated email content with the welcome header and 24-hour expiration notice aligns correctly with the signup JWT expiration time defined in the backend.
backend/structs/auth.go (1)
8-11: LGTM!The struct correctly reflects the new token-based verification flow. The email is now extracted from the JWT token on the backend, so only the token and confirmation code are needed from the client.
backend/controllers/auth.go (3)
160-178: LGTM!The signup flow correctly defers user creation by generating a signed JWT containing the necessary user data. The token will be validated during email verification.
293-297: LGTM!The simplified verification check is correct. With the new flow where users are only created after email verification, this serves as an appropriate safety check.
615-653: LGTM!The validation function properly checks the token type to prevent token confusion attacks and correctly extracts all required claims with appropriate error handling.
frontend/src/context/authContext.tsx (1)
179-184: LGTM!The signup function correctly stores the signup token for use during the email verification step.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/controllers/auth.go (1)
160-201: Critical: Implementation doesn't match PR objectives.The PR objectives clearly state: "SignUp handler now generates a JWT token instead of inserting the user into the database" and "Opt1 (preferred): Do not save user to the database at signup."
However, the current implementation does insert the user into the database at signup (lines 182-188), contradicting the stated approach. This means:
- Unverified users are still persisted — the original issue (#143) where unverified users clutter the database is not resolved
- Unverified users can still appear in queries — any code that queries the users collection will see unverified accounts
- The JWT-based helper functions (lines 598-657) exist but are never used, creating confusion
The implementation appears to be a hybrid approach that wasn't fully completed. Either:
- Complete the JWT-based flow by removing the DB insert here and calling
generateSignupJWTinstead, OR- Update the PR objectives to reflect that this is actually a DB-based approach with verification checks
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/controllers/auth.gofrontend/src/Pages/Authentication/forms.tsxfrontend/src/context/authContext.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/Pages/Authentication/forms.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/context/authContext.tsx (2)
backend/models/user.go (1)
User(10-35)frontend/src/types/user.ts (1)
User(1-21)
backend/controllers/auth.go (4)
backend/models/user.go (1)
User(10-35)frontend/src/types/user.ts (1)
User(1-21)backend/db/db.go (1)
MongoDatabase(18-18)backend/utils/auth.go (1)
Claims(68-73)
🔇 Additional comments (8)
frontend/src/context/authContext.tsx (3)
197-198: Error field changed to align with backend response.Good change to extract the error from
data.errorinstead ofdata.messagefor consistency with the backend error response format.
200-232: Automatic login after verification implemented correctly.The new flow appropriately handles successful email verification by:
- Storing the access token
- Building a fully normalized user object with
isVerified: true- Persisting both token and user to localStorage
- Navigating to the home page
This provides a smooth user experience by eliminating the need for a separate login step after verification.
25-25: AI summary appears incorrect.The
verifyEmailfunction signature in the interface at line 25 correctly matches its implementation and actual usage. The function is called with bothcodeparameters atfrontend/src/Pages/Authentication/forms.tsx:221(await verifyEmail(email, otp)), which aligns with the interface definition(email: string, code: string) => Promise<void>. The AI summary claiming the signature should be(code: string) => Promise<void>does not reflect the actual requirements of the code.backend/controllers/auth.go (5)
175-177: Gamification fields initialized correctly.The initialization of
Score,Badges, andCurrentStreakfor new users is appropriate and prevents null/undefined issues.
215-226: User lookup changed to database query.The implementation now finds the user in the database using email and verification code. This is a reasonable approach for a DB-based verification flow.
However, this contradicts the PR objectives which state: "Refactored VerifyEmail handler to decode the JWT token and create the verified user in the database." The current code doesn't decode any JWT from the request.
228-232: 24-hour verification expiry enforced.Good addition to prevent stale verification codes from being used indefinitely. The check correctly compares
user.CreatedAtto the current time.
249-266: Immediate login after verification implemented.The flow correctly:
- Updates the local user object after database update
- Generates a JWT token for the verified user
- Returns both the access token and user details
This provides a smooth UX by automatically logging in users after verification.
298-298: Login verification check simplified.The condition is now straightforward:
if !user.IsVerified. This correctly blocks all unverified users from logging in, which was the original issue.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/controllers/auth.go (1)
140-148: Unverified users with expired codes get locked out.If a user signs up but never verifies within 24 hours, their verification code expires (line 229). However, since the user record still exists in the database, any subsequent signup attempt with the same email fails with "User already exists". The user cannot:
- Sign up again (blocked by this check)
- Verify (code expired)
- Log in (blocked by verification check)
Consider either:
- Allowing re-signup for unverified users with expired codes (overwrite or delete the old record)
- Adding a TTL index on MongoDB to auto-delete unverified users after 24 hours
- Implementing a "resend verification code" flow
🔎 Suggested fix to handle expired unverified users
err := db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"email": request.Email}).Decode(&existingUser) if err == nil { + // If user exists but is unverified and code expired, delete and allow re-signup + if !existingUser.IsVerified && time.Since(existingUser.CreatedAt) > 24*time.Hour { + _, delErr := db.MongoDatabase.Collection("users").DeleteOne(dbCtx, bson.M{"_id": existingUser.ID}) + if delErr != nil { + ctx.JSON(500, gin.H{"error": "Database error", "message": delErr.Error()}) + return + } + // Continue with signup + } else { ctx.JSON(400, gin.H{"error": "User already exists"}) return + } }
🧹 Nitpick comments (1)
backend/controllers/auth.go (1)
190-195: Consider rollback if email sending fails.If
SendVerificationEmailfails after the user is inserted (line 183), the user record remains in the database without the user receiving their verification code. This creates an orphaned record. The user would need to wait 24 hours for the code to "expire" before re-signup works (assuming the previous suggestion is implemented).Consider wrapping the insert and email send in a transaction-like pattern, or deleting the user if email fails.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/auth.go
🔇 Additional comments (2)
backend/controllers/auth.go (2)
215-266: LGTM! Verification flow with immediate login is well-implemented.The changes add proper verification logic:
- Looking up by both email and verification code prevents enumeration attacks
- The 24-hour expiry check provides reasonable security
- Generating a JWT on successful verification improves UX by eliminating a separate login step
Note: If a "resend verification code" feature is added later, consider tracking
verificationCodeCreatedAtseparately fromCreatedAtto accurately enforce expiry on the new code.
297-301: LGTM! Login verification check correctly blocks unverified users.The simplified
!user.IsVerifiedcheck properly addresses issue #143 by blocking all unverified users from logging in, regardless of other conditions. This is cleaner and more secure than the previous multi-condition check.
|
@bhavik-mangla |
|
Hey @bhavik-mangla |
|
Hey @bhavik-mangla can you please review this PR. |
|
@ayush00git could you include testing evidence of new flow of auth |
|
Looks good |
|
good work @ayush00git |
Thanks a lot @bhavik-mangla |
2 participants
Closes #143
Changes I made -
Previously it relied on two checks - if password is empty and
isVerified = falsethen only it blocks the user, but if only one was correct likeisVerified=falseit does not stops the user, i've added a single check to fix this.jwt tokeninstead of inserting the user to databse.jwt tokenand create verified user.jwt tokenin localStorage.Summary by CodeRabbit
New Features
User Flow
✏️ Tip: You can customize this high-level summary in your review settings.