implementation of Login and refresh token - #78
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a JWT token manager, DB-backed user/role repository, auth service (login/register/refresh), Gin HTTP handlers for auth endpoints, a DB migration creating auth schema/tables, and wires token manager into the server startup. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as "Gin Handler"
participant Service as "Auth Service"
participant Repo as "User Repo"
participant DB as "Database"
participant TokenMgr as "TokenManager"
Client->>Handler: POST /login (email,password)
Handler->>Handler: Bind & validate JSON
Handler->>Service: Login(ctx,email,password)
Service->>Repo: FindByEmail(ctx,email)
Repo->>DB: SELECT user by email
DB-->>Repo: user row / no rows
Repo-->>Service: *User
Service->>Service: bcrypt compare
Service->>TokenMgr: GeneratePair(userID)
TokenMgr-->>Service: accessToken, refreshToken
Service-->>Handler: *Tokens
Handler-->>Client: 200 {access_token,refresh_token}
sequenceDiagram
actor Client
participant Handler as "Gin Handler"
participant Service as "Auth Service"
participant Repo as "User Repo"
participant DB as "Database"
participant TokenMgr as "TokenManager"
Client->>Handler: POST /register (email,password,slug)
Handler->>Handler: Bind & validate JSON
Handler->>Service: Register(ctx,email,password,slug)
Service->>Repo: FindByEmail(ctx,email)
Repo->>DB: SELECT user
DB-->>Repo: nil / user
Service->>Repo: FindRoleBySlug(ctx,slug)
Repo->>DB: SELECT role
DB-->>Repo: role row
Service->>Service: bcrypt hash password
Service->>Repo: Create(ctx, {email,passwordHash})
Repo->>DB: INSERT user RETURNING id,email
DB-->>Repo: created user
Service->>Repo: AssignRole(ctx,userID,roleID)
Repo->>DB: INSERT user_roles
Service->>TokenMgr: GeneratePair(userID)
TokenMgr-->>Service: accessToken, refreshToken
Service-->>Handler: *Tokens
Handler-->>Client: 201 {access_token,refresh_token}
sequenceDiagram
actor Client
participant Handler as "Gin Handler"
participant Service as "Auth Service"
participant TokenMgr as "TokenManager"
Client->>Handler: POST /refresh (refresh_token)
Handler->>Handler: Bind & validate JSON
Handler->>Service: Refresh(ctx,refresh_token)
Service->>TokenMgr: ParseRefreshToken(tokenStr)
TokenMgr-->>Service: userID
Service->>TokenMgr: GeneratePair(userID)
TokenMgr-->>Service: accessToken, refreshToken
Service-->>Handler: *Tokens
Handler-->>Client: 200 {access_token,refresh_token}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/admin-auth-api/main.go`:
- Around line 27-29: The startup currently forces config.Load(".env") which will
call godotenv.Load and fatally exit via logger.Fatal(ctx, "load config:", err)
when no .env exists in container; remove the hard requirement by either deleting
the config.Load(".env") call or making it conditional (e.g., only attempt
config.Load(".env") when a local-dev indicator is set or when the .env file
exists via os.Stat/os.IsNotExist), and keep the existing env.New*Config() path
for normal environment-based loading so the service doesn't exit inside
containers.
In `@internal/admin-auth/handler/auth/handler.go`:
- Around line 38-41: The handlers currently call c.Error(err) for Login (tokens,
err := h.service.Login(...)) and the refresh handler, which causes the generic
500; instead inspect the error and return appropriate HTTP codes (e.g., for
authentication failure return 401 Unauthorized, for invalid/expired refresh
token return 400/401 as appropriate) by using c.AbortWithStatusJSON or c.JSON
with the status and a short message; compare the returned error with
package-level sentinel errors exported from
internal/admin-auth/service/auth/service.go using errors.Is (do not create new
errors in the handler), and map each sentinel (e.g., ErrInvalidCredentials,
ErrInvalidRefreshToken, ErrExpiredToken) to the correct HTTP status and response
body rather than delegating to c.Error.
In `@internal/admin-auth/service/auth/service.go`:
- Around line 46-69: The Refresh method currently accepts any valid JWT signed
with the refresh secret, so access tokens could be reused if secrets ever match;
modify Refresh (in service.Refresh) to validate an explicit token type claim
(e.g., "token_type" or "aud") by extracting claims from token.Claims and
returning an error unless claims["token_type"] == "refresh" (or the chosen
string), and ensure token issuance in generateTokens sets that claim when
creating refresh tokens; reject tokens missing or with a non-"refresh"
token_type.
- Around line 34-36: In the Authenticate flow in service.go, do not treat a
missing user from s.userRepo.FindByEmail as a server error; instead detect the
repository "not found" error (e.g., errors.Is(err, repo.ErrNotFound) or
errors.Is(err, sql.ErrNoRows)) and return the same authentication failure used
for bad passwords (the Auth/ErrInvalidCredentials error path) so unknown emails
map to invalid-credentials, while preserving the current fmt.Errorf("find user:
%w", err) wrapping only for real repo errors.
In `@migrations/20260322135256_auth.sql`:
- Around line 32-37: The migration currently inserts a hardcoded test user via
the INSERT INTO auth.users statement (the VALUES block with 'test@test.com',
'testuser', and the bcrypt hash); remove this source-controlled test account
from the shared migration so production and other environments do not receive a
backdoor account—either delete the INSERT entirely from this migration or move
it into a dev-only seed script that runs conditionally (e.g., only when
NODE_ENV==='development' or via a dedicated seed command), ensuring no hardcoded
credentials remain in schema migrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86c71d36-5a1d-47df-8ded-14543d0bcc4e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cmd/admin-auth-api/main.godocker/compose.yamlgo.modinternal/admin-auth/handler/auth/handler.gointernal/admin-auth/repository/user/repository.gointernal/admin-auth/service/auth/service.gomigrations/20260322135256_auth.sql
| if err := config.Load(".env"); err != nil { | ||
| logger.Fatal(ctx, "load config:", err) | ||
| } |
There was a problem hiding this comment.
Don't require a .env file inside the container.
Line 27 still calls config.Load(".env"), which forces godotenv.Load before env.New*Config(). With Compose supplying environment variables from the host, the file usually is not present in the container FS, so this path will fatal during startup.
🛠️ Suggested fix
- if err := config.Load(".env"); err != nil {
+ if err := config.Load(); err != nil {
logger.Fatal(ctx, "load config:", err)
}Keep the .env path only in a local-dev code path if you still want file-based loading outside Docker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/admin-auth-api/main.go` around lines 27 - 29, The startup currently
forces config.Load(".env") which will call godotenv.Load and fatally exit via
logger.Fatal(ctx, "load config:", err) when no .env exists in container; remove
the hard requirement by either deleting the config.Load(".env") call or making
it conditional (e.g., only attempt config.Load(".env") when a local-dev
indicator is set or when the .env file exists via os.Stat/os.IsNotExist), and
keep the existing env.New*Config() path for normal environment-based loading so
the service doesn't exit inside containers.
| tokens, err := h.service.Login(c.Request.Context(), req.Email, req.Password) | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return |
There was a problem hiding this comment.
Return auth status codes here instead of sending expected failures to the generic 500 middleware.
Lines 38-41 and 61-64 hand login/refresh failures to c.Error, and cmd/admin-auth-api/main.go currently turns those into the generic 500 response. Wrong passwords and bad refresh tokens should come back as 401/400, not "internal server error".
🛠️ Suggested shape
import (
"context"
+ "errors"
"net/http"
@@
tokens, err := h.service.Login(c.Request.Context(), req.Email, req.Password)
if err != nil {
- _ = c.Error(err)
+ switch {
+ case errors.Is(err, authsvc.ErrInvalidCredentials):
+ c.JSON(http.StatusUnauthorized, gin.H{"message": "invalid credentials"})
+ case errors.Is(err, authsvc.ErrInvalidRefreshToken):
+ c.JSON(http.StatusUnauthorized, gin.H{"message": "invalid refresh token"})
+ default:
+ _ = c.Error(err)
+ }
return
}Apply the same pattern in refresh. This will need package-level sentinel errors from internal/admin-auth/service/auth/service.go instead of fresh errors.New(...) values.
Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/handler/auth/handler.go` around lines 38 - 41, The
handlers currently call c.Error(err) for Login (tokens, err :=
h.service.Login(...)) and the refresh handler, which causes the generic 500;
instead inspect the error and return appropriate HTTP codes (e.g., for
authentication failure return 401 Unauthorized, for invalid/expired refresh
token return 400/401 as appropriate) by using c.AbortWithStatusJSON or c.JSON
with the status and a short message; compare the returned error with
package-level sentinel errors exported from
internal/admin-auth/service/auth/service.go using errors.Is (do not create new
errors in the handler), and map each sentinel (e.g., ErrInvalidCredentials,
ErrInvalidRefreshToken, ErrExpiredToken) to the correct HTTP status and response
body rather than delegating to c.Error.
| u, err := s.userRepo.FindByEmail(ctx, email) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("find user: %w", err) |
There was a problem hiding this comment.
Treat a missing user as an auth failure, not a server error.
Line 34 currently wraps FindByEmail failures as find user: ..., so an unknown email falls out as a 500 instead of the same invalid-credentials path as a bad password. Translate not-found to the auth error here and reserve wrapped errors for real repository failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/service/auth/service.go` around lines 34 - 36, In the
Authenticate flow in service.go, do not treat a missing user from
s.userRepo.FindByEmail as a server error; instead detect the repository "not
found" error (e.g., errors.Is(err, repo.ErrNotFound) or errors.Is(err,
sql.ErrNoRows)) and return the same authentication failure used for bad
passwords (the Auth/ErrInvalidCredentials error path) so unknown emails map to
invalid-credentials, while preserving the current fmt.Errorf("find user: %w",
err) wrapping only for real repo errors.
| func (s *service) Refresh(ctx context.Context, refreshToken string) (*Tokens, error) { | ||
| cfg := config.Config().JwtToken | ||
|
|
||
| token, err := jwt.Parse(refreshToken, func(t *jwt.Token) (any, error) { | ||
| if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { | ||
| return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) | ||
| } | ||
| return []byte(cfg.RefreshTokenSecretKey()), nil | ||
| }) | ||
| if err != nil || !token.Valid { | ||
| return nil, errors.New("invalid refresh token") | ||
| } | ||
|
|
||
| claims, ok := token.Claims.(jwt.MapClaims) | ||
| if !ok { | ||
| return nil, errors.New("invalid token claims") | ||
| } | ||
|
|
||
| userID, ok := claims["sub"].(string) | ||
| if !ok { | ||
| return nil, errors.New("invalid token subject") | ||
| } | ||
|
|
||
| return s.generateTokens(userID) |
There was a problem hiding this comment.
Mark refresh tokens explicitly and enforce that in Refresh.
Access and refresh tokens are identical except for secret/TTL. That means the refresh endpoint is relying entirely on config keeping the two secrets different; if they ever match, an access token can be exchanged for a fresh pair. Add a token_type/aud claim when issuing tokens and reject anything but refresh here.
🔒 Suggested hardening
- accessToken, err := s.generateToken(userID, cfg.AccessTokenSecretKey(), cfg.AccessTokenTTL())
+ accessToken, err := s.generateToken(userID, "access", cfg.AccessTokenSecretKey(), cfg.AccessTokenTTL())
@@
- refreshToken, err := s.generateToken(userID, cfg.RefreshTokenSecretKey(), cfg.RefreshTokenTTL())
+ refreshToken, err := s.generateToken(userID, "refresh", cfg.RefreshTokenSecretKey(), cfg.RefreshTokenTTL())
@@
-func (s *service) generateToken(userID, secretKey string, ttl time.Duration) (string, error) {
+func (s *service) generateToken(userID, tokenType, secretKey string, ttl time.Duration) (string, error) {
claims := jwt.MapClaims{
- "sub": userID,
- "exp": time.Now().Add(ttl).Unix(),
- "iat": time.Now().Unix(),
+ "sub": userID,
+ "token_type": tokenType,
+ "exp": time.Now().Add(ttl).Unix(),
+ "iat": time.Now().Unix(),
}And in Refresh:
tokenType, ok := claims["token_type"].(string)
if !ok || tokenType != "refresh" {
return nil, errors.New("invalid refresh token")
}Also applies to: 72-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/service/auth/service.go` around lines 46 - 69, The
Refresh method currently accepts any valid JWT signed with the refresh secret,
so access tokens could be reused if secrets ever match; modify Refresh (in
service.Refresh) to validate an explicit token type claim (e.g., "token_type" or
"aud") by extracting claims from token.Claims and returning an error unless
claims["token_type"] == "refresh" (or the chosen string), and ensure token
issuance in generateTokens sets that claim when creating refresh tokens; reject
tokens missing or with a non-"refresh" token_type.
|
|
||
| include: | ||
| - path: compose.infra.yaml | ||
| env_file: ../.env |
There was a problem hiding this comment.
I think this change is already in the main branch. A quick rebase from main should clear this up.
| "golang.org/x/crypto/bcrypt" | ||
| ) | ||
|
|
||
| type Tokens struct { |
There was a problem hiding this comment.
Mb it's better to move Tokens to the entities package? Importing authsvc.Tokens couples the handler to the specific implementation of the service layer.
There was a problem hiding this comment.
entities are DB layer, and service can have knowledge about it.
We can have service layer (domain/BL) models and DTO (exposed to handlers), but our application is not big enough to do that.
So we can use the DTOs only (and have only DTO <--> Entity conversion). It's ok if higher layer knows about one level below, but it should not be reversed or go few layers deep.
| } | ||
|
|
||
| func (s *service) generateToken(userID, secretKey string, ttl time.Duration) (string, error) { | ||
| claims := jwt.MapClaims{ |
There was a problem hiding this comment.
In my opinion, it's better to use a custom struct with jwt.RegisteredClaims instead of jwt.MapClaims here. It would be more type-safe.
| } | ||
|
|
||
| type Repository interface { | ||
| FindByEmail(ctx context.Context, email string) (*User, error) |
There was a problem hiding this comment.
I think, we should return a domain entity here instead of the db model to decouple layers
There was a problem hiding this comment.
repos return entities, they are lower level than domain/business layer
| ('user', 'Користувач'), | ||
| ('moderator', 'Модератор'); | ||
|
|
||
| INSERT INTO auth.users (email, username, password_hash) |
There was a problem hiding this comment.
I think this should not be in the repository, if we need this specifically to create test user in our database - it's better to create a seeder command to fill in the database.
| r.POST("/refresh", h.refresh) | ||
| } | ||
|
|
||
| type loginRequest struct { |
There was a problem hiding this comment.
I think it would be better to move this to sepparate packege. Maybe we should declare requests in different packages, not in the handler itself
| func (h *handler) login(c *gin.Context) { | ||
| var req loginRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"message": err.Error()}) |
There was a problem hiding this comment.
Can we create helper for tihis to reuse in other handkers?
| ) { | ||
| h := &handler{ | ||
| service: service, | ||
| type refreshRequest struct { |
There was a problem hiding this comment.
Maybe we should move it also to a sepparate package?
|
|
||
| type Repository struct { | ||
| db database.Client | ||
| type User struct { |
There was a problem hiding this comment.
I think we should also move this to seppare pacakge, called models
There was a problem hiding this comment.
We have entities - they should be managed by repository. If these are also entities - need to move to other package.
If these are business logic models - they should not be used on the repository level and converted in BL
| PasswordHash string | ||
| } | ||
|
|
||
| type Repository interface { |
There was a problem hiding this comment.
Could we give more specific name for repository?
| return &repository{client: client} | ||
| } | ||
|
|
||
| func (r *repository) FindByEmail(ctx context.Context, email string) (*User, error) { |
There was a problem hiding this comment.
Could we add a sentinel error, when user is not found?
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
migrations/20260322135256_auth.sql (1)
30-34:⚠️ Potential issue | 🔴 CriticalRemove seeded fixed credentials from shared migration.
Lines 30-34 still create a source-controlled test account. This is a standing auth backdoor risk in every environment running migrations.
🔐 Proposed fix
-INSERT INTO auth.users (email, password_hash) -VALUES ( - 'test@test.com', - '$2a$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi' - ); +-- Create dev/test users only via a dev-only seed process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/20260322135256_auth.sql` around lines 30 - 34, The migration currently seeds a fixed user via the INSERT INTO auth.users statement (email 'test@test.com' and a literal password_hash), creating a source-controlled credential backdoor; remove this INSERT from the shared migration and move any test/dev account creation into a non-checked-in seed or environment-specific bootstrap script, or guard it behind a safe, opt-in mechanism (e.g., a migration flag or dev-only seed run) so that the migration file no longer contains hard-coded credentials.
🧹 Nitpick comments (3)
internal/admin-auth/service/auth/refresh-token.go (1)
9-19: Re-check user state before issuing new tokens.Lines 9-19 reissue tokens from refresh claims alone. Consider validating user existence/active status before
GeneratePair(...)to avoid refreshing sessions for removed/disabled users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin-auth/service/auth/refresh-token.go` around lines 9 - 19, After parsing the refresh token with s.tokenManager.ParseRefreshToken, re-check the user's current state before calling s.tokenManager.GeneratePair: retrieve the user record (e.g., via s.userStore.GetByID or the service's user lookup method) using the returned userID, verify the user exists and is active/enabled, and return an appropriate error if the user is missing/disabled; only then call s.tokenManager.GeneratePair and return &Tokens{AccessToken: at, RefreshToken: rt} so tokens are not issued for removed or disabled accounts.pkg/jwt/jwt.go (1)
17-24: Validate JWT config at construction time.Lines 17-24 accept empty secrets and invalid TTLs silently. Fail fast to avoid insecure signing keys or immediately-expired tokens.
♻️ Proposed refactor
-func NewTokenManager(accessSecret, refreshSecret string, accessTTL, refreshTTL time.Duration) *TokenManager { - return &TokenManager{ +func NewTokenManager(accessSecret, refreshSecret string, accessTTL, refreshTTL time.Duration) (*TokenManager, error) { + if accessSecret == "" || refreshSecret == "" { + return nil, fmt.Errorf("jwt secret must not be empty") + } + if accessTTL <= 0 || refreshTTL <= 0 { + return nil, fmt.Errorf("jwt ttl must be > 0") + } + return &TokenManager{ accessSecret: accessSecret, refreshSecret: refreshSecret, accessTTL: accessTTL, refreshTTL: refreshTTL, - } + }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jwt/jwt.go` around lines 17 - 24, NewTokenManager currently accepts empty secrets and non-positive TTLs silently; change it to validate inputs at construction and fail fast by returning an error. Update NewTokenManager to return (*TokenManager, error), check that accessSecret and refreshSecret are non-empty and that accessTTL and refreshTTL are > 0 (or meet any minimum TTL you require), and return a descriptive error when any check fails; if all checks pass, return the constructed *TokenManager and nil error. Ensure callers of NewTokenManager are updated to handle the error.internal/admin-auth/handler/auth/handler.go (1)
7-7: Consider using entity types instead of service types in the handler layer.The handler imports
authsvc.Tokensfrom the service package, creating a dependency from handler → service for types. Theinternal/admin-auth/entity/entity.goalready defines an identicalTokensstruct. Using the entity type would keep the handler layer decoupled from service internals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin-auth/handler/auth/handler.go` at line 7, The handler currently imports and uses authsvc.Tokens from the service package (import alias authsvc) which couples handler to service types; change the handler to use the entity Tokens type instead (the Tokens struct defined in the entity package) by replacing references to authsvc.Tokens with the entity.Tokens type and removing the service type import usage so the handler depends on the entity package (e.g., update function signatures, variable declarations, and return types in the handler methods that reference authsvc.Tokens such as any occurrences in handler.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 33-34: Reorder the two environment variable lines so
GOOSE_DBSTRING appears before GOOSE_DRIVER to satisfy dotenv-linter; locate the
GOOSE_DBSTRING and GOOSE_DRIVER entries in the .env example and swap their
order, preserving the value formatting (quotes, URL, and any whitespace) so the
content is unchanged except for position.
In `@internal/admin-auth/handler/auth/refresh-token.go`:
- Around line 20-23: The handler currently forwards all errors from
h.service.Refresh to middleware (via c.Error) which yields a 500; change the
refresh-token handler to inspect the returned err from h.service.Refresh and map
known authentication failures to appropriate HTTP statuses (e.g., if
errors.Is(err, service.ErrInvalidRefreshToken) or errors.Is(err,
service.ErrExpiredRefreshToken) respond with c.JSON(http.StatusUnauthorized,
gin.H{"error": err.Error()}) or http.StatusBadRequest where applicable), while
only calling c.Error(err) for unexpected/internal errors; use the existing
symbols h.service.Refresh, service.ErrInvalidRefreshToken,
service.ErrExpiredRefreshToken, c.JSON and c.Error to implement the conditional
error handling.
In `@internal/admin-auth/service/auth/login.go`:
- Around line 12-18: After calling s.userRepo.FindByEmail(ctx, email) in the
login handler, guard against u being nil before dereferencing u.PasswordHash; if
u == nil return the same credentials error (e.g., errors.New("invalid
credentials")) instead of calling bcrypt.CompareHashAndPassword. Update the code
around the FindByEmail call (variable u) so the nil check happens immediately
and only then invoke bcrypt.CompareHashAndPassword with u.PasswordHash.
In `@internal/admin-auth/service/auth/register.go`:
- Around line 34-44: The create + assign sequence can leave orphaned users if
AssignRole fails; modify the Register flow to perform s.userRepo.Create and
s.userRepo.AssignRole inside a single database transaction: begin a transaction
via the userRepo (or a provided DB/Tx method), call Create with the transaction
context to get createdUser, then call AssignRole with the same transaction and
role.ID, and commit on success or rollback on any error; propagate errors using
the same fmt.Errorf wrapping. Ensure you use the transaction-aware variants or
pass the tx into Create/AssignRole so both operations are atomic.
- Around line 13-19: The pre-check using s.userRepo.FindByEmail is race-prone;
keep it if you want UX, but also detect and translate DB unique-constraint
errors returned by s.userRepo.Create (and the other Create call referenced
around lines 34-40) into the same domain error "user with this email already
exists"; update the Create call error handling in register.go to inspect the
error (e.g., by type/assertion or inspecting SQL/driver error codes/messages)
and return errors.New("user with this email already exists") when the DB reports
a duplicate-email/unique-constraint violation, otherwise return the wrapped
error as before.
In `@pkg/jwt/jwt.go`:
- Around line 57-59: The JWT parsing call currently uses jwt.Parse(tokenStr,
func...) without restricting algorithms; update that call to include
jwt.WithValidMethods([]string{"HS256"}) (or jwt.SigningMethodHS256.Alg()) so
parsing only accepts HS256, e.g. call jwt.Parse(tokenStr,
jwt.WithValidMethods([]string{"HS256"}), func(t *jwt.Token) (any, error) {
return []byte(secret), nil }), keeping the same secret callback and error
handling in the same function.
---
Duplicate comments:
In `@migrations/20260322135256_auth.sql`:
- Around line 30-34: The migration currently seeds a fixed user via the INSERT
INTO auth.users statement (email 'test@test.com' and a literal password_hash),
creating a source-controlled credential backdoor; remove this INSERT from the
shared migration and move any test/dev account creation into a non-checked-in
seed or environment-specific bootstrap script, or guard it behind a safe, opt-in
mechanism (e.g., a migration flag or dev-only seed run) so that the migration
file no longer contains hard-coded credentials.
---
Nitpick comments:
In `@internal/admin-auth/handler/auth/handler.go`:
- Line 7: The handler currently imports and uses authsvc.Tokens from the service
package (import alias authsvc) which couples handler to service types; change
the handler to use the entity Tokens type instead (the Tokens struct defined in
the entity package) by replacing references to authsvc.Tokens with the
entity.Tokens type and removing the service type import usage so the handler
depends on the entity package (e.g., update function signatures, variable
declarations, and return types in the handler methods that reference
authsvc.Tokens such as any occurrences in handler.go).
In `@internal/admin-auth/service/auth/refresh-token.go`:
- Around line 9-19: After parsing the refresh token with
s.tokenManager.ParseRefreshToken, re-check the user's current state before
calling s.tokenManager.GeneratePair: retrieve the user record (e.g., via
s.userStore.GetByID or the service's user lookup method) using the returned
userID, verify the user exists and is active/enabled, and return an appropriate
error if the user is missing/disabled; only then call
s.tokenManager.GeneratePair and return &Tokens{AccessToken: at, RefreshToken:
rt} so tokens are not issued for removed or disabled accounts.
In `@pkg/jwt/jwt.go`:
- Around line 17-24: NewTokenManager currently accepts empty secrets and
non-positive TTLs silently; change it to validate inputs at construction and
fail fast by returning an error. Update NewTokenManager to return
(*TokenManager, error), check that accessSecret and refreshSecret are non-empty
and that accessTTL and refreshTTL are > 0 (or meet any minimum TTL you require),
and return a descriptive error when any check fails; if all checks pass, return
the constructed *TokenManager and nil error. Ensure callers of NewTokenManager
are updated to handle the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dabcff3a-cf0f-4924-968a-8cb37aefdc6c
📒 Files selected for processing (15)
.env.examplecmd/admin-auth-api/main.gointernal/admin-auth/entity/.gitkeepinternal/admin-auth/entity/entity.gointernal/admin-auth/handler/auth/handler.gointernal/admin-auth/handler/auth/login.gointernal/admin-auth/handler/auth/refresh-token.gointernal/admin-auth/handler/auth/register.gointernal/admin-auth/repository/user/repository.gointernal/admin-auth/service/auth/login.gointernal/admin-auth/service/auth/refresh-token.gointernal/admin-auth/service/auth/register.gointernal/admin-auth/service/auth/service.gomigrations/20260322135256_auth.sqlpkg/jwt/jwt.go
✅ Files skipped from review due to trivial changes (1)
- internal/admin-auth/entity/entity.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/admin-auth/service/auth/service.go
- internal/admin-auth/repository/user/repository.go
| GOOSE_DRIVER=postgres | ||
| GOOSE_DBSTRING="postgres://share:bite@localhost:5432/share-bite?sslmode=disable" |
There was a problem hiding this comment.
Reorder Goose keys to satisfy dotenv-linter.
dotenv-linter flags key order at Lines 33-34 (GOOSE_DBSTRING should precede GOOSE_DRIVER). If lint is gating CI, this will fail checks.
🧹 Proposed fix
-GOOSE_DRIVER=postgres
GOOSE_DBSTRING="postgres://share:bite@localhost:5432/share-bite?sslmode=disable"
+GOOSE_DRIVER=postgres
GOOSE_MIGRATION_DIR=./migrations📝 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.
| GOOSE_DRIVER=postgres | |
| GOOSE_DBSTRING="postgres://share:bite@localhost:5432/share-bite?sslmode=disable" | |
| GOOSE_DBSTRING="postgres://share:bite@localhost:5432/share-bite?sslmode=disable" | |
| GOOSE_DRIVER=postgres | |
| GOOSE_MIGRATION_DIR=./migrations |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 34-34: [UnorderedKey] The GOOSE_DBSTRING key should go before the GOOSE_DRIVER key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 33 - 34, Reorder the two environment variable
lines so GOOSE_DBSTRING appears before GOOSE_DRIVER to satisfy dotenv-linter;
locate the GOOSE_DBSTRING and GOOSE_DRIVER entries in the .env example and swap
their order, preserving the value formatting (quotes, URL, and any whitespace)
so the content is unchanged except for position.
| tokens, err := h.service.Refresh(c.Request.Context(), req.RefreshToken) | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return |
There was a problem hiding this comment.
Map refresh failures to auth status codes, not generic 500.
Lines 20-23 forward all service errors to middleware, which currently responds with generic 500. Invalid/expired refresh token should return 401 (or 400), not internal server error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/handler/auth/refresh-token.go` around lines 20 - 23, The
handler currently forwards all errors from h.service.Refresh to middleware (via
c.Error) which yields a 500; change the refresh-token handler to inspect the
returned err from h.service.Refresh and map known authentication failures to
appropriate HTTP statuses (e.g., if errors.Is(err,
service.ErrInvalidRefreshToken) or errors.Is(err,
service.ErrExpiredRefreshToken) respond with c.JSON(http.StatusUnauthorized,
gin.H{"error": err.Error()}) or http.StatusBadRequest where applicable), while
only calling c.Error(err) for unexpected/internal errors; use the existing
symbols h.service.Refresh, service.ErrInvalidRefreshToken,
service.ErrExpiredRefreshToken, c.JSON and c.Error to implement the conditional
error handling.
| u, err := s.userRepo.FindByEmail(ctx, email) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("find user: %w", err) | ||
| } | ||
|
|
||
| if err := bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(password)); err != nil { | ||
| return nil, errors.New("invalid credentials") |
There was a problem hiding this comment.
Guard against nil user before bcrypt check.
If FindByEmail returns (nil, nil) for unknown email, Line 17 dereferences u.PasswordHash and panics.
🐛 Proposed fix
u, err := s.userRepo.FindByEmail(ctx, email)
if err != nil {
return nil, fmt.Errorf("find user: %w", err)
}
+ if u == nil {
+ return nil, errors.New("invalid credentials")
+ }
if err := bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(password)); err != nil {
return nil, errors.New("invalid credentials")
}📝 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.
| u, err := s.userRepo.FindByEmail(ctx, email) | |
| if err != nil { | |
| return nil, fmt.Errorf("find user: %w", err) | |
| } | |
| if err := bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(password)); err != nil { | |
| return nil, errors.New("invalid credentials") | |
| u, err := s.userRepo.FindByEmail(ctx, email) | |
| if err != nil { | |
| return nil, fmt.Errorf("find user: %w", err) | |
| } | |
| if u == nil { | |
| return nil, errors.New("invalid credentials") | |
| } | |
| if err := bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(password)); err != nil { | |
| return nil, errors.New("invalid credentials") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/service/auth/login.go` around lines 12 - 18, After
calling s.userRepo.FindByEmail(ctx, email) in the login handler, guard against u
being nil before dereferencing u.PasswordHash; if u == nil return the same
credentials error (e.g., errors.New("invalid credentials")) instead of calling
bcrypt.CompareHashAndPassword. Update the code around the FindByEmail call
(variable u) so the nil check happens immediately and only then invoke
bcrypt.CompareHashAndPassword with u.PasswordHash.
| existingUser, err := s.userRepo.FindByEmail(ctx, email) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("find user by email: %w", err) | ||
| } | ||
| if existingUser != nil { | ||
| return nil, errors.New("user with this email already exists") | ||
| } |
There was a problem hiding this comment.
Handle concurrent duplicate-email registration at write time.
The check at Lines 13-19 is race-prone. Two concurrent requests can both pass it; one fails later on unique constraint and gets a generic create error path.
🛠️ Suggested direction
- Keep the pre-check for UX if desired, but also map DB unique-constraint errors from
Create(...)to the same domain error (user with this email already exists).
Also applies to: 34-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/service/auth/register.go` around lines 13 - 19, The
pre-check using s.userRepo.FindByEmail is race-prone; keep it if you want UX,
but also detect and translate DB unique-constraint errors returned by
s.userRepo.Create (and the other Create call referenced around lines 34-40) into
the same domain error "user with this email already exists"; update the Create
call error handling in register.go to inspect the error (e.g., by type/assertion
or inspecting SQL/driver error codes/messages) and return errors.New("user with
this email already exists") when the DB reports a
duplicate-email/unique-constraint violation, otherwise return the wrapped error
as before.
| createdUser, err := s.userRepo.Create(ctx, user.CreateParams{ | ||
| Email: email, | ||
| PasswordHash: string(passwordHash), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("create user: %w", err) | ||
| } | ||
|
|
||
| if err := s.userRepo.AssignRole(ctx, createdUser.ID, role.ID); err != nil { | ||
| return nil, fmt.Errorf("assign role: %w", err) | ||
| } |
There was a problem hiding this comment.
Make user creation + role assignment atomic.
Lines 34-44 can leave orphaned users without roles when AssignRole fails after Create. Wrap both operations in one DB transaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin-auth/service/auth/register.go` around lines 34 - 44, The
create + assign sequence can leave orphaned users if AssignRole fails; modify
the Register flow to perform s.userRepo.Create and s.userRepo.AssignRole inside
a single database transaction: begin a transaction via the userRepo (or a
provided DB/Tx method), call Create with the transaction context to get
createdUser, then call AssignRole with the same transaction and role.ID, and
commit on success or rollback on any error; propagate errors using the same
fmt.Errorf wrapping. Ensure you use the transaction-aware variants or pass the
tx into Create/AssignRole so both operations are atomic.
| token, err := jwt.Parse(tokenStr, func(t *jwt.Token) (any, error) { | ||
| return []byte(secret), nil | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For github.com/golang-jwt/jwt/v5, what is the recommended way to restrict accepted signing algorithms when parsing tokens? Is jwt.WithValidMethods required/recommended with jwt.Parse?
💡 Result:
The recommended way to restrict accepted signing algorithms when parsing JWT tokens with github.com/golang-jwt/jwt/v5 is to use the jwt.WithValidMethods option with jwt.Parse (or jwt.ParseWithClaims, etc.). This supplies a list of allowed signing methods (e.g., []string{jwt.SigningMethodHS256.Alg}), ensuring only those are accepted and preventing attacks like algorithm confusion (e.g., "none" algorithm). It is strongly recommended (and explicitly stated as "heavily encouraged") to always use jwt.WithValidMethods with jwt.Parse to validate the 'alg' header matches expected algorithms. The library documentation emphasizes this for security: "It's important that you validate the alg presented is what you expect." Example usage: token, err := jwt.Parse(tokenString, keyFunc, jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg})) jwt.WithValidMethods is a ParserOption passed to jwt.Parse, which internally creates a jwt.Parser with validMethods set. The Parser checks token.Method.Alg against this list before verification, returning an error if mismatched.
Citations:
- 1: https://pkg.go.dev/github.com/golang-jwt/jwt/v5
- 2: https://golang-jwt.github.io/jwt/usage/parse/
- 3: https://sources.debian.org/src/golang-github-golang-jwt-jwt-v5/5.2.2-1~bpo12%252B1/parser.go
- 4: https://github.com/golang-jwt/jwt/blob/main/parser.go
🏁 Script executed:
find . -name "jwt.go" -path "*/pkg/*" | head -5Repository: ua-academy-projects/share-bite
Length of output: 90
🏁 Script executed:
cat -n ./pkg/jwt/jwt.goRepository: ua-academy-projects/share-bite
Length of output: 2394
Restrict accepted JWT algorithms during parse.
Line 57 parses tokens without explicit valid-method restriction. The code generates tokens with HS256 (line 52) but doesn't enforce this during parsing, creating an algorithm confusion vulnerability. Add jwt.WithValidMethods to restrict accepted algorithms, as strongly recommended by the library documentation.
🛡️ Proposed fix
func (m *TokenManager) parse(tokenStr, secret string) (string, error) {
token, err := jwt.Parse(tokenStr, func(t *jwt.Token) (any, error) {
+ if t.Method.Alg() != jwt.SigningMethodHS256.Alg() {
+ return nil, fmt.Errorf("unexpected signing method")
+ }
return []byte(secret), nil
- })
+ }, jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()}))📝 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.
| token, err := jwt.Parse(tokenStr, func(t *jwt.Token) (any, error) { | |
| return []byte(secret), nil | |
| }) | |
| func (m *TokenManager) parse(tokenStr, secret string) (string, error) { | |
| token, err := jwt.Parse(tokenStr, func(t *jwt.Token) (any, error) { | |
| if t.Method.Alg() != jwt.SigningMethodHS256.Alg() { | |
| return nil, fmt.Errorf("unexpected signing method") | |
| } | |
| return []byte(secret), nil | |
| }, jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()})) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/jwt/jwt.go` around lines 57 - 59, The JWT parsing call currently uses
jwt.Parse(tokenStr, func...) without restricting algorithms; update that call to
include jwt.WithValidMethods([]string{"HS256"}) (or
jwt.SigningMethodHS256.Alg()) so parsing only accepts HS256, e.g. call
jwt.Parse(tokenStr, jwt.WithValidMethods([]string{"HS256"}), func(t *jwt.Token)
(any, error) { return []byte(secret), nil }), keeping the same secret callback
and error handling in the same function.
Removed initial data insertion for roles and users.
| email VARCHAR(255) UNIQUE NOT NULL, | ||
| password_hash TEXT NOT NULL, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() |
There was a problem hiding this comment.
Both created_at and updated_at are kind of readonly metadata that represent our data state - they should be carefully managed in application and not updated from user data, etc.
Also at first, our entity can have created_at (created it just now, and we never modify it's value), but updated_at = NULL as we have not updated it yet. Then we need to change updated_at every time we update.
Writing custom code for managing updated state can can introduce bugs, especially with Postgres, that does not have ON UPDATE feature like MySQL/Maria.
So investigate if triggers can help here so we never touch updated_at from code ever again 😅
|
|
||
| type Tokens struct { | ||
| AccessToken string | ||
| RefreshToken string |
There was a problem hiding this comment.
if we're saving this in the database - we need to have some field with creation date and expiration date, so we can cleanup expired tokens.
also i would add a type field with a dictionary, e.g., access_token, refresh_token (can be extended with other values that we may need later). Makes it easier to manage token expiration separately.
i would also add the subject (user id) here to find tokens for each user.
and having a surrogate primary key also helps :)
| type authService interface { | ||
| type tokenResponce struct { | ||
| AccessToken string `json:"access_token"` | ||
| RefreshToken string `json:"refresh_token"` |
There was a problem hiding this comment.
i would also add expiration time to the response, so client can understand when the token is "useless" (in case we decide to encrypt the token later)
|
|
||
| type Repository struct { | ||
| db database.Client | ||
| type User struct { |
There was a problem hiding this comment.
We have entities - they should be managed by repository. If these are also entities - need to move to other package.
If these are business logic models - they should not be used on the repository level and converted in BL
| PasswordHash string | ||
| } | ||
|
|
||
| type Repository interface { |
| } | ||
|
|
||
| type Repository interface { | ||
| FindByEmail(ctx context.Context, email string) (*User, error) |
There was a problem hiding this comment.
repos return entities, they are lower level than domain/business layer
| return nil, fmt.Errorf("hash password: %w", err) | ||
| } | ||
|
|
||
| createdUser, err := s.userRepo.Create(ctx, user.CreateParams{ |
There was a problem hiding this comment.
Creating user and assigning roles should run in a transaction, we may need to implement either a Unit of Work pattern (may be not the best implementation, but to understand what's going on), so our services can manage DB transactions via an abstraction.
Or we can do one simple trick for repos, as both sql.DB and sql.Tx have similar/equal methods, we can add our wrapper for BL, e.g.
type DBTX interface {
ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error)
QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error)
QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row
// some other we may need
}so our repos can depend on this, and we can manage both single queries and transaction queries, e.g.,
type ExampleRepository struct {
db DBTX
}
// with some methods like `DoMagicFirst` & `DoMagicSecond`We can add a function like (may need updating to our case):
func RunInTransaction(
ctx context.Context,
db *sql.DB,
// as our DBTX has same methods we can probably be cool with this, or just pass our DBTX
operation func(tx *sql.Tx) error,
) error {
tx, err := db.BeginTx(ctx, nil)
if err != nil {
return err
}
defer tx.Rollback()
if err := operation(tx); err != nil {
return err
}
return tx.Commit()
}Finally, we can run like this (and it works with multiple repositories also):
err := RunInTransaction(ctx, db, func(tx *sql.Tx) error {
exRepo := repo.NewExampleRepository(tx)
if err := exRepo.DoMagicFirst(ctx, arg1, arg2, ...); err != nil {
return err
}
if err := exRepo.DoMagicSecond(ctx, arg3, arg4, ...); err != nil {
return err
}
// by having the `tx` we may do some transaction shenanigans like savepoints inside this function
return nil
})| "golang.org/x/crypto/bcrypt" | ||
| ) | ||
|
|
||
| type Tokens struct { |
There was a problem hiding this comment.
entities are DB layer, and service can have knowledge about it.
We can have service layer (domain/BL) models and DTO (exposed to handlers), but our application is not big enough to do that.
So we can use the DTOs only (and have only DTO <--> Entity conversion). It's ok if higher layer knows about one level below, but it should not be reversed or go few layers deep.
6 participants
Task SB14-auth
Summary by CodeRabbit
New Features
Database
Chores