[jwt] add module and config #191
Conversation
WalkthroughAdds a JWT authentication subsystem and users module, replaces legacy user model/repository usage across many handlers/modules, introduces permission-scope middleware and typed cache utilities, updates OpenAPI and observability assets, and wires FX providers/config for JWT and users. Changes
Sequence Diagram(s)sequenceDiagram
rect `#F5F7FF`
actor Client
participant AuthHandler as Auth Handler
participant JWTSvc as JWT Service
participant Repo as Token Repo
participant DB as Database
end
Client->>AuthHandler: POST /3rdparty/v1/auth/token (Basic)
AuthHandler->>JWTSvc: GenerateToken(ctx, userID, scopes, ttl)
JWTSvc->>JWTSvc: build claims, sign HS256
JWTSvc->>Repo: Insert(ctx, tokenModel)
Repo->>DB: INSERT tokens
Repo-->>JWTSvc: OK
JWTSvc-->>AuthHandler: TokenInfo
AuthHandler-->>Client: 201 Created + token
sequenceDiagram
rect `#F5FFF5`
actor Client
participant MW as JWT Middleware
participant JWTSvc as JWT Service
participant UsersSvc as Users Service
participant Handler as Protected Handler
end
Client->>MW: Request (Authorization: Bearer <token>)
MW->>JWTSvc: ParseToken(ctx, token)
JWTSvc->>JWTSvc: verify signature & claims
JWTSvc->>Repo: IsRevoked(ctx, jti)
Repo-->>JWTSvc: false
JWTSvc-->>MW: Claims(userID, scopes)
MW->>UsersSvc: GetByUsername(ctx, userID)
UsersSvc-->>MW: *User
MW->>MW: SetUser, SetScopes
MW->>Handler: Next()
Handler-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/config/config.go (1)
130-132: Consider a shorter default JWT TTL for better security.The default TTL of 1 year is unusually long for JWT tokens. Industry best practice typically ranges from minutes to hours (e.g., 1 hour to 24 hours). Long-lived tokens increase the window of opportunity for token theft and misuse.
If this extended TTL is intentional for the SMS gateway use case, consider adding a comment explaining the rationale.
Consider a more conservative default:
JWT: JWT{ - TTL: Duration(time.Hour * 24 * 365), // 1 year + TTL: Duration(time.Hour * 24), // 24 hours },internal/sms-gateway/jwt/config.go (1)
8-22: Guard against negative TTL values inConfig.ValidateRight now only
TTL == 0is rejected, so a negative duration would pass validation and later produce already-expired tokens, which is almost certainly a misconfiguration. Consider tightening the check to reject non‑positive values:func (c Config) Validate() error { if c.Secret == "" { return fmt.Errorf("%w: secret is required", ErrInvalidConfig) } - if c.TTL == 0 { - return fmt.Errorf("%w: ttl is required", ErrInvalidConfig) - } + if c.TTL <= 0 { + return fmt.Errorf("%w: ttl must be positive", ErrInvalidConfig) + } return nil }internal/sms-gateway/jwt/service.go (3)
20-45: Consider validatingrevokeddependency inNew
Newassumesrevokedis non‑nil; if wiring ever passesnil,IsRevoked/Revokewill panic at runtime. Either validate here and fail fast or document the non‑nil contract explicitly; validation could look like:func New(config Config, revoked *revokedStorage) (Service, error) { if err := config.Validate(); err != nil { return nil, err } + + if revoked == nil { + return nil, fmt.Errorf("%w: revoked storage is required", ErrInvalidConfig) + }
47-72: Tighten TTL handling and consider making issuer configurableTwo small points in
GenerateToken:
- A negative
ttlargument will currently produce an already‑expired token vianow.Add(min(ttl, s.config.TTL)). If that’s not a valid use case, consider rejecting negative values early:func (s *service) GenerateToken(userID string, scopes []string, ttl time.Duration) (string, error) { - if ttl == 0 { + if ttl < 0 { + return "", fmt.Errorf("ttl must be non-negative") + } + + if ttl == 0 { ttl = s.config.TTL }
- The issuer is hardcoded as
"sms-gate.app". If you have multiple environments or brands, you may want this to come from configuration instead of being fixed in code.
74-103: Align algorithm checks and error signaling inParseToken
ParseTokenis generally solid, but you might want to tighten and standardize behavior:
- You currently accept any
SigningMethodHMAC. To strictly mirrorGenerateToken(which always uses HS256), consider restricting to that method:parsedToken, err := jwt.ParseWithClaims(token, new(Claims), func(t *jwt.Token) (any, error) { - if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { - return nil, fmt.Errorf("%w: %v", ErrUnexpectedSigningMethod, t.Header["alg"]) - } + if t.Method != jwt.SigningMethodHS256 { + return nil, fmt.Errorf("%w: %v", ErrUnexpectedSigningMethod, t.Header["alg"]) + }
- For the
!ok || !parsedToken.Validcase you return a plain"invalid token"error. If callers need to distinguish invalid/expired tokens from other failures (similar to howErrTokenRevokedis used), consider introducing anErrInvalidTokensentinel inerrors.goand returning that here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(1 hunks)internal/config/config.go(4 hunks)internal/config/module.go(2 hunks)internal/config/types.go(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/jwt/config.go(1 hunks)internal/sms-gateway/jwt/disabled.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/jwt.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/revoked.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
internal/sms-gateway/jwt/module.go (6)
internal/config/module.go (1)
Module(23-134)internal/sms-gateway/app.go (1)
Module(34-58)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/config/config.go (2)
Cache(83-85)Config(12-23)internal/sms-gateway/jwt/service.go (2)
New(28-45)Service(14-18)internal/sms-gateway/jwt/config.go (1)
Config(8-11)
internal/config/config.go (1)
internal/config/types.go (1)
Duration(11-11)
internal/sms-gateway/jwt/errors.go (1)
internal/sms-gateway/jwt/service.go (1)
New(28-45)
internal/config/module.go (3)
internal/config/config.go (2)
Config(12-23)JWT(91-94)internal/sms-gateway/jwt/config.go (1)
Config(8-11)internal/config/types.go (1)
Duration(11-11)
internal/sms-gateway/jwt/config.go (2)
internal/config/config.go (1)
Config(12-23)internal/sms-gateway/jwt/errors.go (1)
ErrInvalidConfig(7-7)
internal/sms-gateway/jwt/service.go (3)
internal/sms-gateway/jwt/jwt.go (1)
Claims(5-10)internal/sms-gateway/jwt/config.go (1)
Config(8-11)internal/sms-gateway/jwt/errors.go (2)
ErrUnexpectedSigningMethod(9-9)ErrTokenRevoked(8-8)
internal/sms-gateway/jwt/revoked.go (2)
internal/config/config.go (1)
Cache(83-85)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(23-134)internal/sms-gateway/jwt/module.go (1)
Module(9-25)
internal/sms-gateway/jwt/disabled.go (3)
internal/sms-gateway/jwt/service.go (1)
Service(14-18)internal/sms-gateway/jwt/errors.go (1)
ErrDisabled(6-6)internal/sms-gateway/jwt/jwt.go (1)
Claims(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/config/types.go (1)
1-48: LGTM! Well-designed Duration type.The implementation is clean and robust:
- Nil-safe accessors prevent potential panics
- Proper error wrapping with context
- Clean delegation from YAML to text unmarshalling
- Interface assertions ensure compile-time compatibility
internal/config/module.go (1)
128-133: LGTM! Config provider follows established patterns.The JWT config provider correctly maps configuration fields and follows the same pattern as other providers in this module.
internal/sms-gateway/app.go (1)
10-10: LGTM! Clean module integration.JWT module is properly integrated into the application's dependency graph following the established pattern.
Also applies to: 57-57
internal/sms-gateway/jwt/jwt.go (1)
1-10: LGTM! Clean Claims structure.The Claims struct properly embeds RegisteredClaims and adds custom fields with appropriate JSON tags, following JWT best practices.
internal/sms-gateway/jwt/module.go (1)
9-25: LGTM! Well-structured FX module.The module properly:
- Provides a named logger for JWT operations
- Creates private cache and revoked storage dependencies
- Conditionally enables JWT based on configuration (empty Secret → disabled service)
- Follows established FX patterns
go.mod (1)
18-18: Dependency verified: v5.3.0 is the latest stable version and includes the security fix for the HIGH severity memory allocation vulnerability (CVE affecting versions < 5.2.2).No action required.
internal/sms-gateway/jwt/errors.go (1)
5-10: Error sentinel set looks appropriateThe exported error values cover the key JWT states (disabled, invalid config, revoked, bad signing method) and are well named for use as sentinels in higher layers.
internal/sms-gateway/jwt/disabled.go (1)
8-27: DisabledServiceimplementation is clear and consistentThe
disabledimplementation cleanly satisfiesServiceand consistently returnsErrDisabledfor all operations, which should make wiring and feature‑flagging straightforward for callers.
capcom6
force-pushed
the
jwt/introduce-jwt
branch
from
4a8df1c to
9a89acb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/config/config.go (2)
22-23: Clarify semantics ofJWT.Secretand potential enable/disable behaviorRight now
JWT.Secrethas no default and an empty string is a valid in-memory value. Depending on howjwt.Configand the JWT module consume this, an empty secret could either:
- silently weaken security (e.g., tokens effectively unsigned or trivially forgeable), or
- cause late failures when signing/verifying tokens.
It’s usually safer to make the configuration explicit:
- Either treat “JWT enabled” as a separate boolean in
JWTand validate thatSecretis non-empty when enabled, or- Fail fast during config loading if
Secretis empty whenever the JWT module is wired in.This keeps the security posture obvious and avoids ambiguous “empty secret means disabled?” semantics.
Also applies to: 91-95
3-4: JWT defaults look reasonable; consider deriving or requiringIssuerfor self‑hosted setupsThe default
TTLof 24 hours and a non-emptyIssuerhelp produce usable tokens out of the box. For self-hosted deployments, though, hard-coding"sms-gate.app"could lead toissclaims that don’t match the actual public host.You might consider either:
- deriving the default
Issuerfrom existing HTTP/API config if present, or- leaving
JWT.Issuerempty by default and requiring it (with validation) when JWT is enabled.Not critical, but it can reduce surprises in multi-tenant or custom-domain environments.
Also applies to: 131-134
internal/config/types.go (2)
11-27: Duration wrapper is solid; minor naming/style nitThe wrapper around
time.Durationwith nil‑safeDuration()/String()is straightforward and fits the config use case.Very minor:
Duration.Duration()reads a bit stuttery at call sites (e.g.,cfg.JWT.TTL.Duration()). If you touch this again before it spreads widely, consider a name likeAsDuration()orValue()for slightly clearer usage. Not a blocker.
29-45: Add nil‑guards in unmarshallers to avoid panics on misuse
UnmarshalTextandUnmarshalYAMLassumedis non‑nil. In normal struct-field usage that’s fine, but a manually used*Duration(or a future optional*Durationconfig field) could end up nil and panic when these methods are called.A small defensive check would make the type more robust with negligible cost. For example:
func (d *Duration) UnmarshalText(text []byte) error { - t, err := time.ParseDuration(string(text)) + if d == nil { + return fmt.Errorf("can't unmarshal duration into nil *Duration") + } + + t, err := time.ParseDuration(string(text)) if err != nil { return fmt.Errorf("can't parse duration: %w", err) } *d = Duration(t) return nil } func (d *Duration) UnmarshalYAML(value *yaml.Node) error { + if d == nil { + return fmt.Errorf("can't unmarshal duration into nil *Duration") + } + var s string if err := value.Decode(&s); err != nil { return fmt.Errorf("can't unmarshal duration: %w", err) } return d.UnmarshalText([]byte(s)) }This keeps behavior the same for normal config paths while hardening against accidental nil usage.
internal/sms-gateway/jwt/config.go (1)
14-24: Consider adding minimum secret length validation.For HMAC-based JWT signing, weak secrets can compromise security. Consider enforcing a minimum secret length (e.g., 32 bytes for HS256).
Example validation:
const minSecretLength = 32 func (c Config) Validate() error { if c.Secret == "" { return fmt.Errorf("%w: secret is required", ErrInvalidConfig) } if len(c.Secret) < minSecretLength { return fmt.Errorf("%w: secret must be at least %d bytes", ErrInvalidConfig, minSecretLength) } if c.TTL <= 0 { return fmt.Errorf("%w: ttl must be positive", ErrInvalidConfig) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(1 hunks)internal/config/config.go(4 hunks)internal/config/module.go(2 hunks)internal/config/types.go(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/jwt/config.go(1 hunks)internal/sms-gateway/jwt/disabled.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/jwt.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/revoked.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- go.mod
- internal/sms-gateway/app.go
- internal/sms-gateway/jwt/module.go
- internal/config/module.go
- internal/sms-gateway/jwt/disabled.go
- internal/sms-gateway/jwt/service.go
- internal/sms-gateway/jwt/revoked.go
- internal/sms-gateway/jwt/jwt.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/config/config.go (1)
internal/config/types.go (1)
Duration(11-11)
internal/sms-gateway/jwt/config.go (2)
internal/config/config.go (1)
Config(12-23)internal/sms-gateway/jwt/errors.go (1)
ErrInvalidConfig(8-8)
internal/sms-gateway/jwt/errors.go (1)
internal/sms-gateway/jwt/service.go (1)
New(28-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/jwt/errors.go (1)
5-11: LGTM!The error definitions are clear, follow Go conventions, and align well with their usage across the JWT module.
internal/sms-gateway/jwt/config.go (2)
8-12: LGTM!The Config struct is well-defined with appropriate types for JWT configuration.
15-17: Secret validation is correct.The empty string check properly ensures a secret is provided.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sms-gateway/handlers/3rdparty.go (2)
50-73: Critical bug: nil pointer panic and auth routes incorrectly placed behind authentication.Two critical issues:
Nil pointer dereference at Line 60: The
authHandlerfield is never initialized in the constructor (see lines 75-86), so callingh.authHandler.Register()will panic.Authentication chicken-and-egg problem: The auth routes are registered at Line 60 after the authentication middleware is applied (lines 55-58). This means the
/tokenendpoint requires authentication to obtain an authentication token, which is logically impossible.The
/authroutes should be registered before theuserauthmiddleware is applied, typically right after the health handler registration.Apply this diff to fix both issues:
func (h *thirdPartyHandler) Register(router fiber.Router) { router = router.Group("/3rdparty/v1") h.healthHandler.Register(router) + h.authHandler.Register(router.Group("/auth")) router.Use( userauth.NewBasic(h.authSvc), userauth.UserRequired(), ) - h.authHandler.Register(router.Group("/auth")) - h.messagesHandler.Register(router.Group("/message")) // TODO: remove after 2025-12-31
75-86: Critical bug: authHandler field not initialized.The constructor receives
params.AuthHandlerbut never assigns it to theauthHandlerfield. This will cause a nil pointer dereference panic whenRegister()is called.Apply this diff to fix:
func newThirdPartyHandler(params ThirdPartyHandlerParams) *thirdPartyHandler { return &thirdPartyHandler{ Handler: base.Handler{Logger: params.Logger.Named("ThirdPartyHandler"), Validator: params.Validator}, healthHandler: params.HealthHandler, messagesHandler: params.MessagesHandler, webhooksHandler: params.WebhooksHandler, devicesHandler: params.DevicesHandler, settingsHandler: params.SettingsHandler, logsHandler: params.LogsHandler, + authHandler: params.AuthHandler, authSvc: params.AuthSvc, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/sms-gateway/handlers/3rdparty.go(4 hunks)internal/sms-gateway/handlers/module.go(2 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/handlers/thirdparty/module.go(1 hunks)internal/sms-gateway/jwt/config.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/handlers/thirdparty/module.go
🧬 Code graph analysis (4)
internal/sms-gateway/handlers/thirdparty/module.go (2)
internal/sms-gateway/handlers/module.go (1)
Module(16-41)internal/sms-gateway/handlers/thirdparty/auth.go (1)
NewAuthHandler(14-21)
internal/sms-gateway/jwt/config.go (2)
internal/config/config.go (1)
Config(12-23)internal/sms-gateway/jwt/errors.go (1)
ErrInvalidConfig(8-8)
internal/sms-gateway/handlers/module.go (1)
internal/sms-gateway/handlers/thirdparty/module.go (1)
Module(8-16)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
internal/sms-gateway/jwt/config.go (3)
1-6: LGTM!Package declaration and imports are appropriate. Both
fmtandtimeare used correctly throughout the file.
8-10: LGTM!The minimum secret length of 32 bytes (256 bits) is appropriate for JWT secrets and aligns with security best practices for HMAC-SHA256.
18-32: LGTM! Previous concern has been addressed.The validation logic is comprehensive and correct:
- Secret validation checks for both emptiness and minimum length
- TTL validation correctly rejects both zero and negative values (line 27:
c.TTL <= 0)This addresses the previous review comment about negative TTL values.
internal/sms-gateway/handlers/thirdparty/module.go (1)
8-16: LGTM!The module structure follows the established fx patterns in the codebase. The logger configuration and provider setup are correct.
internal/sms-gateway/handlers/module.go (2)
9-9: LGTM!Import correctly added to support the new thirdparty module registration.
40-40: LGTM!Module registration follows the established pattern and correctly wires the thirdparty module into the handlers module.
internal/sms-gateway/handlers/thirdparty/auth.go (3)
10-21: LGTM!The AuthHandler structure and constructor follow the established patterns in the codebase. The embedded base.Handler is correctly initialized with logger and validator dependencies.
23-26: LGTM!Route registration follows the standard pattern. The endpoints are appropriately defined for JWT token generation and revocation.
28-34: Stub implementation as expected for draft PR.The NotImplemented placeholders are appropriate for the current draft state. These handlers will need proper implementation before the PR is ready for merge.
internal/sms-gateway/handlers/3rdparty.go (3)
10-10: LGTM!Import correctly added for the thirdparty.AuthHandler dependency.
19-34: LGTM!The AuthHandler dependency is correctly added to the parameter struct for fx injection.
36-48: LGTM!The authHandler field is correctly added to the internal handler struct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sms-gateway/jwt/service.go (2)
28-49: Consider unexported constructor or exported storage type.The exported
Newfunction accepts an unexported*revokedStorageparameter, making it unusable by external packages. IfNewis intended only for internal module wiring, consider making it unexported (new). Alternatively, if external packages should construct the service, export therevokedStoragetype or accept an interface.
98-101: Consider wrapping error for better context.Line 100 returns a generic
"invalid token"error. Consider usingfmt.Errorfto wrap or provide additional context about the validation failure, which aids debugging.- if !ok || !parsedToken.Valid { - return nil, fmt.Errorf("invalid token") - } + if !ok { + return nil, fmt.Errorf("invalid token claims type") + } + if !parsedToken.Valid { + return nil, fmt.Errorf("token validation failed") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/requests.http(1 hunks)internal/sms-gateway/handlers/3rdparty.go(5 hunks)internal/sms-gateway/jwt/service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/jwt/service.go (3)
internal/sms-gateway/jwt/jwt.go (1)
Claims(5-10)internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/errors.go (3)
ErrInitFailed(7-7)ErrInvalidConfig(8-8)ErrTokenRevoked(9-9)
internal/sms-gateway/handlers/3rdparty.go (3)
internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(10-12)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/sms-gateway/handlers/3rdparty.go (3)
10-10: LGTM: AuthHandler integration follows existing patterns.The import, field declarations, and initialization of
AuthHandlerare consistent with how other handlers (messages, webhooks, devices, etc.) are integrated into the third-party handler.Also applies to: 28-28, 45-45, 84-84
60-60: Verify middleware ordering for auth endpoints.The auth routes are registered after the
userauth.NewBasicanduserauth.UserRequiredmiddleware (lines 55-58), which means Basic authentication is required to access/auth/tokenand/auth/token/revoke.This appears to implement a token exchange pattern where clients trade Basic credentials for JWT tokens. However, please verify:
- Token generation: Is Basic auth → JWT exchange the intended flow?
- Token revocation: Should the revoke endpoint accept a Bearer token in the Authorization header instead of requiring Basic auth?
Consider whether token revocation should be accessible with Bearer tokens:
// Potential alternative: separate middleware for revoke endpoint authRouter := router.Group("/auth") h.authHandler.Register(authRouter) // Register first // Then apply selective middleware within AuthHandler.Register()
77-77: Logger initialization is consistent with all other handlers.All main handlers in the module (3rdparty, upstream, mobile) initialize their loggers identically using
params.Loggerdirectly without a name qualifier. Sub-handlers appropriately useNamed()for distinguishing their logs. No inconsistency exists.api/requests.http (1)
195-208: These test requests are for unimplemented endpoints—revisit once handlers are complete.The auth endpoint handlers in
internal/sms-gateway/handlers/thirdparty/auth.gocurrently returnfiber.ErrNotImplemented(lines 28-30 and 32-34). The test requests inapi/requests.http(lines 195-208) are placeholders and cannot be meaningfully validated against non-existent logic.Once the
token()andtokenRevoke()handlers are implemented, return here to verify and update these test requests with appropriate sample request bodies based on the actual implementation requirements.internal/sms-gateway/jwt/service.go (1)
82-112: Strong security validation in ParseToken.The implementation correctly enforces expiration, validates the issuer, restricts signing methods to HS256 (preventing algorithm confusion attacks), and checks token revocation. These are essential security practices for JWT handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/sms-gateway/jwt/revoked.go (1)
22-35: Revocation storage correctly uses cache TTL; consider clarifying naming
IsRevoked/Revokecorrectly treat presence in the cache as revocation and now apply a TTL viacache.WithTTL, which avoids unbounded growth in both Redis and in‑memory caches (where TTL is tracked by the app). Based on learnings.You might optionally rename the
tokenparameter (and related local naming) tojtiorkeyto better reflect that you’re storing the token ID, not the full JWT string.internal/sms-gateway/jwt/service.go (2)
51-76: Consider avoiding reliance on builtinminfor broader Go version support
ExpiresAtusesnow.Add(min(ttl, s.config.TTL)). If the project targets Go versions without the builtinminfortime.Duration, this will not compile; a small local helper keeps behavior explicit and portable:func minDuration(a, b time.Duration) time.Duration { if a < b { return a } return b }and then:
ExpiresAt: jwt.NewNumericDate(now.Add(minDuration(ttl, s.config.TTL))),Functionally the capping behavior (never exceeding configured TTL) is sound; this is mainly about portability and clarity.
90-124: Normalize ParseToken error mapping around ErrInvalidToken / ErrUnexpectedSigningMethodRight now:
- Any
jwt.ParseWithClaimserror is wrapped asfmt.Errorf("failed to parse token: %w", err).ErrInvalidTokenis only returned when the claims type assertion fails orparsedToken.Validis false.ErrUnexpectedSigningMethodis defined but not used; signing-method mismatches will surface as generic parse errors.For callers, it may be more convenient if all “token is syntactically/cryptographically/temporally invalid” cases (bad signature, expired, issuer mismatch, invalid method, etc.) are surfaced under
ErrInvalidToken, with the underlying jwt error still wrapped for logging/debugging. For example:parsedToken, err := jwt.ParseWithClaims(/* ... */) if err != nil { return nil, fmt.Errorf("%w: %v", ErrInvalidToken, err) } claims, ok := parsedToken.Claims.(*Claims) if !ok || !parsedToken.Valid { return nil, ErrInvalidToken }Optionally, if you want to distinguish signing‑method issues, you could inspect the error returned by the library and wrap it with
ErrUnexpectedSigningMethod. This would give a clearer and more stable public error contract while still usingErrTokenRevokedfor the revocation path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/revoked.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
internal/sms-gateway/jwt/revoked.go
🧬 Code graph analysis (4)
internal/sms-gateway/jwt/module.go (5)
internal/sms-gateway/app.go (1)
Module(34-58)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/jwt/service.go (2)
New(28-49)Service(14-18)internal/sms-gateway/jwt/revoked.go (2)
NewRevokedStorage(16-20)RevokedStorage(12-14)internal/sms-gateway/jwt/config.go (1)
Config(12-16)
internal/sms-gateway/jwt/errors.go (1)
internal/sms-gateway/jwt/service.go (1)
New(28-49)
internal/sms-gateway/jwt/service.go (4)
internal/sms-gateway/jwt/jwt.go (1)
Claims(5-10)internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/revoked.go (1)
RevokedStorage(12-14)internal/sms-gateway/jwt/errors.go (4)
ErrInitFailed(7-7)ErrInvalidConfig(8-8)ErrInvalidToken(9-9)ErrTokenRevoked(10-10)
internal/sms-gateway/jwt/revoked.go (3)
internal/config/config.go (1)
Cache(83-85)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)pkg/cache/options.go (1)
WithTTL(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/jwt/errors.go (1)
5-12: Centralized JWT error definitions look goodError values are clear, specific, and align with the usages in the service (config/init/token/revocation), which should keep downstream error handling consistent.
internal/sms-gateway/jwt/module.go (1)
9-24: FX wiring and conditional disablement are coherentThe module cleanly encapsulates its own cache and
RevokedStorageas private providers and exposes onlyService, with a straightforward disabled path whenConfig.Secretis empty. This keeps JWT concerns modular without leaking cache internals.internal/sms-gateway/jwt/service.go (1)
28-48: Constructor enforces config and dependencies appropriately
NewvalidatesConfig, rejects a nilRevokedStorage, and wires anidFactoryfrom nanoid, so misconfigurations or missing dependencies fail fast at startup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/sms-gateway/jwt/revoked.go (3)
12-20: Add godoc comments for exported types and methods.The exported
RevokedStoragetype andNewRevokedStorageconstructor lack documentation. Adding godoc comments will improve API discoverability and maintainability.Apply this diff to add documentation:
+// RevokedStorage manages revoked JWT tokens using a cache backend. +// Revoked tokens are stored with a TTL matching their expiration time +// to prevent unbounded cache growth. type RevokedStorage struct { storage cache.Cache } +// NewRevokedStorage creates a new RevokedStorage backed by the given cache. func NewRevokedStorage(storage cache.Cache) *RevokedStorage { return &RevokedStorage{ storage: storage, } }Optionally, add a nil check for defensive coding:
func NewRevokedStorage(storage cache.Cache) *RevokedStorage { + if storage == nil { + panic("storage cannot be nil") + } return &RevokedStorage{ storage: storage, } }
22-32: Add godoc comment; implementation is correct.The
IsRevokedmethod correctly handles all three cases: token not found (not revoked), token found (revoked), and errors. Error wrapping provides helpful context.Add documentation:
+// IsRevoked checks whether a JWT with the given ID has been revoked. +// Returns (false, nil) if the token is not revoked, (true, nil) if revoked, +// or (false, error) on lookup failure. func (r *RevokedStorage) IsRevoked(ctx context.Context, jti string) (bool, error) { _, err := r.storage.Get(ctx, jti) if errors.Is(err, cache.ErrKeyNotFound) {
34-36: TTL implementation looks good; consider error wrapping for consistency.The addition of the
ttlparameter andcache.WithTTL(ttl)correctly addresses the previous concern about unbounded cache growth. Revoked tokens will now expire automatically.Add godoc and optionally wrap errors for consistency with
IsRevoked:+// Revoke marks a JWT with the given ID as revoked until the specified TTL expires. +// The TTL should typically match the token's remaining lifetime. func (r *RevokedStorage) Revoke(ctx context.Context, jti string, ttl time.Duration) error { - return r.storage.Set(ctx, jti, nil, cache.WithTTL(ttl)) + if err := r.storage.Set(ctx, jti, nil, cache.WithTTL(ttl)); err != nil { + return fmt.Errorf("failed to revoke token: %w", err) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/jwt/revoked.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
internal/sms-gateway/jwt/revoked.go
🧬 Code graph analysis (1)
internal/sms-gateway/jwt/revoked.go (2)
pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)pkg/cache/options.go (1)
WithTTL(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
jwt/introduce-jwt
branch
from
9673e3c to
9d5cf91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
configs/config.example.yml (1)
41-44: Document that empty secret disables JWT.The
jwt.secretfield is empty by default, which will cause the JWT service to operate in disabled mode (returningErrDisabledfor all operations). Consider adding a comment to make this behavior explicit for users configuring the system.Apply this diff to clarify the behavior:
jwt: - secret: # jwt secret [JWT__SECRET] + secret: # jwt secret (leave empty to disable JWT functionality) [JWT__SECRET] ttl: 24h # jwt ttl [JWT__TTL] issuer: # jwt issuer [JWT__ISSUER]internal/sms-gateway/handlers/thirdparty/auth.go (1)
70-72: Token revocation endpoint is not implemented.The
deleteTokenhandler returnsErrNotImplemented. According to the JWT service interface, this should calljwtSvc.RevokeToken(ctx, jti)to revoke the token.Would you like me to generate the implementation? The basic logic would be:
func (h *AuthHandler) deleteToken(user users.User, c *fiber.Ctx) error { - return fiber.ErrNotImplemented + jti := c.Params("jti") + if jti == "" { + return fiber.NewError(fiber.StatusBadRequest, "JTI is required") + } + + if err := h.jwtSvc.RevokeToken(c.Context(), jti); err != nil { + return err + } + + return c.SendStatus(fiber.StatusNoContent) }internal/sms-gateway/users/repository.go (1)
43-45: Consider wrapping error for consistency.Unlike
GetByIDandInsert, this method returns the raw GORM error. For consistency, consider wrapping it:func (r *repository) UpdatePassword(ID string, passwordHash string) error { - return r.db.Model((*User)(nil)).Where("id = ?", ID).Update("password_hash", passwordHash).Error + if err := r.db.Model((*User)(nil)).Where("id = ?", ID).Update("password_hash", passwordHash).Error; err != nil { + return fmt.Errorf("can't update password: %w", err) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
api/requests.http(1 hunks)configs/config.example.yml(1 hunks)go.mod(1 hunks)internal/config/config.go(4 hunks)internal/config/module.go(2 hunks)internal/config/types.go(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/3rdparty.go(5 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(3 hunks)internal/sms-gateway/handlers/logs/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(5 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(3 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/handlers/module.go(2 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(4 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/handlers/thirdparty/module.go(1 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(4 hunks)internal/sms-gateway/jwt/config.go(1 hunks)internal/sms-gateway/jwt/disabled.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/jwt.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/revoked.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)internal/sms-gateway/models/migration.go(1 hunks)internal/sms-gateway/models/models.go(0 hunks)internal/sms-gateway/modules/auth/repository.go(2 hunks)internal/sms-gateway/modules/auth/service.go(8 hunks)internal/sms-gateway/modules/messages/service.go(3 hunks)internal/sms-gateway/modules/settings/models.go(1 hunks)internal/sms-gateway/modules/webhooks/models.go(2 hunks)internal/sms-gateway/users/domain.go(1 hunks)internal/sms-gateway/users/errors.go(1 hunks)internal/sms-gateway/users/models.go(1 hunks)internal/sms-gateway/users/module.go(1 hunks)internal/sms-gateway/users/repository.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/models/models.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/config/module.go
- internal/sms-gateway/users/errors.go
- internal/sms-gateway/users/models.go
- go.mod
- internal/sms-gateway/handlers/settings/3rdparty.go
- internal/sms-gateway/modules/webhooks/models.go
- internal/sms-gateway/jwt/jwt.go
- internal/sms-gateway/users/module.go
- internal/sms-gateway/handlers/messages/3rdparty.go
- internal/sms-gateway/handlers/devices/3rdparty.go
- internal/sms-gateway/jwt/errors.go
- internal/sms-gateway/modules/settings/models.go
- internal/sms-gateway/modules/messages/service.go
- api/requests.http
- internal/sms-gateway/jwt/config.go
- internal/sms-gateway/handlers/thirdparty/module.go
- internal/sms-gateway/handlers/module.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
internal/sms-gateway/jwt/revoked.go
🧬 Code graph analysis (16)
internal/sms-gateway/jwt/module.go (7)
internal/config/module.go (1)
Module(23-135)internal/sms-gateway/app.go (1)
Module(34-58)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/jwt/service.go (1)
New(22-43)internal/sms-gateway/jwt/revoked.go (2)
NewRevokedStorage(16-20)RevokedStorage(12-14)internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)
internal/sms-gateway/jwt/disabled.go (3)
internal/sms-gateway/jwt/jwt.go (3)
Service(10-14)TokenInfo(23-27)Claims(16-21)internal/config/types.go (1)
Duration(11-11)internal/sms-gateway/jwt/errors.go (1)
ErrDisabled(6-6)
internal/sms-gateway/app.go (2)
internal/sms-gateway/jwt/module.go (1)
Module(9-25)internal/config/module.go (1)
Module(23-135)
internal/sms-gateway/handlers/mobile.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/jwt/revoked.go (2)
pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)pkg/cache/options.go (1)
WithTTL(22-30)
internal/sms-gateway/handlers/3rdparty.go (3)
internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(15-19)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/sms-gateway/handlers/logs/3rdparty.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/config/config.go (1)
internal/config/types.go (1)
Duration(11-11)
internal/sms-gateway/handlers/thirdparty/auth.go (3)
internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(121-125)internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/handlers/webhooks/3rdparty.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/jwt/service.go (4)
internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/revoked.go (1)
RevokedStorage(12-14)internal/sms-gateway/jwt/jwt.go (3)
Service(10-14)TokenInfo(23-27)Claims(16-21)internal/sms-gateway/jwt/errors.go (4)
ErrInitFailed(7-7)ErrInvalidConfig(8-8)ErrInvalidToken(9-9)ErrTokenRevoked(10-10)
internal/sms-gateway/models/migration.go (1)
internal/sms-gateway/models/models.go (1)
Device(17-28)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/modules/auth/repository.go (1)
internal/sms-gateway/users/models.go (1)
User(10-16)
internal/sms-gateway/users/repository.go (2)
internal/sms-gateway/users/models.go (1)
User(10-16)internal/sms-gateway/users/errors.go (1)
ErrNotFound(6-6)
🪛 GitHub Actions: Go
internal/sms-gateway/users/repository.go
[error] 10-10: golangci-lint: type repository is unused (unused).
🪛 GitHub Check: Lint
internal/sms-gateway/users/repository.go
[failure] 43-43:
func (*repository).UpdatePassword is unused (unused)
[failure] 35-35:
func (*repository).Insert is unused (unused)
[failure] 22-22:
func (*repository).GetByID is unused (unused)
[failure] 15-15:
func newRepository is unused (unused)
[failure] 10-10:
type repository is unused (unused)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Build / Docker image (linux/amd64)
🔇 Additional comments (28)
internal/sms-gateway/models/migration.go (1)
12-13: Confirm users migrations are now handled outsidemodels.MigrateRestricting
AutoMigratehere toDeviceonly matches the move ofUserinto theuserspackage, so this function’s scope is now clear and focused. Just make sure:
- There is a corresponding
users.Migrate(or similar) that migrates theusersschema.- Whatever initializes the DB now calls both migration entry points in a deterministic order (typically users before devices, if you ever add FKs).
If both are true, this change looks good.
internal/sms-gateway/jwt/disabled.go (1)
8-28: LGTM! Clean null object pattern implementation.The disabled service implementation correctly returns
ErrDisabledfor all JWT operations when the service is not configured. This provides graceful degradation whenconfig.Secretis empty.internal/config/types.go (1)
11-48: LGTM! Well-implemented Duration type with proper nil-safety.The custom Duration type correctly implements YAML and text unmarshaling with nil-safe accessors. Error wrapping is appropriate, and compile-time interface checks ensure conformance.
internal/sms-gateway/handlers/3rdparty.go (2)
10-10: LGTM! AuthHandler integration is correct.The third-party authentication handler is properly wired through FX dependency injection and registered at the
/authroute group.Also applies to: 28-28, 45-45, 60-60, 84-84
77-77: Verify logger naming change is intentional.The logger no longer uses
.Named("ThirdPartyHandler"), which changes the logger identifier from "ThirdPartyHandler" to the parent logger name. This may affect log filtering and observability. Please confirm this change is intentional.Previous pattern (based on other handlers):
Logger: params.Logger.Named("ThirdPartyHandler")Current pattern:
Logger: params.Loggerinternal/sms-gateway/users/domain.go (1)
1-1: Question: Purpose of empty domain.go file?This file contains only a package declaration with no types, functions, or imports. Is this a placeholder for future domain logic, or was content intended to be added in this PR?
internal/sms-gateway/handlers/mobile.go (1)
19-19: LGTM! Clean refactoring to use the users package.The User type references have been correctly updated from
models.Usertousers.User, aligning with the new package structure. No logic changes were introduced.Also applies to: 104-104, 179-179
internal/sms-gateway/jwt/module.go (1)
9-25: LGTM! Clean FX module with conditional service activation.The JWT module correctly provides a disabled service when
config.Secretis empty (line 18) and a fully functional service otherwise. The private cache and revoked storage are properly scoped within the module.internal/sms-gateway/app.go (1)
10-10: LGTM! JWT module correctly integrated into the application.The JWT module is properly imported and registered in the FX module list, following the established pattern for other modules.
Also applies to: 57-57
internal/sms-gateway/handlers/logs/3rdparty.go (1)
6-6: LGTM! Clean refactor to the new users package.The type migration from
models.Usertousers.Useris consistent with the broader refactoring across the codebase.Also applies to: 38-38
internal/sms-gateway/modules/auth/repository.go (1)
4-4: LGTM! Consistent type migration across repository methods.All repository methods have been updated to use
users.Userconsistently, aligning with the new users package structure.Also applies to: 19-36
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
8-8: LGTM! Middleware properly updated to use new users package.The middleware functions and comments have been consistently updated to use
users.Userthroughout.Also applies to: 95-99, 121-121
internal/sms-gateway/jwt/revoked.go (2)
22-32: LGTM! IsRevoked implementation is correct.The logic properly distinguishes between not-found (not revoked) and error cases, with appropriate error wrapping.
34-36: LGTM! Revoke correctly includes TTL to prevent unbounded cache growth.The implementation now properly sets a TTL for revoked tokens, addressing the concern from the previous review. This ensures that revoked tokens are automatically cleaned up after the JWT expires.
internal/sms-gateway/handlers/thirdparty/auth.go (2)
39-42: LGTM! Token request validation is properly configured.The validation rules correctly enforce that at least one scope is provided, with TTL being optional.
51-68: LGTM! Token generation handler is correctly implemented.The handler properly validates input, generates the token, and returns a 201 status with the token details.
internal/sms-gateway/users/repository.go (2)
22-33: LGTM! GetByID properly handles errors and maps GORM's not found error.The implementation correctly uses
Takefor single-record queries and properly wraps errors.
35-41: LGTM! Insert method is correctly implemented.The method properly creates the user record with appropriate error wrapping.
internal/sms-gateway/handlers/webhooks/3rdparty.go (1)
10-10: LGTM! Webhooks handler consistently updated to use new users package.All three handler methods (
get,post,delete) have been consistently updated to useusers.User.Also applies to: 43-43, 66-66, 96-96
internal/config/config.go (1)
91-95: The codebase properly validates empty JWT secrets. TheConfig.Validate()method ininternal/sms-gateway/jwt/config.go(line 19) explicitly checks for empty secrets and returns an error, with an additional minimum length requirement (line 23). Additionally, the module initialization ininternal/sms-gateway/jwt/module.go(line 18) provides a defensive check that returns a disabled service if the secret is empty. No action required.internal/sms-gateway/jwt/service.go (5)
22-43: LGTM! Constructor properly validates dependencies.The constructor correctly validates the config, ensures revoked storage is not nil, and initializes the nanoid-based ID factory with appropriate error handling.
46-48: Excellent! UserID validation now implemented.The addition of userID validation prevents generating JWT tokens with an empty Subject claim, addressing the concern from the previous review.
45-82: Verify TTL capping behavior is documented for API consumers.Line 69 caps the token expiry at
min(ttl, s.config.TTL), which enforces a maximum TTL from configuration. When a caller requests a TTL larger than the configured maximum, it will be silently reduced. This is a reasonable design to prevent excessively long-lived tokens, but ensure this behavior is documented in the API so callers understand their requested TTL may not be honored.
84-114: LGTM! ParseToken has comprehensive validation.The token parsing includes proper security validations (expiration, issuer, signing method) and correctly integrates revocation checks. The error handling appropriately wraps errors for context while returning sentinel errors for revoked/invalid tokens.
116-118: LGTM! Clean delegation to revocation storage.The method appropriately delegates to the revocation storage layer with the configured TTL, ensuring revocation records are retained for the appropriate duration.
internal/sms-gateway/modules/auth/service.go (3)
15-15: LGTM! Import added for user type migration.The addition of the
userspackage import supports the migration of the User type frommodelstousers, while themodelsimport remains for Device and other types.
45-45: LGTM! Cache type updated consistently.The
usersCachefield type and initialization correctly reflect the migration tousers.User, maintaining the existing cache configuration.Also applies to: 67-67
99-114: Type migration verified successfully—all call sites properly updated.All public methods in the auth service that return or accept
users.Userhave been consistently updated. Verification confirms:
RegisterUserreturn values assigned tousers.Uservariables (mobile.go:104)RegisterDevicereceivesusers.Userparameters correctly (mobile.go:123)AuthorizeUserreturn values assigned tousers.Uservariables (userauth.go:48)AuthorizeUserByCodereturn values assigned tousers.Uservariables (userauth.go:75)- No remaining
models.Userreferences outside the auth moduleThe type migration is complete and consistent throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
configs/config.example.yml(1 hunks)internal/sms-gateway/users/module.go(1 hunks)internal/sms-gateway/users/repository.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- configs/config.example.yml
- internal/sms-gateway/users/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/users/repository.go (2)
internal/sms-gateway/users/models.go (1)
User(10-16)internal/sms-gateway/users/errors.go (1)
ErrNotFound(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/users/repository.go (3)
21-33: Good error handling for record not found.The method correctly distinguishes between "record not found" and other database errors, returning the appropriate custom error.
35-41: LGTM!The insert operation is correctly implemented with proper error wrapping.
10-51: Repository is properly provided to the DI container—concern resolved.The repository is correctly registered in
users/module.goat line 13 viafx.Provide(newRepository, fx.Private), which addresses the previous review concern. Thefx.Privatescope appropriately restricts visibility to the module while making it available for injection to internal consumers.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
api/mobile.http (1)
71-72: Test credentials in version control require caution.The static analysis tool flagged these values as potential generic API keys. While these are clearly placeholder test credentials (not production secrets), it's generally best practice to avoid committing any credentials—even test ones—to version control.
Consider moving these to environment variables or a gitignored
.http.localfile that developers can customize locally without risk of accidental exposure.internal/sms-gateway/handlers/3rdparty.go (1)
36-83:authSvcis unused; confirm intended auth coverage for/3rdparty/v1/auth
- The
authSvc *auth.Servicefield (and corresponding constructor parameter) is no longer used anywhere in this handler. It can be safely dropped to keep the dependency surface clean.- With
router.Use(userauth.NewBasic(h.usersSvc), userauth.UserRequired())applied beforeh.authHandler.Register(router.Group("/auth")), all/3rdparty/v1/auth/**routes now require Basic auth as well. That’s probably what you want (e.g. exchanging Basic for JWT), but it’s a behavior change worth double‑checking against the API contract.type thirdPartyHandler struct { base.Handler - authSvc *auth.Service usersSvc *users.Service @@ -func newThirdPartyHandler( - authSvc *auth.Service, +func newThirdPartyHandler( usersSvc *users.Service, @@ - authSvc: authSvc, usersSvc: usersSvc,internal/sms-gateway/users/service.go (1)
33-108: Don’t let cache failures block password changesThe service logic looks good overall (bcrypt usage, error wrapping, and login cache behavior). One place that’s a bit harsh is
ChangePassword:if err := s.cache.Delete(ctx, username, currentPassword); err != nil { return err }Given that
Loginalready treats the cache as best‑effort (miss or failure just falls back to DB) andSetonly logs on error, making password changes fail when the cache is unavailable is inconsistent and can hurt reliability.Consider aligning it with
Login:- if err := s.cache.Delete(ctx, username, currentPassword); err != nil { - return err - } + if err := s.cache.Delete(ctx, username, currentPassword); err != nil { + s.logger.Error("failed to delete login cache entry", zap.String("username", username), zap.Error(err)) + }You may also want to be aware that
Create’sExists→Insertsequence is still racy under concurrent creation attempts; the DB primary key will save you, but the resulting error won’t beErrExists.internal/sms-gateway/modules/auth/service.go (1)
24-36: Auth service integration withusers.Serviceand code-based auth looks sane
- Injecting
usersSvc *users.Serviceand delegatingRegisterDeviceandAuthorizeUserByCodeto it is a clean separation of concerns.AuthorizeUserByCode’s flow (codesCache.GetAndDelete→usersSvc.GetByUsername) is straightforward; with the userauth middleware storing a value into Locals, types now line up end‑to‑end.No issues from these changes; just consider renaming
userIDinGenerateUserCode/codesCachetousernamefor clarity, since it’s passed toGetByUsername.Also applies to: 38-59, 90-97, 133-145
internal/sms-gateway/handlers/mobile.go (2)
28-41: Remove commented-out code.The old fx.In dependency injection structure is no longer used. Consider removing this commented code to improve maintainability.
142-144: Add defensive nil check afterGetUser.While
HasUserchecksGetUser(c) != nil, adding an explicit nil check improves defensive coding and prevents potential panics if the middleware implementation changes.Apply this diff:
if userauth.HasUser(c) { user = userauth.GetUser(c) + if user == nil { + return fmt.Errorf("user authentication inconsistency") + } username = user.ID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
api/mobile.http(1 hunks)api/requests.http(1 hunks)internal/sms-gateway/app.go(4 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(4 hunks)internal/sms-gateway/handlers/mobile.go(8 hunks)internal/sms-gateway/modules/auth/module.go(0 hunks)internal/sms-gateway/modules/auth/repository.go(0 hunks)internal/sms-gateway/modules/auth/service.go(4 hunks)internal/sms-gateway/users/cache.go(1 hunks)internal/sms-gateway/users/domain.go(1 hunks)internal/sms-gateway/users/errors.go(1 hunks)internal/sms-gateway/users/models.go(1 hunks)internal/sms-gateway/users/module.go(1 hunks)internal/sms-gateway/users/repository.go(1 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/typed.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/sms-gateway/modules/auth/module.go
- internal/sms-gateway/modules/auth/repository.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/sms-gateway/users/module.go
- internal/sms-gateway/users/errors.go
- api/requests.http
- internal/sms-gateway/users/domain.go
- internal/sms-gateway/users/repository.go
🧰 Additional context used
🧬 Code graph analysis (9)
internal/sms-gateway/users/cache.go (3)
pkg/cache/typed.go (2)
Typed(13-15)NewTyped(17-21)internal/sms-gateway/users/domain.go (1)
User(5-10)pkg/cache/options.go (2)
AndSetTTL(64-68)WithTTL(22-30)
internal/sms-gateway/users/service.go (3)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/users/errors.go (1)
ErrExists(7-7)pkg/crypto/passwords.go (2)
MakeBCryptHash(14-20)CompareBCryptHash(22-27)
internal/sms-gateway/app.go (3)
internal/sms-gateway/users/module.go (1)
Module(12-27)internal/sms-gateway/jwt/module.go (1)
Module(9-25)internal/config/module.go (1)
Module(23-135)
pkg/cache/typed.go (1)
pkg/cache/options.go (1)
GetOption(48-48)
internal/sms-gateway/users/models.go (3)
internal/sms-gateway/models/models.go (1)
SoftDeletableModel(12-15)internal/sms-gateway/models/migration.go (1)
Migrate(12-14)internal/sms-gateway/modules/settings/models.go (1)
Migrate(20-25)
internal/sms-gateway/handlers/mobile.go (4)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(11-17)internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (3)
HasUser(89-91)GetUser(96-103)NewBasic(20-57)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
internal/sms-gateway/users/service.go (1)
Service(11-17)internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/handlers/3rdparty.go (5)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(11-17)internal/sms-gateway/modules/auth/service.go (1)
Service(24-36)internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(15-19)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
NewBasic(20-57)UserRequired(108-116)
internal/sms-gateway/modules/auth/service.go (4)
internal/sms-gateway/users/service.go (1)
Service(11-17)internal/sms-gateway/online/service.go (1)
Service(14-17)internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/models/models.go (1)
Device(17-28)
🪛 Gitleaks (8.29.0)
api/mobile.http
[high] 71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 72-72: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/users/models.go (1)
10-27: User persistence model & migration look consistentSchema and
Migrateimplementation match existing patterns (soft-deletable model, explicit table name, wrapped migration error). No issues from this diff alone.internal/sms-gateway/users/cache.go (1)
14-79: Login cache behavior is reasonable given the TTL and keying strategyUsing a 1‑hour TTL and deriving the cache key as
sha256(username || 0x00 || password)keeps only successful logins cached and avoids storing raw credentials. With the Typed cache fixed, Get/Set/Delete here look correct and errors are surfaced cleanly to the service layer.internal/sms-gateway/app.go (1)
7-60: JWT and users modules are wired correctly into the Fx graphAdding
users.Module()andjwt.Module()to the top‑levelModuleis consistent with how other modules are registered, and should give both services access to config/logger/cache as intended. No issues from this diff.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cache/redis.go (1)
41-44: Fix inconsistent HEXPIRE syntax on line 44.HEXPIRE requires
FIELDS count fieldsyntax. Line 40 correctly uses'FIELDS', '1', field, but line 44 omits the FIELDS keyword. Line 44 must be updated to match:- redis.call('HExpire', KEYS[1], newTtl, field) + redis.call('HExpire', KEYS[1], newTtl, 'FIELDS', '1', field)
♻️ Duplicate comments (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
128-131: GuardWithUseragainst missing user for a clearer failure mode
WithUsercurrently does:return handler(*GetUser(c), c)If it’s ever used without a preceding auth middleware (or if that middleware fails to set the user), this will panic with a nil dereference, which is harder to diagnose.
You already document that it will panic when the user is missing; consider adding an explicit check to produce a clearer panic message:
func WithUser(handler func(users.User, *fiber.Ctx) error) fiber.Handler { return func(c *fiber.Ctx) error { - return handler(*GetUser(c), c) + user := GetUser(c) + if user == nil { + panic("userauth.WithUser called without an authenticated user in context") + } + return handler(*user, c) } }This keeps the contract (“panics if user is missing”) but makes debugging much easier if the middleware chain is misconfigured.
internal/sms-gateway/handlers/mobile.go (1)
43-74: Handlenanoid.Standarderror to avoid nilidGenpanicsAt Line 56 you ignore the error from
nanoid.Standard(21):idGen, _ := nanoid.Standard(21)If initialization fails,
idGenwill be nil and any laterh.idGen()call will panic at runtime. This was already flagged in a previous review; it’s still a critical correctness issue.A minimal fix is to fail fast during handler construction:
func newMobileHandler( @@ - idGen, _ := nanoid.Standard(21) + idGen, err := nanoid.Standard(21) + if err != nil { + panic(fmt.Sprintf("failed to initialize ID generator: %v", err)) + }This guarantees
idGenis never nil in live handlers.
🧹 Nitpick comments (4)
internal/sms-gateway/users/service.go (1)
96-112: Avoid failing password changes on missing/expired cache entries
ChangePasswordcurrently treats any error froms.cache.Deleteas fatal. If the login entry has already expired or was never cached,Deletewill likely returncache.ErrKeyNotFound, which would cause a valid password change to fail purely due to cache state, even though you’ve already authenticated viaLogin.You already treat
cache.ErrKeyNotFoundas benign inLogin; consider doing the same here so cache misses don’t block updates:func (s *Service) ChangePassword(ctx context.Context, username, currentPassword, newPassword string) error { @@ - if err := s.cache.Delete(ctx, username, currentPassword); err != nil { - return err - } + if err := s.cache.Delete(ctx, username, currentPassword); err != nil { + if !errors.Is(err, cache.ErrKeyNotFound) { + return err + } + }The rest of the flow (authenticate via
Login, hash new password, thenUpdatePassword) looks good.internal/sms-gateway/handlers/mobile.go (2)
114-152: Clarify response semantics for existing vs new users inpostDeviceThe flow for creating/registering a device with the new
users.Servicelooks consistent (reusing an authenticated user when present and creating a new one otherwise). One detail to double‑check is the response payload:
- For new users, you set both
LoginandPassword.- For already authenticated users, you set
Login = usernamebut leavePasswordempty.The docstring says “Returns user credentials only for new users”, which might suggest that both fields should be omitted/empty for existing users, not just
Password. If clients interpret any non‑emptyLoginas “new credentials”, this could be confusing.If the intent is truly “credentials only for new users”, consider leaving
Loginempty (or documenting that onlyPasswordis conditional) in the authenticated‑user branch.
223-236: Differentiate invalid password from internal failures inchangePassword
changePasswordcurrently treats any error fromh.usersSvc.ChangePasswordas “Invalid current password” and returns401:if err := h.usersSvc.ChangePassword(...); err != nil { h.Logger.Error("failed to change password", zap.Error(err)) return fiber.NewError(fiber.StatusUnauthorized, "Invalid current password") }But
ChangePasswordcan fail for reasons other than a bad current password (e.g. DB error, cache backend failure, hashing error). Mapping all of those to401hides server‑side issues and misleads clients.Consider distinguishing:
- Invalid current password →
401 Unauthorizedwith the current message.- Other errors → propagate or wrap as
500 Internal Server Error.You can do this by checking
errors.Is(err, crypto.ErrPasswordInvalid)(the error returned fromCompareBCryptHash) versus other error types.internal/sms-gateway/handlers/3rdparty.go (1)
64-87: Confirm that protecting/3rdparty/v1/authwith Basic auth is intentionalIn
Registeryou add:router.Use( userauth.NewBasic(h.usersSvc), userauth.UserRequired(), ) h.authHandler.Register(router.Group("/auth"))Given this order, the
/3rdparty/v1/authendpoints registered byauthHandlerwill also go throughNewBasic+UserRequired, effectively requiring basic authentication for the auth routes themselves.If the design is “exchange Basic credentials for JWT tokens” then this is fine; if the
/authendpoints are meant to be usable without prior Basic auth (e.g. by presenting some other credential or for login itself), you may want to:
- Apply the Basic middleware only to specific sub‑groups (messages/devices/settings/etc.), or
- Register
/authbefore callingrouter.Use.Please verify that the current middleware scope matches the desired API behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(4 hunks)internal/sms-gateway/handlers/mobile.go(8 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(1 hunks)pkg/cache/typed.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cache/typed.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/redis.go
📚 Learning: 2025-09-10T23:54:16.828Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:118-121
Timestamp: 2025-09-10T23:54:16.828Z
Learning: Redis 7.4.0 introduced HEXPIRE, HEXPIREAT, and related commands that support per-field TTL on hash data structures. go-redis v9.6.0+ includes support for these commands with methods like HExpireAt.
Applied to files:
pkg/cache/redis.go
🧬 Code graph analysis (4)
internal/sms-gateway/handlers/3rdparty.go (4)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(15-19)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
NewBasic(20-57)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/users/service.go (4)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/users/errors.go (1)
ErrExists(7-7)pkg/crypto/passwords.go (2)
MakeBCryptHash(14-20)CompareBCryptHash(22-27)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)
internal/sms-gateway/handlers/mobile.go (4)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
GetUser(100-107)NewBasic(20-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
pkg/cache/memory.go (1)
30-30: LGTM! Good refactoring.The rename from
newItemtonewMemoryItemimproves clarity by making the function's purpose more explicit.Also applies to: 155-155
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
15-54: User storage in context is now consistent across Basic and Code authThe switch to
users.ServiceinNewBasicplus the newsetUser/GetUserpairing makes the user storage consistent:
- Both
NewBasicandNewCodenow store ausers.Uservalue viasetUser.GetUsertype‑asserts tousers.Userand returns a pointer, andHasUserjust checks for non‑nil.This removes the earlier pointer/value mismatch between Basic and Code flows and should make
GetUser/HasUserreliable for all callers.Also applies to: 80-107
internal/sms-gateway/handlers/3rdparty.go (1)
18-62: Constructor and dependency wiring for third‑party handler look consistentThe new
thirdPartyHandlershape andnewThirdPartyHandlerconstructor wire in:
usersSvc *users.Servicefor basic auth viauserauth.NewBasic.authHandler *thirdparty.AuthHandlerfor JWT/3rd‑party auth endpoints.base.Handlerwithloggerandvalidator.The explicit constructor keeps dependencies clear and avoids fx‑specific parameter structs here. No issues from a correctness or maintainability standpoint.
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)
internal/sms-gateway/handlers/devices/3rdparty.go (1)
46-89: Delete route path is missing required leading slash—this is a functional bugFiber's grouped routers require leading slashes in route paths, as routes are flattened to their full path.
Current code at line 88:
router.Delete(":id", permissions.RequireScope(ScopeDelete), userauth.WithUser(h.remove))This should be:
router.Delete("/:id", permissions.RequireScope(ScopeDelete), userauth.WithUser(h.remove))This aligns with the webhooks handler (line 113 of
internal/sms-gateway/handlers/webhooks/3rdparty.go) and the documented path/3rdparty/v1/devices/{id}.
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
16-133: User storage bug is fixed; consider makingWithUser’s failure mode clearerNice improvement: both
NewBasicandNewCodenow callSetUser(c, *user), which matchesGetUser’s.(users.User)assertion, so the previous pointer/value mismatch is resolved.HasUsernow correctly delegates toGetUser.One remaining sharp edge is
WithUser:func WithUser(handler func(users.User, *fiber.Ctx) error) fiber.Handler { return func(c *fiber.Ctx) error { return handler(*GetUser(c), c) } }If a route accidentally uses
WithUserwithout any prior auth middleware setting the user,GetUser(c)returns nil and this will panic with a generic nil‑pointer deref, even though the comment says “it will panic”.For a clearer failure mode, you could guard this:
func WithUser(handler func(users.User, *fiber.Ctx) error) fiber.Handler { return func(c *fiber.Ctx) error { - return handler(*GetUser(c), c) + user := GetUser(c) + if user == nil { + panic("WithUser called without authenticated user in context") + } + return handler(*user, c) } }This keeps the contract (it still panics) but makes misconfiguration easier to diagnose.
🧹 Nitpick comments (3)
internal/sms-gateway/handlers/messages/permissions.go (1)
3-7: LGTM! Consider adding documentation.The permission scope constants are well-named and follow the standard
resource:actionpattern, which aligns with JWT/OAuth2 best practices. The granularity (send, read, list) appropriately separates concerns.Consider adding godoc comments to clarify each scope's purpose:
+// ScopeSend grants permission to send new messages. const ( ScopeSend = "messages:send" + // ScopeRead grants permission to read individual messages. ScopeRead = "messages:read" + // ScopeList grants permission to list messages. ScopeList = "messages:list" )internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (1)
28-31: Consider adding error logging for debugging.When user lookup fails, the middleware returns a generic 401 without logging. While this is good for security (not leaking information), consider adding debug-level logging to help diagnose authentication issues during development.
Example enhancement:
user, err := usersSvc.GetByUsername(claims.UserID) if err != nil { + // Optional: Add debug logging here + // logger.Debug("JWT auth failed: user not found", zap.String("userID", claims.UserID)) return fiber.ErrUnauthorized }internal/sms-gateway/handlers/3rdparty.go (1)
35-67: Consider a parameter struct if constructor complexity grows.The constructor currently accepts 8 parameters, which is at the upper limit of what's manageable. While acceptable for explicit dependency injection, consider introducing a parameter struct if more dependencies are added in the future.
Example refactor:
type ThirdPartyHandlerParams struct { UsersSvc *users.Service JwtService jwt.Service HealthHandler *HealthHandler MessagesHandler *messages.ThirdPartyController // ... other handlers Logger *zap.Logger Validator *validator.Validate } func newThirdPartyHandler(params ThirdPartyHandlerParams) *thirdPartyHandler { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/requests.http(4 hunks)cmd/sms-gateway/main.go(1 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(6 hunks)internal/sms-gateway/handlers/devices/permissions.go(1 hunks)internal/sms-gateway/handlers/logs/3rdparty.go(3 hunks)internal/sms-gateway/handlers/logs/permissions.go(1 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(10 hunks)internal/sms-gateway/handlers/messages/permissions.go(1 hunks)internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go(1 hunks)internal/sms-gateway/handlers/middlewares/permissions/permissions.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(4 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(8 hunks)internal/sms-gateway/handlers/settings/permissions.go(1 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/handlers/thirdparty/permissions.go(1 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(8 hunks)internal/sms-gateway/handlers/webhooks/permissions.go(1 hunks)internal/sms-gateway/openapi/docs.go(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/messages/3rdparty.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/sms-gateway/handlers/thirdparty/auth.go (5)
internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/thirdparty/permissions.go (1)
ScopeTokensManage(4-4)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/handlers/settings/3rdparty.go (4)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/settings/permissions.go (2)
ScopeRead(4-4)ScopeWrite(5-5)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (4)
internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/modules/auth/service.go (1)
Service(24-36)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (2)
SetScopes(15-17)ScopeAll(10-10)internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
SetUser(88-90)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
SetScopes(15-17)
internal/sms-gateway/handlers/webhooks/3rdparty.go (4)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/webhooks/permissions.go (3)
ScopeList(4-4)ScopeWrite(5-5)ScopeDelete(6-6)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)
internal/sms-gateway/handlers/devices/3rdparty.go (4)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/devices/permissions.go (2)
ScopeList(4-4)ScopeDelete(5-5)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)
internal/sms-gateway/handlers/3rdparty.go (5)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/handlers/thirdparty/auth.go (1)
AuthHandler(16-20)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
NewBasic(21-59)UserRequired(114-122)internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (1)
NewJWT(13-38)
internal/sms-gateway/handlers/logs/3rdparty.go (4)
internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/logs/permissions.go (1)
ScopeRead(4-4)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (19)
internal/sms-gateway/handlers/logs/permissions.go (1)
3-5: LGTM!Clean permission scope definition following the established pattern.
internal/sms-gateway/handlers/devices/permissions.go (1)
3-6: LGTM!Permission scopes are clearly defined and follow the established naming convention.
api/requests.http (2)
4-4: LGTM!JWT token variable added to support Bearer authentication alongside Basic auth.
38-39: Verify Bearer token authentication works with JWT service.The Authorization header now uses Bearer token instead of Basic auth. Ensure the JWT middleware and service are properly wired to handle this authentication method.
internal/sms-gateway/handlers/webhooks/permissions.go (1)
3-7: LGTM!Permission scopes are well-defined. Note that the HTTP request examples use
"webhooks:read"which doesn't matchScopeList = "webhooks:list"- this was flagged in the api/requests.http review.cmd/sms-gateway/main.go (1)
13-16: LGTM!JWTAuth security definition properly added to Swagger documentation, following the same pattern as existing security definitions.
internal/sms-gateway/handlers/thirdparty/permissions.go (1)
3-5: LGTM!Permission scope for token management is clearly defined. Note that this scope isn't included in the example token request in api/requests.http - this may be intentional for security reasons (requiring separate privileged access for token management).
internal/sms-gateway/handlers/settings/permissions.go (1)
3-6: LGTM!Permission scopes correctly defined and align with the token request examples in api/requests.http.
internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (3)
13-19: LGTM - Proper Bearer token detection and fallback.The middleware correctly:
- Uses case-insensitive comparison for "Bearer " (per HTTP specs)
- Validates minimum token length (> 7 characters for "Bearer " + token)
- Falls through to next handler for non-Bearer auth, allowing other authentication methods
33-36: LGTM - Context properly populated.The middleware correctly:
- Sets the authenticated user in request context
- Applies JWT token scopes for permission checks
- Continues to the next handler
The scope propagation integrates well with the permission middleware system.
23-26: Review comment is incorrect—ParseToken already implements revocation checks.The code in
internal/sms-gateway/jwt/service.go(lines 105–111) explicitly validates tokens against the revocation list withinParseToken. The method callss.revoked.IsRevoked(ctx, claims.ID)and returnsErrTokenRevokedif the token is revoked. TheRevokedStoragedependency is properly initialized and required by the service constructor.Likely an incorrect or invalid review comment.
internal/sms-gateway/handlers/logs/3rdparty.go (1)
3-46: Logs 3rd‑party handler wiring looks consistentSwitch to
users.Userpluspermissions.RequireScope(ScopeRead)+userauth.WithUsermatches the new auth model; SwaggerJWTAuthannotation also aligns. No issues from this change.internal/sms-gateway/handlers/settings/3rdparty.go (1)
8-129: Settings 3rd‑party handlers align with new user/scope modelMigration to
users.Userpluspermissions.RequireScope(ScopeRead/ScopeWrite)anduserauth.WithUseris consistent and preserves existing business logic aroundsettingsSvc. No issues spotted.internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
1-36: Permission middleware is straightforward and consistent
SetScopes,HasScope, andRequireScopeare simple and do what the handlers expect (including honoringScopeAll). The locals key is kept internal, which is good for encapsulation.internal/sms-gateway/handlers/webhooks/3rdparty.go (1)
8-113: Webhooks 3rd‑party handlers: user/scopes wiring looks goodThe switch to
users.Userplus per‑route scopes (ScopeList,ScopeWrite,ScopeDelete) anduserauth.WithUseris coherent, and the delete route path"/:id"matches the documented/3rdparty/v1/webhooks/{id}. Implementation looks solid.internal/sms-gateway/handlers/3rdparty.go (4)
8-8: LGTM! Import additions align with new dependencies.The new imports for JWT authentication, users service, and third-party auth handler are all utilized in the updated code.
Also applies to: 11-11, 13-14
23-25: LGTM! Service and handler fields properly typed.The new fields for users service, JWT service, and auth handler are correctly typed and well-organized.
Also applies to: 32-32
74-78: LGTM! Authentication middleware chain is correctly ordered.The middleware sequence (basic auth → JWT auth → user required) properly allows both authentication methods while enforcing that at least one succeeds.
80-80: Auth handler registration is correct.The
/authendpoints are for token management only (POST/tokento create managed tokens, DELETE/token/:jtito revoke). Both are already protected byuserauth.WithUser()middleware in their handlers, and requiring group-level authentication is the correct design. No issues detected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/mobile.go (1)
56-56: Handle the error fromnanoid.Standard.This issue was already flagged in a previous review. The error from
nanoid.Standard(21)is still being ignored, which can cause a nil pointer panic whenh.idGen()is called at line 131.
🧹 Nitpick comments (4)
internal/sms-gateway/handlers/thirdparty/auth.go (1)
87-89: Token revocation endpoint is stubbed.The
DELETE /token/:jtiendpoint is registered but returnsErrNotImplemented. Given this is a draft PR, this is expected.When ready to implement this handler, it should:
- Extract the
jtiparameter from the route- Call
h.jwtSvc.RevokeToken(c.Context(), jti)- Return appropriate success/error responses
Would you like me to generate the implementation or open a tracking issue?
internal/sms-gateway/users/cache.go (2)
15-15: Consider making the TTL configurable.A hardcoded 1-hour TTL may be too long in security-sensitive environments. Consider allowing this to be configured via environment variables or application config, especially since this cache is keyed by authentication credentials.
47-58: Review the sliding expiration strategy.The
cache.AndSetTTL(loginCacheTTL)call extends the cache TTL on every access, implementing a sliding expiration window. While this improves user experience by keeping frequently-accessed credentials cached, it also means:
- Compromised credentials could remain cached indefinitely if accessed regularly
- Cache entries persist longer than the initial 1-hour TTL suggests
Consider whether absolute expiration (without TTL extension on reads) would be more appropriate for security-sensitive credential caching.
Additionally, consider adding observability for cache hit/miss rates to monitor effectiveness and detect anomalies.
internal/sms-gateway/handlers/mobile.go (1)
132-133: Clarify the slicing logic that skips index 6.The generated ID is 21 characters, but
usernameusesid[:6](indices 0-5) andpasswordusesid[7:](indices 7+), deliberately skipping the character at index 6. While this might be intentional (e.g., as a separator or to avoid ambiguous characters), the logic isn't documented and could confuse future maintainers.Consider adding a comment explaining why index 6 is skipped:
+ // Generate credentials from nanoid: first 6 chars for username, chars after index 6 for password username = strings.ToUpper(id[:6]) password = strings.ToLower(id[7:])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/requests.http(5 hunks)internal/sms-gateway/handlers/messages/permissions.go(1 hunks)internal/sms-gateway/handlers/mobile.go(8 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/users/cache.go(1 hunks)pkg/cache/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sms-gateway/handlers/messages/permissions.go
- pkg/cache/redis.go
- api/requests.http
🧰 Additional context used
🧬 Code graph analysis (3)
internal/sms-gateway/users/cache.go (4)
pkg/cache/typed.go (2)
Typed(14-16)NewTyped(18-22)internal/sms-gateway/users/domain.go (1)
User(5-10)pkg/cache/options.go (2)
AndSetTTL(64-68)WithTTL(22-30)pkg/cache/errors.go (2)
ErrKeyNotFound(7-7)ErrKeyExpired(9-9)
internal/sms-gateway/handlers/thirdparty/auth.go (5)
internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/thirdparty/permissions.go (1)
ScopeTokensManage(4-4)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/handlers/mobile.go (5)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/users/service.go (1)
Service(13-19)internal/sms-gateway/modules/auth/service.go (1)
Service(24-36)internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
GetUser(102-109)NewBasic(21-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/sms-gateway/handlers/thirdparty/auth.go (3)
35-38: Routes properly secured with scope-based access control.Both endpoints correctly require the
tokens:managescope and user authentication via middleware. The past review concern about missing@Securityannotations in the OpenAPI documentation has been addressed (see lines 54-55).
52-85: Security annotations properly added per previous review.The OpenAPI documentation now includes
@Security ApiAuthand@Security JWTAuthannotations (lines 54-55), correctly reflecting the authentication and scope requirements enforced by the middleware. The handler implementation is straightforward and correct.
40-43: ****The code already handles zero-value TTL appropriately. When
ttlis omitted from the request,GenerateToken(line 58–60 ofinternal/sms-gateway/jwt/service.go) explicitly defaults it tos.config.TTL, which is guaranteed to be positive by configuration validation. Tokens will always receive a valid expiration time.Likely an incorrect or invalid review comment.
internal/sms-gateway/users/cache.go (3)
17-30: LGTM!The wrapper struct correctly mirrors the User domain model and implements the required cache.Item interface using standard JSON serialization.
60-72: LGTM!The Set method correctly converts the domain User to the cache wrapper and stores it with the configured TTL. Error handling is appropriate.
74-81: Cache invalidation confirmed in password change flowThe Delete method implementation is correct and properly integrated. Cache invalidation occurs at
internal/sms-gateway/users/service.go:102within the ChangePassword method, where the old password credentials are removed from cache immediately after password validation and before updating the password hash. This prevents stale login cache entries from persisting after a password change.internal/sms-gateway/handlers/mobile.go (4)
197-207: LGTM! Clean migration tousers.User.The signature change from
models.Usertousers.Useris consistent with the broader migration. The function correctly usesuser.IDwhich exists in the new type.
230-232: LGTM! Correct delegation to users service.The change properly delegates password changes to
usersSvc.ChangePasswordwith the context and user ID from the authenticated device. The updated error message is appropriately generic.Note: Line 275 has a pre-existing comment indicating this endpoint should eventually use
userauthprotection instead ofdeviceauth, but that's outside the scope of this change.
242-243: LGTM! Consistent authentication middleware migration.The routes correctly use
userauth.NewBasic(h.usersSvc)for user authentication, which is consistent with the migration from auth service to users service for user-related authentication flows.Also applies to: 260-260
127-150: The review comment's concern is incorrect and should be disregarded.The verification confirms that
users.User.IDis always equal to the login username:
- In
internal/sms-gateway/users/service.go:51, theCreatemethod explicitly setsID: username, using the provided username parameter as the user's ID.- The
Userstruct contains only anIDfield with no separate Login or Username field.- For new users, the generated 6-character string is passed as
usernametoCreate(), which becomes theuser.ID.- For existing users,
user.IDIS the login username.Both code paths return the correct login username in the response.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/requests.http (1)
4-223: Re‑verify tokenscopesagainst permission constants (especiallydevices:write).The addition of
@jwtTokenand the JWT‑secured examples for messages and devices look good, and the new/3rdparty/v1/auth/tokenand/3rdparty/v1/auth/token/{id}endpoints make sense.However, the scopes in the token request body still deserve another pass:
"scopes": [ "messages:send", "messages:read", "devices:list", "devices:write", "webhooks:list", "webhooks:write", "settings:read", "settings:write", "logs:read" ]A previous review found that the defined device permissions were
"devices:list"and"devices:delete"(not"devices:write"), and that webhooks scopes needed to be"webhooks:list"/"webhooks:write"/"webhooks:delete". You’ve already aligned the webhooks scopes here, but it’s unclear whether"devices:write"now exists as a constant or still mismatches the code.To avoid hard‑to‑debug authorization failures, please re‑check that every scope string in this sample exactly matches the corresponding permission constants (devices, webhooks, messages, settings, logs) and update either the constants or this request accordingly.
You can re‑run a quick scan like this to confirm:
#!/bin/bash # List all defined permission scopes rg -n 'Scope[A-Za-z0-9_]*\s*=\s*".*:.*"' --type go # Show where scopes are enforced in handlers/middleware rg -n 'RequireScope\(' --type go -A2
🧹 Nitpick comments (10)
internal/sms-gateway/jwt/models.go (1)
11-37: Keep GORM model and SQL migration in sync (and consider FK definition here).The
tokenModelshape and tags line up with thetokensDDL in20251121071748_add_tokens.sql, but the SQL migration also defines thefk_tokens_userforeign key while the GORM model does not. SinceMigrateusesAutoMigrateand you also ship an explicit SQL migration, you now have two schema sources of truth.To avoid future drift (e.g., if columns or indexes change) and to keep FKs consistent across environments that only run
Migrate, consider either:
- documenting that goose SQL is authoritative and
Migrateis only for tests/dev, or- adding equivalent FK metadata to the GORM model (or dropping one of the mechanisms).
internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql (1)
3-18: Consider indexingexpires_atif you’ll query/cleanup by expiry.The tokens schema looks consistent with the Go model and has good indexes on
user_idandrevoked_at. If you plan to frequently select or delete tokens based onexpires_at(e.g., cleanup of expired tokens), consider adding an index onexpires_at(or a composite like(user_id, expires_at)) to avoid future table scans.internal/sms-gateway/jwt/module.go (1)
10-30: JWT module wiring looks correct; double‑check need for the private cache provider.The FX wiring for
Service(real vsnewDisabled()depending onConfig.Secret) and the migration registration viadb.RegisterMigration(Migrate)both look good.One minor thing: the private
cache.Cacheprovider created withfactory.New("jwt")isn’t consumed by any other provider in this module as shown. If nothing injwtcurrently depends oncache.Cache, you could drop this provider to reduce noise; if you plan to use it for revocation caching or similar, leaving it is fine but maybe add a brief comment for intent.internal/sms-gateway/jwt/repository.go (1)
28-46: Token revocation semantics and timestamp portability
Revokealways returns nil even when no row matches(id,user_id,revoked_at is null), and it usesgorm.Expr("NOW()")for the timestamp. If you ever need to distinguish “no such token / belongs to another user / already revoked” from “successfully revoked”, consider checking the update’sRowsAffectedand surfacing a domain error. Also, using a Gotime.Now()value (e.g.,Update("revoked_at", time.Now())) would avoid tying this to dialects that don’t supportNOW().internal/sms-gateway/handlers/thirdparty/auth.go (3)
36-40: Middleware wiring is sound; consider renamingerrorHanlderand clarifying responsibilitiesThe route setup with
router.Use(h.errorHanlder)pluspermissions.RequireScopeanduserauth.WithUsergives a clear protection story. Two small polish points:
- The method name
errorHanlderlooks like a typo; renaming toerrorHandlerwill make usages easier to grep.- This middleware currently only maps
ErrDisabled,ErrInitFailed, andErrInvalidConfig. If other JWT‑related errors need consistent HTTP mapping (e.g. always 401 vs 500), consider documenting whether they’re expected to be handled here or by a shared/global error handler.Also applies to: 111-128
42-52: Clarify TTL behavior in docs to match implementation
tokenRequest.TTLis optional (omitempty) andGenerateTokenfalls back toconfig.TTLwhen it’s zero while also capping by the configured maximum. The Swagger description currently just says “with specified scopes and ttl”, which reads as if TTL were always required.Consider adjusting the comment above
postTokento explicitly state that:
scopesare required, andttlis optional and, when omitted or zero, defaults to the configured maximum TTL.This will make the behavior more obvious to API consumers.
Also applies to: 54-87
89-109: Confirm desired behavior for revoking non‑existent or foreign tokens
deleteTokendelegates tojwtSvc.RevokeToken, which in turn calls the repository’sRevokewithout surfacing whether any row was actually updated. As a result, a request with a non‑existentjti(or one that belongs to another user) will still return 204.If you want idempotent “best‑effort revoke” semantics, this is fine; if instead clients should get feedback (e.g. 404 when no matching token exists), you’ll need the repository to report that distinction and translate it here.
internal/sms-gateway/openapi/docs.go (1)
1016-1050: TTL /time.Durationschema may not match actual JSON shape
internal_sms-gateway_handlers_thirdparty.tokenRequest.ttlreferencestime.Duration, which this spec models as anintegerwith nanosecond‑based enum values. In Go,time.Durationis typically marshaled as a string (e.g."1h","30s") in JSON, so clients relying on the schema may send integers while the server expects duration strings.If the intended wire format is the default Go duration string, consider:
- Using a string alias type for TTL and annotating it accordingly in the handler/model, or
- Overriding the schema for
time.Durationvia swag annotations/custom type configuration,and then regenerating this file rather than editing it directly.
Also applies to: 1822-1844
internal/sms-gateway/jwt/service.go (2)
45-86: TTL validation/capping is good; watch Go version for builtinminThe validation around
userID,scopes, and non‑negative TTL plus the “0 means default config TTL” behavior all look solid, and capping expiry withmin(ttl, s.config.TTL)enforces a max TTL.The only caveat is that the generic builtin
minrequires Go 1.21+. If yourgo.modstill targets an older Go version, this won’t compile. In that case you could replace it with an explicit clamp, e.g.:- now := time.Now() - claims := &Claims{ - RegisteredClaims: jwt.RegisteredClaims{ - ID: s.idFactory(), - Issuer: s.config.Issuer, - Subject: userID, - IssuedAt: jwt.NewNumericDate(now), - ExpiresAt: jwt.NewNumericDate(now.Add(min(ttl, s.config.TTL))), - }, - UserID: userID, - Scopes: scopes, - } + now := time.Now() + effectiveTTL := ttl + if effectiveTTL > s.config.TTL { + effectiveTTL = s.config.TTL + } + claims := &Claims{ + RegisteredClaims: jwt.RegisteredClaims{ + ID: s.idFactory(), + Issuer: s.config.Issuer, + Subject: userID, + IssuedAt: jwt.NewNumericDate(now), + ExpiresAt: jwt.NewNumericDate(now.Add(effectiveTTL)), + }, + UserID: userID, + Scopes: scopes, + }
88-118: Make all token‑validation failures surface asErrInvalidTokenfor callersRight now, parse/validation failures from
jwt.ParseWithClaimsare wrapped asfmt.Errorf("failed to parse token: %w", err), while only the!ok || !parsedToken.Validbranch returnsErrInvalidToken. That means callers can’t reliably useerrors.Is(err, ErrInvalidToken)to detect “invalid/expired token” in all cases and may end up treating many invalid tokens as generic server errors.If you want a consistent contract for downstream code (e.g., jwtauth middleware mapping all invalid tokens to 401), consider mapping all token‑validation failures to
ErrInvalidToken(possibly logging the wrapped error separately) instead of returning a wrapped error here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/requests.http(5 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/jwt/disabled.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/jwt.go(1 hunks)internal/sms-gateway/jwt/models.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/repository.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql(1 hunks)internal/sms-gateway/openapi/docs.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/jwt/errors.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/sms-gateway/jwt/models.go (1)
internal/sms-gateway/models/models.go (1)
TimedModel(7-10)
internal/sms-gateway/jwt/module.go (8)
internal/sms-gateway/app.go (1)
Module(35-60)internal/config/module.go (1)
Module(23-135)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/jwt/service.go (1)
New(22-43)internal/sms-gateway/jwt/repository.go (2)
NewRepository(14-18)Repository(10-12)internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)internal/sms-gateway/jwt/models.go (1)
Migrate(32-37)
internal/sms-gateway/handlers/thirdparty/auth.go (6)
internal/sms-gateway/jwt/jwt.go (1)
Service(10-14)internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
RequireScope(28-36)internal/sms-gateway/handlers/thirdparty/permissions.go (1)
ScopeTokensManage(4-4)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
WithUser(130-134)internal/sms-gateway/users/domain.go (1)
User(5-10)internal/sms-gateway/jwt/errors.go (3)
ErrDisabled(6-6)ErrInitFailed(7-7)ErrInvalidConfig(8-8)
internal/sms-gateway/jwt/disabled.go (2)
internal/sms-gateway/jwt/jwt.go (3)
Service(10-14)TokenInfo(23-27)Claims(16-21)internal/sms-gateway/jwt/errors.go (1)
ErrDisabled(6-6)
internal/sms-gateway/jwt/service.go (4)
internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/repository.go (1)
Repository(10-12)internal/sms-gateway/jwt/jwt.go (3)
Service(10-14)TokenInfo(23-27)Claims(16-21)internal/sms-gateway/jwt/errors.go (4)
ErrInitFailed(7-7)ErrInvalidConfig(8-8)ErrInvalidToken(9-9)ErrTokenRevoked(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/jwt/disabled.go (1)
8-27: Disabled service implementation looks solid.The disabled implementation cleanly satisfies
Serviceand consistently returnsErrDisabledfrom all methods, giving a predictable behavior when JWT is turned off. No state or concurrency concerns here.internal/sms-gateway/jwt/jwt.go (1)
10-27: JWT service interface and types look well‑shapedThe Service contract plus Claims/TokenInfo structs are coherent with the implementation in
service.goand the third‑party handlers; fields and JSON tags give consumers what they need without overexposing internals.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
api/requests.http (1)
201-219: Verify scope name "devices:write" matches defined permission constants.The past review flagged that
"devices:write"(line 212) does not match the permission constants defined indevices/permissions.go. According to the previous review, the available device permissions are"devices:list"and"devices:delete".Run the following script to confirm the available device permission scopes:
#!/bin/bash # Find all device permission scope constants rg -nP 'Scope.*=.*\"devices:' --type goUpdate line 212 to use a valid device permission constant.
🧹 Nitpick comments (4)
deployments/grafana/dashboards/jwt.json (1)
38-38: Consider using a variable for the datasource UID.The datasource UID
"edqp0a73uh2bka"is hardcoded throughout the dashboard. If the datasource changes in different environments, this will require manual updates.Consider using
${DS_PROMETHEUS}or a similar variable instead of hardcoding the UID, or ensure the UID is documented in deployment instructions.Also applies to: 85-85, 149-149
internal/sms-gateway/jwt/service.go (2)
78-88: Consider removing the redundant UserID field from Claims.The
Claimsstruct storesUserIDseparately (line 86) even thoughRegisteredClaims.Subjectalready holds the same value (line 82). This duplication may cause maintenance issues if they ever diverge.If the custom
UserIDfield serves no distinct purpose, consider removing it from theClaimsstruct injwt.goand using onlySubjectthroughout the codebase.
63-66: Consider validating that scopes are from an allowed set.The function requires
len(scopes) > 0but does not validate that the provided scopes are valid permission constants. Invalid scopes will be accepted and stored, potentially causing authorization failures later.Consider adding a scope validation step that checks each scope against the defined permission constants (e.g.,
messages:send,devices:list, etc.) to catch configuration errors early.internal/sms-gateway/jwt/metrics.go (1)
99-103: Consider adding panic recovery in duration observation helpers.If the callback
f()panics,timer.ObserveDuration()is never called, which could skew latency metrics. This applies to all three Observe* methods.Apply this pattern to all three observation methods:
func (m *Metrics) ObserveIssuance(f func()) { timer := prometheus.NewTimer(m.issuanceDurationHistogram) + defer timer.ObserveDuration() f() - timer.ObserveDuration() }Using
deferensures the duration is recorded even iff()panics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/requests.http(5 hunks)deployments/grafana/dashboards/jwt.json(1 hunks)deployments/prometheus/alerts/jwt-alerts.yml(1 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/metrics.go(1 hunks)internal/sms-gateway/jwt/models.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sms-gateway/jwt/module.go
- internal/sms-gateway/handlers/thirdparty/auth.go
- internal/sms-gateway/jwt/errors.go
- internal/sms-gateway/jwt/models.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/jwt/service.go (5)
internal/sms-gateway/jwt/config.go (1)
Config(12-16)internal/sms-gateway/jwt/repository.go (1)
Repository(10-12)internal/sms-gateway/jwt/metrics.go (3)
Metrics(24-31)StatusError(20-20)StatusSuccess(19-19)internal/sms-gateway/jwt/jwt.go (3)
Service(10-14)TokenInfo(23-27)Claims(16-21)internal/sms-gateway/jwt/errors.go (4)
ErrInitFailed(7-7)ErrInvalidParams(9-9)ErrInvalidToken(10-10)ErrTokenRevoked(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/jwt/service.go (1)
58-61: Good fix—userID validation now prevents empty Subject claims.The validation added here addresses the past review concern about empty "sub" claims. This ensures JWTs are spec-compliant.
internal/sms-gateway/jwt/metrics.go (1)
23-31: LGTM—Metrics structure is well-organized.The metrics cover all three core JWT operations (issuance, validation, revocation) with both counters and duration histograms. The use of a single
statuslabel keeps cardinality low.Note: The absence of a
reasonlabel for granular error classification is consistent with the simplified error tracking approach, though it limits observability of specific failure modes (expired vs. revoked vs. malformed tokens).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
deployments/prometheus/alerts/jwt-alerts.yml (2)
24-32: Critical: Label mismatch persists—metric does not expose a "reason" label.This issue was previously identified but remains unresolved. The alert queries
reason="database_error", buttokensRevokedCounterininternal/sms-gateway/jwt/metrics.go(line 50-55) only exposes astatuslabel. The alert will never fire.
34-42: Critical: Label mismatch persists, and inconsistent division protection.Two issues:
Label mismatch (same as previously identified): The alert queries
reason=~"expired|revoked", buttokensValidatedCounterininternal/sms-gateway/jwt/metrics.go(lines 43-48) only exposes astatuslabel. This alert will never fire.Inconsistent division protection: This alert uses
max(..., 0.00001)while line 5 usesclamp_min(..., 1e-9). Both work, but the inconsistency makes the codebase harder to maintain.Consider standardizing on
clamp_minfor consistency:- expr: rate(sms_auth_jwt_tokens_validated_total{status="error",reason=~"expired|revoked"}[5m]) / max(rate(sms_auth_jwt_tokens_validated_total[5m]), 0.00001) > 0.1 + expr: rate(sms_auth_jwt_tokens_validated_total{status="error"}[5m]) / clamp_min(rate(sms_auth_jwt_tokens_validated_total[5m]), 1e-9) > 0.1Note: This also removes the non-existent
reasonlabel filter. If you need per-reason alerting, add thereasonlabel to the metric inmetrics.gofirst.
🧹 Nitpick comments (1)
internal/sms-gateway/jwt/metrics.go (1)
57-79: Consider extracting shared histogram buckets constant.All three histograms use identical bucket definitions. For maintainability, consider extracting to a shared constant:
+var jwtDurationBuckets = []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1} + func NewMetrics() *Metrics { return &Metrics{ ... issuanceDurationHistogram: promauto.NewHistogram(prometheus.HistogramOpts{ Namespace: "sms", Subsystem: "auth", Name: MetricIssuanceDurationSeconds, Help: "JWT issuance duration in seconds", - Buckets: []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1}, + Buckets: jwtDurationBuckets, }),This makes it easier to adjust bucket ranges consistently across all JWT operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/grafana/dashboards/jwt.json(1 hunks)deployments/prometheus/alerts/jwt-alerts.yml(1 hunks)internal/sms-gateway/jwt/metrics.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
deployments/prometheus/alerts/jwt-alerts.yml (2)
4-12: LGTM! Division-by-zero protection correctly implemented.The alert correctly uses
clamp_minto prevent division by zero and queries thestatuslabel that exists in the metrics definition.
14-22: LGTM! Latency alert correctly configured.The histogram quantile query correctly references the issuance duration metric, and the 0.5s threshold is appropriate given the histogram buckets defined in
metrics.go.deployments/grafana/dashboards/jwt.json (12)
82-92: LGTM! Panel correctly queries the tokens issued metric.The query correctly references
sms_auth_jwt_tokens_issued_total, which matches the metric defined ininternal/sms-gateway/jwt/metrics.go.
146-156: LGTM! Validation success rate correctly calculated.The panel correctly uses
sms_auth_jwt_tokens_validated_totalwith proper division-by-zero protection viaclamp_min.
208-218: LGTM! Revocation rate panel correctly configured.The query correctly references
sms_auth_jwt_tokens_revoked_total.
308-318: LGTM! Issuance rate timeseries correctly configured.The query correctly aggregates by
status, which will display separate series for success and error rates.
383-396: LGTM! Panel correctly uses the existing metric with status filter.The query correctly filters
sms_auth_jwt_tokens_issued_totalbystatus="error", which is the proper approach given the metric definition inmetrics.go. The past concern about a non-existent metric has been resolved.
483-513: LGTM! Latency distribution panel correctly configured.The histogram quantile queries correctly reference
sms_auth_jwt_issuance_duration_seconds_bucketfor p99, p95, and p50 percentiles.
603-613: LGTM! Validation success rate timeseries correctly configured.The query correctly uses
sms_auth_jwt_tokens_validated_totalwith proper division protection.
667-681: LGTM! Panel correctly uses the existing metric aggregated by status.The query correctly aggregates
sms_auth_jwt_tokens_validated_totalbystatus, which will show the breakdown in a pie chart. The past concern about a non-existent metric has been resolved.
770-800: LGTM! Validation latency panel correctly configured.The histogram quantile queries correctly reference
sms_auth_jwt_validation_duration_seconds_bucket.
890-900: LGTM! Revocation rate timeseries correctly configured.The query correctly aggregates by
statusto show success and error rates separately.
990-1000: LGTM! Panel correctly uses the existing metric with status filter.The query correctly filters
sms_auth_jwt_tokens_revoked_totalbystatus="error"and includes proper division protection withclamp_min. The past concern about a non-existent metric has been resolved.
1090-1120: LGTM! Revocation latency panel correctly configured.The histogram quantile queries correctly reference
sms_auth_jwt_revocation_duration_seconds_bucket.internal/sms-gateway/jwt/metrics.go (3)
23-31: LGTM! Metrics struct is well-designed.The struct appropriately uses
CounterVecfor labeled counters andHistogramfor duration tracking. Field names are clear and follow Go conventions.
83-96: LGTM! Increment methods correctly implemented.The increment methods properly delegate to the underlying counter's
WithLabelValuesandIncmethods.
98-117: LGTM! Observation methods correctly use timer pattern.The observe methods correctly use
prometheus.NewTimerwith deferredObserveDurationto measure operation duration, which ensures timing is captured even if the operation panics.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployments/prometheus/alerts/jwt-alerts.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
deployments/prometheus/alerts/jwt-alerts.yml (1)
5-5: PromQL expressions look correct with proper safeguards.The past division-by-zero and label-mismatch issues have been addressed: expressions now use
clamp_min()/max()to guard against zero denominators, and label selectors use the correctstatuslabel that exists in the metrics.Also applies to: 15-15, 25-25, 35-35
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deployments/prometheus/alerts/jwt-alerts.yml (1)
5-5: Harmonize zero-protection thresholds for consistency.Alert 1 uses
clamp_min(..., 1e-9)while Alert 4 usesmax(..., 0.00001). Both prevent division-by-zero, but the minimum thresholds differ by several orders of magnitude. For consistency across alerts with similar semantics, consider using the same minimum value.Apply this diff to align both to the same minimum:
- expr: rate(sms_auth_jwt_tokens_validated_total{status="error"}[5m]) / clamp_min(rate(sms_auth_jwt_tokens_validated_total[5m]), 1e-9) > 0.05 + expr: rate(sms_auth_jwt_tokens_validated_total{status="error"}[5m]) / clamp_min(rate(sms_auth_jwt_tokens_validated_total[5m]), 0.00001) > 0.05Or update Alert 4 to match Alert 1:
- expr: rate(sms_auth_jwt_tokens_validated_total{status="error"}[5m]) / max(rate(sms_auth_jwt_tokens_validated_total[5m]), 0.00001) > 0.1 + expr: rate(sms_auth_jwt_tokens_validated_total{status="error"}[5m]) / clamp_min(rate(sms_auth_jwt_tokens_validated_total[5m]), 0.00001) > 0.1Also applies to: 35-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployments/prometheus/alerts/jwt-alerts.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
deployments/prometheus/alerts/jwt-alerts.yml (2)
1-42: Excellent work addressing previous critical issues.All four alerts now correctly use the
status="error"label (fixed from non-existentreasonlabel references in earlier commits) and both division operations are protected against zero denominators viaclamp_min()andmax().
24-32: Verify Alert 3 sensitivity for production operations.The
JWT_Revocation_Failuresalert fires onrate(...) > 0—any revocation error within a 5m window with critical severity. Over 5 minutes, even a single error will trigger. Depending on your operational baseline, this may cause alert fatigue or may be intentional given the security criticality of revocation.Confirm this threshold aligns with expected error rates and operational oncall procedures.
capcom6
force-pushed
the
jwt/introduce-jwt
branch
from
c533392 to
a66a972
Compare
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 (3)
pkg/cache/memory.go (1)
177-241: Fix data race onmemoryItem.validUntilwhen updating TTL inGetIn the
Getpath with options,item.validUntilis mutated underm.mux.Lock, butgetItemlater callsitem.isExpired(time.Time)without holding any lock. Other goroutines can concurrently callGetwith TTL‑updating options on the same key, leading to unsynchronized reads/writes ofvalidUntiland a data race under the Go memory model (and likely flagged by the race detector).A minimal fix is to avoid mutating
memoryItemin place and instead create a newmemoryItemwith the updated TTL and swap it into the map while holding the write lock, so the structs themselves become immutable after creation. That way, readers may see either the old or new item pointer, but no field is ever written concurrently with being read.One way to adjust
Getis:func (m *memoryCache) Get(_ context.Context, key string, opts ...GetOption) ([]byte, error) { return m.getValue(func() (*memoryItem, bool) { if len(opts) == 0 { m.mux.RLock() item, ok := m.items[key] m.mux.RUnlock() return item, ok } o := getOptions{} o.apply(opts...) - m.mux.Lock() - item, ok := m.items[key] - if !ok { - // item not found, nothing to do - } else if o.delete { - delete(m.items, key) - } else if !item.isExpired(time.Now()) { - if o.validUntil != nil { - item.validUntil = *o.validUntil - } else if o.setTTL != nil { - item.validUntil = time.Now().Add(*o.setTTL) - } else if o.updateTTL != nil { - item.validUntil = item.validUntil.Add(*o.updateTTL) - } else if o.defaultTTL { - item.validUntil = time.Now().Add(m.ttl) - } - } - m.mux.Unlock() - - return item, ok + m.mux.Lock() + defer m.mux.Unlock() + + item, ok := m.items[key] + if !ok { + return nil, false + } + + if o.delete { + delete(m.items, key) + return item, true + } + + now := time.Now() + if item.isExpired(now) { + // Let getItem report the expired state; cleanup() will remove it later. + return item, true + } + + // Avoid mutating shared state in-place; create a new item with updated TTL. + updated := &memoryItem{ + value: item.value, + validUntil: item.validUntil, + } + + if o.validUntil != nil { + updated.validUntil = *o.validUntil + } else if o.setTTL != nil { + updated.validUntil = now.Add(*o.setTTL) + } else if o.updateTTL != nil { + updated.validUntil = updated.validUntil.Add(*o.updateTTL) + } else if o.defaultTTL { + updated.validUntil = now.Add(m.ttl) + } + + m.items[key] = updated + return updated, true }) }This preserves the existing API and semantics while removing the shared‑state mutation that causes the race.
Also applies to: 366-400
pkg/cache/options.go (1)
25-48:WithTTLimplementation contradicts its own documentationDocs say “A TTL of zero or negative means the item will not expire”, but the implementation always sets
validUntil = time.Now().Add(ttl). Forttl <= 0this yields an immediate or past expiration instead of “no expiry”.You probably want to return early when
ttl <= 0:func WithTTL(ttl time.Duration) Option { return func(o *options) { - if ttl <= 0 { - o.validUntil = time.Time{} - } - - o.validUntil = time.Now().Add(ttl) + if ttl <= 0 { + // Zero value means "no expiration" + o.validUntil = time.Time{} + return + } + + o.validUntil = time.Now().Add(ttl) } }internal/sms-gateway/handlers/mobile.go (1)
197-233: Change‑password handler conflates auth failures with server errorsSwitching
getUserCodeto useusers.Useris aligned with the new users module anduserauth.WithUser, that looks fine.In
changePassword, every error fromusersSvc.ChangePasswordis logged and then returned as401 Unauthorized:if err := h.usersSvc.ChangePassword(...); err != nil { h.Logger.Error("failed to change password", zap.Error(err)) return fiber.NewError(fiber.StatusUnauthorized, "failed to change password") }This lumps together invalid current password, missing user, cache issues, and database failures as “unauthorized”. It would be more accurate (and easier to debug) to distinguish credential errors from internal ones (e.g., map a known “login failed”/“bad credentials” sentinel to 401, and everything else to 500 while keeping the generic message).
♻️ Duplicate comments (5)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
130-133: MakeWithUserpanic message explicit instead of a nil‑pointer deref
WithUserstill doesreturn handler(*GetUser(c), c), so if it’s ever used without a preceding auth middleware that callsSetUser, it will panic with a generic nil‑pointer error. Since the comment already promises a panic, you can make the failure mode clearer:func WithUser(handler func(users.User, *fiber.Ctx) error) fiber.Handler { return func(c *fiber.Ctx) error { - return handler(*GetUser(c), c) + user := GetUser(c) + if user == nil { + panic("WithUser called without authenticated user in context") + } + return handler(*user, c) } }This keeps the contract (panic on missing user) but makes debugging mis‑wired routes much easier.
internal/sms-gateway/users/cache.go (1)
42-45: Reconsider credential-based cache keys for login cache
makeKeyhashesusernameandpassworddirectly into the cache key. This keeps deterministic, unsalted credential material in the cache layer and makes the cache usable as an authentication oracle. It also complicates safe invalidation on password changes (especially admin resets where the old password is unknown), potentially allowing old passwords to remain valid until TTL expiry.Consider instead:
- Keying by a stable, non-secret identifier (e.g., user ID) and caching whatever you actually want to reuse (e.g., user record, precomputed scopes), or
- If you must depend on credentials, deriving keys with an HMAC that includes a server-side secret and a per-user salt, and wiring explicit invalidation on password changes.
At minimum, document how password-change flows guarantee invalidation of old-credential entries.
internal/sms-gateway/handlers/mobile.go (1)
31-74: Still ignoring nanoid initialization error; can panic if idGen is nil
nanoid.Standard(21)’s error is ignored andidGenis used later (e.g., inpostDevice). If initialization fails and returns a nil function, callingh.idGen()will panic.Handle the error and fail fast (or propagate it) instead of constructing a handler with a potentially nil generator, e.g.:
- idGen, _ := nanoid.Standard(21) + idGen, err := nanoid.Standard(21) + if err != nil { + panic(fmt.Sprintf("failed to initialize ID generator: %v", err)) + }(or return the error from the constructor, depending on your DI pattern).
internal/sms-gateway/jwt/config.go (1)
12-16: Consider validatingIssuer(or documenting that it is optional)
Config.Validatedoesn’t currently enforce thatIssueris non‑empty, even though it’s written into tokens and is later required inParseTokenviajwt.WithIssuer. If your design expects a meaningful issuer, consider adding a similar non‑empty check here; otherwise, documenting that an empty issuer is explicitly supported would avoid ambiguity.Also applies to: 18-31
internal/sms-gateway/users/repository.go (1)
54-62: Still need to handleRowsAffected == 0inUpdatePassword
UpdatePasswordwraps onlyresult.Errorand treats “no rows updated” as success, so a non‑existent user ID silently passes, unlikeGetByIDwhich returnsErrNotFound. A past review already suggested checkingRowsAffected; that feedback still applies here.You can adopt the earlier diff to make the behavior consistent:
func (r *repository) UpdatePassword(ID string, passwordHash string) error { - if err := r.db.Model((*userModel)(nil)). - Where("id = ?", ID). - Update("password_hash", passwordHash).Error; err != nil { - return fmt.Errorf("can't update password: %w", err) - } + result := r.db.Model((*userModel)(nil)). + Where("id = ?", ID). + Update("password_hash", passwordHash) + + if err := result.Error; err != nil { + return fmt.Errorf("can't update password: %w", err) + } + if result.RowsAffected == 0 { + return ErrNotFound + } return nil }Please confirm this matches your expectations for GORM’s
Updatesemantics in your current version (specifically thatRowsAffected == 0indicates “no matching rows” and not a soft‑error case):GORM Update method: meaning of RowsAffected and handling non-existent rows
🧹 Nitpick comments (17)
pkg/cache/redis.go (1)
423-446: Consider atomic SetOrFail with TTL.The current implementation has a race condition: HSetNX (line 424) and HExpireAt (line 440) are separate operations. If HSetNX succeeds but HExpireAt fails, the key will be set without TTL, potentially never expiring.
While the error is returned to the caller, the key remains in Redis without expiration, which could lead to memory issues over time.
Consider implementing this atomically using a Lua script:
local key = KEYS[1] local field = ARGV[1] local value = ARGV[2] local ttl = tonumber(ARGV[3]) or 0 local exists = redis.call('HEXISTS', key, field) if exists == 1 then return {err = 'key exists'} end redis.call('HSET', key, field, value) if ttl > 0 then redis.call('HEXPIREAT', key, ttl, 'FIELDS', '1', field) end return {ok = 'OK'}Then update the Go code:
func (r *redisCache) SetOrFail(ctx context.Context, key string, value []byte, opts ...Option) error { + options := new(options) + if r.ttl > 0 { + options.validUntil = time.Now().Add(r.ttl) + } + options.apply(opts...) + + var ttl int64 + if !options.validUntil.IsZero() { + ttl = options.validUntil.Unix() + } + + result, err := r.client.Eval(ctx, setOrFailScript, []string{r.key}, key, value, ttl).Result() - val, err := r.client.HSetNX(ctx, r.key, key, value).Result() if err != nil { + if strings.Contains(err.Error(), "key exists") { + return ErrKeyExists + } return fmt.Errorf("can't set cache item: %w", err) } - - if !val { - return ErrKeyExists - } - - options := new(options) - if r.ttl > 0 { - options.validUntil = time.Now().Add(r.ttl) - } - options.apply(opts...) - - if !options.validUntil.IsZero() { - if err := r.client.HExpireAt(ctx, r.key, options.validUntil, key).Err(); err != nil { - return fmt.Errorf("can't set cache item ttl: %w", err) - } - } - return nil }internal/config/types.go (1)
29-48: Clarify semantics for empty / missing duration strings (optional)
UnmarshalTextcurrently feeds the raw string intotime.ParseDuration, which will fail on an empty string. If you expect configs likettl: ""orttl: null(or want “missing or empty” to mean “0 / disabled”), you may want to treat empty input specially, e.g. map it to0instead of returning an error. If you want it to be strictly required, this behavior is fine but should be documented in the surrounding config docs.internal/sms-gateway/jwt/disabled.go (1)
1-28: Disabled JWT service behavior is clear and consistentReturning
ErrDisabledfrom allServicemethods is a clean way to represent a globally disabled JWT subsystem, andnewDisabledhiding the concrete type behind theServiceinterface keeps the wiring simple. If you want an extra guard, you could add:var _ Service = (*disabled)(nil)but the current code is functionally fine as‑is.
internal/sms-gateway/users/domain.go (1)
5-19: User domain model and mapper are straightforwardThe
Userstruct is minimal and thenewUserhelper cleanly maps fromuserModelto the domain type. One thing to decide explicitly: ifmodelshould never benil, it may be worth documenting that contract; if it’s possible for repository code to passnil, adding a quick guard (or returningnil/error higher up) would prevent a panic.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
66-83: Consider assigning scopes for Code‑based auth as well
NewCodenow correctly usesAuthorizeUserByCodeandSetUser, but it doesn’t set any scopes. Any routes guarded only bypermissions.RequireScope(...)(like the updated 3rd‑party devices handlers) will deny requests authenticated via the"Code …"header unless some other middleware also sets scopes.If Code‑based auth is meant to have the same effective permissions as Basic/JWT on those endpoints, consider mirroring the Basic behavior:
func NewCode(authSvc *auth.Service) fiber.Handler { @@ - user, err := authSvc.AuthorizeUserByCode(code) + user, err := authSvc.AuthorizeUserByCode(code) if err != nil { return fiber.ErrUnauthorized } - SetUser(c, *user) + SetUser(c, *user) + // Give code-based auth full access, like Basic. + permissions.SetScopes(c, []string{permissions.ScopeAll})If instead Code auth is intentionally restricted from scope‑protected endpoints, it would help to document that explicitly.
internal/sms-gateway/handlers/logs/3rdparty.go (1)
25-46: JWT security + scope middleware look good; consider documenting scope and unused paramThe added
@Security JWTAuthandpermissions.RequireScope(ScopeRead)plususerauth.WithUserare consistent and tighten access control without changing the 501 behavior. Theuser users.Userargument ingetis currently unused; if that’s intentional for future use, consider naming it_ users.Userto keep linters quiet. Also, if your OpenAPI tooling supports it, documenting that this route requires theScopeReadscope (not just the JWT scheme) would make the spec match the runtime behavior more closely.internal/sms-gateway/handlers/middlewares/permissions/permissions.go (1)
9-36: Scope middleware is clear and correct; consider adding observability later
SetScopes,HasScope, andRequireScopecorrectly wire scopes via Fiber locals, andScopeAllbehaves as expected viaslices.ContainsFunc. For typical small scope lists this is perfectly fine. Longer‑term, you might want to add logging/metrics on forbidden responses inRequireScope(or at a higher layer) to aid debugging permission issues.internal/sms-gateway/handlers/thirdparty/auth.go (1)
89-131: Align errorHandler status codes with documented responses
errorHandlercan return501 Not Implementedforjwt.ErrDisabled, and potentially400 Bad Requestforjwt.ErrInvalidParams. The swagger for both/3rdparty/v1/auth/tokenand/3rdparty/v1/auth/token/{jti}doesn’t currently list 501 (and DELETE doesn’t list 400), so generated OpenAPI will under‑document possible outcomes.If you rely on these statuses, consider adding the corresponding
@Failure 400/@Failure 501entries to the comments so API consumers see the complete set of responses.internal/sms-gateway/handlers/mobile.go (1)
241-277: Basic auth now correctly wired through usersSvc; consider revisiting password route TODOUsing
userauth.NewBasic(h.usersSvc)for/mobile/v1/deviceand/mobile/v1/user/codealigns these endpoints with the new users service and theusers.Userdomain type, which looks good.There’s still a TODO noting that
/mobile/v1/user/password“Should be underuserauth.NewBasicprotection instead ofdeviceauth”. Now that usersSvc is available here, it may be a good time in a follow‑up to revisit that route’s protection model if you intend password changes to require user credentials rather than device identity.internal/sms-gateway/users/service.go (1)
71-112: Login/password caching and Delete semantics deserve a closer lookThe
Loginflow (cache → DB → cache.Set) is sensible for performance, but two aspects are worth tightening:
- You’re passing the raw
passwordinto theloginCacheAPI (Get,Set,Delete). That’s fine ifloginCacheonly uses it to derive an in‑memory key and never logs or persists the raw value, but it’s unusual enough that it’s worth double‑checking the implementation to ensure passwords aren’t stored or emitted anywhere.- In
ChangePassword, a failure fromcache.Delete(ctx, username, currentPassword)aborts the password change, even though the cache is only an optimization. It may be more robust to treatcache.ErrKeyNotFound(and possibly other transient cache errors) as non‑fatal: log a warning but still proceed to update the stored password, mirroring the tolerant behavior inLogin.Adjusting these would improve both security posture and resilience around the cache layer.
internal/sms-gateway/openapi/docs.go (2)
129-137: Clarify intended auth semantics for combinedApiAuthandJWTAuthIn Swagger 2.0, a
securityarray like:"security": [ { "ApiAuth": [] }, { "JWTAuth": [] } ]means either ApiAuth or JWTAuth is sufficient (logical OR of the objects), not that both are required. Please verify that the runtime middleware for these endpoints also allows either mechanism; if the intent is to require both, the OpenAPI should instead use a single object with both schemes.
Also applies to: 180-188, 264-273, 330-339, 421-429, 509-515, 569-577, 625-633, 665-672, 724-731, 785-792, 827-833, 887-895
1016-1050: TTL /time.Durationrepresentation may be confusing to API consumers
internal_sms-gateway_handlers_thirdparty.tokenRequest.ttlis modeled astime.Durationwith an underlying integer and enum values like nanosecond, second, minute, hour. For external clients this can be non‑obvious (nanoseconds vs seconds, human durations, etc.). Consider adjusting the handler type or swagger annotations so the docs clearly state the accepted format and units forttl(e.g., seconds as integer, or Go‑style duration strings), then regenerate this file.Also applies to: 1822-1844
internal/sms-gateway/jwt/service.go (1)
77-85: Ensure Go toolchain version supportsminontime.DurationThis code relies on
min(ttl, s.config.TTL)overtime.Duration. If you are using the Go 1.21+ builtinmin, confirm thatgo.modand CI are set to a compatible Go version; otherwise, provide a local helperfunc minDuration(a, b time.Duration) time.Durationinstead.internal/sms-gateway/modules/auth/service.go (2)
62-88:GenerateUserCodeloop depends on Go’s integerrangeand could use a clearer retry patternThe
for range 3construct is valid only on newer Go versions that support ranging over integers. Please ensure yourgo.modand CI toolchain target a compatible Go release; otherwise, rewrite as an explicitfor i := 0; i < 3; i++ { ... }loop. The retry logic itself (up to 3 attempts onrand.Read/SetOrFail) looks fine.
90-97: User lookup viaAuthorizeUserByCoderelies on usernames; consider clarifying naming
AuthorizeUserByCodepulls a string fromcodesCacheand passes it tousersSvc.GetByUsername, whileGenerateUserCode’s parameter is nameduserID. If the stored value is actually the username (which seems likely), renaming the parameter and local variable tousernamewould better reflect the behavior and avoid confusion for future maintainers.Also applies to: 132-145
pkg/cache/typed.go (1)
89-122: Clarify and surface the pointer-type requirement forTThe
newItem[T]helper correctly enforces thatTis a pointer type and fixes the earlier nil‑pointerUnmarshalissue, but that requirement is only enforced at runtime and only on read paths (Get,GetAndDelete,Drain). A misconfiguredNewTyped[SomeStruct]will happily compile and evenSet, but will start failing with"cache: type ... must be a pointer"once something callsGet.To make this easier to discover for callers, consider either:
- Explicitly documenting in the exported comments for
Typed/NewTypedthatTmust be a pointer type implementingItem(e.g.NewTyped[*User], notNewTyped[User]), or- Adding a cheap validation in
NewTyped(e.g. callingnewItem[T]()once and panicking/logging on error) so misconfiguration is caught at construction time rather than on first cache read.This doesn’t require changing the current behavior, but would make the API safer to use for new call sites.
Also applies to: 361-387
internal/sms-gateway/handlers/3rdparty.go (1)
23-33: Verify auth middleware interplay and/authroute protectionThe new wiring makes all
/3rdparty/v1routes (including/auth) go through:router.Use( userauth.NewBasic(h.usersSvc), jwtauth.NewJWT(h.jwtSvc, h.usersSvc), userauth.UserRequired(), )This is a reasonable composition (Basic OR JWT must set a user, then
UserRequiredenforces it), but it relies on bothuserauth.NewBasicandjwtauth.NewJWTbehaving as no‑ops when their respective credentials are absent (e.g. noAuthorizationheader for JWT, no Basic header for non‑login calls). If either middleware actively returns 401 on “no credentials”, it could unintentionally block the other mechanism.In particular:
/3rdparty/v1/auth/...endpoints are now behind this chain as well. If the intention is “obtain tokens via Basic, use them via JWT”, this is fine as long as missing JWT on those endpoints doesn’t cause failures while Basic succeeds.- For purely JWT‑protected endpoints, missing Basic headers must not cause errors from
NewBasic.It’d be good to confirm these contracts in the respective middlewares (and ideally with tests) so that adding JWT auth here doesn’t regress existing Basic‑auth flows.
If you’d like, I can sketch table‑driven tests that assert the desired behavior for combinations of
{Basic, JWT}presence on representative endpoints.Also applies to: 35-67, 69-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (62)
api/mobile.http(1 hunks)api/requests.http(5 hunks)cmd/sms-gateway/main.go(1 hunks)configs/config.example.yml(1 hunks)deployments/grafana/dashboards/jwt.json(1 hunks)deployments/prometheus/alerts/jwt-alerts.yml(1 hunks)go.mod(1 hunks)internal/config/config.go(4 hunks)internal/config/module.go(2 hunks)internal/config/types.go(1 hunks)internal/sms-gateway/app.go(4 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(6 hunks)internal/sms-gateway/handlers/devices/permissions.go(1 hunks)internal/sms-gateway/handlers/logs/3rdparty.go(3 hunks)internal/sms-gateway/handlers/logs/permissions.go(1 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(10 hunks)internal/sms-gateway/handlers/messages/permissions.go(1 hunks)internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go(1 hunks)internal/sms-gateway/handlers/middlewares/permissions/permissions.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(4 hunks)internal/sms-gateway/handlers/mobile.go(8 hunks)internal/sms-gateway/handlers/module.go(2 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(8 hunks)internal/sms-gateway/handlers/settings/permissions.go(1 hunks)internal/sms-gateway/handlers/thirdparty/auth.go(1 hunks)internal/sms-gateway/handlers/thirdparty/module.go(1 hunks)internal/sms-gateway/handlers/thirdparty/permissions.go(1 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(8 hunks)internal/sms-gateway/handlers/webhooks/permissions.go(1 hunks)internal/sms-gateway/jwt/config.go(1 hunks)internal/sms-gateway/jwt/disabled.go(1 hunks)internal/sms-gateway/jwt/errors.go(1 hunks)internal/sms-gateway/jwt/jwt.go(1 hunks)internal/sms-gateway/jwt/metrics.go(1 hunks)internal/sms-gateway/jwt/models.go(1 hunks)internal/sms-gateway/jwt/module.go(1 hunks)internal/sms-gateway/jwt/repository.go(1 hunks)internal/sms-gateway/jwt/service.go(1 hunks)internal/sms-gateway/models/migration.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql(1 hunks)internal/sms-gateway/models/models.go(0 hunks)internal/sms-gateway/modules/auth/module.go(0 hunks)internal/sms-gateway/modules/auth/repository.go(0 hunks)internal/sms-gateway/modules/auth/service.go(4 hunks)internal/sms-gateway/modules/messages/service.go(3 hunks)internal/sms-gateway/modules/settings/models.go(1 hunks)internal/sms-gateway/modules/webhooks/models.go(2 hunks)internal/sms-gateway/openapi/docs.go(17 hunks)internal/sms-gateway/users/cache.go(1 hunks)internal/sms-gateway/users/domain.go(1 hunks)internal/sms-gateway/users/errors.go(1 hunks)internal/sms-gateway/users/models.go(1 hunks)internal/sms-gateway/users/module.go(1 hunks)internal/sms-gateway/users/repository.go(1 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/errors.go(1 hunks)pkg/cache/memory.go(13 hunks)pkg/cache/options.go(5 hunks)pkg/cache/redis.go(10 hunks)pkg/cache/typed.go(1 hunks)
💤 Files with no reviewable changes (3)
- internal/sms-gateway/models/models.go
- internal/sms-gateway/modules/auth/module.go
- internal/sms-gateway/modules/auth/repository.go
✅ Files skipped from review due to trivial changes (2)
- pkg/cache/errors.go
- pkg/cache/cache.go
🚧 Files skipped from review as they are similar to previous changes (25)
- internal/config/module.go
- internal/sms-gateway/users/models.go
- internal/config/config.go
- configs/config.example.yml
- internal/sms-gateway/handlers/thirdparty/permissions.go
- internal/sms-gateway/jwt/models.go
- internal/sms-gateway/users/errors.go
- deployments/prometheus/alerts/jwt-alerts.yml
- internal/sms-gateway/jwt/module.go
- api/mobile.http
- internal/sms-gateway/handlers/settings/permissions.go
- internal/sms-gateway/handlers/messages/3rdparty.go
- internal/sms-gateway/handlers/devices/permissions.go
- internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go
- internal/sms-gateway/modules/settings/models.go
- internal/sms-gateway/handlers/messages/permissions.go
- internal/sms-gateway/handlers/logs/permissions.go
- internal/sms-gateway/handlers/webhooks/3rdparty.go
- internal/sms-gateway/handlers/settings/3rdparty.go
- cmd/sms-gateway/main.go
- api/requests.http
- internal/sms-gateway/users/module.go
- internal/sms-gateway/models/migrations/mysql/20251121071748_add_tokens.sql
- go.mod
- deployments/grafana/dashboards/jwt.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/handlers/thirdparty/module.go
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/memory.gopkg/cache/redis.go
📚 Learning: 2025-09-10T11:11:43.462Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/memory.go:59-74
Timestamp: 2025-09-10T11:11:43.462Z
Learning: In pkg/cache/memory.go, the cleanup method removes expired items first, then executes the provided callback function. This means methods like Drain that use cleanup don't need additional expiration filtering since expired items are already removed before the callback runs.
Applied to files:
pkg/cache/memory.gopkg/cache/redis.go
📚 Learning: 2025-09-10T23:54:16.828Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:118-121
Timestamp: 2025-09-10T23:54:16.828Z
Learning: Redis 7.4.0 introduced HEXPIRE, HEXPIREAT, and related commands that support per-field TTL on hash data structures. go-redis v9.6.0+ includes support for these commands with methods like HExpireAt.
Applied to files:
pkg/cache/redis.go
🔇 Additional comments (29)
internal/sms-gateway/handlers/webhooks/permissions.go (1)
3-7: LGTM! Clean permission scope definitions.The permission constants follow a clear
resource:actionpattern and Go naming conventions. The three scopes (list, write, delete) provide a sensible access control model for webhook operations.One optional consideration: verify that
ScopeWriteappropriately covers both create and update operations in your API design. If you need granular control between creating new webhooks versus updating existing ones, you might consider separate scopes in the future.internal/sms-gateway/models/migration.go (2)
9-10: Unused embedded filesystem.The
migrationsvariable is declared but never used in this file. If it's not used elsewhere in the package, consider removing it.Run the following script to check if
migrationsis referenced elsewhere:
13-13: Verify User migration is handled elsewhere and check Device dependencies.The removal of User from the migration aligns with the broader refactor, but please confirm:
- User migration is properly handled in the new jwt or users module
- The Device model doesn't have foreign key dependencies on User that could break
Run the following script to verify:
pkg/cache/redis.go (1)
40-44: Previous critical issue resolved.All Lua script Redis commands (HExpireAt, HTTL, HExpire) now correctly use the Redis 7.4.0+ format with
'FIELDS', '1', fieldparameters. The migration is complete and consistent.internal/config/types.go (1)
11-27: Duration wrapper and nil‑safe accessors look goodThe
Durationalias and itsDuration()/String()methods are straightforward, handlenilsafely, and are appropriate for config use as a thin wrapper overtime.Duration. No issues from a correctness or API‑shape standpoint.pkg/cache/memory.go (2)
9-28: Overall memory cache structure and TTL model look solidThe
memoryCache/memoryItemdesign, default TTL handling inNewMemoryandnewItem, and theisExpiredhelper form a clear, easy‑to‑reason‑about in‑memory implementation. Zero TTL meaning “no expiration by default” is consistent with the docs, andErrKeyNotFoundvsErrKeyExpiredseparation viagetItem/getValueis clean.Also applies to: 58-92, 331-353
93-176: Cleanup/Drain semantics correctly preserve non‑expired items only
Cleanupremoves expired entries first and only then invokes the callback, andDrainusescleanupto snapshotm.itemsand swap in a fresh map before copying out values. This matches the prior contract that:
- expired items are dropped before
Drainruns, andDrainreturns only non‑expired items and leaves the cache empty afterward.This keeps the behavior aligned with the existing Redis cache behavior and previous
memoryCache.cleanupsemantics. Based on learnings.Also applies to: 401-421
internal/sms-gateway/handlers/thirdparty/module.go (1)
3-16: FX module wiring and named logger usage look correct
Module()follows the established pattern: anfx.Modulewith a package‑specific name, a scoped named logger vialogger.WithNamedLogger("3rdparty"), andfx.Provide(NewAuthHandler)to expose the handler constructor. This matches the existing fx/logger conventions in the project and should compose cleanly with the top‑level handlers module. Based on learnings.internal/sms-gateway/modules/webhooks/models.go (1)
6-21: Confirmusers.UserGORM mapping matches previousmodels.UserSwapping the association type to
users.Useris consistent with the new users module and should work as long asusers.Userkeeps the same table name/PK type thatUserIDpoints to. Please double‑check that the new model’s GORM config (table name, primary key column/type) still matches the existingwebhooksFK/constraints soAutoMigratedoesn’t unexpectedly rewrite them.internal/sms-gateway/handlers/module.go (1)
9-10: thirdparty handlers module wiring looks consistentImporting
handlers/thirdpartyand addingthirdparty.Module()to the handlersfx.Modulecleanly integrates the new submodule; this aligns with the existingnewThirdPartyHandlerusage and keeps composition idiomatic.Also applies to: 40-41
internal/sms-gateway/app.go (1)
10-11: JWT and users modules are wired appropriately into the Fx appAdding
users.Module()andjwt.Module()to the top‑levelModuleis consistent with the rest of the DI setup and should make the new services available to handlers and middlewares. Please ensure you run the app (orfx.ValidateApp/tests) to confirm there are no missing bindings or new dependency cycles introduced by these modules.Also applies to: 24-25, 44-45, 59-60
internal/sms-gateway/handlers/devices/3rdparty.go (1)
9-13: Devices 3rd‑party handlers now depend on scopes andusers.User– verify upstream auth pipelineSwitching the handler signatures to
users.Userand wrapping the routes withpermissions.RequireScope(ScopeList|ScopeDelete)plususerauth.WithUseris consistent with the new auth/users design. This does mean:
- These endpoints now require scopes to be present in the context (set by Basic/JWT middleware via
permissions.SetScopes).- They also assume a user has been stored via
userauth.SetUserbeforeWithUserruns; otherwiseWithUserwill panic.Please double‑check the 3rd‑party router setup (
newThirdPartyHandlerand JWT/basic middlewares) to ensure that, for every security scheme advertised here (ApiAuth,JWTAuth), both the user and appropriate scopes are always set before these handlers are hit.Also applies to: 36-37, 46-48, 60-61, 72-76, 87-88
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
7-8: Basic auth → users.Service + ScopeAll wiring looks correct
NewBasicnow logs in viausers.Service, stores ausers.UserviaSetUser, and assignspermissions.ScopeAll, whileSetUser/GetUserconsistently use a value type inLocals. This resolves the earlier pointer/value mismatch and aligns Basic auth with the new permissions model.Also applies to: 16-21, 49-56, 88-90, 96-97, 99-109
internal/sms-gateway/modules/messages/service.go (1)
124-168: Switch tousers.Userlooks consistent and non-breakingUpdating
SelectStatesandGetStateto acceptusers.Userwhile still keying everything offuser.IDkeeps behavior unchanged and aligns this service with the new users domain. Callers should be updated by the compiler; no further changes needed here.internal/sms-gateway/jwt/errors.go (1)
5-12: Error set is clear and well-scopedThe exported error variables cover the main JWT failure modes cleanly and are suitable for use with
errors.Isacross the JWT subsystem and callers.internal/sms-gateway/jwt/jwt.go (1)
10-27: JWT service interface and models are well-shapedThe
Serviceinterface and theClaims/TokenInfotypes define a minimal, cohesive JWT API (issue/parse/revoke with user ID and scopes) that should integrate cleanly with handlers and middleware.internal/sms-gateway/handlers/thirdparty/auth.go (1)
17-40: Auth handler wiring looks consistent with the rest of the stackThe
AuthHandlerconstructor andRegistermethod are wired cleanly: sharedbase.Handler, per‑routererrorHandler, and per‑routepermissions.RequireScope(ScopeTokensManage)plususerauth.WithUsermatch the security model implied by the swagger comments. I don’t see functional issues in this wiring.internal/sms-gateway/handlers/mobile.go (1)
121-152: Device registration flow with usersSvc looks consistentThe refactor to:
- Prefer
userauth.GetUser(c)when present, and- Fall back to generating a username/password and creating a user via
usersSvc.Create,is consistent with the new users service design. Passing
*userintoauthSvc.RegisterDeviceand returningLogin/Passwordonly for the newly created‑user path matches the endpoint’s description. I don’t see functional problems in this section (aside from theidGeninitialization issue called out separately).internal/sms-gateway/users/service.go (2)
13-33: Service wiring is straightforward and idiomaticThe
Servicestruct andNewServiceconstructor cleanly encapsulate the repository, login cache, and logger. Dependency injection here looks consistent with the rest of the codebase.
35-69: User creation and lookup logic look solid
Createcorrectly checks for existing users, hashes the password with BCrypt, and wraps repository errors with useful context.GetByUsernameis a simple and appropriate wrapper overGetByID. These pieces look good as‑is.internal/sms-gateway/openapi/docs.go (3)
21-87: JWT token issue/ revoke endpoints now correctly documented with security and schemasThe
/3rdparty/v1/auth/tokenPOST and/3rdparty/v1/auth/token/{jti}DELETE operations now include bothApiAuthandJWTAuthsecurity plus concrete request/response/error schemas, which aligns the OpenAPI spec with the new token handlers and resolves the earlier missing‑security problem.
936-1013: Health endpoints and response schema look consistentThe new
/health/live,/health/ready, and/health/startupendpoints, together withsmsgateway.HealthResponseand related types, provide clear liveness/readiness/startup probes with 200/503 semantics. This is a solid addition for observability and Kubernetes-style health checks.Also applies to: 1230-1288
1847-1855: JWTAuth security definition and updated titleDefining
JWTAuthas anapiKeyin theAuthorizationheader and updating the Swagger title to “SMSGate API” both look appropriate and consistent with the rest of the spec and new JWT functionality.Also applies to: 1879-1884
internal/sms-gateway/jwt/config.go (1)
12-32: Config validation for secret and TTL looks solid
Validatecorrectly enforces a non-empty secret with a minimum length and rejects non‑positive TTL values, which prevents misconfigured JWT services from starting.internal/sms-gateway/jwt/service.go (2)
24-51: JWT token generation flow is well-validated and side-effect safeConstructor validation, userID/scopes/TTL checks, default TTL handling, max‑TTL capping via
min(ttl, s.config.TTL), signing with HS256, and persisting the token through the repository all look correct. Error paths are wrapped with context, and issuance metrics are updated consistently based on success vs error.Also applies to: 53-112
114-161: Token parsing, validation, and revocation checks look correct
ParseTokenenforces expiry, issued-at, issuer, and algorithm, then consults the repository to fail revoked JTIs, with validation metrics updated accordingly. This matches typical JWT best practices and integrates cleanly with your revocation store.internal/sms-gateway/jwt/repository.go (1)
10-47: Repository methods correctly encapsulate token persistence and revocation checks
Insert,Revoke, andIsRevokeduseWithContext, narrow queries ontokenModel, and wrap underlying GORM errors with descriptive messages. The revocation update is idempotent and theIsRevokedcheck is simple and efficient.internal/sms-gateway/modules/auth/service.go (1)
24-36: Auth service wiring to users/devices/online modules looks cleanThe updated
Servicestruct andNewconstructor neatly injectusersSvc,devicesSvc,onlineSvc, andlogger, and keep the one-time-code cache localized. This simplifies auth to rely on the new users module without extra repositories or caches.Also applies to: 38-60
internal/sms-gateway/jwt/metrics.go (1)
8-81: JWT metrics wiring looks solid and idiomaticThe metrics setup here is clean:
- Namespaces/subsystem are consistent (
sms/auth).- Counters and histograms are clearly separated per operation (issuance/validation/revocation).
- Helper methods (
Increment*,Observe*) keep call sites simple and reduce copy‑paste of metric logic.Assuming the alerting and dashboards are now aligned with the single
statuslabel, this is good to go.Please confirm that your Prometheus alert rules and dashboards reference these exact metric names and the
statuslabel only (or update them accordingly). For example:Also applies to: 83-117
| // GetByID retrieves a user by their ID. | ||
| func (r *repository) GetByID(id string) (*userModel, error) { | ||
| user := new(userModel) | ||
|
|
||
| if err := r.db.Where("id = ?", id).Take(&user).Error; err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| return nil, ErrNotFound | ||
| } | ||
| return nil, fmt.Errorf("can't get user: %w", err) | ||
| } | ||
|
|
||
| return user, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix GetByID to pass the correct pointer type to GORM
GetByID currently does:
user := new(userModel)
if err := r.db.Where("id = ?", id).Take(&user).Error; err != nil {
...
}Here user is of type *userModel, so &user is **userModel. GORM expects a pointer to the destination value (e.g. *userModel), not a pointer to that pointer. Passing &user can lead to unexpected behavior or outright failure depending on the GORM version.
This should instead pass user directly:
func (r *repository) GetByID(id string) (*userModel, error) {
- user := new(userModel)
-
- if err := r.db.Where("id = ?", id).Take(&user).Error; err != nil {
+ user := new(userModel)
+
+ if err := r.db.Where("id = ?", id).Take(user).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return nil, ErrNotFound
}
return nil, fmt.Errorf("can't get user: %w", err)
}
return user, nil
}Please double‑check this against the GORM docs for your version to confirm that Take should receive a *userModel and not **userModel. For example:
🌐 Web query:
GORM Take method expected argument type for querying into a struct pointer
💡 Result:
Use a pointer to the destination struct (or a pointer to a destination that GORM accepts such as a map when using Model/Table). For example:
- Single struct (recommended): var user User; db.Take(&user).
- If you want to avoid passing a struct pointer, specify the model: db.Model(&User{}).Take(&resultMap).
Reference: GORM query docs. [1]
Sources:
[1] GORM - Query (First/Take/Last require a pointer to the destination struct).
Fix GetByID to pass the correct pointer type to GORM
GetByID currently passes &user (type **userModel) to Take, but GORM requires a pointer to the destination struct (*userModel). Change line 37 from Take(&user) to Take(user):
func (r *repository) GetByID(id string) (*userModel, error) {
user := new(userModel)
- if err := r.db.Where("id = ?", id).Take(&user).Error; err != nil {
+ if err := r.db.Where("id = ?", id).Take(user).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return nil, ErrNotFound
}
return nil, fmt.Errorf("can't get user: %w", err)
}
return user, nil
}📝 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.
| // GetByID retrieves a user by their ID. | |
| func (r *repository) GetByID(id string) (*userModel, error) { | |
| user := new(userModel) | |
| if err := r.db.Where("id = ?", id).Take(&user).Error; err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return nil, ErrNotFound | |
| } | |
| return nil, fmt.Errorf("can't get user: %w", err) | |
| } | |
| return user, nil | |
| } | |
| // GetByID retrieves a user by their ID. | |
| func (r *repository) GetByID(id string) (*userModel, error) { | |
| user := new(userModel) | |
| if err := r.db.Where("id = ?", id).Take(user).Error; err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return nil, ErrNotFound | |
| } | |
| return nil, fmt.Errorf("can't get user: %w", err) | |
| } | |
| return user, nil | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/users/repository.go around lines 32 to 44, GetByID
currently passes the address of a pointer (&user, type **userModel) into GORM's
Take call which expects a pointer to the destination struct (*userModel); change
the Take call to pass the pointer value (Take(user)) since user is already
created with new(userModel), leaving the error handling intact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/cache/options.go (2)
5-73: Set options (Option,WithTTL,WithValidUntil) look correct; maybe clarify “no expiry” semanticsThe data model and helpers are straightforward and match the comments:
WithTTLusingtime.Now().Add(ttl)on apply and treatingttl <= 0as “no expiry” viatime.Time{}is consistent with the documentation, andWithValidUntilis a simple override.One potential improvement: callers only see that “TTL <= 0 means the item will not expire,” but the implementation encodes that specifically as
validUntil.IsZero(). It would help to call that out explicitly in the docstring (and ensure the concrete caches consistently interprettime.Time{}as “no expiration”), and perhaps mention thatWithValidUntilcannot currently express “no expiry” and thatWithTTL(0)is the supported way to get that behavior.
107-208: GetOption helpers are implemented correctly; consider documenting interactions and edge casesThe
AndSetTTL/AndUpdateTTL/AndSetValidUntil/AndDefaultTTL/AndDeletehelpers are all thin, correct setters ongetOptionsand the pointer‑capture pattern forttl/validUntilis idiomatic in Go (the closure causes escape to heap, so there’s no lifetime issue).Two minor API/contract points you might want to tighten up:
- The docs for
AndSetTTLandAndUpdateTTLdon’t mention how non‑positive TTLs are interpreted, even thoughWithTTLclearly definesttl <= 0as “no expiry.” If the underlying Get path uses the same convention, mirroring that in the comments (or explicitly stating if they are rejected/treated differently) would avoid surprises.- When multiple
GetOptions are combined (e.g.,AndSetTTL+AndSetValidUntil+AndDefaultTTLorAndDeleteplus a TTL update), the effective precedence is delegated to the consumers ofgetOptions. It might be worth documenting the intended precedence or combination rules somewhere nearGetOptionso callers know what to expect.Aside from those clarifications, the surface looks clean and composable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cache/options.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/cache/options.go (1)
75-105:getOptionsplumbing andisEmptyhelper are solidThe
getOptionsstruct andapply/isEmptyhelpers are minimal and correctly model optional behavior via pointer fields and booleans.isEmptychecks all flags, so consumers can cheaply skip post‑processing when noGetOptions were supplied.No functional issues here from what’s visible.
Summary by CodeRabbit
New Features
Configuration
Security
Observability
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.