Add login and signup with email #330
Conversation
Completed Working on "Code Review"✅ Published all review comments from 3 chunk files (6 comments total) and submitted a REQUEST_CHANGES review on PR #330. Summary:
Review event: REQUEST_CHANGES. All steps completed. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
| setShowLoginModal(true); | ||
| } | ||
| setShowSignupMode(false); | ||
| setLoginModalCanClose(false); // Don't allow closing until authenticated |
There was a problem hiding this comment.
[MAJOR]: This effect now unconditionally flips loginModalCanClose to false whenever the initial auth check resolves unauthenticated, even if the user had manually opened the modal while authLoading was still true. In that scenario the dialog suddenly becomes non-dismissible (the onOpenChange guard refuses to close it because loginModalCanClose is false and the user isn’t authenticated), effectively trapping them until they hard-refresh. Please only disable closing when the modal was auto-opened (e.g., track an autoOpened flag or check showLoginModal before overriding loginModalCanClose) so user-initiated open/close flows still work.
| /** | ||
| * Login with email and password | ||
| */ | ||
| static async loginWithEmail(email: string, password: string): Promise<{ success: boolean; error?: string }> { |
There was a problem hiding this comment.
[MEDIUM]: The new loginWithEmail/signupWithEmail flows effectively introduce a third auth provider (email), but the shared User interface that this service imports (from @/types/api) still hard-codes provider to 'google' | 'github'. As soon as the backend starts returning 'email' through /auth-status, the runtime value no longer satisfies the declared type, so any code branching on user.provider will either break type-checking or be forced to cast away safety. Please expand User.provider (and any related discriminated unions) to include 'email' so that AuthStatus and every consumer stay type-correct.
|
Review summary
Tests: Not run (review only). |
There was a problem hiding this comment.
Summary:
- Email signup now conflates backend failures with duplicate accounts and leaks whether an address already exists, so the API both hides real outages and exposes user data.
- The new email login/signup UI shipped keyboard-inaccessible password toggles, blocking non-pointer users.
- The AuthContext now surfaces email auth but the shared
Usertype still forbids the'email'provider, so the new flow cannot be represented safely in TypeScript.
Please address these regressions before merging.
api/routes/auth.py
Outdated
| logging.info("User already exists: %s", _sanitize_for_log(email)) | ||
| # User already exists - return error instead of success | ||
| logging.info("Signup attempt for existing user: [%s]", _sanitize_for_log(email)) | ||
| return JSONResponse( |
There was a problem hiding this comment.
[major] This new branch now treats any failure from ensure_user_in_organizations() as “user already exists” and forces a 409. When the helper returns success = False because the graph DB or callback handler failed, we’ll still execute this block, tell the user their email is taken, and hide the real 5xx error. That makes triage impossible and forces users to retry endlessly even though the backend is down. We need to distinguish between success is False (return a 500/"Registration failed") and the legit success=True/new_identity=False case before emitting this response.
api/routes/auth.py
Outdated
| # User already exists - return error instead of success | ||
| logging.info("Signup attempt for existing user: [%s]", _sanitize_for_log(email)) | ||
| return JSONResponse( | ||
| {"success": False, "error": "An account with this email already exists"}, |
There was a problem hiding this comment.
[major] Returning "An account with this email already exists" along with a 409 leaks whether a particular email has ever registered. Attackers can now enumerate valid accounts just by hitting /signup/email. The earlier implementation always returned a generic success payload, so this is a new information disclosure. Please respond with a generic error (e.g. “Registration failed”/400) even when the identity already exists, and surface the specific reason only in logs.
| login: { | ||
| google: () => Promise<void>; | ||
| github: () => Promise<void>; | ||
| email: (email: string, password: string) => Promise<{ success: boolean; error?: string }>; |
There was a problem hiding this comment.
[major] Now that the context exposes login.email/signup.email, refreshAuth() will eventually populate user.provider with the literal 'email'. Our shared User type (imported from @/types/api) still restricts provider to 'google' | 'github', so this assignment will be rejected by TypeScript (or worse, forced to any) and anything narrowing on provider will never match the email case. Please extend the User interface to include 'email' (and update the client code accordingly) so the new auth path is representable.
Error Handling - Fixed properly with proper status codes Email Enumeration - Fixed with generic error messages
No description provided.