Feat/login with google 2#25
Open
OffCrazyFreak wants to merge 4 commits into
Open
Conversation
The existing custom JWT system (JwtService, refresh tokens in DB,
localStorage access token on frontend) is replaced with better-auth
acting as the identity provider in Next.js; Spring becomes an OAuth2
resource server validating JWTs via JWKS.
Changes:
- frontend: better-auth 1.6.14 + Drizzle/pg adapter; kysely pinned
to 0.28.17 via pnpm-workspace.yaml to resolve Turbopack import bug
- frontend: src/lib/auth.ts (betterAuth config), src/lib/auth-client.ts,
src/app/api/auth/[...all]/route.ts, src/db/ (Drizzle schema + index)
- frontend: api-base.ts rewritten with in-memory JWT cache + backoff
(replaces localStorage token + refresh-cookie flow)
- frontend: user-context.tsx session-driven via useSession(); same
IUserContext surface preserved so all ~22 consumers unchanged
- frontend: login/signup forms use signIn.email / signUp.email (email
field replaces usernameOrEmail; name field added to signup); auth-modal
Google button uses signIn.social; account-details drops stayLoggedInDays,
uses authClient.revokeOtherSessions + two-step delete
- frontend: watchlist + shopping-list hooks gain { enabled } option;
notifications-context gates watchlist query on isAuthenticated;
add-to-shopping-list-form gates its query on isOpen && !!user
- frontend: old src/lib/api/auth/ deleted; accessToken removed from
localStorage typings and helpers
- backend: spring-boot-starter-oauth2-resource-server added; jjwt,
google-api-client, spring-security-oauth2-client removed
- backend: SecurityConfig uses oauth2ResourceServer().jwt() + ES256 JWKS
- backend: entire disscount.auth.* package deleted; JwtAuthenticationFilter,
PublicEndpointConfig, PasswordValidator, EmailValidator deleted
- backend: User entity — @GeneratedValue removed (ID assigned by
better-auth), email nullable, passwordHash/googleId/stayLoggedInDays/
lastLoginAt removed
- backend: UserService adds ensureActiveProfile (lazy upsert on every
request) and deleteAccount (anonymize + soft-delete)
- backend: UserProvisioningFilter runs after BearerTokenAuthenticationFilter
to provision profile from JWT claims
- backend: tests updated to use explicit UUID, no passwordHash
Notes:
- TODOs in auth.ts: Resend forgot-password, kysely override removal,
Facebook provider
The "Ime" field was unnecessary since users complete an onboarding flow post-signup; the email prefix is used as a placeholder name. The last-used badge helps users immediately recognise which method they previously used. Changes: - Remove name field from registerRequestSchema and SignUpForm; auto-derive from email prefix - Add lastLoginMethod to AppData type with get/set helpers in local-storage utils - LoginForm: show "Zadnja prijava" badge when isLastUsed prop is true; record method on success - SignUpForm: record email as last login method on successful registration - AuthModal: read lastLoginMethod on open, pass to LoginForm, float badge on Google button; set google method before redirect
❌ Deploy Preview for disscount failed. Why did it fail? →
|
Contributor
WalkthroughMigrate backend to OAuth2 resource-server (JWKS/ES256) and add a frontend better-auth integration with Drizzle/Postgres; remove the custom JWT/auth service and update APIs, DTOs, UI flows, and client token handling. ChangesAuthentication platform shift
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/products/components/forms/add-to-shopping-list-form.tsx (1)
3-3:⚠️ Potential issue | 🔴 CriticalFix incorrect
Activityimport:Activityis imported from"react"(import { useState, useEffect, Activity } from "react";) but React doesn’t export it; importActivityfrom"lucide-react"instead (alongside the existingSaveimport) and remove it from the React import.import { useState, useEffect } from "react"; import { Save, Activity } from "lucide-react";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/products/components/forms/add-to-shopping-list-form.tsx` at line 3, The import mistakenly pulls Activity from React; remove Activity from the React import (leave useState and useEffect) and add Activity to the lucide-react import alongside Save (i.e., import Save and Activity from "lucide-react"); update the import statement that currently reads import { useState, useEffect, Activity } from "react" to only include useState and useEffect and add Activity to the existing lucide-react import so components using Activity and Save reference the correct symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 72: Fix the typo in the README line that references the lockfile: replace
the misspelled "packege-lock.json" with the correct "package-lock.json" in the
AGENTS.md entry so the guidance reads "Never edit the package.json or
package-lock.json files directly, but instead use 'pnpm add ...' or 'pnpm remove
...' to manage dependencies."
- Around line 23-30: The AGENTS.md dependency list needs an explicit
confirmation of security-advisory impact and a note about checking for newer
stable releases: verify GitHub Advisory Database and npm advisories for
better-auth@1.6.14, `@daveyplate/better-auth-ui`@^3.2.13, drizzle-orm@0.45.2,
drizzle-kit@^0.31.10, and pg@8.21.0; record that better-auth@1.6.14 has no
applicable NPM vulnerabilities (nearest HIGH was patched in 1.6.11),
drizzle-orm@0.45.2 has the fix included, and pg@8.21.0 has no applicable
advisories, and explicitly state that no advisories were found for
`@daveyplate/better-auth-ui`@^3.2.13 or drizzle-kit@^0.31.10; also check npm
registry/changelogs for newer stable releases and append the confirmation, date
checked, and any recommended upgrade actions to AGENTS.md so the dependency
block documents the security status and next steps.
In `@backend/src/main/java/disscount/config/UserProvisioningFilter.java`:
- Around line 38-42: The current catch in UserProvisioningFilter around
userService.ensureActiveProfile(UUID.fromString(sub), email) swallows all
exceptions and lets the request continue; change it to fail-closed by logging
the error and aborting the request instead of proceeding: either rethrow a
ServletException/RuntimeException or call
HttpServletResponse.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
"User provisioning failed") (and return) so downstream handlers do not run
without a synchronized user row; keep the log but include the exception, and
apply this behavior for parsing and provisioning failures arising from
ensureActiveProfile or UUID.fromString.
In `@backend/src/main/java/disscount/user/service/UserService.java`:
- Around line 81-93: The current deleteAccount(UUID userId) only nulls PII and
sets deletedAt on the User row but leaves auth identity deletion to the
frontend, which allows ensureActiveProfile() to revive the account; update
deleteAccount to perform the external auth deletion (call
authClient.deleteUser(...) synchronously or via a transactional/outbox pattern)
and only save the anonymized user if the auth-provider deletion succeeds, or
alternatively persist an irreversible tombstone flag (e.g.
accountDeletedPermanent or accountProvisioningState = DELETED) on User so
ensureActiveProfile() and provisioning logic refuse to revive the row; ensure to
handle failures by rolling back the DB changes or queuing a durable retry and
update references to ensureActiveProfile() to respect the new tombstone field.
- Around line 34-55: ensureActiveProfile has a race: two concurrent calls can
both see userRepository.findById(id) empty and try to insert, causing a
duplicate-key 500; replace the read-then-write with an atomic upsert or handle
duplicate-key on save by wrapping the insert path in a transaction and catching
the DB duplicate-key/DataIntegrityViolationException (or DuplicateKeyException),
then re-read userRepository.findById(id) and apply the same post-insert updates
(e.g., clear deletedAt or set email) if necessary; alternatively add a
repository-level upsert/merge method (or use JPA save with proper unique
constraints and locking) so that ensureActiveProfile (and callers like
UserProvisioningFilter) never fails on concurrent first-login attempts.
In `@backend/src/main/resources/application.properties`:
- Around line 15-17: Replace the current permissive default for
spring.security.oauth2.resourceserver.jwt.jwk-set-uri so the app fails startup
when BETTER_AUTH_JWKS_URI is not provided: remove the inline default value
(currently "http://localhost:3000/api/auth/jwks") and reference the environment
property only (BETTER_AUTH_JWKS_URI) so Spring will error on missing property;
move the local-dev fallback into a profile-specific properties file (e.g.,
application-local.properties) where you set the local JWKS URI for the dev
profile, ensuring production/non-local profiles have no default and will refuse
to start without BETTER_AUTH_JWKS_URI.
- Around line 15-17: The JWT resource-server config only sets jwk-set-uri and
jws-algorithms and therefore does not validate issuer/audience; update the
configuration so the OAuth2 Resource Server enforces iss/aud checks — either add
spring.security.oauth2.resourceserver.jwt.issuer-uri and
spring.security.oauth2.resourceserver.jwt.audiences properties (or equivalent
issuer and audience values) or replace oauth2.jwt(Customizer.withDefaults())
with a custom JwtDecoder/Validator wiring that adds JwtValidators for the
expected issuer and audience; locate the jwt setup in your security
configuration (the oauth2.jwt(Customizer.withDefaults()) call) and the existing
properties spring.security.oauth2.resourceserver.jwt.jwk-set-uri and
jws-algorithms to implement the change.
In `@frontend/.npmrc`:
- Line 1: The .npmrc currently forces pnpm to accept brand-new package releases
by setting minimum-release-age=0; either remove this line from frontend/.npmrc
or change the value to a safer minimum (e.g., set minimum-release-age=3) so pnpm
will block packages younger than the given days and reduce supply-chain risk;
update the repository accordingly and ensure any CI/dev docs reflect the change.
In `@frontend/drizzle.config.ts`:
- Around line 10-12: The config currently uses a non-null assertion
process.env.DATABASE_URL! inside dbCredentials which will crash if the env var
is missing; replace the assertion with explicit validation in the drizzle config
(e.g., read DATABASE_URL into a const, check if it's falsy, and throw a clear
error or provide a safe fallback) so migrations/tools fail fast with a helpful
message; update the symbol references around dbCredentials and any functions
that import it to rely on the validated value instead of using
process.env.DATABASE_URL! directly.
In `@frontend/drizzle/meta/0000_snapshot.json`:
- Around line 18-29: The account table is missing a composite index on
provider_id and account_id which hurts OAuth account lookup performance; update
your schema (the account table definition in auth-schema, i.e., the account
model) to add a composite index (e.g., named account_provider_account_idx) on
account.providerId and account.accountId using the schema builder's
.index(...)/.on(...) API, then regenerate the migration so the new index is
created in the database.
In `@frontend/pnpm-workspace.yaml`:
- Around line 3-10: The allowBuilds block currently lists packages (allowBuilds,
esbuild, sharp, core-js, unrs-resolver, '`@nestjs/core`',
'`@openapitools/openapi-generator-cli`', '`@tailwindcss/oxide`') without rationale;
add short inline comments or a top-line comment above the allowBuilds key
documenting why build scripts are permitted for each listed package (e.g.,
native binaries for sharp, bundler/runtime needs for esbuild and '`@nestjs/core`',
codegen for '`@openapitools/openapi-generator-cli`', tailwind processor for
'`@tailwindcss/oxide`', runtime resolution for unrs-resolver, polyfill reasons for
core-js), and note any security review or acceptance criteria that justify
deviating from default pnpm restrictions.
In `@frontend/src/components/custom/header/forms/account-details-modal.tsx`:
- Around line 111-125: Both handleLogoutAll and handleDeleteUser currently catch
all errors and show generic toasts; update their catch blocks to inspect the
thrown error (e.g., use isAxiosError(error) or check error.response?.status and
message, or fallback to error.message) and display specific toasts for network
failures, 401/403 auth errors, and server errors; reference
authClient.revokeOtherSessions and setIsRevoking in handleLogoutAll, and
userService.deleteCurrentUser, authClient.deleteUser and setIsDeleting in
handleDeleteUser when implementing the differentiated error handling and ensure
the finally blocks still reset the loading flags and the success paths keep
their existing toast/onOpenChange behavior.
- Around line 112-113: The native confirm() calls inside the AccountDetailsModal
component should be replaced with a non-blocking, styled confirmation modal;
create or reuse a ConfirmDialog/Modal component and add local state (e.g.,
showConfirm, pendingAction) in AccountDetailsModal to open it instead of calling
confirm(), pass the confirmation text ("Jeste li sigurni..."), and execute the
sign-out-from-all-devices or other actions only when the modal's onConfirm
callback fires; ensure to wire onCancel to close the modal without side effects
and import the shared modal component used elsewhere in the app for consistent
styling.
- Around line 127-155: The current handleDeleteUser flow can leave an anonymized
backend profile if authClient.deleteUser() fails; change the sequence to remove
the auth identity first and only then call userService.deleteCurrentUser(),
i.e., call authClient.deleteUser() before userService.deleteCurrentUser(), or if
you must keep the current order implement compensating rollback by calling a
backend restore method when authClient.deleteUser() throws; update the try/catch
around userService.deleteCurrentUser and authClient.deleteUser in
handleDeleteUser to perform the new order or to call the rollback endpoint on
failure, then proceed to clearAuthToken(), queryClient.clear(), setUser(null)
and the success/error toasts as appropriate.
In `@frontend/src/components/custom/header/forms/auth-modal.tsx`:
- Around line 52-60: Add a re-entry guard to prevent double submission in
handleGoogleSignIn: create a local boolean state or ref (e.g.,
isGoogleSigningIn) and check it at the top of handleGoogleSignIn to return early
if already true, set it to true immediately before calling await signIn.social({
provider: "google", callbackURL: "/" }), and reset it to false in a finally
block so it always clears on success/error; also use that same guard state to
disable the Google sign-in button in the component UI. Apply the identical
pattern to the other OAuth handler(s) around lines 99-103 (the same click
handler that calls signIn.social) so all social sign-in flows are protected from
double-clicks.
In `@frontend/src/components/custom/header/forms/login-form.tsx`:
- Around line 64-68: The current flow displays toast.success, resets the form,
and calls setLastLoginMethod("email") before awaiting handleUserLogin(), which
can show success even if session hydration fails; update the logic in
login-form.tsx so that toast.success("Prijava uspješna!"), form.reset(),
setLastLoginMethod("email"), and onSuccess?.() are executed only after await
handleUserLogin() completes without error (i.e., move these success-only side
effects to after the successful completion of handleUserLogin()), and ensure any
errors from handleUserLogin prevent those side effects from running.
In `@frontend/src/components/custom/header/forms/signup-form.tsx`:
- Around line 64-68: The UI shows success/toast/reset before post-signup login
completes; move the success UI updates (toast.success("Uspješno ste se
registrirali!"), form.reset(), setLastLoginMethod("email")) so they run only
after await handleUserLogin() resolves successfully (and after onSuccess?.() if
that must run post-login), leaving error handling for handleUserLogin() to
surface failures without having already cleared the form or shown success;
update the sequence around handleUserLogin, toast.success, form.reset,
setLastLoginMethod, and onSuccess to ensure the account creation + UI update is
atomic from the user's perspective.
In `@frontend/src/context/user-context.tsx`:
- Around line 17-20: Change IUserContext.logout's signature from () => void to
() => Promise<void> so callers await failures; update the implementation of
handleLogout to ensure authClient.signOut() is awaited and that local cleanup
(clearing user state, pinned stores/places, tokens) runs inside a finally block
so it always executes even if signOut rejects; update any occurrences/usages
noted around handleLogout and the context provider to return the Promise and
adjust callers as needed.
- Around line 32-64: refreshUser currently awaits multiple async calls and
unconditionally calls setUser/setIsLoading, allowing stale responses from a
previous session to overwrite the current user; fix by adding a generation/token
guard (e.g., a useRef requestIdRef that you increment at the start of
refreshUser and capture as localRequestId) and before any state mutation
(setUser, setIsLoading) verify requestIdRef.current === localRequestId,
returning early if not; apply the same pattern to the other async callback
mentioned around lines 67-79 so late promises are discarded when the session
changes rather than mutating state.
In `@frontend/src/db/auth-schema.ts`:
- Around line 20-23: Remove the unnecessary /* `@__PURE__` */ annotations from the
Drizzle schema $onUpdate callbacks in frontend/src/db/auth-schema.ts: locate the
.$onUpdate(() => /* `@__PURE__` */ new Date()) usages (e.g., the updatedAt
definition and the three other occurrences) and delete only the comment tokens
so the callbacks become .$onUpdate(() => new Date()) while leaving the rest of
the schema code unchanged.
In `@frontend/src/db/index.ts`:
- Line 6: Validate process.env.DATABASE_URL before constructing the Pool so
module initialization fails fast: check that process.env.DATABASE_URL is set and
non-empty in frontend/src/db/index.ts and, if missing/invalid, throw an explicit
error or call process.exit with a clear message; only then instantiate new
Pool({ connectionString: process.env.DATABASE_URL }) to avoid obscure runtime
query errors.
In `@frontend/src/lib/api/api-base.ts`:
- Around line 45-53: The 401-retry path currently calls clearAuthToken which
sets tokenBackoffUntil and prevents getToken() from fetching a new token; change
the retry logic to call resetAuthToken() instead of clearAuthToken so
tokenBackoffUntil is cleared and getToken() can attempt a fresh /api/auth/token
request; update the same replacement in the other retry block referenced (lines
~76-83) and verify usage of getToken(), clearAuthToken(), and resetAuthToken()
so only resetAuthToken() is used when forcing an immediate retry after a 401.
In `@frontend/src/lib/auth-client.ts`:
- Around line 4-6: The auth client currently falls back to a hardcoded dev
origin; update the baseURL logic in authClient/createAuthClient so it does not
default to "http://localhost:3000". Use the NEXT_PUBLIC_APP_URL when provided,
otherwise use the browser's origin at runtime (e.g., window.location.origin) or
leave baseURL undefined server-side so requests are same-origin; specifically
change the baseURL expression used when constructing authClient (the baseURL
passed into createAuthClient in frontend/src/lib/auth-client.ts) from the
hardcoded fallback to process.env.NEXT_PUBLIC_APP_URL ?? (typeof window !==
"undefined" ? window.location.origin : undefined).
In `@frontend/src/lib/auth.ts`:
- Around line 31-38: The account linking config currently allows implicit Google
linking for unverified local accounts; update the account.accountLinking
settings by restoring the local-email safety gate: set requireLocalEmailVerified
back to true (or remove "google" from trustedProviders) in the accountLinking
object so a Google login cannot automatically attach to an unverified local
account until the Resend email verification flow is implemented; locate the
accountLinking block (symbols: account, accountLinking, trustedProviders,
requireLocalEmailVerified) and change the flag accordingly.
---
Outside diff comments:
In `@frontend/src/app/products/components/forms/add-to-shopping-list-form.tsx`:
- Line 3: The import mistakenly pulls Activity from React; remove Activity from
the React import (leave useState and useEffect) and add Activity to the
lucide-react import alongside Save (i.e., import Save and Activity from
"lucide-react"); update the import statement that currently reads import {
useState, useEffect, Activity } from "react" to only include useState and
useEffect and add Activity to the existing lucide-react import so components
using Activity and Save reference the correct symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10fc9ef0-ca1f-4f4c-abd1-8ebb732bf733
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
AGENTS.mdbackend/pom.xmlbackend/src/main/java/disscount/auth/dao/RefreshTokenRepository.javabackend/src/main/java/disscount/auth/domain/RefreshToken.javabackend/src/main/java/disscount/auth/dto/AuthResponse.javabackend/src/main/java/disscount/auth/dto/GoogleAuthRequest.javabackend/src/main/java/disscount/auth/dto/LoginRequest.javabackend/src/main/java/disscount/auth/dto/RegisterRequest.javabackend/src/main/java/disscount/auth/rest/AuthController.javabackend/src/main/java/disscount/auth/service/AuthService.javabackend/src/main/java/disscount/auth/service/JwtService.javabackend/src/main/java/disscount/config/JwtAuthenticationFilter.javabackend/src/main/java/disscount/config/PublicEndpointConfig.javabackend/src/main/java/disscount/config/SecurityConfig.javabackend/src/main/java/disscount/config/UserProvisioningFilter.javabackend/src/main/java/disscount/user/dao/UserRepository.javabackend/src/main/java/disscount/user/domain/User.javabackend/src/main/java/disscount/user/dto/UserDto.javabackend/src/main/java/disscount/user/dto/UserRequest.javabackend/src/main/java/disscount/user/rest/UserController.javabackend/src/main/java/disscount/user/service/UserService.javabackend/src/main/java/disscount/util/EmailValidator.javabackend/src/main/java/disscount/util/PasswordValidator.javabackend/src/main/resources/application.propertiesbackend/src/test/java/disscount/shoppingList/rest/ShoppingListControllerTest.javafrontend/.npmrcfrontend/drizzle.config.tsfrontend/drizzle/0000_stale_mister_sinister.sqlfrontend/drizzle/meta/0000_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/next.config.tsfrontend/package.jsonfrontend/pnpm-workspace.yamlfrontend/src/app/api/auth/[...all]/route.tsfrontend/src/app/products/components/forms/add-to-shopping-list-form.tsxfrontend/src/components/custom/header/forms/account-details-modal.tsxfrontend/src/components/custom/header/forms/auth-modal.tsxfrontend/src/components/custom/header/forms/login-form.tsxfrontend/src/components/custom/header/forms/signup-form.tsxfrontend/src/context/notifications-context.tsxfrontend/src/context/user-context.tsxfrontend/src/db/auth-schema.tsfrontend/src/db/index.tsfrontend/src/lib/api/api-base.tsfrontend/src/lib/api/auth/index.tsfrontend/src/lib/api/index.tsfrontend/src/lib/api/schemas/auth-user.tsfrontend/src/lib/api/shopping-lists/index.tsfrontend/src/lib/api/types.tsfrontend/src/lib/api/users/index.tsfrontend/src/lib/api/watchlist/index.tsfrontend/src/lib/auth-client.tsfrontend/src/lib/auth.tsfrontend/src/typings/local-storage.tsfrontend/src/utils/browser/local-storage.ts
💤 Files with no reviewable changes (18)
- backend/src/main/java/disscount/auth/dao/RefreshTokenRepository.java
- backend/src/main/java/disscount/auth/dto/RegisterRequest.java
- backend/src/main/java/disscount/auth/dto/GoogleAuthRequest.java
- frontend/src/lib/api/index.ts
- backend/src/main/java/disscount/util/PasswordValidator.java
- backend/src/main/java/disscount/config/PublicEndpointConfig.java
- frontend/src/lib/api/auth/index.ts
- backend/src/main/java/disscount/auth/rest/AuthController.java
- backend/src/main/java/disscount/util/EmailValidator.java
- backend/src/main/java/disscount/user/dto/UserRequest.java
- backend/src/main/java/disscount/auth/domain/RefreshToken.java
- backend/src/main/java/disscount/auth/service/AuthService.java
- backend/src/main/java/disscount/auth/dto/AuthResponse.java
- backend/src/main/java/disscount/config/JwtAuthenticationFilter.java
- backend/src/main/java/disscount/auth/dto/LoginRequest.java
- backend/src/main/java/disscount/auth/service/JwtService.java
- backend/src/main/java/disscount/user/dao/UserRepository.java
- frontend/src/lib/api/types.ts
Comment on lines
+23
to
+30
| - better-auth: `1.6.14` | ||
| - @daveyplate/better-auth-ui: `^3.2.13` | ||
|
|
||
| **Database (frontend — better-auth identity store):** | ||
|
|
||
| - drizzle-orm: `0.45.2` | ||
| - drizzle-kit: `^0.31.10` | ||
| - pg: `8.21.0` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for security advisories for auth and database packages
echo "Checking better-auth..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "better-auth") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
echo "Checking drizzle-orm..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "drizzle-orm") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
echo "Checking pg..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "pg") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: OffCrazyFreak/Disscount
Length of output: 3173
🏁 Script executed:
#!/bin/bash
# Description: Check for security advisories for missing auth/database packages + broaden pg results
check_pkg () {
local pkg="$1"
echo "Checking $pkg..."
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: NPM, package: $pkg) {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}' -f pkg="$pkg"
}
check_pkg "better-auth"
check_pkg "`@daveyplate/better-auth-ui`"
check_pkg "drizzle-orm"
check_pkg "drizzle-kit"
check_pkg "pg"Repository: OffCrazyFreak/Disscount
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Description: Check for security advisories for missing auth/database packages + broaden pg results
check_pkg () {
local pkg="$1"
echo "Checking $pkg..."
gh api graphql -f pkg="$pkg" -f query='
query($pkg: String!) {
securityVulnerabilities(first: 20, ecosystem: NPM, package: $pkg) {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}'
}
check_pkg "better-auth"
check_pkg "`@daveyplate/better-auth-ui`"
check_pkg "drizzle-orm"
check_pkg "drizzle-kit"
check_pkg "pg"Repository: OffCrazyFreak/Disscount
Length of output: 6775
Confirm security advisory impact for the AGENTS.md dependency versions.
- GitHub advisory data shows no known NPM vulnerabilities apply to
better-auth@1.6.14(nearest HIGH issue is patched in1.6.11),drizzle-orm@0.45.2(issue fixed at0.45.2), orpg@8.21.0. - No NPM advisories are listed for
@daveyplate/better-auth-ui@^3.2.13ordrizzle-kit@^0.31.10. - Still check separately whether newer stable releases exist (not covered by the advisory lookup).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 23 - 30, The AGENTS.md dependency list needs an
explicit confirmation of security-advisory impact and a note about checking for
newer stable releases: verify GitHub Advisory Database and npm advisories for
better-auth@1.6.14, `@daveyplate/better-auth-ui`@^3.2.13, drizzle-orm@0.45.2,
drizzle-kit@^0.31.10, and pg@8.21.0; record that better-auth@1.6.14 has no
applicable NPM vulnerabilities (nearest HIGH was patched in 1.6.11),
drizzle-orm@0.45.2 has the fix included, and pg@8.21.0 has no applicable
advisories, and explicitly state that no advisories were found for
`@daveyplate/better-auth-ui`@^3.2.13 or drizzle-kit@^0.31.10; also check npm
registry/changelogs for newer stable releases and append the confirmation, date
checked, and any recommended upgrade actions to AGENTS.md so the dependency
block documents the security status and next steps.
Comment on lines
+38
to
+42
| try { | ||
| userService.ensureActiveProfile(UUID.fromString(sub), email); | ||
| } catch (Exception e) { | ||
| log.warn("User provisioning failed for sub={}: {}", sub, e.getMessage()); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail closed when profile provisioning breaks.
This filter is supposed to guarantee that an authenticated JWT maps to an active local profile, but it catches every exception—including invalid sub parsing—and still lets the request continue. That leaves downstream handlers operating on an authenticated principal with no synchronized user row.
Suggested change
try {
userService.ensureActiveProfile(UUID.fromString(sub), email);
+ } catch (IllegalArgumentException e) {
+ log.warn("Invalid JWT subject for provisioning: {}", sub, e);
+ response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid token subject");
+ return;
} catch (Exception e) {
- log.warn("User provisioning failed for sub={}: {}", sub, e.getMessage());
+ log.error("User provisioning failed for sub={}", sub, e);
+ response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "User provisioning failed");
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| userService.ensureActiveProfile(UUID.fromString(sub), email); | |
| } catch (Exception e) { | |
| log.warn("User provisioning failed for sub={}: {}", sub, e.getMessage()); | |
| } | |
| try { | |
| userService.ensureActiveProfile(UUID.fromString(sub), email); | |
| } catch (IllegalArgumentException e) { | |
| log.warn("Invalid JWT subject for provisioning: {}", sub, e); | |
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid token subject"); | |
| return; | |
| } catch (Exception e) { | |
| log.error("User provisioning failed for sub={}", sub, e); | |
| response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "User provisioning failed"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/disscount/config/UserProvisioningFilter.java` around
lines 38 - 42, The current catch in UserProvisioningFilter around
userService.ensureActiveProfile(UUID.fromString(sub), email) swallows all
exceptions and lets the request continue; change it to fail-closed by logging
the error and aborting the request instead of proceeding: either rethrow a
ServletException/RuntimeException or call
HttpServletResponse.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
"User provisioning failed") (and return) so downstream handlers do not run
without a synchronized user row; keep the log but include the exception, and
apply this behavior for parsing and provisioning failures arising from
ensureActiveProfile or UUID.fromString.
Comment on lines
+34
to
+55
| public void ensureActiveProfile(UUID id, String email) { | ||
| if (email == null) return; | ||
|
|
||
| Optional<User> existing = userRepository.findById(id); | ||
| if (existing.isPresent()) { | ||
| User user = existing.get(); | ||
| boolean changed = false; | ||
|
|
||
| if (user.getDeletedAt() != null) { | ||
| user.setDeletedAt(null); | ||
| changed = true; | ||
| } | ||
| if (user.getEmail() == null) { | ||
| user.setEmail(email); | ||
| changed = true; | ||
| } | ||
| if (changed) { | ||
| userRepository.save(user); | ||
| } | ||
| } else { | ||
| userRepository.save(User.builder().id(id).email(email).build()); | ||
| } |
Contributor
There was a problem hiding this comment.
ensureActiveProfile still has a first-login race.
Because this method runs on every authenticated request, two concurrent requests for a brand-new user can both see findById(id) as empty and then both execute the insert path. One of them will lose on the primary-key/unique constraint and surface as a 500 during login warm-up. Replace the read-then-write pair with an actual upsert/insert-if-absent, or catch duplicate-key failures and re-read inside the transaction. Based on the cohort context, this code is invoked from UserProvisioningFilter on every authenticated request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/disscount/user/service/UserService.java` around lines
34 - 55, ensureActiveProfile has a race: two concurrent calls can both see
userRepository.findById(id) empty and try to insert, causing a duplicate-key
500; replace the read-then-write with an atomic upsert or handle duplicate-key
on save by wrapping the insert path in a transaction and catching the DB
duplicate-key/DataIntegrityViolationException (or DuplicateKeyException), then
re-read userRepository.findById(id) and apply the same post-insert updates
(e.g., clear deletedAt or set email) if necessary; alternatively add a
repository-level upsert/merge method (or use JPA save with proper unique
constraints and locking) so that ensureActiveProfile (and callers like
UserProvisioningFilter) never fails on concurrent first-login attempts.
Comment on lines
+81
to
93
| /** | ||
| * Anonymizes the profile: nulls PII fields and soft-deletes. | ||
| * The row is kept so business data (watchlists, shopping lists, etc.) remains intact. | ||
| * better-auth identity must be deleted separately via authClient.deleteUser() on the frontend. | ||
| */ | ||
| public void deleteAccount(UUID userId) { | ||
| User user = userRepository.findById(userId) | ||
| .filter(u -> u.getDeletedAt() == null) | ||
| .orElseThrow(() -> new BadRequestException("User not found")); | ||
|
|
||
| user.setUsername(null); | ||
| user.setEmail(null); | ||
| user.setDeletedAt(LocalDateTime.now()); | ||
| userRepository.save(user); |
Contributor
There was a problem hiding this comment.
Account deletion can be undone by the next authenticated request.
This method only anonymizes the local row and explicitly leaves auth-identity deletion to a separate frontend step. If that second step fails after this method succeeds, ensureActiveProfile() will clear deletedAt and repopulate email on the next bearer-authenticated request, so the backend can report a successful deletion that does not stick. The backend needs to own the auth-provider deletion in the same flow, or persist a terminal tombstone that provisioning refuses to revive implicitly. Based on the inline contract here and the provisioning behavior documented above.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/disscount/user/service/UserService.java` around lines
81 - 93, The current deleteAccount(UUID userId) only nulls PII and sets
deletedAt on the User row but leaves auth identity deletion to the frontend,
which allows ensureActiveProfile() to revive the account; update deleteAccount
to perform the external auth deletion (call authClient.deleteUser(...)
synchronously or via a transactional/outbox pattern) and only save the
anonymized user if the auth-provider deletion succeeds, or alternatively persist
an irreversible tombstone flag (e.g. accountDeletedPermanent or
accountProvisioningState = DELETED) on User so ensureActiveProfile() and
provisioning logic refuse to revive the row; ensure to handle failures by
rolling back the DB changes or queuing a durable retry and update references to
ensureActiveProfile() to respect the new tombstone field.
Comment on lines
+20
to
+23
| updatedAt: timestamp("updated_at") | ||
| .defaultNow() | ||
| .$onUpdate(() => /* @__PURE__ */ new Date()) | ||
| .notNull(), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove unnecessary /* @pure */ comments.
The /* @PURE */ annotation is a bundler hint for tree-shaking and serves no purpose in Drizzle schema definitions. These comments can be safely removed for cleaner code.
♻️ Proposed cleanup
updatedAt: timestamp("updated_at")
.defaultNow()
- .$onUpdate(() => /* `@__PURE__` */ new Date())
+ .$onUpdate(() => new Date())
.notNull(),Apply this pattern to all four occurrences (lines 22, 36, 67, 85).
Also applies to: 35-37, 66-68, 83-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/db/auth-schema.ts` around lines 20 - 23, Remove the unnecessary
/* `@__PURE__` */ annotations from the Drizzle schema $onUpdate callbacks in
frontend/src/db/auth-schema.ts: locate the .$onUpdate(() => /* `@__PURE__` */ new
Date()) usages (e.g., the updatedAt definition and the three other occurrences)
and delete only the comment tokens so the callbacks become .$onUpdate(() => new
Date()) while leaving the rest of the schema code unchanged.
Comment on lines
+4
to
+6
| export const authClient = createAuthClient({ | ||
| baseURL: process.env.NEXT_PUBLIC_APP_URL ?? "http://localhost:3000", | ||
| plugins: [jwtClient()], |
Contributor
There was a problem hiding this comment.
Avoid a localhost fallback in the browser.
If NEXT_PUBLIC_APP_URL is missing outside local development, every auth request goes to http://localhost:3000 on the end user's machine, which breaks sign-in, sign-out, and session calls. Prefer a same-origin default instead of a hardcoded dev origin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/lib/auth-client.ts` around lines 4 - 6, The auth client
currently falls back to a hardcoded dev origin; update the baseURL logic in
authClient/createAuthClient so it does not default to "http://localhost:3000".
Use the NEXT_PUBLIC_APP_URL when provided, otherwise use the browser's origin at
runtime (e.g., window.location.origin) or leave baseURL undefined server-side so
requests are same-origin; specifically change the baseURL expression used when
constructing authClient (the baseURL passed into createAuthClient in
frontend/src/lib/auth-client.ts) from the hardcoded fallback to
process.env.NEXT_PUBLIC_APP_URL ?? (typeof window !== "undefined" ?
window.location.origin : undefined).
Comment on lines
+31
to
+38
| account: { | ||
| accountLinking: { | ||
| enabled: true, | ||
| trustedProviders: ["google"], | ||
| // TODO: remove requireLocalEmailVerified:false once Resend email verification is wired up | ||
| // Both flags are required: trustedProviders clears the provider check, this clears the local-email check | ||
| requireLocalEmailVerified: false, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the config file with line numbers
sed -n '1,120p' frontend/src/lib/auth.ts | cat -n
# Show just the referenced lines (31-50) with more context
sed -n '25,55p' frontend/src/lib/auth.ts | cat -n
# Search for accountLinking / trustedProviders / requireLocalEmailVerified usage
rg -n "accountLinking|trustedProviders|requireLocalEmailVerified" frontend/src/lib frontend/src
# Show Better Auth version from package.json/lock if present
ls -la frontend/package.json 2>/dev/null || true
rg -n "\"better-auth\"" -S frontend/package.json frontend/pnpm-lock.yaml pnpm-lock.yaml 2>/dev/null || trueRepository: OffCrazyFreak/Disscount
Length of output: 1931
🌐 Web query:
Better Auth docs accountLinking trustedProviders requireLocalEmailVerified
💡 Result:
In Better Auth, the account linking configuration provides security controls to manage how social or OAuth identities are associated with existing local accounts [1][2]. requireLocalEmailVerified This option is a security gate introduced to prevent account hijacking (nOAuth-class vulnerabilities) where an attacker might attempt to link an unauthorized OAuth identity to an existing local account [3][4]. Default value: true [3][4]. Behavior: When set to true, Better Auth blocks implicit account linking unless the local user record already has emailVerified set to true [3][4]. Usage: Apps that allow users to sign up through OAuth without verifying their local email can set this to false to opt out of the requirement, though this is deprecated and insecure [3][4]. The library developers intend to make this check unconditional in a future minor release [3][4]. trustedProviders This option defines a list of authentication providers that are considered trusted for automatic (implicit) account linking [1][2]. Configuration: It accepts a static array of provider IDs (e.g., ["google", "github"]) or an async function that returns the list dynamically based on the current request [1][5]. Behavior: When a user authenticates with a provider listed in trustedProviders, the system may automatically link the accounts even if the provider does not explicitly confirm the user's email verification status [2][6]. Security Note: This should be used with caution, as bypassing email verification checks increases the risk of account takeover if an attacker can manipulate the authentication flow of a trusted provider [2][4][6]. General Account Linking Context Account linking is enabled by default [2][6]. You can disable it entirely by setting account.accountLinking.enabled to false, or prevent implicit linking specifically by setting account.accountLinking.disableImplicitLinking to true, which forces all linking to occur via the /link-social endpoint where the user must already be authenticated [4].
Citations:
- 1: https://www.better-auth.com/docs/reference/options
- 2: https://www.better-auth.com/docs/concepts/users-accounts
- 3: fix(oauth): block OAuth linking to unverified local accounts better-auth/better-auth#9578
- 4: GHSA-g38m-r43w-p2q7
- 5: https://github.com/better-auth/better-auth/blob/canary/docs/content/docs/reference/options.mdx
- 6: https://github.com/better-auth/better-auth/blob/3f75ee3e/docs/content/docs/concepts/users-accounts.mdx
🌐 Web query:
site:better-auth.com trustedProviders trustedProviders accountLinking semantics auto link
💡 Result:
In Better Auth, the trustedProviders setting within the accountLinking configuration is used to define which authentication providers are trusted to automatically link accounts, even if those providers do not confirm the user's email verification status [1][2]. When a user logs in using a provider listed in the trustedProviders array, Better Auth will automatically link the provider account to the user's existing account if the email addresses match, bypassing the standard requirement for verified email status [1][2]. Because this mechanism can increase the risk of account takeover if an untrusted or spoofed provider were used, it should be configured with caution [1][2]. Key details regarding this configuration include: 1. Configuration: The trustedProviders option is defined within the account.accountLinking object in the Better Auth configuration [3][2]. It accepts an array of strings (e.g., ["google", "github"]) or an asynchronous function that returns the list of providers dynamically based on the request [3]. 2. Relationship to Auto-Linking: While standard account linking requires email verification from the provider, trusted providers permit forced or automatic linking without that specific verification check [1][2]. 3. Security Warning: Enabling this feature essentially trusts the provider's assertion of identity more broadly than default settings, which is why it is explicitly named trustedProviders to emphasize that it grants elevated trust for the account linking process [1][2]. This functionality is distinct from standard SSO auto-linking, where accounts are linked automatically based on a match between an existing account's email and a provider's verified domain [4].
Citations:
- 1: https://beta.better-auth.com/docs/concepts/users-accounts
- 2: https://www.better-auth.com/docs/concepts/users-accounts
- 3: https://better-auth.com/docs/reference/options
- 4: https://beta.better-auth.com/docs/plugins/sso
🌐 Web query:
site:better-auth.com requireLocalEmailVerified semantics local email verification account linking
💡 Result:
In Better Auth, the concept of requiring email verification for account linking is governed by the library's account linking policies rather than a single parameter named requireLocalEmailVerified. Instead, Better Auth manages security and account linking through explicit configurations [1][2]. Key mechanisms include: 1. Verification-Based Linking: By default, Better Auth allows users to link social or OAuth providers to an existing account if the provider confirms the user's email is verified [1]. This prevents unauthorized account takeovers by ensuring the linking party owns the email address associated with the account [1]. 2. Trusted Providers: You can bypass verification requirements for specific providers by adding them to the trustedProviders list in the accountLinking configuration [1][2]. When a user logs in via a trusted provider, their account is automatically linked even if the provider does not verify the email, though this should be used with caution [1]. 3. Domain Verification: For SSO/OIDC configurations, you can enable domain verification [3]. When a provider's domain is verified, it is treated as a trusted source, and automatic account linking is allowed for users whose email domain matches the provider's verified domain [3]. 4. Preventing Mismatches: Better Auth strictly enforces identity alignment during linking. If a user attempts to link an OAuth provider account where the returned email does not match the email of the currently authenticated user, the operation is blocked with an email_doesn't_match error [4][5]. This is a security safeguard independent of the verification status itself [4]. To require email verification globally for credential-based logins (email and password), you should set requireEmailVerification: true within your emailAndPassword configuration [6][2]. This ensures users must verify their email before they can sign in, maintaining consistent security standards across different authentication methods [6].
Citations:
- 1: https://beta.better-auth.com/docs/concepts/users-accounts
- 2: https://www.better-auth.com/docs/guides/workos-migration-guide
- 3: https://beta.better-auth.com/docs/plugins/sso
- 4: https://www.better-auth.com/docs/reference/errors/email_doesn%27t_match
- 5: https://beta.better-auth.com/docs/reference/errors/email_doesn't_match
- 6: https://beta.better-auth.com/docs/concepts/email
Avoid implicit Google linking for unverified local accounts.
accountLinking.trustedProviders: ["google"] enables automatic/implicit linking via Google, and requireLocalEmailVerified: false removes the local “email must already be verified” safety gate (default is true). Together, this allows a Google login for a matching email to attach to an existing unverified local account, increasing account takeover risk. Keep requireLocalEmailVerified enabled (or remove google from trustedProviders) until the local-email verification flow is fully wired up.
(https://www.better-auth.com/docs/reference/options, https://www.better-auth.com/docs/concepts/users-accounts)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/lib/auth.ts` around lines 31 - 38, The account linking config
currently allows implicit Google linking for unverified local accounts; update
the account.accountLinking settings by restoring the local-email safety gate:
set requireLocalEmailVerified back to true (or remove "google" from
trustedProviders) in the accountLinking object so a Google login cannot
automatically attach to an unverified local account until the Resend email
verification flow is implemented; locate the accountLinking block (symbols:
account, accountLinking, trustedProviders, requireLocalEmailVerified) and change
the flag accordingly.
1. auth.ts — baseURL + secret → KEEP Both are legitimately required. Without secret, better-auth generates a random one per process — every server restart invalidates all active sessions. Without baseURL, Google OAuth callbacks break outside localhost. These aren't optional niceties, they're correctness requirements. 2. UserProvisioningFilter.java — @nonnull + split catch → KEEP @nonnull is correct — Spring's OncePerRequestFilter contract declares those params non-null and some static analyzers warn without it. The split catch is also right: a non-UUID sub (token not from your better-auth instance) is harmless and should be silently skipped; a real DB failure mid-provisioning should be a 500 rather than letting the request continue with no profile row and then fail mysteriously as a 404 "user not found" downstream. 3. UserService.java — email sync + race condition guard → KEEP BOTH The !email.equals(user.getEmail()) fix is a real bug fix, not an overcomplication — anyone who changes their Google email would be permanently stuck with the old one. The DataIntegrityViolationException catch on the insert is exactly the right pattern for optimistic insert races; two simultaneous first-logins are rare but a constraint violation crash is a hard error that would log loudly. 4. api-base.ts — retry with linear backoff → KEEP The original branch removed retries entirely. The restored version is strictly better than both the original (which retried 4xx errors indiscriminately) and the removed version: 5xx/network errors get 3 retries with backoff, 4xx errors (deterministic client faults) are rejected immediately. This is standard practice. 5. user-context.tsx — mergedUser at render + sessionPending in isLoading → KEEP The mergedUser pattern is the right call. Having session in refreshUser's useCallback deps meant a new session object reference (which useSession returns after every fetch) would recreate refreshUser and re-trigger the effect, causing spurious profile re-fetches. Moving the merge to render time breaks that cycle while still exposing unified name/image to consumers. isLoading: isLoading || sessionPending is also correct — without it, consumers see a brief isAuthenticated: false flash during the initial session load. 6. .env.local.example → KEEP The missing DATABASE_URL would silently leave developers confused why Drizzle migrations don't run. GOOGLE_CLIENT_ID → NEXT_PUBLIC_GOOGLE_CLIENT_ID is a genuine bug fix — auth.ts reads NEXT_PUBLIC_GOOGLE_CLIENT_ID, so the old name would cause Google OAuth to fail silently with an empty client ID.
Addresses security, correctness, and UX issues across backend and frontend. Changes: - Remove inline JWKS URI default; add application-local.properties for dev fallback - Add JwtDecoder bean with issuer (iss) validation via JwtValidators - Throw on missing DATABASE_URL in drizzle.config.ts and db/index.ts - Add composite index on account(providerId, accountId) in auth-schema - Use resetAuthToken() in 401 retry path to clear any active backoff - Change logout interface to () => Promise<void> with finally-block cleanup - Move login/signup success side effects to after handleUserLogin() resolves - Add isGoogleSigningIn guard; disable all auth buttons during Google redirect - Add externalDisabled prop to LoginForm and SignUpForm - Replace native confirm() with ConfirmDialog component in AccountDetailsModal - Differentiate error handling in handleLogoutAll and handleDeleteUser Notes: - Delete order kept as backend-first (anonymize PII) then authClient.deleteUser(); tombstone prevents revival if auth deletion fails — safer failure mode than swapping order - Run pnpm exec drizzle-kit generate && migrate to create the new account index - Activate local Spring profile with --spring.profiles.active=local for dev
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/disscount/user/service/UserService.java (1)
35-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not skip provisioning when
/api/users/meflows with “User not found”.Suggested fix
public void ensureActiveProfile(UUID id, String email) { - if (email == null) return; - Optional<User> existing = userRepository.findById(id); if (existing.isPresent()) { User user = existing.get(); boolean changed = false; @@ - if (!email.equals(user.getEmail())) { + if (email != null && !email.equals(user.getEmail())) { user.setEmail(email); changed = true; } @@ - userRepository.save(User.builder().id(id).email(email).build()); + userRepository.save(User.builder().id(id).email(email).build());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/disscount/user/service/UserService.java` around lines 35 - 37, The method ensureActiveProfile(UUID id, String email) must not early-return when email is null; instead ensure a local user/profile row is provisioned using the provided id and optionally set/update the email if present. Remove the "if (email == null) return;" and implement an upsert/lookup flow: try to find existing profile by id (e.g., userRepository.findById or findBySubject), create a new profile if missing using the id and null email, and if email is non-null update the profile.email and persist via userRepository.save or the equivalent; keep using ensureActiveProfile(UUID id, String email) as the entry point.
♻️ Duplicate comments (2)
frontend/src/context/user-context.tsx (1)
32-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale
refreshUser()responses can still overwrite user state after logout.The race condition from the past review remains: if
refreshUser()is in flight when the user signs out or switches accounts, the stale response can still callsetUser(userData)at line 50, repopulating the previous account's data. Add a request generation guard to discard late results.🛡️ Suggested fix with generation guard
+const requestIdRef = useRef(0); + const refreshUser = useCallback(async () => { + const localRequestId = ++requestIdRef.current; try { setIsLoading(true); const userData = await userService.getCurrentUser(); + if (requestIdRef.current !== localRequestId) return; // stale if (userData && (!userData.pinnedStores || !userData.pinnedPlaces)) { try { const [stores, places] = await Promise.all([ preferencesService.getPinnedStores(), preferencesService.getPinnedPlaces(), ]); + if (requestIdRef.current !== localRequestId) return; // stale userData.pinnedStores = stores; userData.pinnedPlaces = places; } catch (prefError) { console.error("Failed to fetch preferences:", prefError); } } setUser(userData); return userData; } catch (error) { + if (requestIdRef.current !== localRequestId) return; console.error("Failed to fetch user profile:", error); setUser(null); } finally { + if (requestIdRef.current === localRequestId) { setIsLoading(false); + } } }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/context/user-context.tsx` around lines 32 - 58, refreshUser() can be racing with sign-out/switch and must ignore stale responses: add a generation guard (e.g., a useRef<number> requestGen) that you increment before starting the async work inside refreshUser, capture the currentGen in the function, and before mutating state (setUser(...) and setIsLoading(...)) only apply results if currentGen === requestGen.current; also increment requestGen on sign-out/switch to invalidate in-flight calls. Apply the same guard checks around the preferences Promise.all branch so pref results are ignored when stale.frontend/src/components/custom/header/forms/auth-modal.tsx (1)
53-66: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
functionsyntax forhandleGoogleSignIn.Per coding guidelines, functions should use
function functionName() {}syntax instead of arrow functions, unless they are small inline functions. This handler has multiple statements and side effects.♻️ Suggested refactor
- const handleGoogleSignIn = async () => { + async function handleGoogleSignIn() { if (isGoogleSigningIn) return; setIsGoogleSigningIn(true); try { setLastLoginMethod("google"); setLastLoginMethodState("google"); await signIn.social({ provider: "google", callbackURL: "/" }); } catch { toast.error("Greška pri Google prijavi. Pokušaj ponovo."); } finally { setIsGoogleSigningIn(false); } - }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/custom/header/forms/auth-modal.tsx` around lines 53 - 66, Replace the arrow function handler with a named function declaration: convert the const handleGoogleSignIn = async () => { ... } into function handleGoogleSignIn() { ... } (keeping it async) and preserve the existing logic and references to isGoogleSigningIn, setIsGoogleSigningIn, setLastLoginMethod, setLastLoginMethodState, signIn.social and toast.error; ensure the early return (if (isGoogleSigningIn) return;), try/catch/finally block, and state updates remain identical and that the function is exported/used the same way where referenced.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/ui/confirm-dialog.tsx`:
- Line 39: The default icon choice in ConfirmDialog sets Icon via "const Icon =
variant === 'destructive' ? AlertTriangle : LogOut", which can mislead for
non-logout actions; change the component so Icon is either a prop (e.g., icon?:
React.ComponentType) with a sensible fallback like Info or HelpCircle, or
replace the fallback LogOut with a generic icon (Info or HelpCircle) and
preserve AlertTriangle for variant === "destructive" — update the ConfirmDialog
props/type to accept an optional icon, use that icon when provided, otherwise
use AlertTriangle for destructive and Info (or HelpCircle) as the default.
In `@frontend/src/db/auth-schema.ts`:
- Around line 70-73: The account table currently defines a non-unique composite
index named "account_provider_account_idx" on table.providerId and
table.accountId; change this to a unique composite index to enforce
UNIQUE(provider_id, account_id): import uniqueIndex from "drizzle-orm/pg-core",
replace index("account_provider_account_idx").on(table.providerId,
table.accountId) with
uniqueIndex("account_provider_account_uidx").on(table.providerId,
table.accountId), and update the migration for the account table to add the
matching UNIQUE constraint so the database prevents linking the same OAuth
account multiple times.
In `@frontend/src/lib/auth.ts`:
- Around line 9-10: Ensure required auth env vars are validated once at startup
before constructing betterAuth: add a single guard that checks
process.env.BETTER_AUTH_URL and process.env.BETTER_AUTH_SECRET (and any other
envs used at lines ~28-30) and throw a clear error if missing, then pass the
validated values into the betterAuth config instead of reading process.env
inline or using non-null assertions; update the code that instantiates
betterAuth to use these validated variables (referencing the betterAuth creation
and the auth config variables).
---
Outside diff comments:
In `@backend/src/main/java/disscount/user/service/UserService.java`:
- Around line 35-37: The method ensureActiveProfile(UUID id, String email) must
not early-return when email is null; instead ensure a local user/profile row is
provisioned using the provided id and optionally set/update the email if
present. Remove the "if (email == null) return;" and implement an upsert/lookup
flow: try to find existing profile by id (e.g., userRepository.findById or
findBySubject), create a new profile if missing using the id and null email, and
if email is non-null update the profile.email and persist via
userRepository.save or the equivalent; keep using ensureActiveProfile(UUID id,
String email) as the entry point.
---
Duplicate comments:
In `@frontend/src/components/custom/header/forms/auth-modal.tsx`:
- Around line 53-66: Replace the arrow function handler with a named function
declaration: convert the const handleGoogleSignIn = async () => { ... } into
function handleGoogleSignIn() { ... } (keeping it async) and preserve the
existing logic and references to isGoogleSigningIn, setIsGoogleSigningIn,
setLastLoginMethod, setLastLoginMethodState, signIn.social and toast.error;
ensure the early return (if (isGoogleSigningIn) return;), try/catch/finally
block, and state updates remain identical and that the function is exported/used
the same way where referenced.
In `@frontend/src/context/user-context.tsx`:
- Around line 32-58: refreshUser() can be racing with sign-out/switch and must
ignore stale responses: add a generation guard (e.g., a useRef<number>
requestGen) that you increment before starting the async work inside
refreshUser, capture the currentGen in the function, and before mutating state
(setUser(...) and setIsLoading(...)) only apply results if currentGen ===
requestGen.current; also increment requestGen on sign-out/switch to invalidate
in-flight calls. Apply the same guard checks around the preferences Promise.all
branch so pref results are ignored when stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17dd1fda-35fe-46a6-a72b-ce188f166b3f
📒 Files selected for processing (18)
AGENTS.mdbackend/src/main/java/disscount/config/SecurityConfig.javabackend/src/main/java/disscount/config/UserProvisioningFilter.javabackend/src/main/java/disscount/user/service/UserService.javabackend/src/main/resources/application-local.propertiesbackend/src/main/resources/application.propertiesfrontend/.env.local.examplefrontend/drizzle.config.tsfrontend/src/components/custom/header/forms/account-details-modal.tsxfrontend/src/components/custom/header/forms/auth-modal.tsxfrontend/src/components/custom/header/forms/login-form.tsxfrontend/src/components/custom/header/forms/signup-form.tsxfrontend/src/components/ui/confirm-dialog.tsxfrontend/src/context/user-context.tsxfrontend/src/db/auth-schema.tsfrontend/src/db/index.tsfrontend/src/lib/api/api-base.tsfrontend/src/lib/auth.ts
| onConfirm, | ||
| isLoading = false, | ||
| }: ConfirmDialogProps) { | ||
| const Icon = variant === "destructive" ? AlertTriangle : LogOut; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Default icon may be misleading for non-logout confirmations.
LogOut is used as the default icon for non-destructive confirmations. If ConfirmDialog is reused for other actions (e.g., "discard unsaved changes"), this icon could confuse users. Consider using a more generic icon like Info or HelpCircle, or making Icon a prop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ui/confirm-dialog.tsx` at line 39, The default icon
choice in ConfirmDialog sets Icon via "const Icon = variant === 'destructive' ?
AlertTriangle : LogOut", which can mislead for non-logout actions; change the
component so Icon is either a prop (e.g., icon?: React.ComponentType) with a
sensible fallback like Info or HelpCircle, or replace the fallback LogOut with a
generic icon (Info or HelpCircle) and preserve AlertTriangle for variant ===
"destructive" — update the ConfirmDialog props/type to accept an optional icon,
use that icon when provided, otherwise use AlertTriangle for destructive and
Info (or HelpCircle) as the default.
Comment on lines
+70
to
+73
| (table) => [ | ||
| index("account_userId_idx").on(table.userId), | ||
| index("account_provider_account_idx").on(table.providerId, table.accountId), | ||
| ], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Better Auth’s Drizzle/Postgres schema, is (providerId, accountId) expected to be uniquely constrained in the account table?
💡 Result:
Yes. In Better Auth’s PostgreSQL core schema, the account table is expected to have a UNIQUE constraint on (provider_id, account_id) (i.e., the pair (providerId, accountId) is unique). [1] In the Better Auth “database” docs for PostgreSQL, the generated SQL for the accounts table includes UNIQUE(provider_id, account_id). [1]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the file
sed -n '1,140p' frontend/src/db/auth-schema.ts | nl -ba | sed -n '50,110p'
# Search for existing uniqueIndex / unique constraint usage
rg -n "uniqueIndex|UNIQUE\(|unique\W*constraint|providerId.*accountId|account_provider_account" frontend/src/db/auth-schema.tsRepository: OffCrazyFreak/Disscount
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== auth-schema.ts lines 50-110 ==="
sed -n '50,110p' frontend/src/db/auth-schema.ts
echo
echo "=== Search for uniqueIndex / UNIQUE constraint / provider+account index ==="
rg -n "uniqueIndex|UNIQUE\(|account_provider_account|providerId.*accountId|accountId.*providerId" frontend/src/db/auth-schema.tsRepository: OffCrazyFreak/Disscount
Length of output: 2245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show imports at top of file
sed -n '1,60p' frontend/src/db/auth-schema.ts
echo
echo "=== Find account table definition ==="
rg -n "export const account|pgTable\(\s*[\"']?account" frontend/src/db/auth-schema.ts
echo
echo "=== Show account table around providerId/accountId indexes ==="
rg -n "account_provider_account_idx" frontend/src/db/auth-schema.ts -n
start_line=$(rg -n "account_provider_account_idx" frontend/src/db/auth-schema.ts | cut -d: -f1 | head -n1)
# print a bit above/below that index definition
from=$((start_line-30))
to=$((start_line+10))
sed -n "${from},${to}p" frontend/src/db/auth-schema.tsRepository: OffCrazyFreak/Disscount
Length of output: 3366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for uniqueIndex usage in db schemas ==="
rg -n "uniqueIndex\(" frontend/src/db
echo
echo
echo "=== Search for account_provider_account_idx / provider_account unique variants across repo ==="
rg -n "account_provider_account_idx|provider_account_uidx|provider.*account.*unique|UNIQUE\\(provider|UNIQUE\\s+\\(provider" .
echo
echo
echo "=== Check for other occurrences of providerId+accountId pair constraints ==="
rg -n "providerId.*accountId|accountId.*providerId" frontend/src/dbRepository: OffCrazyFreak/Disscount
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -u
echo "=== Search for uniqueIndex usage in db schemas ==="
rg -n "uniqueIndex\(" frontend/src/db || true
echo
echo "=== Search for account_provider_account_idx / provider_account unique variants across repo ==="
rg -n "account_provider_account_idx|provider_account_uidx|provider.*account.*unique|UNIQUE\\(provider|UNIQUE\\s+\\(provider" . || true
echo
echo "=== Check for other occurrences of providerId+accountId pair constraints ==="
rg -n "providerId.*accountId|accountId.*providerId" frontend/src/db || trueRepository: OffCrazyFreak/Disscount
Length of output: 524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for migrations/schema SQL that may already add UNIQUE(provider_id, account_id) ==="
# Limit search to likely directories
rg -n "UNIQUE\\s*\\(\\s*provider_id\\s*,\\s*account_id\\s*\\)|UNIQUE\\s*\\(\\s*providerId\\s*,\\s*accountId\\s*\\)|provider_id.*account_id|providerId.*accountId" . -S || true
echo
echo "=== List drizzle/migrations directories (best-effort) ==="
ls -la frontend 2>/dev/null || true
ls -la . 2>/dev/null || true
find . -maxdepth 4 -type d -iname "*migrat*"? -o -iname "*drizzle*" 2>/dev/null | head -n 50Repository: OffCrazyFreak/Disscount
Length of output: 2342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List frontend/drizzle contents ==="
find frontend/drizzle -maxdepth 3 -type f | sed 's|^|FILE: |' | head -n 200
echo
echo "=== Search for UNIQUE(provider_id, account_id) or similar in drizzle/migrations ==="
rg -n "UNIQUE\\s*\\(\\s*provider(_id)?\\s*,\\s*account(_id)?\\s*\\)|UNIQUE\\s*\\(\\s*providerId\\s*,\\s*accountId\\s*\\)|provider[_ ]id.*account[_ ]id|account[_ ]id.*provider[_ ]id" frontend/drizzle frontend/src/db . -S || true
echo
echo "=== Search for uniqueIndex in repo (to confirm helper usage) ==="
rg -n "uniqueIndex\\b" . || true
echo
echo "=== Check drizzle-orm version from frontend/package.json ==="
cat frontend/package.json | sed -n '1,160p' | rg -n "drizzle-orm|`@drizzle`|drizzle" || trueRepository: OffCrazyFreak/Disscount
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== migration SQL: frontend/drizzle/0000_stale_mister_sinister.sql (look for account/provider/account_id uniqueness) ==="
sed -n '1,200p' frontend/drizzle/0000_stale_mister_sinister.sql | cat
echo
echo "=== Search within migration for account/provider/account_id ==="
rg -n "account|provider_id|account_id|account_provider_account_idx|UNIQUE" frontend/drizzle/0000_stale_mister_sinister.sql || true
echo
echo "=== snapshot: frontend/drizzle/meta/0000_snapshot.json (look for unique indexes/constraints on account) ==="
rg -n "\"account\"|provider_id|account_id|UNIQUE|uniqueIndex|account_provider_account_idx" frontend/drizzle/meta/0000_snapshot.json || true
echo
echo "=== (optional) show small snippet around account_provider_account_idx occurrence in snapshot ==="
line=$(rg -n "account_provider_account_idx" frontend/drizzle/meta/0000_snapshot.json | head -n1 | cut -d: -f1 || true)
if [ -n "${line}" ]; then
from=$((line-20)); to=$((line+30));
sed -n "${from},${to}p" frontend/drizzle/meta/0000_snapshot.json | cat
fiRepository: OffCrazyFreak/Disscount
Length of output: 3551
Enforce UNIQUE(provider_id, account_id) for accounts.
account_provider_account_uidx is currently only declared as a non-unique index (index("account_provider_account_idx")...), and the existing Drizzle migration for account doesn’t add any UNIQUE(provider_id, account_id) constraint. Add a composite unique index on (providerId, accountId) (and import uniqueIndex from drizzle-orm/pg-core) to prevent linking the same OAuth account multiple times.
Suggested fix
import {
pgTable,
text,
timestamp,
boolean,
uuid,
index,
+ uniqueIndex,
} from "drizzle-orm/pg-core";
@@
(table) => [
index("account_userId_idx").on(table.userId),
- index("account_provider_account_idx").on(table.providerId, table.accountId),
+ uniqueIndex("account_provider_account_uidx").on(table.providerId, table.accountId),
],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (table) => [ | |
| index("account_userId_idx").on(table.userId), | |
| index("account_provider_account_idx").on(table.providerId, table.accountId), | |
| ], | |
| (table) => [ | |
| index("account_userId_idx").on(table.userId), | |
| uniqueIndex("account_provider_account_uidx").on(table.providerId, table.accountId), | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/db/auth-schema.ts` around lines 70 - 73, The account table
currently defines a non-unique composite index named
"account_provider_account_idx" on table.providerId and table.accountId; change
this to a unique composite index to enforce UNIQUE(provider_id, account_id):
import uniqueIndex from "drizzle-orm/pg-core", replace
index("account_provider_account_idx").on(table.providerId, table.accountId) with
uniqueIndex("account_provider_account_uidx").on(table.providerId,
table.accountId), and update the migration for the account table to add the
matching UNIQUE constraint so the database prevents linking the same OAuth
account multiple times.
Comment on lines
+9
to
+10
| baseURL: process.env.BETTER_AUTH_URL, | ||
| secret: process.env.BETTER_AUTH_SECRET, |
Contributor
There was a problem hiding this comment.
Validate required auth env vars once before creating betterAuth.
Auth config currently mixes unchecked env reads and non-null assertions; fail fast with a single explicit guard so startup errors are deterministic and actionable.
Suggested fix
import { db } from "../db";
+const betterAuthUrl = process.env.BETTER_AUTH_URL;
+const betterAuthSecret = process.env.BETTER_AUTH_SECRET;
+const googleClientId = process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID;
+const googleClientSecret = process.env.GOOGLE_CLIENT_SECRET;
+
+if (!betterAuthUrl || !betterAuthSecret || !googleClientId || !googleClientSecret) {
+ throw new Error(
+ "Missing required auth env vars: BETTER_AUTH_URL, BETTER_AUTH_SECRET, NEXT_PUBLIC_GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET.",
+ );
+}
+
export const auth = betterAuth({
- baseURL: process.env.BETTER_AUTH_URL,
- secret: process.env.BETTER_AUTH_SECRET,
+ baseURL: betterAuthUrl,
+ secret: betterAuthSecret,
@@
socialProviders: {
google: {
- clientId: process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID!,
- clientSecret: process.env.GOOGLE_CLIENT_SECRET!,
+ clientId: googleClientId,
+ clientSecret: googleClientSecret,
},Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/lib/auth.ts` around lines 9 - 10, Ensure required auth env vars
are validated once at startup before constructing betterAuth: add a single guard
that checks process.env.BETTER_AUTH_URL and process.env.BETTER_AUTH_SECRET (and
any other envs used at lines ~28-30) and throw a clear error if missing, then
pass the validated values into the betterAuth config instead of reading
process.env inline or using non-null assertions; update the code that
instantiates betterAuth to use these validated variables (referencing the
betterAuth creation and the auth config variables).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor