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 ✨ 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 |
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}
| ); | ||
| }; | ||
|
|
||
| export default SignupModal; |
There was a problem hiding this comment.
[MAJOR]: This modal is never imported or rendered anywhere in the app, so none of the email-signup UI you added here is reachable—users still only see LoginModal, leaving signup-by-email impossible. Please either wire this component into the same places that open the auth dialog (or merge its functionality into LoginModal, which already accepts startInSignupMode) so the new flow can be accessed, otherwise drop the unused file to avoid shipping dead code.
| startWithEmailForm?: boolean; // Start with email form already open | ||
| } | ||
|
|
||
| const SignupModal = ({ open, onOpenChange, onSwitchToLogin, canClose = true, startWithEmailForm = false }: SignupModalProps) => { |
There was a problem hiding this comment.
[MAJOR]: SignupModal reimplements the entire OAuth + email signup experience (state, validation, success handling, etc.) that now also exists in LoginModal via the new startInSignupMode prop. Having two separate components duplicating hundreds of lines of identical form logic guarantees that fixes (e.g., password validation, error copy, refresh behavior) will quickly drift. Please reuse the existing modal (e.g., by toggling signup mode or extracting a shared form hook/component) instead of maintaining a second copy.
| } | ||
| }, [open, startWithEmailForm]); | ||
|
|
||
| const handleGoogleSignup = () => { |
There was a problem hiding this comment.
[MAJOR]: These handlers build the Google/GitHub auth redirects manually, bypassing the centralized useAuth().login.google/github helpers that encapsulate endpoint URLs and error handling. If those endpoints or login side effects change, this modal will silently break while the rest of the app keeps working. Please call the context-provided login helpers here (or plumb them through props) so all OAuth entry points share the same source of truth.
| 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.
| <Label htmlFor="password">Password</Label> | ||
| <div className="relative"> | ||
| <Input id="password" type={showPassword ? "text" : "password"} placeholder="••••••••" value={password} onChange={(e) => setPassword(e.target.value)} required disabled={isLoading} className="pr-10" /> | ||
| <button type="button" onClick={() => setShowPassword(!showPassword)} className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground" tabIndex={-1}> |
There was a problem hiding this comment.
[MAJOR]: The password visibility toggle is removed from the tab order by setting tabIndex={-1}, so keyboard users and screen readers cannot focus or activate it. Because the modal is intended to be accessible, every interactive control must be reachable via keyboard; otherwise users relying on keyboard/assistive tech cannot reveal their password, which breaks the form flow. Please allow the toggle buttons to receive focus (e.g., remove the negative tab index or set it to 0) so they remain operable for all users.
| /** | ||
| * 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.
| 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.
| # 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.
| <Label htmlFor="password">Password</Label> | ||
| <div className="relative"> | ||
| <Input id="password" type={showPassword ? "text" : "password"} placeholder="••••••••" value={password} onChange={(e) => setPassword(e.target.value)} required disabled={isLoading} className="pr-10" /> | ||
| <button type="button" onClick={() => setShowPassword(!showPassword)} className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground" tabIndex={-1}> |
There was a problem hiding this comment.
[medium] Both password-visibility toggle buttons are rendered with tabIndex={-1}, which removes them from the tab order entirely. Keyboard and assistive-technology users can no longer reach the control, violating WCAG 2.1.2/2.4.3. Please leave the default tab index (and ideally add an aria-label such as “Show password”) so the toggle is operable without a pointer.
| 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.
No description provided.