Skip to content

Forgot password in login#83

Open
nazarli-shabnam wants to merge 9 commits intomainfrom
forgot-password-in-login
Open

Forgot password in login#83
nazarli-shabnam wants to merge 9 commits intomainfrom
forgot-password-in-login

Conversation

@nazarli-shabnam
Copy link
Copy Markdown
Member

closes #82
This pull request implements a complete "forgot password" and password reset flow, including backend API, database schema, and frontend support. It adds secure password reset token generation and validation, email delivery via a queue, and user-facing forms for requesting and completing password resets. Additionally, it makes minor UI improvements by replacing a navigation chevron with a simpler divider.

Password Reset Feature Implementation

Backend (API, Store, Model, Migration):

  • Adds a new PasswordResetToken model, database table, and store to securely generate, store, and validate password reset tokens. Tokens are time-limited, single-use, and invalidated on use or replacement. [1] [2] [3] [4]
  • Updates the auth.Service to support generating and validating reset tokens, and to allow dependency injection of the token store. Adds ForgotPassword and ResetPassword methods. [1] [2]
  • Extends the authentication handler with two new endpoints: POST /auth/forgot-password/ to request a reset link, and POST /auth/reset-password/ to set a new password using a token. Integrates with the email queue to send reset links, and prevents user enumeration by always returning a generic success message. [1] [2] [3] [4]

Frontend (UI, Types, Pages):

  • Adds TypeScript types for forgot/reset password requests.
  • Implements a new ForgotPasswordPage with form, error handling, and resend cooldown.

UI Improvements

  • Replaces the ProjectSectionNavChevron component with a simple slash divider for navigation clarity in project/module headers. [1] [2] [3] [4] [5]

@nazarli-shabnam nazarli-shabnam self-assigned this Apr 4, 2026
@nazarli-shabnam nazarli-shabnam added bug Something isn't working enhancement New feature or request labels Apr 4, 2026
Copilot AI review requested due to automatic review settings April 4, 2026 09:46
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a full “forgot password” + “reset password” flow across the Go API (token persistence, endpoints, email queuing) and the React UI (request/reset pages + login entry point), plus a small navigation UI tweak.

Changes:

  • Add password reset token model/store + DB migration, and expose /auth/forgot-password/ + /auth/reset-password/ endpoints.
  • Add UI pages and client service methods for requesting and completing password resets; update login page UX and routing.
  • Add Vite dev proxy for /api and /auth, and replace a nav chevron with a slash divider.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
ui/vite.config.ts Add dev server proxy to forward /api and /auth to the Go server.
ui/src/services/authService.ts Add client methods for forgot/reset password endpoints.
ui/src/routes/index.tsx Register routes for forgot/reset password pages.
ui/src/pages/ForgotPasswordPage.tsx New “request reset link” form with resend cooldown.
ui/src/pages/ResetPasswordPage.tsx New “set new password” form with strength checks.
ui/src/pages/LoginPage.tsx Add forgot-password link + combined sign-in/sign-up UI.
ui/src/components/layout/PageHeader.tsx Replace chevron component with a slash divider.
ui/src/components/layout/ModuleDetailHeader.tsx Replace chevron component with slash dividers.
ui/src/api/types.ts Add request types for forgot/reset password calls.
api/migrations/000002_password_reset_tokens.up.sql Create password_reset_tokens table + indexes.
api/migrations/000002_password_reset_tokens.down.sql Drop password_reset_tokens table.
api/internal/store/password_reset_token.go Token generation, lookup, and single-use marking.
api/internal/router/router.go Wire token store + queue/base URL into AuthHandler; add new routes.
api/internal/model/password_reset_token.go New Gorm model for password reset tokens.
api/internal/handler/auth.go Implement /auth/forgot-password/ and /auth/reset-password/ handlers + email enqueue.
api/internal/auth/service.go Add ForgotPassword/ResetPassword service methods and reset token store injection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +24 to +28
// Invalidate any existing unused tokens for this user.
s.db.WithContext(ctx).
Where("user_id = ? AND used_at IS NULL", userID).
Delete(&model.PasswordResetToken{})

Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Create(), the deletion that invalidates existing unused tokens ignores any DB error. If the delete fails (permissions/connection/tx), the method will still mint a new token and return success, leaving multiple valid tokens or masking DB problems. Capture the delete result and return an error when it fails (ideally in a transaction with the subsequent insert).

Copilot uses AI. Check for mistakes.
}
email = strings.TrimSpace(strings.ToLower(email))
u, err := s.userStore.GetByEmail(ctx, email)
if err != nil || u == nil {
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ForgotPassword() treats any error from userStore.GetByEmail as "email not found" and returns ("", nil, nil). This will silently hide real DB/connection errors and cause the handler to respond 200 even when the system cannot look up users. Consider only swallowing gorm.ErrRecordNotFound and returning other errors so operational issues surface appropriately.

Suggested change
if err != nil || u == nil {
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return "", nil, nil
}
return "", nil, err
}
if u == nil {

Copilot uses AI. Check for mistakes.
u, err := s.userStore.GetByID(ctx, rec.UserID)
if err != nil || u == nil {
return ErrResetTokenInvalid
}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResetPassword() loads the user but doesn't check IsActive before allowing the password change. If an account is deactivated after a reset token is issued, this would still allow resetting the password. Consider rejecting resets for inactive users (returning ErrResetTokenInvalid) to match ForgotPassword() behavior.

Suggested change
}
}
if !u.IsActive {
return ErrResetTokenInvalid
}

