[otp] separate package from the auth #195
Conversation
WalkthroughAdds a new OTP module (config, domain, storage, service, FX wiring), moves one-time-code generation/validation from the auth module into that service, updates auth/handler/middleware signatures to accept contexts and user pointers, and adjusts the mobile API auth header example. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MobileHandler as Mobile Handler
participant AuthService as Auth Service
participant OTPService as OTP Service
participant Storage as Storage/Cache
rect rgb(200,220,255)
Note over MobileHandler,OTPService: Generate code flow
Client->>MobileHandler: POST /user/code (Basic auth)
MobileHandler->>AuthService: GenerateUserCode(ctx, userID)
AuthService->>OTPService: Generate(ctx, userID)
OTPService->>OTPService: generate 6-digit random code
OTPService->>Storage: SetOrFail(ctx, code, userID, validUntil)
Storage-->>OTPService: success / error
OTPService-->>AuthService: Code{Code, ValidUntil} / error
AuthService-->>MobileHandler: code / error
MobileHandler-->>Client: response
end
rect rgb(220,200,255)
Note over MobileHandler,OTPService: Validate code flow
Client->>MobileHandler: authenticate with code
MobileHandler->>AuthService: AuthorizeUserByCode(ctx, code)
AuthService->>OTPService: Validate(ctx, code)
OTPService->>Storage: GetAndDelete(ctx, code)
Storage-->>OTPService: userID / error
OTPService-->>AuthService: userID / error
AuthService-->>MobileHandler: authenticated user / error
MobileHandler-->>Client: token / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code graph analysis (3)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/otp/service.go (6)
internal/sms-gateway/modules/auth/service.go (1)
⏰ 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)
🔇 Additional comments (16)
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)
configs/config.example.yml (1)
41-43: Consider clarifying the "retries" comment.The comment "otp generation retries" could be ambiguous. Does this refer to:
- Retries for generating a unique OTP code (collision handling), or
- Maximum validation attempts allowed per code?
Based on typical OTP implementations, this likely controls code generation collision handling, but a clearer comment would help users configure this correctly.
otp: # otp config ttl: 300 # otp ttl in seconds [OTP__TTL] - retries: 3 # otp generation retries [OTP__RETRIES] + retries: 3 # otp code generation retries (collision handling) [OTP__RETRIES]internal/sms-gateway/otp/errors.go (1)
1-8: Consider adding doc comments for exported error variables
ErrInvalidConfigandErrInitFailedare exported but undocumented. Adding short comments will help tooling and make their intended usage (config validation vs. initialization) clearer.-var ( - ErrInvalidConfig = errors.New("invalid config") - ErrInitFailed = errors.New("initialization failed") -) +var ( + // ErrInvalidConfig indicates that the OTP configuration is invalid. + ErrInvalidConfig = errors.New("invalid config") + + // ErrInitFailed indicates that the OTP service failed to initialize. + ErrInitFailed = errors.New("initialization failed") +)internal/config/config.go (1)
20-21: OTP config integration looks good; consider clarifying TTL unitsThe new
OTPblock onConfigand its defaults (TTL=300, Retries=3) are consistent with the downstreamotp.Configmapping and validation. To avoid confusion, you may want to clarify thatOTP.TTLis in seconds, in line with other*Secondsfields:-type OTP struct { - TTL uint16 `yaml:"ttl" envconfig:"OTP__TTL"` - Retries uint8 `yaml:"retries" envconfig:"OTP__RETRIES"` -} +type OTP struct { + // TTL specifies OTP lifetime in seconds. + TTL uint16 `yaml:"ttl" envconfig:"OTP__TTL"` + Retries uint8 `yaml:"retries" envconfig:"OTP__RETRIES"` +}Otherwise, the wiring into
defaultConfigand the mainConfigstruct looks solid.Also applies to: 89-93, 128-131
internal/sms-gateway/otp/storage.go (1)
1-33: String-focused storage wrapper is fine for OTP useThe
Storagewrapper correctly adapts the byte-based cache API to string values for OTP codes (SetOrFail,GetAndDelete,Cleanupall delegate cleanly). Only very minor nit is the receiver namecc(could besorst), but that’s purely cosmetic.internal/sms-gateway/modules/auth/service.go (1)
49-49: Consider handling the error fromnanoid.Standard.The error from
nanoid.Standard(21)is silently ignored. While it's unlikely to fail with a valid length of 21, ignoring errors can mask issues during initialization. Consider logging or panicking on error since this is a critical initialization step.- idgen, _ := nanoid.Standard(21) + idgen, err := nanoid.Standard(21) + if err != nil { + panic(fmt.Sprintf("failed to initialize nanoid generator: %v", err)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🧰 Additional context used
🧠 Learnings (2)
📚 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:
internal/sms-gateway/modules/auth/service.go
📚 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/modules/auth/module.go
🧬 Code graph analysis (8)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(41-65)
internal/config/module.go (2)
internal/config/config.go (2)
Config(10-21)OTP(89-92)internal/sms-gateway/otp/config.go (1)
Config(8-11)
internal/sms-gateway/otp/module.go (3)
internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/otp/storage.go (1)
NewStorage(15-17)internal/sms-gateway/otp/service.go (1)
NewService(21-35)
internal/sms-gateway/modules/auth/service.go (4)
internal/sms-gateway/otp/service.go (1)
Service(13-19)internal/sms-gateway/models/models.go (1)
User(17-23)internal/sms-gateway/otp/config.go (1)
Config(8-11)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(41-65)
internal/sms-gateway/otp/service.go (6)
internal/config/config.go (1)
Config(10-21)internal/sms-gateway/otp/config.go (1)
Config(8-11)internal/sms-gateway/otp/storage.go (1)
Storage(10-12)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(7-7)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(34-38)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(81-83)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(23-134)internal/sms-gateway/otp/module.go (1)
Module(9-19)
⏰ 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)
api/mobile.http (1)
11-13: LGTM!The authorization change from Bearer to Basic for the
/user/codeendpoint aligns correctly with the OTP-based authentication flow. Basic auth with user credentials is the appropriate mechanism for requesting a one-time code, while Bearer tokens are used for subsequent authenticated requests.internal/config/module.go (1)
128-133: LGTM!The OTP config provider follows the established pattern in this module. The conversions from
uint16totime.Durationanduint8tointare safe and consistent with how other time-based configurations are handled (e.g.,push.Config,messages.Config).internal/sms-gateway/modules/auth/module.go (1)
15-16: LGTM!The provider ordering with
newRepositorybeforeNew(service constructor) is clean and reflects the dependency hierarchy. While fx handles actual initialization order based on the dependency graph, this explicit ordering improves readability. Based on learnings, thefx.Decoratepattern for module-scoped loggers is correctly applied here.internal/sms-gateway/otp/domain.go (1)
5-9: LGTM!Clean domain model for OTP codes. The struct appropriately exposes fields for cross-package use.
Minor consideration: the
Codefield name within aCodetype leads tocode.Codeaccess patterns. An alternative likeValuecould be considered, but this is a stylistic preference and the current naming is acceptable.internal/sms-gateway/handlers/mobile.go (1)
178-188: LGTM!Good improvement passing
c.Context()toGenerateUserCode. This enables:
- Request cancellation propagation
- Deadline/timeout handling
- Distributed tracing support
The response mapping from
*otp.Codeto the DTO is clean and correct.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
64-76: Context-aware auth call inNewCodelooks correctPassing
c.Context()intoAuthorizeUserByCodekeeps cancellation/timeouts tied to the HTTP request and matches the updated auth service signature. No further changes needed here.internal/sms-gateway/app.go (1)
22-58: OTP module wiring into Fx graph is consistentImporting
otpand addingotp.Module()to the mainModulekeeps OTP concerns nicely isolated while making the service injectable (e.g., into auth). Givenotp.NewServicecan fail on invalid config, just make sure you’re happy with “fail-fast on startup” behavior if OTP config is misconfigured.internal/sms-gateway/otp/module.go (1)
1-19: Fx wiring is idiomatic; just confirm cache type alignmentThe OTP Fx module looks clean: named logger, a dedicated cache namespace via
factory.New("otp"),NewStoragechained off that cache, andNewServiceprovided separately, all markedfx.Privateso internals don’t leak out of the module.One thing to double-check:
Module()usesinternal/sms-gateway/cache.FactorywhileNewStoragedepends ongithub.com/android-sms-gateway/server/pkg/cache.Cache. As long asinternal/sms-gateway/cache.Cacheis an alias or the same concrete type aspkg/cache.Cache, Fx will resolve this correctly. If they’re distinct types,NewStoragewill never get its dependency.If you haven’t already, it’s worth confirming those two
Cachetypes are aligned.internal/sms-gateway/otp/config.go (1)
1-23: OTP Config validation is straightforward and correct
Config.Validatecleanly enforcesTTL > 0andRetries > 0, wrappingErrInvalidConfigso callers can distinguish config issues while still having a readable message. This fits well with theNewServiceconstructor flow.internal/sms-gateway/otp/service.go (1)
21-66: Thefor rangeover integer syntax is valid in Go 1.22+; no fix needed.The repository uses Go 1.24.3, which supports "range over integers"—a feature introduced in Go 1.22 (released February 2024). The syntax
for range s.cfg.Retriesis correct and will compile as expected. No changes are required.Likely an incorrect or invalid review comment.
internal/sms-gateway/modules/auth/service.go (2)
26-39: LGTM!The Service struct is well-organized with clear separation of dependencies. The addition of
otpSvcfor delegating OTP operations is appropriate.
161-174: LGTM!The refactored method correctly propagates context to
otpSvc.Validate, has proper error wrapping, and the pointer return type allows for cleaner nil returns on error.
capcom6
force-pushed
the
otp/introduce-package
branch
from
bf02218 to
22d772b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/sms-gateway/modules/auth/service.go (1)
26-39: Service wiring and caching look good; consider removing unusedidgenThe updated
Servicestruct andNewconstructor correctly wire inusers,otpSvc,devicesSvc,onlineSvc, andlogger, and initializeusersCachewith a 1h TTL, which fits the existing auth caching pattern.However,
idgenis now initialized but no longer used anywhere in this file after the OTP refactor. Unless you plan to reintroduce ID-based auth codes, consider removing theidgenfield and its initialization to keep the service lean.Also applies to: 41-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/otp/domain.go
- internal/sms-gateway/handlers/mobile.go
- internal/sms-gateway/otp/errors.go
- internal/config/module.go
- internal/sms-gateway/otp/storage.go
- internal/sms-gateway/otp/module.go
- internal/sms-gateway/otp/service.go
- api/mobile.http
🧰 Additional context used
🧠 Learnings (2)
📚 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:
internal/sms-gateway/modules/auth/service.go
📚 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/modules/auth/module.go
🧬 Code graph analysis (3)
internal/sms-gateway/modules/auth/service.go (4)
internal/sms-gateway/otp/service.go (1)
Service(13-19)internal/sms-gateway/cache/factory.go (1)
Cache(14-14)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(41-65)
internal/sms-gateway/otp/config.go (2)
internal/worker/config/types.go (1)
Duration(11-11)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
⏰ 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 (7)
internal/sms-gateway/otp/config.go (1)
8-23: OTP Config and validation look consistent and robust
Configmodels TTL and retries cleanly, andValidatecorrectly enforces both > 0 while preservingErrInvalidConfigvia%w, which is useful for classification and wrapping.configs/config.example.yml (1)
41-43: OTP example config matches Go config and defaultsThe new
otpblock (ttl=300, retries=3) matchesConfig.OTPanddefaultConfig, and the comments/env var hints are consistent with the struct tags.internal/sms-gateway/modules/auth/module.go (1)
16-16: Auth module wiring remains correct with the new constructorKeeping
fx.Provide(New)here cleanly wires the updatedNewconstructor (now depending on the OTP service and other deps) while preserving the existing module structure.internal/sms-gateway/app.go (1)
22-22: OTP module is correctly wired into the application module graphImporting
internal/sms-gateway/otpand addingotp.Module()to the top-levelModulecleanly integrates the new OTP subsystem without disturbing existing modules.Also applies to: 57-57
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
75-82: Context-aware user authorization for Code header looks goodPassing
c.Context()intoAuthorizeUserByCodeenables proper context propagation (cancellation, deadlines, tracing) through the auth/OTP stack while preserving the middleware flow.internal/config/config.go (1)
20-21: OTP config struct and defaults integrate cleanlyAdding
OTPtoConfig, definingOTPwithttl/retriesfields, and initializing sensible defaults (300, 3) keeps the configuration surface consistent with the example YAML and the OTP package’s expectations.Also applies to: 89-92, 128-131
internal/sms-gateway/modules/auth/service.go (1)
67-75: GenerateUserCode delegation to OTP service is straightforward
GenerateUserCodenow simply delegates tos.otpSvc.Generateand wraps errors with contextual information, which keeps OTP concerns encapsulated in the OTP service and keeps the auth service thin.
capcom6
force-pushed
the
otp/introduce-package
branch
from
22d772b to
c460972
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/sms-gateway/modules/auth/service.go (1)
156-169: Normalize error returns inAuthorizeUserByCodeTo keep the contract simple (user is zero value whenever
err != nil), consider returningmodels.User{}in theGetByIDfailure path as well, mirroring the validation error branch.func (s *Service) AuthorizeUserByCode(ctx context.Context, code string) (models.User, error) { userID, err := s.otpSvc.Validate(ctx, code) if err != nil { return models.User{}, fmt.Errorf("failed to validate code: %w", err) } user, err := s.users.GetByID(userID) if err != nil { - return user, fmt.Errorf("failed to get user: %w", err) + return models.User{}, fmt.Errorf("failed to get user: %w", err) } return user, nil }
🧹 Nitpick comments (1)
internal/sms-gateway/otp/storage.go (1)
20-33: Consider renaming receiver for consistency.The receiver name
ccsuggests "CodesCache" but the type is nowStorage. For consistency, consider usingsas the receiver name.Apply this diff if you'd like to align the receiver name with the type:
-func (cc *Storage) SetOrFail(ctx context.Context, key string, value string, opts ...cache.Option) error { - return cc.cache.SetOrFail(ctx, key, []byte(value), opts...) +func (s *Storage) SetOrFail(ctx context.Context, key string, value string, opts ...cache.Option) error { + return s.cache.SetOrFail(ctx, key, []byte(value), opts...) } -func (cc *Storage) GetAndDelete(ctx context.Context, key string) (string, error) { - data, err := cc.cache.GetAndDelete(ctx, key) +func (s *Storage) GetAndDelete(ctx context.Context, key string) (string, error) { + data, err := s.cache.GetAndDelete(ctx, key) return string(data), err } -func (cc *Storage) Cleanup(ctx context.Context) error { - return cc.cache.Cleanup(ctx) +func (s *Storage) Cleanup(ctx context.Context) error { + return s.cache.Cleanup(ctx) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/config/module.go
- internal/sms-gateway/otp/errors.go
- configs/config.example.yml
- internal/sms-gateway/otp/module.go
- internal/sms-gateway/handlers/mobile.go
- internal/config/config.go
- internal/sms-gateway/handlers/middlewares/userauth/userauth.go
- api/mobile.http
- internal/sms-gateway/app.go
- internal/sms-gateway/modules/auth/module.go
- internal/sms-gateway/otp/domain.go
- internal/sms-gateway/otp/service.go
🧰 Additional context used
🧠 Learnings (1)
📚 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:
internal/sms-gateway/modules/auth/service.go
🧬 Code graph analysis (2)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(81-83)
internal/sms-gateway/otp/config.go (2)
internal/config/config.go (1)
Config(10-21)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
⏰ 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 (3)
internal/sms-gateway/otp/config.go (1)
8-24: LGTM! Clean validation logic.The Config struct and validation method are well-implemented. The validation checks ensure both TTL and Retries are positive values, with clear error messages wrapped appropriately.
internal/sms-gateway/modules/auth/service.go (2)
14-18: Dependency injection and cache setup look consistentThe new
otpSvcdependency andusers *repositoryare wired cleanly throughServiceandNew, andusersCacheis initialized with a 1h TTL that matches the hourlyRun/cleanloop. Given thecache.Cleanupsemantics (expired entries are dropped before callbacks), this simpleCleanup()call is sufficient and you don’t need extra expiry handling in callers. Based on learnings, this looks correct.If not already done, just confirm that
Service.Runis actually started in the app wiring so the hourly cleanup runs in production.Also applies to: 25-37, 39-60
62-69: Context is now correctly propagated to OTP generation
GenerateUserCodenow passes the incomingctxtos.otpSvc.Generate, instead of using a background context, which fixes cancellation/timeout/tracing propagation through the OTP path. Implementation and error wrapping look good.
capcom6
force-pushed
the
otp/introduce-package
branch
from
c460972 to
f48971a
Compare
capcom6
force-pushed
the
otp/introduce-package
branch
from
f48971a to
b7b8354
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/sms-gateway/otp/storage.go (1)
14-14: Update the stale comment reference.The comment references "CodesCache" but the function is named "NewStorage".
-// NewStorage creates a new CodesCache with the underlying cache +// NewStorage creates a new Storage with the underlying cache.
🧹 Nitpick comments (1)
internal/sms-gateway/otp/service.go (1)
57-66: Extract magic numbers to named constants.The values
3(bytes for random data) and16,8(bit shifts) are flagged by the linter. Consider defining constants for clarity.+const ( + codeByteLen = 3 // 3 bytes = 24 bits of randomness + codeModulo = 1000000 +) + func (s *Service) Generate(ctx context.Context, userID string) (*Code, error) { var code string var err error - b := make([]byte, 3) + b := make([]byte, codeByteLen) validUntil := time.Now().Add(s.cfg.TTL) for range s.cfg.Retries { if _, err = rand.Read(b); err != nil { s.logger.Warn("failed to read random bytes", zap.Error(err)) continue } - num := (int(b[0]) << 16) | (int(b[1]) << 8) | int(b[2]) - code = fmt.Sprintf("%06d", num%1000000) + num := int(b[0])<<16 | int(b[1])<<8 | int(b[2]) + code = fmt.Sprintf("%06d", num%codeModulo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sms-gateway/handlers/middlewares/userauth/userauth.go
- internal/sms-gateway/otp/errors.go
- internal/sms-gateway/app.go
- internal/sms-gateway/otp/module.go
🧰 Additional context used
🧠 Learnings (2)
📚 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:
internal/sms-gateway/modules/auth/service.go
📚 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/modules/auth/module.go
🧬 Code graph analysis (6)
internal/config/module.go (2)
internal/config/config.go (2)
Config(10-21)OTP(90-93)internal/sms-gateway/otp/config.go (1)
Config(9-12)
internal/sms-gateway/modules/auth/service.go (3)
internal/sms-gateway/otp/service.go (1)
Service(13-19)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/otp/service.go (5)
internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(10-12)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(35-39)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/otp/config.go (2)
internal/config/config.go (1)
Config(10-21)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(81-83)
🪛 GitHub Check: Lint
internal/config/config.go
[failure] 91-91:
File is not properly formatted (golines)
internal/sms-gateway/otp/service.go
[failure] 65-65:
Magic number: 16, in detected (mnd)
[failure] 57-57:
Magic number: 3, in detected (mnd)
internal/sms-gateway/otp/config.go
[failure] 10-10:
File is not properly formatted (golines)
internal/sms-gateway/otp/domain.go
[failure] 5-5:
Comment should end in a period (godot)
internal/sms-gateway/otp/storage.go
[failure] 30-30:
Comment should end in a period (godot)
[failure] 24-24:
Comment should end in a period (godot)
[failure] 19-19:
Comment should end in a period (godot)
[failure] 14-14:
Comment should end in a period (godot)
[failure] 9-9:
Comment should end in a period (godot)
⏰ 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 (16)
api/mobile.http (1)
11-13: LGTM! Auth scheme change aligns with OTP flow.The switch from Bearer token to Basic authentication for the
/user/codeendpoint is appropriate, as users need to authenticate with credentials to obtain an OTP code. The commented line preserves context for the change.internal/sms-gateway/modules/auth/module.go (1)
16-17: Provider reordering is functionally safe but unnecessary.While the reordering of
fx.Provide(New)afterfx.Provide(newRepository, fx.Private)doesn't introduce any issues, Uber FX automatically resolves dependencies regardless of provider declaration order. The dependency graph is built at runtime, so this change has no functional impact.internal/sms-gateway/otp/domain.go (1)
6-9: LGTM! Clean domain model.The
Codestruct is well-designed as a simple data container for OTP codes with clear field semantics.internal/sms-gateway/handlers/mobile.go (1)
181-192: LGTM! Context propagation follows best practices.Adding
c.Context()as the first parameter toGenerateUserCodeenables proper request cancellation, timeout handling, and distributed tracing. This is a best practice for context-aware operations.configs/config.example.yml (1)
41-43: LGTM! Sensible OTP configuration defaults.The OTP configuration values are appropriate:
- TTL of 300 seconds (5 minutes) is standard for OTP validity
- 3 retries adequately handles code generation collisions
- Clear documentation with environment variable mappings
internal/config/config.go (2)
20-20: LGTM! OTP field properly integrated into Config.The OTP field is correctly positioned with other service configurations and has clear documentation.
132-135: LGTM! Sensible OTP defaults.The default values (300s TTL, 3 retries) are appropriate and consistent with the example configuration.
internal/sms-gateway/otp/config.go (1)
14-24: LGTM! Solid validation logic.The
Validate()method correctly ensures both TTL and Retries are positive values with clear error messages and proper error wrapping.internal/config/module.go (2)
14-14: LGTM! Import properly positioned.The OTP package import is correctly placed with other internal imports and is used in the provider below.
140-145: LGTM! Clean type conversion and wiring.The provider correctly bridges the global configuration to the OTP-specific config, with safe type conversions from
uint16/uint8totime.Duration/int. The pattern is consistent with other config providers in this module.internal/sms-gateway/modules/auth/service.go (3)
35-56: Clean constructor refactoring.The constructor signature is now explicit with all dependencies, and the initialization is clear and well-organized.
58-66: Clean delegation to OTP service.The
GenerateUserCodemethod now properly forwards the context to the OTP service and returns the domain type directly.
147-160: I'll help you verify the return type consistency betweenAuthorizeUserByCodeand the middleware'sGetUserfunction. Let me search for theGetUserimplementation and check the type assertion.
<function_calls>
#!/bin/bashSearch for GetUser function in middleware-related files
rg -n -A10 'func GetUser' --type=go
</function_calls>
<function_calls>
#!/bin/bashAlso search for GetUser in all Go files to be thorough
rg -n 'GetUser' --type=go -B2 -A10 | head -100
</function_calls>Let me also check the models.User type definition:
<function_calls>
#!/bin/bashFind models package and User type definition
fd -e go | xargs grep -l "type User struct" | head -5
</function_calls>Let me look at the auth service file more broadly:
<function_calls>
#!/bin/bashCheck the auth service file structure and related middleware
cat internal/sms-gateway/modules/auth/service.go | head -50
</function_calls><function_calls>
#!/bin/bashSearch for middleware files that might contain GetUser
fd -type f -name 'middleware' --type=go
</function_calls>internal/sms-gateway/otp/service.go (3)
21-43: Well-structured constructor with proper validation.The constructor validates configuration, storage, and logger dependencies appropriately, returning clear initialization errors.
84-94: Clean validation implementation.The
Validatemethod correctly retrieves and atomically deletes the code, preventing replay attacks.
53-82: I'll verify the review comment by investigating whether the configuration validatesRetries > 0and whether the edge case is a real issue.
<function_calls>
#!/bin/bashFind Config struct and Validate() method in the same package/file
fd -e go internal/sms-gateway/otp/ | head -20
</function_calls>
<function_calls>
#!/bin/bashSearch for Config struct definition and Validate method
rg -n "type.Config\sstruct|func.*Validate" internal/sms-gateway/otp/ -A 5
</function_calls>
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/otp/storage.go (1)
15-18: Fix stale constructor doc comment ("CodesCache" vsStorage).The doc comment still refers to "CodesCache" while the function/type are
NewStorage/Storage. This is a leftover from refactoring and can confuse readers and tooling.-// NewStorage creates a new CodesCache with the underlying cache. +// NewStorage creates a new Storage with the underlying cache. func NewStorage(c cache.Cache) *Storage { return &Storage{cache: c} }
🧹 Nitpick comments (2)
internal/sms-gateway/otp/storage.go (1)
29-33: Consider adding contextual error wrapping inGetAndDelete.
GetAndDeletecurrently returns the raw cache error, whileSetOrFailandCleanupwrap errors with contextual messages. For consistency and easier debugging, you might wrap the error similarly (using%wso callers can still useerrors.Is).func (s *Storage) GetAndDelete(ctx context.Context, key string) (string, error) { - data, err := s.cache.GetAndDelete(ctx, key) - return string(data), err + data, err := s.cache.GetAndDelete(ctx, key) + if err != nil { + return "", fmt.Errorf("failed to get and delete cache item: %w", err) + } + return string(data), nil }internal/config/config.go (1)
90-93: TTL/Retries scalar types are fine; consider future headroom (optional)Using
uint16forTTL(seconds) anduint8forRetriesis perfectly reasonable for typical OTP lifetimes and retry counts. If you ever foresee significantly longer TTLs or want to align with other duration‑style config (e.g., using a common duration wrapper or a wider integer), you could switchTTLtouint32or a shared duration type, but that’s purely a future‑proofing tweak.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/config/config.go(3 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sms-gateway/otp/module.go
- internal/sms-gateway/otp/service.go
- internal/sms-gateway/otp/domain.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(81-83)
internal/sms-gateway/otp/config.go (3)
internal/worker/config/types.go (1)
Duration(11-11)internal/config/config.go (1)
Config(10-21)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
⏰ 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: goreleaser
- GitHub Check: Benchmark
🔇 Additional comments (3)
internal/config/config.go (2)
20-20: OTP section wiring into global Config looks consistentAdding
OTP OTP 'yaml:"otp"'follows the existing pattern for other top‑level config sections and keeps the YAML surface clear; I don’t see any issues with how this is introduced.
132-135: OTP defaults and configuration are properly documented and in syncVerification confirms that
configs/config.example.yml(lines 41-43) correctly documents the OTP section with matching default values (ttl: 300andretries: 3), yaml field tags align with the struct definition, and the top-level layout matches the Config structure. No divergence between code and documentation was found.internal/sms-gateway/otp/config.go (1)
1-24: OTP Config and validation are well-structuredUsing
time.DurationforTTLandintforRetriesat the service level, plus aValidate()that enforces both are > 0 and wrapsErrInvalidConfig, is a clean and type‑safe design. This should integrate nicely with the wiring from the global config.
capcom6
force-pushed
the
otp/introduce-package
branch
2 times, most recently
from
2c0ebe5 to
28bffef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/modules/auth/service.go (1)
147-160: Inconsistent error return on line 156.When
s.users.GetByID(userID)fails, the code returnsuser(line 156) which may be a zero-value or partially initialized, rather than returningnilfor consistency with the error on line 151.user, err := s.users.GetByID(userID) if err != nil { - return user, fmt.Errorf("failed to get user: %w", err) + return nil, fmt.Errorf("failed to get user: %w", err) }
🧹 Nitpick comments (2)
internal/sms-gateway/otp/config.go (1)
14-24: Consider accumulating validation errors.The current implementation returns on the first validation failure. For a better developer experience during configuration, consider collecting all validation errors before returning.
+import "errors" + func (c Config) Validate() error { + var errs []error + if c.TTL <= 0 { - return fmt.Errorf("%w: TTL must be greater than 0", ErrInvalidConfig) + errs = append(errs, fmt.Errorf("TTL must be greater than 0")) } if c.Retries <= 0 { - return fmt.Errorf("%w: retries must be greater than 0", ErrInvalidConfig) + errs = append(errs, fmt.Errorf("retries must be greater than 0")) } + if len(errs) > 0 { + return fmt.Errorf("%w: %w", ErrInvalidConfig, errors.Join(errs...)) + } + return nil }internal/sms-gateway/otp/service.go (1)
14-44: Align NewService docs with actual storage typeThe NewService comment calls the second parameter “a storage interface”, but the field and parameter are
*Storage(a concrete struct). Either update the comment to drop “interface” or change the type to an interface if that’s the intended abstraction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/otp/module.go
- api/mobile.http
- internal/config/config.go
- internal/sms-gateway/otp/storage.go
- internal/config/module.go
- internal/sms-gateway/handlers/middlewares/userauth/userauth.go
- configs/config.example.yml
- internal/sms-gateway/handlers/mobile.go
🧰 Additional context used
🧠 Learnings (2)
📚 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:
internal/sms-gateway/modules/auth/service.go
📚 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/modules/auth/module.go
🧬 Code graph analysis (5)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(24-147)internal/sms-gateway/otp/module.go (1)
Module(9-22)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/otp/config.go (1)
internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
internal/sms-gateway/otp/service.go (6)
internal/config/config.go (1)
Config(10-21)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(11-13)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(35-39)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
⏰ 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 (9)
internal/sms-gateway/otp/domain.go (1)
1-9: LGTM!The
Codedomain type is well-structured with appropriate field types. Usingtime.TimeforValidUntilprovides proper time handling semantics.internal/sms-gateway/modules/auth/module.go (1)
16-17: LGTM!The reordering of
fx.Provide(New)afterfx.Provide(newRepository, fx.Private)doesn't affect dependency resolution since fx resolves based on types. The*Servicecan now be injected into other modules if needed.internal/sms-gateway/app.go (1)
22-22: LGTM!The OTP module is correctly imported and wired into the fx application lifecycle. The dependency graph between
auth.Module()andotp.Module()will be properly resolved by fx regardless of declaration order.Also applies to: 57-58
internal/sms-gateway/otp/config.go (1)
8-12: Config struct design is sound.The use of
time.Durationfor TTL provides type safety, and the struct tags are properly aligned. The validation logic correctly enforces positive values.internal/sms-gateway/modules/auth/service.go (2)
35-56: LGTM!The constructor properly wires the new
*otp.Servicedependency and initializes all fields correctly. The delegation to the OTP service for code generation cleanly separates concerns.
58-66: LGTM!The
GenerateUserCodemethod correctly delegates tootpSvc.Generatewith proper context propagation and error wrapping.internal/sms-gateway/otp/errors.go (1)
5-10: LGTM! Both sentinel errors are properly used.The error definitions follow Go idioms with correct godoc comments.
ErrInvalidConfigandErrInitFailedare both actively used for wrapping configuration and initialization validation errors in the OTP module (service.go, lines 36 and 40).internal/sms-gateway/otp/service.go (2)
95-105: Validate wrapper looks correct; confirm ErrKeyNotFound contract
Validateis a thin pass-through tostorage.GetAndDelete, which is good for keeping behavior centralized. Just ensureStorage.GetAndDeleteactually returnsErrKeyNotFoundfor missing/expired codes so the doc comment here stays accurate.
54-93: Config validation already guaranteesRetries > 0; the review concern is addressedThe codebase already prevents the scenario described in the review comment.
Config.Validate()ininternal/sms-gateway/otp/config.go(lines 19–20) explicitly checks thatRetries > 0and returns an error if violated. SinceNewService()callscfg.Validate()before creating the Service, theGenerate()function can never be called withs.cfg.Retries <= 0. The Go version requirement is satisfied (go.moddeclaresgo 1.24.3, which fully supports range-over-int syntax).No changes needed; the invariants are already properly enforced.
capcom6
force-pushed
the
otp/introduce-package
branch
from
28bffef to
4d57117
Compare
capcom6
force-pushed
the
otp/introduce-package
branch
from
4d57117 to
b374055
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/config/config.go (1)
90-93: OTP configuration looks reasonable.The defaults (TTL=300s, Retries=3) are sensible for OTP use cases—5-minute expiry provides usability while limiting attack window, and 3 retries mitigate brute-force attempts. The type choices (
uint16,uint8) are appropriate for these value ranges.Note: A previous review flagged a line-length formatting issue on line 91 per
golinesrules. Please ensure the formatter has been run.internal/sms-gateway/otp/storage.go (1)
15-17: Fix stale doc comment wording forNewStorage.The comment still says “creates a new CodesCache” while the function returns
*Storage. Updating the wording will keep the doc comment accurate and avoid confusion.-// NewStorage creates a new CodesCache with the underlying cache. +// NewStorage creates a new Storage with the underlying cache.
🧹 Nitpick comments (1)
internal/sms-gateway/modules/auth/service.go (1)
147-160: VerifyAuthorizeUserByCodereturn type matches what middleware expects.
AuthorizeUserByCodereturns*models.Userand fetches vias.users.GetByID(userID), which is consistent with howAuthorizeUserworks. However, earlier middleware (GetUserinuserauth.go) historically asserted amodels.Uservalue fromc.Locals, which caused a panic when a*models.Userwas stored for code-based auth.Please double-check that:
- The middleware storing the user from
AuthorizeUserByCodeandGetUser’s type assertionare now aligned on either
models.Useror*models.Userso the type is consistent across Basic and OTP flows. IfGetUserstill expects a value, consider switching this method to returnmodels.User(by value) as previously suggested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/sms-gateway/app.go
- internal/sms-gateway/otp/config.go
- api/mobile.http
- internal/sms-gateway/otp/errors.go
- configs/config.example.yml
- internal/sms-gateway/otp/service.go
- internal/sms-gateway/otp/domain.go
🧰 Additional context used
🧠 Learnings (2)
📚 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:
internal/sms-gateway/modules/auth/service.go
📚 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/modules/auth/module.go
🧬 Code graph analysis (5)
internal/config/module.go (2)
internal/config/config.go (2)
Config(10-21)OTP(90-93)internal/sms-gateway/otp/config.go (1)
Config(9-12)
internal/sms-gateway/otp/module.go (6)
internal/config/module.go (1)
Module(24-147)internal/sms-gateway/app.go (1)
Module(34-60)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/config/config.go (1)
Cache(81-83)internal/sms-gateway/otp/storage.go (1)
NewStorage(16-18)internal/sms-gateway/otp/service.go (1)
NewService(30-44)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(81-83)
internal/sms-gateway/modules/auth/service.go (3)
internal/sms-gateway/otp/service.go (1)
Service(14-20)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
⏰ 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 (9)
internal/sms-gateway/handlers/mobile.go (1)
181-192: LGTM! Proper context propagation.Passing
c.Context()toGenerateUserCodecorrectly threads the request context through to the OTP service layer, enabling proper cancellation and tracing support.internal/sms-gateway/otp/module.go (1)
9-22: Well-structured fx module with proper scoping.The module correctly:
- Uses
logger.WithNamedLogger("otp")for a module-scoped logger- Applies
fx.Privateto the cache factory result andNewStorage, preventing them from leaking into the global dependency graph- Exposes only
NewServicepublicly for consumption by other modules likeauthThis follows good fx module encapsulation patterns.
internal/sms-gateway/modules/auth/module.go (1)
10-31: LGTM! Clean module structure.The reordering of providers is cosmetic (fx resolves by type, not order). The
Newfunction now receives*otp.Serviceas a dependency (per the updated signature in service.go), which fx will inject from the newly addedotp.Module(). Based on learnings, thefx.Decoratepattern for the named logger correctly creates module-scoped decorated dependencies.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
64-84: LGTM! Context correctly threaded to authorization.Passing
c.Context()toAuthorizeUserByCodeproperly propagates the request context through the middleware to the OTP validation logic, enabling cancellation support and request-scoped tracing.internal/config/config.go (1)
132-135: Default OTP values properly initialized.The defaults align with the OTP struct definition and provide sensible out-of-the-box behavior.
internal/config/module.go (1)
14-14: OTP config wiring and type conversion look correct.The new
fx.Providecleanly mapsConfig.OTPtootp.Config, convertingTTLfrom seconds (uint16) totime.Durationand wideningRetries(uint8) toint. This matches existing time conversion patterns in this module; no issues from a correctness or DI perspective.Also applies to: 140-145
internal/sms-gateway/otp/storage.go (1)
10-13: String-oriented cache wrapper and cleanup behavior look good.Wrapping
cache.Cachefor string values viaSetOrFail/GetAndDeleteand delegatingCleanupwith additional error context is straightforward and consistent with existing cache semantics;Cleanup’s doc (“removes expired items”) matches the underlying behavior noted inpkg/cache/memory.go.Based on learnings,
Cleanupalready removes expired entries before any further processing.Also applies to: 20-41
internal/sms-gateway/modules/auth/service.go (2)
12-12: Constructor wiring ofotpSvcand other deps looks consistent.Adding
otpSvc *otp.ServicetoServiceand switchingNewto an explicit-parameter constructor keeps dependencies clear and matches how other services are injected;usersCacheinitialization is preserved. No functional issues from this change.Also applies to: 28-33, 35-56
58-66: GenerateUserCode now correctly delegates to OTP service with caller context.
GenerateUserCodeforwards the providedctxintos.otpSvc.Generateand wraps errors with context, eliminating the previouscontext.Background()issue and centralizing code generation in the OTP module. The signature and behavior look sound.
capcom6
force-pushed
the
otp/introduce-package
branch
2 times, most recently
from
d5148f6 to
d5337ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/otp/storage.go (1)
15-15: Update stale comment reference.The comment references "CodesCache" but the function is named
NewStorage.Apply this diff to fix the comment:
-// NewStorage creates a new CodesCache with the underlying cache. +// NewStorage creates a new Storage with the underlying cache.
🧹 Nitpick comments (1)
api/mobile.http (1)
12-13: Basic auth for/user/codelooks correct; optionally clarify the commented Bearer headerSwitching
/user/codetoAuthorization: Basic {{credentials}}aligns with an OTP-style “pre-token” flow, and keeping the old Bearer form as a comment is useful for reference. To avoid confusion later, you might add a short note to the commented Bearer line (e.g.,# ... (when already authenticated with a mobile token)) so it’s obvious in which scenario each header should be used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/config/module.go
- internal/sms-gateway/app.go
- internal/sms-gateway/otp/config.go
- configs/config.example.yml
- internal/config/config.go
- internal/sms-gateway/otp/service.go
- internal/sms-gateway/modules/auth/module.go
- internal/sms-gateway/otp/domain.go
🧰 Additional context used
🧠 Learnings (1)
📚 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:
internal/sms-gateway/modules/auth/service.go
🧬 Code graph analysis (5)
internal/sms-gateway/modules/auth/service.go (4)
internal/sms-gateway/otp/service.go (1)
Service(14-20)internal/config/config.go (1)
Config(10-20)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/otp/module.go (7)
internal/config/module.go (1)
Module(24-143)internal/sms-gateway/app.go (1)
Module(34-60)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/config/config.go (1)
Cache(76-78)internal/sms-gateway/modules/auth/service.go (1)
New(35-56)internal/sms-gateway/otp/storage.go (1)
NewStorage(16-18)internal/sms-gateway/otp/service.go (1)
NewService(30-44)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(76-78)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(18-24)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
⏰ 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 (11)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
53-53: LGTM: Clean refactor with proper context propagation.The introduction of the
SetUserhelper consolidates user storage logic, and passingc.Context()toAuthorizeUserByCodecorrectly propagates request context for cancellation and timeout support.Also applies to: 75-75, 80-80
86-88: LGTM: Helper function is type-safe and well-designed.The exported
SetUserfunction provides a consistent interface for storing user data in Fiber locals, maintaining type compatibility withGetUser.internal/sms-gateway/modules/auth/service.go (3)
35-56: LGTM: Constructor properly wired with OTP service dependency.The updated constructor cleanly integrates the OTP service dependency following dependency injection best practices.
58-66: LGTM: Context propagation correctly implemented.The method properly passes the caller's context to the OTP service, enabling cancellation, timeout, and tracing as intended.
148-160: LGTM: OTP-based authorization correctly implemented.The method properly validates codes via the OTP service with context propagation, retrieves the user, and maintains type consistency with the middleware layer.
internal/sms-gateway/handlers/mobile.go (1)
181-192: LGTM: Handler correctly propagates context to OTP service.The handler properly passes the Fiber context to
GenerateUserCode, ensuring request-scoped cancellation and timeout behavior.internal/sms-gateway/otp/module.go (1)
9-22: LGTM: Module wiring follows fx best practices.The OTP module correctly:
- Registers a scoped logger for observability
- Uses
fx.Privateto encapsulate cache/storage implementation details- Exposes only the
Servicefor dependency injectioninternal/sms-gateway/otp/errors.go (1)
5-10: LGTM: Error tokens follow Go conventions.The sentinel errors provide clear, package-level error types for configuration and initialization failures, enabling proper error handling with
errors.Is.internal/sms-gateway/otp/storage.go (3)
10-13: LGTM: Storage wrapper provides clean abstraction.The Storage struct appropriately encapsulates the cache interface for string-based operations.
20-27: LGTM: Method correctly wraps cache with error context.The
SetOrFailmethod properly converts string values to bytes and provides meaningful error wrapping.
29-33: LGTM: Cleanup and retrieval methods are well-implemented.Both
GetAndDeleteandCleanupcorrectly delegate to the underlying cache with appropriate type conversions and error handling.Also applies to: 35-41
capcom6
force-pushed
the
otp/introduce-package
branch
from
d5337ba to
6cae7c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/sms-gateway/otp/storage.go (1)
15-18: UpdateNewStoragedoc comment to match the type nameThe doc comment still mentions “CodesCache” while the function and type are named
NewStorage/Storage, which can be confusing and contradicts the earlier review guidance.Consider updating it to something like:
// NewStorage creates a new Storage with the underlying cache.
🧹 Nitpick comments (1)
internal/sms-gateway/otp/service.go (1)
70-77: Minor: modulo bias in code generation.With 3 bytes (24 bits = 16,777,216 values) and
num % 1,000,000, codes 0–777,215 have 17 chances each while 777,216–999,999 have only 16. This creates ~6% higher probability for lower codes.For OTP purposes this is likely acceptable, but if uniformity matters, consider rejection sampling or using 4 bytes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/sms-gateway/otp/config.go
- configs/config.example.yml
- internal/config/module.go
- internal/sms-gateway/otp/module.go
- api/mobile.http
🧰 Additional context used
🧠 Learnings (2)
📚 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/modules/auth/module.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:
internal/sms-gateway/modules/auth/service.go
🧬 Code graph analysis (5)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/modules/auth/service.go (3)
internal/sms-gateway/otp/service.go (1)
Service(14-20)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/otp/service.go (5)
internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(11-13)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(35-39)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(18-24)
⏰ 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 (13)
internal/sms-gateway/modules/auth/module.go (1)
13-18: Auth module wiring and logger decoration look solidThe scoped logger via
fx.Decorateand the ordering offx.Provide(newRepository, fx.Private)beforefx.Provide(New)are consistent with the established fx patterns and keep the auth module nicely self-contained. Based on learnings, this matches the recommended module-specific logger setup.internal/sms-gateway/otp/domain.go (1)
5-9: OTP Code domain model is clear and minimal
Codecleanly captures the value plus expiry (ValidUntil) and aligns with how it’s consumed by the OTP/auth services and handlers.internal/config/config.go (1)
19-20: OTP config struct and defaults are consistent with existing patternsThe new
OTPblock onConfig, its YAML/env tags, and the default values (TTL=300, Retries=3) follow the same conventions as other config sections and provide sensible defaults for the OTP module.Also applies to: 85-88, 122-125
internal/sms-gateway/otp/errors.go (1)
5-9: Error sentinel definitions are appropriate
ErrInvalidConfigandErrInitFailedgive clear, reusable markers for higher-level wrapping anderrors.Ischecks across the OTP config/service.internal/sms-gateway/app.go (1)
22-22: OTP module wiring into the app looks correctImporting
otpand registeringotp.Module()alongside the other fx modules keeps the OTP subsystem integrated with existing config/cache/logging without changing the server startup flow.Also applies to: 58-58
internal/sms-gateway/handlers/mobile.go (1)
181-191: The review's concern about a type mismatch is unfounded. In Fiber v2,c.Context()returnscontext.Context(not*fasthttp.RequestCtx), making it fully compatible withGenerateUserCode's parameter type. The code is correct as written.internal/sms-gateway/modules/auth/service.go (4)
35-56: Clean dependency injection pattern.The refactored constructor properly accepts all explicit dependencies, improving testability and making the dependency graph clearer. The initialization of
usersCacheinternally is appropriate since it's an implementation detail.
58-66: OTP integration looks correct.The delegation to
otpSvc.Generateproperly propagates the context (addressing the earlier feedback). The error wrapping provides good context for debugging.
147-160: Code-based authorization properly integrated.The method correctly delegates OTP validation to the dedicated service and retrieves the user by the validated ID. The context propagation and error wrapping are appropriate.
205-206: Simplified cleanup reflects architecture change.Since OTP code expiration is now managed internally by the OTP storage layer with TTL-based eviction, removing
codesCachecleanup here is correct.internal/sms-gateway/otp/service.go (3)
30-44: Good defensive initialization checks.The constructor properly validates the configuration and ensures required dependencies are non-nil before constructing the service.
95-104: LGTM!The validation method cleanly delegates to storage for atomic get-and-delete, ensuring one-time use semantics.
54-93: The edge case is already prevented by validation.The concern about
Retries = 0returning an empty code is not valid.Config.Validate()ininternal/sms-gateway/otp/config.go(lines 19–20) already enforces thatRetriesmust be greater than 0:if c.Retries <= 0 { return fmt.Errorf("%w: retries must be greater than 0", ErrInvalidConfig) }Since
NewService()callscfg.Validate()and returns an error if validation fails, an invalid config withRetries <= 0cannot proceed to theGenerate()method.Regarding the
err = nilassignment on line 31: While technically redundant (since a successfulSetOrFail()already returns nil), it's a stylistic choice and not a correctness issue.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/otp/service.go (1)
30-43: Unify NewService error wrapping for config vs storage/logger
cfg.Validate()errors are returned raw, while storage/logger nil errors are wrapped withErrInitFailed. If callers key offErrInitFailedto detect “service failed to initialize”, config issues will be treated differently.Consider wrapping config errors the same way:
func NewService(cfg Config, storage *Storage, logger *zap.Logger) (*Service, error) { - if err := cfg.Validate(); err != nil { - return nil, err - } + if err := cfg.Validate(); err != nil { + return nil, fmt.Errorf("%w: invalid config: %w", ErrInitFailed, err) + }This keeps all initialization failures discoverable via
errors.Is(err, ErrInitFailed). Based on relevant_code_snippets forErrInitFailedininternal/sms-gateway/otp/errors.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/otp/service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/otp/service.go (5)
internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(11-13)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(35-39)
⏰ 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: Test
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/otp/service.go (2)
96-98: Validate delegating to storage looks appropriateThis thin wrapper around
storage.GetAndDeletematches the documented behavior (lookup + delete) and keeps the OTP validity semantics centralized in the storage layer. No issues from my side here.
54-85: The code is valid;for rangeover integers is supported in Go 1.22+The project uses Go 1.24.3, which supports
for rangeover integer values as a valid language feature (introduced in Go 1.22). The code at line 62 will compile and run correctly.Additionally,
Config.Validate()already enforces thatRetries > 0, so the validation concern is already addressed—the loop will never run with zero or negative retries.The only remaining suggestion is the optional micro-optimization to hoist
big.NewInt(maxValue)outside the loop to avoid reallocation on each iteration.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
86-88: Optional: align comments with the actual locals key nameSince
setUseris now the single point writinglocalsUser("user") into the context, consider updating the NewBasic/NewCode doc comments that mention the key “LocalsUser” to either cite"user"directly or referencelocalsUserfor clarity and to avoid confusion.For example:
-// user in the request's Locals under the key LocalsUser. If the header is invalid, +// user in the request's Locals under the key "user". If the header is invalid,(and similarly for
NewCode).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(18-24)
⏰ 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). (1)
- GitHub Check: goreleaser
🔇 Additional comments (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
53-53: Good centralization of user locals handling viasetUserDelegating to
setUser(c, user)removes duplication and keeps the locals key usage in one place; this also nicely addresses the earlier concern about an exported helper by keeping the helper package‑private.
75-80: AllAuthorizeUserByCodecall sites already use the updated signature consistentlyThe codebase contains only one call site for
AuthorizeUserByCode(at line 75 in userauth.go), and it correctly passesc.Context()as the first argument, matching the function signature defined in./internal/sms-gateway/modules/auth/service.go:148. No inconsistencies or outdated call conventions exist.
capcom6
force-pushed
the
otp/introduce-package
branch
from
5b42514 to
3aff6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/otp/storage.go (1)
15-18: Fix stale comment reference.The comment still references "CodesCache" but the function is named "NewStorage".
Apply this diff:
-// NewStorage creates a new CodesCache with the underlying cache. +// NewStorage creates a new Storage with the underlying cache.
🧹 Nitpick comments (2)
api/mobile.http (1)
11-13: Clarify Basic vs Bearer for/user/codeexampleSwitching
/user/codetoAuthorization: Basic {{credentials}}and keeping the old Bearer form commented out makes sense for the new flow, but it’s a bit ambiguous for consumers of this file. If Bearer is now deprecated for this endpoint, consider either removing the commented line or annotating it (e.g.# Legacy Bearer example) so it’s clear which scheme is actually supported.internal/sms-gateway/otp/storage.go (1)
29-33: Consider consistent error wrapping.
SetOrFailandCleanupwrap errors with context, butGetAndDeletereturns the raw error. For consistency in error messages and debugging:// GetAndDelete retrieves and deletes a string value. func (s *Storage) GetAndDelete(ctx context.Context, key string) (string, error) { data, err := s.cache.GetAndDelete(ctx, key) + if err != nil { + return "", fmt.Errorf("failed to get and delete cache item: %w", err) + } - return string(data), err + return string(data), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- configs/config.example.yml
- internal/sms-gateway/otp/module.go
- internal/sms-gateway/otp/domain.go
- internal/config/module.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/modules/auth/module.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:
internal/sms-gateway/modules/auth/service.go
🧬 Code graph analysis (8)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(18-24)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(76-78)
internal/sms-gateway/modules/auth/module.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(35-56)
internal/sms-gateway/otp/config.go (3)
internal/worker/config/types.go (1)
Duration(11-11)internal/config/config.go (1)
Config(10-20)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
internal/sms-gateway/otp/service.go (7)
internal/config/config.go (1)
Config(10-20)internal/sms-gateway/modules/auth/service.go (2)
Config(17-20)Service(22-33)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(11-13)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(35-39)
internal/sms-gateway/modules/auth/service.go (3)
internal/sms-gateway/otp/service.go (1)
Service(14-20)internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/domain.go (1)
Code(6-9)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(24-143)internal/sms-gateway/otp/module.go (1)
Module(9-22)
⏰ 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: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
internal/sms-gateway/modules/auth/module.go (1)
16-17: LGTM!The reordering to list the private
newRepositoryprovider beforeNewimproves readability and reflects the dependency order. Based on learnings, thefx.Decoratepattern for module-scoped loggers is correct.internal/config/config.go (2)
85-88: LGTM!The
OTPstruct definition follows the existing configuration patterns in the codebase with appropriate YAML and envconfig tags. The choice ofuint16for TTL anduint8for Retries provides reasonable bounds.
122-125: Sensible defaults.TTL of 300 seconds (5 minutes) and 3 retries are reasonable defaults for OTP codes, balancing security and usability.
internal/sms-gateway/handlers/mobile.go (1)
181-186: LGTM!Proper context propagation from the Fiber request to the service layer. This enables cancellation and timeout handling throughout the OTP generation flow.
internal/sms-gateway/app.go (2)
22-22: LGTM!Import correctly added for the new OTP package.
58-58: LGTM!The OTP module is correctly wired into the application's dependency injection graph. The module's private providers (cache, storage) and public service will be properly scoped within the fx module.
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
53-53: LGTM! Clean refactor with thesetUserhelper.The extraction of
setUsereliminates duplication between the Basic and Code authentication paths. Making it package-private (lowercase) is appropriate since it's only used within this package.Also applies to: 80-80, 86-88
75-75: LGTM! Context propagation is correct.Passing
c.Context()toAuthorizeUserByCodeproperly propagates cancellation, timeouts, and tracing through the OTP validation flow.internal/sms-gateway/otp/errors.go (1)
1-10: LGTM! Standard error sentinel pattern.The error definitions follow Go conventions with clear doc comments and will integrate well with the config validation and service initialization flows.
internal/sms-gateway/otp/config.go (1)
8-24: LGTM! Config validation is sound.The
Validatemethod correctly enforces positive values for bothTTLandRetries, with properly wrapped error messages. Usingtime.Durationfor TTL provides better type safety than raw integers.internal/sms-gateway/modules/auth/service.go (3)
35-56: LGTM! Constructor explicitly wires dependencies.The updated
Newfunction signature makes dependencies explicit, which improves testability and aligns with dependency injection best practices. The transition from in-memory code handling to the OTP service is clean.
59-66: LGTM! Context propagation is correct.
GenerateUserCodenow properly uses the providedctxinstead ofcontext.Background(), ensuring cancellation and timeouts propagate through the OTP generation flow.
148-160: LGTM! OTP-based authorization flow is correct.The method properly delegates validation to
otpSvc.Validate(ctx, code)and retrieves the user by ID. Error handling and context propagation are correct.internal/sms-gateway/otp/service.go (3)
22-44: LGTM! Constructor validates dependencies thoroughly.The constructor properly validates the config and checks for nil dependencies before initializing the service. Error handling with
ErrInitFailedwrapping is appropriate.
54-86: LGTM! Code generation and retry logic are sound.The implementation correctly:
- Generates 6-digit codes (0-999999) using cryptographically secure randomness
- Computes
validUntilonce before retries for consistency (avoiding race conditions where different attempts might have different expiration times)- Retries on both random generation and storage failures
- Logs warnings without exposing codes in logs (good security practice)
88-98: LGTM! Validation delegates cleanly to storage.The
Validatemethod correctly delegates tostorage.GetAndDelete, which atomically retrieves and removes the code, preventing reuse.
capcom6
force-pushed
the
otp/introduce-package
branch
from
3aff6b6 to
218da98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
48-53: Fix user storage/retrieval mismatch in Locals (this currently breaks all userauth flows).
SetUsernow stores a*users.UserinLocals, butGetUserstill assertsusers.Userby value:
SetUser:c.Locals(localsUser, user)whereuseris*users.User.GetUser:c.Locals(localsUser).(users.User).This type mismatch means the assertion always fails, so
GetUserreturnsnil,HasUseris always false, andWithUserwill respond with 401 even whenSetUserwas called. Handlers likemobile.postDevicethat rely onuserauth.GetUser(c)will treat authenticated users as anonymous and create new accounts instead of attaching devices.Update
GetUserto work with the pointer stored bySetUserand (optionally) add a doc comment:-// NewBasic returns a middleware that optionally performs HTTP Basic authentication. +// NewBasic returns a middleware that optionally performs HTTP Basic authentication. @@ - user, err := usersSvc.Login(c.Context(), username, password) + user, err := usersSvc.Login(c.Context(), username, password) @@ - SetUser(c, user) + SetUser(c, user) @@ - user, err := authSvc.AuthorizeUserByCode(c.Context(), code) + user, err := authSvc.AuthorizeUserByCode(c.Context(), code) @@ -func SetUser(c *fiber.Ctx, user *users.User) { - c.Locals(localsUser, user) -} +// SetUser stores the given user in the request's Locals. +func SetUser(c *fiber.Ctx, user *users.User) { + c.Locals(localsUser, user) +} @@ -func GetUser(c *fiber.Ctx) *users.User { - user, ok := c.Locals(localsUser).(users.User) - if !ok { - return nil - } - - return &user -} +func GetUser(c *fiber.Ctx) *users.User { + user, ok := c.Locals(localsUser).(*users.User) + if !ok { + return nil + } + + return user +}You may also want to tweak the
GetUsercomment to say it returns a*users.Userdirectly, instead of a value re-wrapped as a pointer.Also applies to: 75-81, 86-108
🧹 Nitpick comments (2)
internal/sms-gateway/otp/storage.go (1)
10-41: Update NewStorage doc comment to reflect the actual type.The implementation is sound, but the comment on
NewStoragestill mentionsCodesCache, which looks like a leftover from refactoring and can confuse future readers.-// NewStorage creates a new CodesCache with the underlying cache. +// NewStorage creates a new Storage with the underlying cache.You may also consider returning an empty string when
GetAndDeletereturns an error (instead ofstring(data)), but current call sites appear to ignore the value on non-nil errors, so this is optional.internal/sms-gateway/modules/auth/service.go (1)
126-144:cleanis now a no-op whileRunstill schedules it hourly.
Runcontinues to tick and calls.clean(ctx)every hour, butcleanhas an empty body. Previously this hook was used to clean OTP-related cache entries; now there appears to be no cleanup wired for OTP storage.Consider either:
- Delegating to an OTP-level cleanup (e.g., exposing a
Cleanup/Runon the OTP service or wiringStorage.Cleanupsomewhere), or- Removing
Run/cleanfrom auth if cleanup is handled entirely inside the cache or OTP module.This avoids confusing, effectively dead scheduling code and ensures expired OTP entries are actually reclaimed. Based on learnings, cache.Cleanup is the intended place to evict expired items.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/mobile.http(1 hunks)configs/config.example.yml(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(1 hunks)internal/sms-gateway/modules/auth/service.go(4 hunks)internal/sms-gateway/modules/auth/types.go(0 hunks)internal/sms-gateway/otp/config.go(1 hunks)internal/sms-gateway/otp/domain.go(1 hunks)internal/sms-gateway/otp/errors.go(1 hunks)internal/sms-gateway/otp/module.go(1 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/sms-gateway/app.go
- internal/sms-gateway/otp/module.go
- configs/config.example.yml
- internal/config/module.go
- internal/sms-gateway/otp/domain.go
- internal/config/config.go
- api/mobile.http
🧰 Additional context used
🧠 Learnings (1)
📚 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:
internal/sms-gateway/modules/auth/service.go
🧬 Code graph analysis (6)
internal/sms-gateway/otp/errors.go (1)
internal/sms-gateway/modules/auth/service.go (1)
New(33-52)
internal/sms-gateway/otp/storage.go (1)
internal/config/config.go (1)
Cache(79-81)
internal/sms-gateway/otp/service.go (5)
internal/sms-gateway/otp/config.go (1)
Config(9-12)internal/sms-gateway/otp/storage.go (1)
Storage(11-13)internal/sms-gateway/otp/errors.go (1)
ErrInitFailed(9-9)internal/sms-gateway/otp/domain.go (1)
Code(6-9)pkg/cache/options.go (1)
WithValidUntil(70-74)
internal/sms-gateway/otp/config.go (2)
internal/config/config.go (1)
Config(12-23)internal/sms-gateway/otp/errors.go (1)
ErrInvalidConfig(7-7)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/users/domain.go (1)
User(5-10)
internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
SetUser(86-88)
⏰ 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 (9)
internal/sms-gateway/modules/auth/service.go (3)
22-52: otpSvc wiring and constructor changes look good.Injecting
*otp.ServiceintoServiceand threading it throughNewis consistent with how other dependencies are handled and keeps auth focused on orchestration rather than OTP implementation.
55-62: GenerateUserCode now properly delegates to otpSvc with context.Using
ctxins.otpSvc.Generate(ctx, userID)fixes the earlier context propagation issue and cleanly separates OTP generation from auth.
111-124: AuthorizeUserByCode integration with otpSvc is consistent.Delegating validation to
s.otpSvc.Validate(ctx, code)and then loading the user viausersSvc.GetByUsernameis coherent with how the rest of the module models user identity; error wrapping is also clear.internal/sms-gateway/handlers/mobile.go (1)
200-212: Context propagation to GenerateUserCode is correct.Passing
c.Context()intoGenerateUserCodealigns this handler with the new auth API and preserves cancellation/timeout semantics.internal/sms-gateway/handlers/middlewares/jwtauth/jwtauth.go (1)
29-39: JWT middleware now aligns with pointer-based SetUser.Using
userauth.SetUser(c, user)with*users.UserfromGetByUsernamecorrectly matches the updated SetUser signature; no further changes needed here.internal/sms-gateway/otp/errors.go (1)
1-10: Error sentinel definitions are clear and conventional.
ErrInvalidConfigandErrInitFailedare well-named, documented, and suitable for use witherrors.Isin callers; no changes needed.internal/sms-gateway/otp/config.go (1)
8-24: OTP Config and validation look solid.Using
time.Durationfor TTL and validating both TTL and Retries againstErrInvalidConfiggives clear, type-safe configuration errors; no adjustments needed.internal/sms-gateway/otp/service.go (2)
30-44: NewService initialization guardrails look correct.Config validation, plus explicit nil checks for storage and logger with
ErrInitFailed, provide clear, early failures for misconfiguration; this constructor looks good as is.
88-98: Validate is a thin, appropriate wrapper over storage.Delegating validation to
s.storage.GetAndDelete(ctx, code)matches the intended behavior of one-time codes and keeps logic centralized in the storage/cache layer.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
88-90: Add doc comment to exportedSetUserfunction.The exported
SetUserfunction still lacks a doc comment. As noted in a previous review, either add a doc comment (e.g.,// SetUser stores the user in the request's Locals.) or make it unexported by renaming tosetUser.
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
132-140: Consider updating handler signature to use pointer for consistency.The
WithUserdecorator's handler signature still expectsusers.Userby value (line 132), while the rest of the codebase now uses*users.Userpointers. Consider updating the signature tofunc(*users.User, *fiber.Ctx) errorand removing the dereference on line 139 for consistency with the pointer-based approach.This may be intentional to provide handlers with an immutable copy, but pointer consistency would align better with the current refactoring direction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/users/domain.go (1)
User(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). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
54-54: LGTM! Context-aware authentication and consistent pointer handling.The changes correctly:
- Pass request context to
AuthorizeUserByCode(line 77), enabling proper propagation of deadlines and cancellation signals- Use the returned
*users.Userpointers directly withSetUserAlso applies to: 77-77, 82-82
104-109: LGTM! Correct pointer type assertion and return.The logic correctly asserts to
*users.Userand returns the pointer directly, matching the updatedSetUsersignature that stores pointers in Locals.
capcom6
force-pushed
the
otp/introduce-package
branch
from
a0ea59b to
3c25b21
Compare
Summary by CodeRabbit
New Features
Configuration
Authentication
Changes
✏️ Tip: You can customize this high-level summary in your review settings.