Copilot uses AI. Check for mistakes.
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
);
CREATE INDEX IF NOT EXISTS idx_password_reset_tokens_user ON password_reset_tokens (user_id);
CREATE INDEX IF NOT EXISTS idx_password_reset_tokens_token ON password_reset_tokens (token);
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UNIQUE constraint on token already creates an index in Postgres, so idx_password_reset_tokens_token is redundant and adds write overhead. Consider dropping the explicit token index (keep the user_id index if needed for lookups).

Suggested change
CREATE INDEX IF NOT EXISTS idx_password_reset_tokens_token ON password_reset_tokens (token);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this

Comment on lines +220 to +221
slog.Warn("password reset link (queue not configured — use this URL to reset)",
"email", *user.Email, "reset_url", resetURL)
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Queue is nil, this logs the full reset URL (including the raw reset token). Password-reset tokens are secrets; emitting them to logs is a security risk if logs are accessible. Avoid logging the token/reset URL (or log only that a reset was requested, and provide the URL via a safer dev-only mechanism).

Suggested change
slog.Warn("password reset link (queue not configured — use this URL to reset)",
"email", *user.Email, "reset_url", resetURL)
slog.Warn("password reset requested but email queue is not configured",
"email", *user.Email)

Copilot uses AI. Check for mistakes.
onClick={() => setShowPassword((p) => !p)}
className="absolute right-2.5 top-1/2 -translate-y-1/2 text-(--txt-icon-tertiary) hover:text-(--txt-secondary)"
aria-label={showPassword ? 'Hide password' : 'Show password'}
tabIndex={-1}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These show/hide password buttons are removed from the tab order via tabIndex={-1}, which prevents keyboard-only users from toggling password visibility. Prefer leaving them focusable (default tabIndex) so the control is accessible, and rely on styling to avoid unwanted focus rings if needed.

Suggested change
tabIndex={-1}

Copilot uses AI. Check for mistakes.
onClick={() => setShowConfirm((p) => !p)}
className="absolute right-2.5 top-1/2 -translate-y-1/2 text-(--txt-icon-tertiary) hover:text-(--txt-secondary)"
aria-label={showConfirm ? 'Hide password' : 'Show password'}
tabIndex={-1}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue for the confirm-password reveal button: tabIndex={-1} makes it unreachable by keyboard navigation. Keep it in the tab order to meet basic accessibility expectations for form controls.

Suggested change
tabIndex={-1}

Copilot uses AI. Check for mistakes.
Comment on lines +226 to +232
<button
type="button"
onClick={() => setShowPassword((p) => !p)}
className="absolute right-2.5 top-1/2 -translate-y-1/2 text-(--txt-icon-tertiary) hover:text-(--txt-secondary)"
aria-label={showPassword ? 'Hide password' : 'Show password'}
tabIndex={-1}
>
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The password reveal button is removed from the tab order via tabIndex={-1}, preventing keyboard users from toggling visibility. Leave it focusable (or provide an alternate keyboard-accessible mechanism).

Copilot uses AI. Check for mistakes.
onClick={() => setShowConfirm((p) => !p)}
className="absolute right-2.5 top-1/2 -translate-y-1/2 text-(--txt-icon-tertiary) hover:text-(--txt-secondary)"
aria-label={showConfirm ? 'Hide password' : 'Show password'}
tabIndex={-1}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same accessibility issue for the confirm-password reveal button: tabIndex={-1} makes it unreachable via keyboard navigation. It should remain in the tab order.

Suggested change
tabIndex={-1}

Copilot uses AI. Check for mistakes.
@martian56 martian56 requested review from a team, Javenn0 and Rafetikus April 4, 2026 09:58
Copy link
Copy Markdown
Member

@martian56 martian56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when email settings are not set, worker gets stuck in infinite loop

created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
);
CREATE INDEX IF NOT EXISTS idx_password_reset_tokens_user ON password_reset_tokens (user_id);
CREATE INDEX IF NOT EXISTS idx_password_reset_tokens_token ON password_reset_tokens (token);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this

Copy link
Copy Markdown
Member

@Javenn0 Javenn0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done

@Javenn0 Javenn0 requested a review from martian56 April 4, 2026 11:04
Copy link
Copy Markdown
Member

@Rafetikus Rafetikus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

Copy link
Copy Markdown
Member Author

@nazarli-shabnam nazarli-shabnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@Javenn0 Javenn0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Job!

Copy link
Copy Markdown
Member

@Javenn0 Javenn0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: Implement forgot password auth

5 participants