[mobile] add registration by one-time code #130
Conversation
WalkthroughThis pull request introduces new security definitions and functionality to support one-time code authentication alongside existing methods. Key changes include updates to the main application configuration, middleware, and mobile handler to facilitate user code generation and verification. Enhancements in the authentication module incorporate caching, context management, and periodic cleanup of expired codes. Additionally, the API documentation (Swagger and HTTP examples) and dependency versions have been updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Device
participant Router
participant MobileHandler
participant AuthService
Device->>Router: GET /user/code (with valid device credentials)
Router->>MobileHandler: Invoke getUserCode()
MobileHandler->>AuthService: GenerateUserCode(userID)
AuthService-->>MobileHandler: Return {code, validUntil} or error
MobileHandler-->>Device: JSON response with code details or error
sequenceDiagram
participant Client
participant Middleware
participant AuthService
Client->>Middleware: Request with "Authorization" header
Middleware->>Middleware: Check for header presence
alt "Authorization" starts with "Basic"
Middleware->>AuthService: basicAuth(Authorization)
AuthService-->>Middleware: Return user object or error
else "Authorization" starts with "Code"
Middleware->>AuthService: codeAuth(Authorization)
AuthService-->>Middleware: Return user object or error
end
Middleware-->>Client: Proceed with request or respond with unauthorized error
Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/swagger/docs/mobile.http (1)
17-18: Avoid hardcoded authentication values.The hardcoded authorization code "518876" in the example should be replaced with a variable like
{{userCode}}to follow the pattern of other environment variables in this file.# Authorization: Basic {{credentials}} -Authorization: Code 518876 +Authorization: Code {{userCode}}internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
17-48: Solid implementation of basic auth with proper error handling.The
basicAuthfunction correctly parses and validates Basic authentication headers, following RFC standards for base64 decoding and credential extraction. Error handling is comprehensive, returning appropriate unauthorized errors for various failure scenarios.However, consider adding logging of specific error details to aid in debugging, rather than converting all errors to the generic
fiber.ErrUnauthorized.- if err != nil { - return emptyUser, fiber.ErrUnauthorized - } + if err != nil { + // Log the specific error for debugging + return emptyUser, fiber.ErrUnauthorized + }
76-87: Clean fallback authentication mechanism.The middleware now elegantly cascades through authentication methods, first trying basic auth and then falling back to code auth. This approach supports both authentication methods without requiring client changes.
Consider adding a debug log when switching from basic auth to code auth to help with troubleshooting authentication issues.
if err != nil { + // Debug logging for auth method fallback + // logger.Debug("Basic auth failed, trying code auth", zap.Error(err)) user, err = codeAuth(auth, authSvc) }internal/sms-gateway/handlers/mobile.go (1)
202-223: Well-implemented endpoint for generating one-time codes.The
getUserCodemethod is appropriately documented with Swagger annotations and correctly implemented to generate and return a one-time code for device registration. The code properly handles errors from the auth service.Consider adding validation for the
device.UserIDto ensure it's not empty before callingGenerateUserCode.func (h *mobileHandler) getUserCode(device models.Device, c *fiber.Ctx) error { + // Validate user ID to prevent generating codes for invalid users + if device.UserID == "" { + return fiber.NewError(fiber.StatusBadRequest, "Invalid user ID") + } + code, err := h.authSvc.GenerateUserCode(device.UserID) if err != nil { return err } return c.JSON(smsgateway.MobileUserCodeResponse{ Code: code.Code, ValidUntil: code.ValidUntil, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/sms-gateway/main.go(1 hunks)go.mod(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/modules/auth/module.go(2 hunks)internal/sms-gateway/modules/auth/repository.go(1 hunks)internal/sms-gateway/modules/auth/service.go(5 hunks)internal/sms-gateway/modules/auth/types.go(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/swagger.json(4 hunks)pkg/swagger/docs/swagger.yaml(4 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
internal/sms-gateway/handlers/mobile.go (2)
internal/sms-gateway/models/models.go (1)
Device(31-42)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (1)
WithDevice(83-87)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/modules/auth/types.go (1)
AuthCode(15-18)
internal/sms-gateway/modules/auth/module.go (2)
internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)internal/sms-gateway/app.go (1)
Run(48-59)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/modules/auth/service.go (2)
Service(37-49)New(51-64)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (28)
internal/sms-gateway/modules/auth/repository.go (1)
18-23: Clean implementation of the GetByID method.The new method provides a clear way to fetch users by their ID, which complements the existing repository functionality. The implementation follows the pattern established by other repository methods in this file.
pkg/swagger/docs/mobile.http (1)
10-13: LGTM: New user code endpoint added.The new endpoint aligns with the objectives to support one-time code authentication.
go.mod (2)
11-11:❓ Verification inconclusive
Verify that the pre-release go-helpers library is stable.
The updated pre-release version of the go-helpers library includes a development timestamp that appears to be from March 2025. Ensure this is the correct version and that it's stable for production use.
🏁 Script executed:
#!/bin/bash # Check if this dependency is used in the codebase and which features are being used echo "Checking imports of the go-helpers package..." rg -l "github.com/capcom6/go-helpers" | sort echo -e "\nChecking specific imports related to code authentication..." rg "github.com/capcom6/go-helpers.*Code" --type goLength of output: 915
Action Required: Please Manually Verify the Stability of the Pre-release go-helpers Version
- The dependency line in
go.modnow points to a pre-release version with a development timestamp from March 2025:github.com/capcom6/go-helpers v0.1.2-0.20250326030929-5d1f34b4936b- A search across the codebase confirms that this dependency is referenced in multiple files.
- Our targeted search for imports related to code authentication did not return any results. This might be expected, but it prevents fully automated verification of all intended features.
- Next Step: Please manually review the release notes or stability assurances for this version of go-helpers to ensure it meets production standards.
9-9:❓ Verification inconclusive
Verify that the pre-release client library is stable.
The updated pre-release version of the client-go library includes a development timestamp that appears to be from March 2025. Ensure this is the correct version and that it's stable for production use.
🏁 Script executed:
#!/bin/bash # Check if this dependency is used in the codebase and which features are being used echo "Checking imports of the client-go package..." rg -l "github.com/android-sms-gateway/client-go" | sort echo -e "\nChecking specific imports related to code authentication..." rg "github.com/android-sms-gateway/client-go.*Code" --type goLength of output: 1099
Action: Verify Production Stability of Pre-Release Client-Go Library
The dependency
github.com/android-sms-gateway/client-gois imported in several parts of the codebase (e.g.,go.mod, various internal handlers and modules). Although the dependency is widely used, the pre-release version (v1.5.6-0.20250326025946-2625b20dcccd) includes a development timestamp from March 2025. Notably, our search for authentication-related usages (e.g., matching.*Code) returned no results, which might indicate that those features aren’t being exercised currently.Before merging, please manually verify that this pre-release version is indeed intended for production use and that it meets stability requirements. In particular, consider checking upstream release notes and running any available integration tests to confirm that no regressions or unintended behaviors have been introduced.
- Files to note:
go.mod(dependency declaration)- Several internal packages (e.g.,
internal/sms-gateway/handlers/*,internal/sms-gateway/modules/*)internal/sms-gateway/modules/auth/types.go (2)
3-6: Good TTL constant definition.The 5-minute time-to-live for authentication codes is a reasonable duration that balances security with user experience. This constant will help ensure consistent timeout behavior throughout the authentication system.
14-18: Well-designed AuthCode structure.The AuthCode struct is clear and properly designed with fields for both the code itself and its expiration time, facilitating the implementation of one-time code authentication.
cmd/sms-gateway/main.go (1)
10-14: New security definition looks good.The added
UserCodesecurity definition is well-structured and follows the same pattern as the existing security definitions. It properly defines the header-based authentication method for one-time code authentication.internal/sms-gateway/modules/auth/module.go (2)
4-5: Added context import looks good.The context import is necessary for creating and managing context with cancellation functionality.
17-29: Good implementation of lifecycle hooks for background tasks.The added code correctly implements proper lifecycle management for the auth service:
- Creates a cancellable context
- Starts the service's
Runmethod in a background goroutine during app startup- Properly cancels the context when the application is shutting down
This is the correct pattern for managing background tasks with clean shutdown capabilities in Go applications using the fx framework.
pkg/swagger/docs/swagger.yaml (4)
304-312: Good implementation of UserCodeResponse schema.The MobileUserCodeResponse schema is well defined with appropriate fields for the one-time code and its expiration time. The example values make the documentation clear and easy to understand.
860-861: Security definition correctly added to device registration endpoint.Adding the UserCode security option to the existing device registration endpoint allows for the new authentication method to be used alongside existing methods.
921-941: Well-defined endpoint for retrieving one-time codes.The new
/mobile/v1/user/codeendpoint is properly documented with:
- Appropriate HTTP method (GET)
- Clear description and summary
- Proper security requirement (MobileToken)
- Well-defined response schema
- Error handling
The endpoint is logically placed under the user operations and properly tagged.
1051-1055: Security definition correctly added to the swagger spec.The UserCode security definition properly mirrors the one defined in main.go with consistent naming and description.
pkg/swagger/docs/swagger.json (4)
570-572: UserCode security option correctly added to device registration endpoint.The security option is properly added to the JSON specification, allowing the new authentication method to be used for device registration.
770-802: Well-implemented endpoint for one-time codes.The
/mobile/v1/user/codeendpoint is properly defined in the JSON specification with all necessary details:
- Correct HTTP method
- Proper security requirements
- Clear description
- Appropriate response schema mapping
This implementation is consistent with the YAML specification.
1359-1371: Good implementation of response schema for user codes.The MobileUserCodeResponse schema is well-defined with appropriate fields for:
- The one-time code string
- Expiration timestamp
The example values enhance the documentation.
1537-1542: Security definition correctly added to JSON specification.The UserCode security definition is properly added to the JSON specification with consistent naming and description across all files.
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (3)
15-16: Good addition of a reusable empty user variable.Creating a package-level variable for an empty user model is a good approach to avoid repeated creation of empty structs when handling error cases.
50-64: Well-structured implementation of code-based authentication.The
codeAuthfunction provides a clean implementation for one-time code authentication following the same pattern asbasicAuth. The code correctly extracts the authorization code and delegates to the auth service for verification.
66-71: Clear and comprehensive documentation update.The updated function documentation properly describes both supported authentication methods, which helps developers understand the middleware's capabilities.
internal/sms-gateway/handlers/mobile.go (2)
63-64: Security annotation correctly updated.Adding the
UserCodesecurity annotation to thepostDevicemethod properly documents that this endpoint now supports code-based authentication as an alternative method.
287-288: Route registration is clean and follows existing patterns.The new route for getting user codes is properly registered with the appropriate middleware (
deviceauth.WithDevice), which ensures that only authenticated devices can request codes.internal/sms-gateway/modules/auth/service.go (6)
4-10: Required imports for new functionality.The addition of
contextandmath/randimports support the new functionality for code generation and cache cleanup.
41-42: Cache field addition for code storage.The
codesCachefield is appropriately added to theServicestruct for storing one-time authorization codes.
61-62: Cache initialization in constructor.The
codesCacheis correctly initialized in theNewfunction with default settings.
172-187: Good implementation of code-based authorization.The
AuthorizeUserByCodemethod correctly retrieves and deletes the code from the cache, then gets the user by ID. The function handles errors appropriately and includes helpful documentation comments.
218-237:❓ Verification inconclusive
Well-implemented periodic cleanup process.
The
Runmethod properly sets up a ticker for cleaning expired entries from both caches. It correctly handles context cancellation for graceful shutdown.Ensure that this method is called in a goroutine since it blocks until the context is canceled.
Run the following script to check if the
Runmethod is called in a goroutine:
🏁 Script executed:
#!/bin/bash # Check if the Run method is called in a goroutine rg -A 2 'authSvc.Run\(' --glob '*.go'Length of output: 39
I'll generate a modified shell script to search for calls to
authSvc.Runthat include thegokeyword. This should help verify whether the method is invoked in a goroutine.
🏁 Script executed:
#!/bin/bash # Check if authSvc.Run is called as a goroutine by looking for "go authSvc.Run(" rg -A 2 'go\s+authSvc\.Run\(' --glob '*.go'Length of output: 45
Action Required: Confirm Goroutine Invocation for
RunMethodThe periodic cleanup implementation looks solid. However, automated searches (using
rg) did not reveal any explicit call toauthSvc.Runprefixed with a goroutine (go authSvc.Run(...)). Please manually verify that theRunmethod is indeed invoked from a separate goroutine to prevent blocking the main thread.
66-86:✅ Verification successful
Code generation should use a cryptographically secure random number generator.
The current implementation uses
math/rand, which is not cryptographically secure and could lead to predictable codes. For security-critical features like authentication codes, you should usecrypto/randinstead.Additionally, consider adding validation to ensure
userIDis not empty.Apply this diff to improve security:
-import ( - "context" - "crypto/sha256" - "crypto/subtle" - "encoding/hex" - "fmt" - "math/rand" - "time" +import ( + "context" + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "fmt" + "time" // ... func (s *Service) GenerateUserCode(userID string) (AuthCode, error) { + if userID == "" { + return AuthCode{}, fmt.Errorf("user ID cannot be empty") + } + var code string var err error validUntil := time.Now().Add(codeTTL) for range 3 { - code = fmt.Sprintf("%06d", rand.Intn(1000000)) + // Use crypto/rand for secure random number generation + var randomNumber [4]byte + _, err = rand.Read(randomNumber[:]) + if err != nil { + return AuthCode{}, fmt.Errorf("failed to generate random number: %w", err) + } + + // Convert to a 6-digit number + randomInt := int(randomNumber[0]) | int(randomNumber[1])<<8 | int(randomNumber[2])<<16 | int(randomNumber[3])<<24 + randomInt = randomInt % 1000000 + code = fmt.Sprintf("%06d", randomInt) if err = s.codesCache.SetOrFail(code, userID, cache.WithValidUntil(validUntil)); err != nil { + // Only retry if the error is due to a duplicate key + if err == cache.ErrKeyExists { continue + } + return AuthCode{}, fmt.Errorf("can't store code: %w", err) } break } if err != nil { return AuthCode{}, fmt.Errorf("can't generate code: %w", err) } return AuthCode{Code: code, ValidUntil: validUntil}, nil }
🌐 Web query:
Is math/rand in Go cryptographically secure for generating authentication codes?💡 Result:
In Go, the security of
math/randdepends on the version and usage context:Go 1.22 and Later: Improved but Still Not Ideal
- ChaCha8Rand was introduced in Go 1.22 as the default generator for
math/rand, replacing the older, weaker algorithm. This new generator is cryptographically secure ([1][7][11]).- Accidental use of
math/randfor security-sensitive tasks (like authentication codes) now has a smaller blast radius compared to earlier versions, as the outputs are no longer trivially predictable ([1][7]).- While ChaCha8Rand is secure in theory,
math/randremains officially unsuited for security-sensitive work. The Go team still recommendscrypto/randfor critical operations ([5][8][11]).Key Recommendations
- Authentication Codes: Use
crypto/rand. It leverages OS-level entropy (e.g.,/dev/urandom) and undergoes rigorous security scrutiny, ensuring unpredictability even for secrets like tokens or keys ([3][6][7][11][14]).math/randUse Cases: Reserved for non-security tasks like simulations, shuffling data, or testing. Its documentation explicitly warns against security-sensitive use ([5][8][14]).Risks of Misusing
math/rand
- Prior to Go 1.22, using
math/randfor authentication codes was extremely insecure due to predictable seeds/sequences ([6][7][9]).- In 1.22+, even with ChaCha8Rand, issues arise if developers rely on default seeding or circumvent best practices. The package’s design does not prioritize secret safeguarding (e.g., lack of guaranteed entropy refresh) ([1][7][14]).
Example: Secure Code with
crypto/randpackage main import ( "crypto/rand" "encoding/base64" "fmt" ) func generateSecureToken(length int) string { token := make([]byte, length) _, err := rand.Read(token) if err != nil { panic(err) // Handle error properly in production } return base64.URLEncoding.EncodeToString(token) } func main() { fmt.Println("Secure authentication token:", generateSecureToken(32)) }Conclusion
While Go 1.22’s
math/randis stronger,crypto/randremains the gold standard for authentication codes. Adopt it for security-critical tasks to align with best practices and minimize risk. Always verify the intended use of randomness in your code to avoid accidental vulnerabilities.Citations:
- 1: https://go.dev/blog/chacha8rand
- 2: https://github.com/golang/go/blob/master/src/math/rand/rand.go
- 3: https://reliasoftware.com/blog/secure-random-number-generation-in-golang
- 4: https://github.com/andrew-d/csmrand
- 5: https://pkg.go.dev/math/rand/v2
- 6: https://docs.bearer.com/reference/rules/go_gosec_crypto_weak_random/
- 7: https://blog.devtrovert.com/p/go-ep1-avoid-using-mathrand-use-cryptorand
- 8: https://pkg.go.dev/math/rand
- 9: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/sast-policies/go-policies/sast-policy-248
- 10: https://help.fluidattacks.com/portal/en/kb/articles/criteria-fixes-go-034
- 11: https://github.com/core-go/auth/blob/main/random.go
- 12: https://pkg.go.dev/crypto/rand
- 13: https://labex.io/tutorials/go-how-to-use-math-rand-package-safely-437802
- 14: https://docs.datadoghq.com/security/code_security/static_analysis/static_analysis_rules/go-security/math-rand-insecure/
Security: Replaced math/rand with crypto/rand and added user ID validation
The updated code now meets our security requirements by:
- Replacing the insecure math/rand usage with crypto/rand for generating a 6-digit code.
- Ensuring that an empty userID is caught early and results in an error.
- Retrying only when a duplicate key is encountered, thus avoiding undue looping on unexpected errors.
These changes align with best practices since crypto/rand remains the recommended choice for security-critical operations, even though Go 1.22 improved math/rand. The diff snippet provided addresses the critical issue effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/swagger/docs/mobile.http (1)
17-18: Consider clarifying the authorization options with comments.All three authorization methods are commented out, which might be confusing for developers. Consider adding a comment explaining which authorization method is preferred or when each should be used.
-# Authorization: Basic {{credentials}} -# Authorization: Code 065379 +# Authorization methods (uncomment one): +# Authorization: Basic {{credentials}} # For username/password auth +# Authorization: Code 065379 # For one-time code auth
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/sms-gateway/handlers/3rdparty.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(4 hunks)pkg/swagger/docs/mobile.http(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
NewBasic(20-57)
internal/sms-gateway/handlers/mobile.go (3)
internal/sms-gateway/models/models.go (1)
Device(31-42)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
NewBasic(20-57)NewCode(64-84)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (1)
WithDevice(83-87)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (8)
pkg/swagger/docs/mobile.http (1)
10-13: Excellent implementation of the new user code endpoint.The new GET endpoint for retrieving one-time codes is properly defined with the correct authorization header using a Bearer token.
internal/sms-gateway/handlers/3rdparty.go (1)
50-50: Updated middleware to use explicit basic authentication.The change from
userauth.Newtouserauth.NewBasicmakes the authentication mechanism more explicit, which is a good practice.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
15-20: Clear documentation for the renamed authentication middleware.The previous generic
Newfunction has been renamed toNewBasicwith updated comments that clearly describe its purpose and behavior.
59-84: Excellent implementation of code-based authentication middleware.The new
NewCodemiddleware follows the same pattern asNewBasic, properly extracting and validating one-time codes. The implementation correctly:
- Checks for the "Code" authorization header prefix
- Extracts the code
- Authenticates the user via the auth service
- Stores the user in the context for downstream handlers
The middleware also includes comprehensive documentation that clearly explains its purpose and behavior.
internal/sms-gateway/handlers/mobile.go (4)
63-63: Added UserCode security annotation for Swagger documentation.The addition of the UserCode security scheme in the Swagger annotations is appropriate for the new authentication method.
202-223: Well-implemented handler for generating one-time codes.The new
getUserCodehandler correctly:
- Calls the auth service to generate a user code for the authenticated device's user
- Returns appropriate error responses if code generation fails
- Returns the code and its validity period in a properly structured JSON response
- Includes complete Swagger documentation
The handler follows the established pattern of the existing endpoints in this file.
258-260: Updated authentication middleware chain to support multiple auth methods.The changes correctly implement both basic authentication and code-based authentication for the device registration endpoint, allowing users to authenticate using either method.
288-288: Properly registered the new user code endpoint.The new endpoint is correctly registered with the appropriate middleware (
deviceauth.WithDevice) to ensure that only authenticated devices can access it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/mobile.go (1)
202-223: Well-implemented user code generation endpoint.The implementation of the
getUserCodefunction is clean and follows the established pattern in the codebase. It correctly uses the auth service to generate a code and returns a properly formatted response.Consider enhancing the error handling to provide more specific error responses rather than passing the error directly from the service layer.
func (h *mobileHandler) getUserCode(user models.User, c *fiber.Ctx) error { code, err := h.authSvc.GenerateUserCode(user.ID) if err != nil { - return err + h.Logger.Error("failed to generate user code", zap.Error(err), zap.String("userID", user.ID)) + return fiber.NewError(fiber.StatusInternalServerError, "Failed to generate user code") } return c.JSON(smsgateway.MobileUserCodeResponse{ Code: code.Code, ValidUntil: code.ValidUntil, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/sms-gateway/handlers/mobile.go(4 hunks)pkg/swagger/docs/swagger.json(4 hunks)pkg/swagger/docs/swagger.yaml(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/sms-gateway/handlers/mobile.go (2)
internal/sms-gateway/models/models.go (1)
User(23-29)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (4)
NewBasic(20-57)NewCode(64-84)UserRequired(105-113)WithUser(121-125)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (11)
pkg/swagger/docs/swagger.yaml (4)
304-312: Well-structured new definition for mobile user code response.The definition for
smsgateway.MobileUserCodeResponseis clear and concise, containing the essential properties for a one-time code: the code itself and its expiration time.
860-861: Good addition of UserCode as authentication option for device registration.Adding the UserCode security definition to the device registration endpoint provides an alternative authentication method, which aligns with the PR objective of supporting one-time code authentication.
921-942: New endpoint for one-time code generation is well defined.The new
/mobile/v1/user/codeendpoint is properly documented with appropriate tags, description, security requirements, and response schema.
1051-1055: Security definition for one-time code is properly configured.The UserCode security definition is correctly set up as an API key in the Authorization header, with a clear description of its purpose.
pkg/swagger/docs/swagger.json (4)
570-572: Consistent security definition in JSON documentation.The addition of UserCode as a security requirement for device registration matches the YAML configuration, ensuring consistency across documentation formats.
770-802: New endpoint definition is consistent with YAML configuration.The
/mobile/v1/user/codeendpoint definition matches its YAML counterpart, including all parameters, responses, and security requirements.
1359-1371: Response schema for user code is properly defined.The
smsgateway.MobileUserCodeResponseschema contains the same properties as defined in the YAML, with appropriate examples that help illustrate the expected response format.
1537-1542: Security definition is consistently defined across formats.The UserCode security definition in the JSON matches its YAML counterpart, ensuring proper API documentation across all formats.
internal/sms-gateway/handlers/mobile.go (3)
63-64: API documentation updated for new authentication method.The Swagger annotation now includes the UserCode security requirement, properly documenting the new authentication option for the device registration endpoint.
258-260: Middle chain properly supports multiple authentication methods.The middleware chain now includes both
userauth.NewBasicanduserauth.NewCode, allowing authentication via either username/password or one-time code, which aligns with the PR objective.
275-279: New route registration follows established patterns.The registration of the
/user/codeendpoint follows the same pattern as other routes, with appropriate middleware to ensure only authenticated users can access it. The use ofuserauth.WithUsercorrectly passes the authenticated user to the handler.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/sms-gateway/modules/auth/service.go (1)
61-61: Consider specifying a cache-wide TTL or capacity limit.
Although each code has its own expiry, it may be beneficial to configure the cache capacity or default TTL if usage grows to ensure memory management remains efficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/modules/auth/service.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Build / Docker image (linux/amd64)
🔇 Additional comments (4)
internal/sms-gateway/modules/auth/service.go (4)
4-4: No concerns.
This is just a new import of thecontextpackage, which looks appropriate for handling the added context usage.
41-41: Cache initialization looks good.
The introduction ofcodesCache *cache.Cache[string]can streamline one-time code lookups, though keep an eye on concurrency usage.
172-185: One-time code authorization logic looks good.
By retrieving and deleting the code in one step, you prevent reuse, which is a good practice. Consider adding a more descriptive error if the code does not exist in the cache.
216-235: Validate the cleanup interval.
Runningcleanevery hour might be suitable for most use cases, but consider using a more frequent interval if stale or expired entries need faster removal.Would you like me to generate a script that profiles memory usage to verify if an hourly cleanup is sufficient?
capcom6
force-pushed
the
feature/register-by-one-time-code
branch
from
4294675 to
70c771b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/modules/auth/service.go (2)
9-9:⚠️ Potential issueUse a cryptographically secure random source for auth codes.
math/randis not sufficiently secure for generating authentication codes. Consider switching tocrypto/randto prevent potential brute-force vulnerabilities.
66-86:⚠️ Potential issueGenerate user code with a cryptographically secure approach.
While the retry logic and TTL are appropriate,
rand.Intn(1000000)may generate predictable codes. Usecrypto/randto reduce the risk of code collision or brute-force attacks.To fix this, you should:
- Import crypto/rand instead of math/rand
- Use a secure method to generate random numbers
-import "math/rand" +import crand "crypto/rand" ... func (s *Service) GenerateUserCode(userID string) (AuthCode, error) { var code string var err error validUntil := time.Now().Add(codeTTL) for range 3 { - code = fmt.Sprintf("%06d", rand.Intn(1000000)) + // More secure random approach + b := make([]byte, 3) + if _, genErr := crand.Read(b); genErr != nil { + err = genErr + continue + } + num := (int(b[0]) << 16) | (int(b[1]) << 8) | int(b[2]) + code = fmt.Sprintf("%06d", num % 1000000) if err = s.codesCache.SetOrFail(code, userID, cache.WithValidUntil(validUntil)); err != nil { continue } break } ... }
🧹 Nitpick comments (2)
internal/sms-gateway/handlers/mobile.go (1)
294-294: Valid architectural observation about authentication.This comment correctly points out that password changes should ideally be protected by user authentication rather than device authentication. Consider implementing this change in a future update.
internal/sms-gateway/modules/auth/service.go (1)
61-62: Consider configurable TTL for code cache.While the initialization is good, consider making the TTL for the codes cache configurable, similar to the users cache. This would provide more flexibility in adjusting code expiration without code changes.
- codesCache: cache.New[string](cache.Config{}), + codesCache: cache.New[string](cache.Config{TTL: codeTTL}),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/sms-gateway/main.go(1 hunks)deployments/docker-swarm-terraform/main.tf(2 hunks)go.mod(1 hunks)internal/sms-gateway/handlers/3rdparty.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(5 hunks)internal/sms-gateway/modules/auth/module.go(2 hunks)internal/sms-gateway/modules/auth/repository.go(1 hunks)internal/sms-gateway/modules/auth/service.go(5 hunks)internal/sms-gateway/modules/auth/types.go(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/swagger.json(4 hunks)pkg/swagger/docs/swagger.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/sms-gateway/modules/auth/repository.go
- internal/sms-gateway/handlers/3rdparty.go
- cmd/sms-gateway/main.go
- pkg/swagger/docs/mobile.http
- go.mod
- internal/sms-gateway/modules/auth/types.go
- internal/sms-gateway/modules/auth/module.go
🧰 Additional context used
🧬 Code Definitions (3)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/modules/auth/types.go (1)
AuthCode(15-18)
internal/sms-gateway/handlers/mobile.go (2)
internal/sms-gateway/models/models.go (1)
User(23-29)internal/sms-gateway/handlers/middlewares/userauth/userauth.go (4)
NewBasic(20-57)NewCode(64-84)UserRequired(105-113)WithUser(121-125)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (19)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
15-57: Authentication middleware refactored to support Basic authThe
Newfunction has been appropriately renamed toNewBasicto clearly indicate its purpose for basic authentication. The implementation maintains the same functionality while making its purpose more explicit in the codebase.
59-84: Well-implemented Code authorization middlewareThe new
NewCodemiddleware provides a clean implementation for one-time code authentication. It follows the same pattern as the basic auth middleware, checking for a "Code" prefix in the Authorization header and then validating the provided code against the auth service.pkg/swagger/docs/swagger.yaml (4)
304-312: User code response model correctly definedThe
MobileUserCodeResponsemodel appropriately includes both the code itself and an expiration timestamp, which is important for security with one-time codes.
860-861: Device registration endpoint now supports code-based authAdding the
UserCodesecurity option to the device registration endpoint allows for registration using the one-time code, which completes the implementation of this authentication method.
921-942: New endpoint for generating one-time codesThe GET endpoint for
/mobile/v1/user/codeis properly defined with appropriate authorization requirements (ApiAuth), which ensures that only authenticated users can generate one-time codes.
1051-1055: Security definition for one-time codesThe
UserCodesecurity definition properly specifies that it's an API key sent in the Authorization header, maintaining consistency with other authentication methods in the API.pkg/swagger/docs/swagger.json (4)
570-572: Device registration endpoint now supports code-based auth in JSON specThe
UserCodesecurity option is correctly added to the device registration endpoint in the JSON Swagger specification, consistent with the YAML version.
770-802: New endpoint for one-time codes in JSON specThe endpoint for generating one-time codes is properly defined in the JSON Swagger specification, including all necessary parameters and response schemas.
1359-1371: User code response model in JSON specThe
MobileUserCodeResponsemodel is correctly defined in the JSON Swagger specification with both the code and expiration time.
1537-1542: Security definition for one-time codes in JSON specThe
UserCodesecurity definition is properly added to the JSON Swagger specification, maintaining consistency with the YAML version.deployments/docker-swarm-terraform/main.tf (2)
150-151: Improved organization with region commentGood practice to add a specific region comment for device registration, making the configuration more maintainable.
169-185: Rate-limited route for user operationsThe Traefik routing configuration properly applies rate limiting for the user operations endpoint, which is important for security when dealing with authentication features. The configuration correctly uses the same rate limit (5 requests per minute) as the device registration endpoint.
internal/sms-gateway/handlers/mobile.go (4)
63-63: Security enhancement with additional authorization method.The addition of
UserCodesecurity definition to the API documentation properly indicates that the endpoint now supports one-time code authentication alongside existing methods.
202-223: Well-structured implementation of one-time code generation endpoint.This new endpoint properly generates and returns a one-time code for device registration, including the code itself and its validity period. The structure is clean and follows the pattern of other API endpoints in the file.
258-260: Good enhancement to authentication chain.The authentication middleware chain now properly supports both Basic authentication and the new Code-based authentication, allowing for multiple authentication methods to be used for device registration.
275-279: Appropriate security for the new code generation endpoint.This new route is correctly protected with Basic authentication and requires a validated user, ensuring that only authenticated users can generate one-time codes.
internal/sms-gateway/modules/auth/service.go (3)
41-42: Good addition of cache for one-time codes.Adding a dedicated cache for one-time codes is a clean approach to manage their lifecycle separately from user authentication.
172-185: Well-implemented user authorization by code.The implementation correctly retrieves and deletes the code from cache in a single operation, preventing potential race conditions where a code could be used multiple times.
216-235: Good addition of periodic cache maintenance.The implementation of periodic cache cleanup is a good practice to prevent memory leaks from expired cache entries. The use of a context ensures that the cleanup can be properly canceled when the service shuts down.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/sms-gateway/modules/auth/service.go (2)
66-92: Code generation implementation looks secure.The implementation correctly uses crypto/rand for secure randomness and includes retry logic to handle potential collisions. The bit manipulation approach to generate a 6-digit code is effective but could benefit from a brief explanation.
Consider adding a brief comment explaining the bit manipulation logic:
num := (int(b[0]) << 16) | (int(b[1]) << 8) | int(b[2]) + // Converts 3 random bytes into a number and uses modulo to ensure it's a 6-digit code code = fmt.Sprintf("%06d", num%1000000)
238-241: Unused context parameter.The clean method accepts a context parameter but doesn't use it. This might confuse future developers.
-func (s *Service) clean(_ context.Context) { +func (s *Service) clean() { s.codesCache.Cleanup() s.usersCache.Cleanup() }And update the caller:
- s.clean(ctx) + s.clean()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/modules/auth/service.go(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/modules/auth/types.go (1)
AuthCode(15-18)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: lint
🔇 Additional comments (5)
internal/sms-gateway/modules/auth/service.go (5)
4-5: Good choice using crypto/rand for secure random generation.Switching from math/rand to crypto/rand is a great security improvement and properly addresses the previous review comments related to secure random generation.
41-41: LGTM: Cache implementation for one-time codes.The addition of the codesCache field and its initialization in the New function are well-implemented. Unlike usersCache which has a global TTL, the codesCache uses per-entry expiration through WithValidUntil, which is appropriate for one-time codes.
Also applies to: 61-61
178-191: User authorization by code is implemented correctly.The method retrieves and immediately deletes the code from cache, preventing reuse. The error handling is appropriate, providing clear feedback when the code is invalid or expired.
222-236: Periodic cleanup is a good maintenance practice.The Run method properly implements a periodic cleanup using a ticker and gracefully handles context cancellation. This prevents memory leaks from expired entries remaining in the cache.
71-72:✅ Verification successful
Verify if codeTTL is properly defined.
The code uses
codeTTLto set the validation time for the code, but this constant is not visible in the provided file.
🏁 Script executed:
#!/bin/bash # Check if codeTTL is defined in this file or imported from elsewhere rg "codeTTL\s*:?=" internal/sms-gateway/modules/auth/Length of output: 130
Constant codeTTL is properly defined
I verified that the constant
codeTTLis declared ininternal/sms-gateway/modules/auth/types.goas follows:const codeTTL = 5 * time.MinuteSince the definition is present and correctly imported for use in
internal/sms-gateway/modules/auth/service.go, there is no issue with its definition.
capcom6
force-pushed
the
feature/register-by-one-time-code
branch
from
c562dca to
aae4717
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/modules/auth/service.go (1)
71-78: Consider extracting code length as a constant.The code length (6 digits) is currently hardcoded in the format string. It might be better to extract this as a constant at the package level for easier maintenance if you ever need to change the code length.
+// codeLength defines the number of digits in the authorization code +const codeLength = 6 func (s *Service) GenerateUserCode(userID string) (AuthCode, error) { // ... num := (int(b[0]) << 16) | (int(b[1]) << 8) | int(b[2]) - code = fmt.Sprintf("%06d", num%1000000) + code = fmt.Sprintf("%0*d", codeLength, num%(10^codeLength)) // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/sms-gateway/main.go(1 hunks)deployments/docker-swarm-terraform/main.tf(2 hunks)go.mod(1 hunks)internal/sms-gateway/handlers/3rdparty.go(1 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(5 hunks)internal/sms-gateway/modules/auth/module.go(2 hunks)internal/sms-gateway/modules/auth/repository.go(1 hunks)internal/sms-gateway/modules/auth/service.go(5 hunks)internal/sms-gateway/modules/auth/types.go(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/swagger.json(4 hunks)pkg/swagger/docs/swagger.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/sms-gateway/handlers/3rdparty.go
- pkg/swagger/docs/mobile.http
- internal/sms-gateway/modules/auth/types.go
- internal/sms-gateway/modules/auth/module.go
- go.mod
- cmd/sms-gateway/main.go
- internal/sms-gateway/handlers/mobile.go
- pkg/swagger/docs/swagger.json
- internal/sms-gateway/modules/auth/repository.go
- deployments/docker-swarm-terraform/main.tf
🧰 Additional context used
🧬 Code Definitions (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/modules/auth/types.go (1)
AuthCode(15-18)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (11)
pkg/swagger/docs/swagger.yaml (4)
304-312: Well-structured definition for one-time code response.The
smsgateway.MobileUserCodeResponsedefinition provides a clear structure for the one-time code authentication mechanism with appropriate fields for the code itself and its expiration time.
921-941: Appropriate API endpoint implementation for retrieving one-time codes.The endpoint definition for
/mobile/v1/user/codeis well-designed, properly secured withApiAuth, and includes appropriate response status codes. This allows clients to retrieve a one-time code for device registration, enhancing the authentication options.
1051-1055: Good security definition for one-time code authentication.The
UserCodesecurity definition is consistent with other security definitions in the file and properly configures how one-time code authentication will be handled.
860-860: Correctly added UserCode security to device registration endpoint.Adding
UserCodeto the security options for the device registration endpoint ensures the new authentication method can be used alongside existing methods.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (2)
15-20: Improved naming clarity with function rename.Renaming from generic
Newto more specificNewBasicmakes the purpose of this middleware clearer, especially with the addition of alternative authentication methods.
59-84: Well-implemented code-based authentication middleware.The
NewCodemiddleware follows the same pattern asNewBasicwith appropriate modifications for code-based authentication. The error handling is consistent and the middleware properly stores the authenticated user in the request context.The code checks for the "Code" prefix in the authorization header, extracts the code, and verifies it using the authentication service.
internal/sms-gateway/modules/auth/service.go (5)
41-41: Well-structured cache field for authorization codes.Adding the
codesCachefield to theServicestruct provides a good mechanism for storing and retrieving temporary authorization codes.
61-62: Proper cache initialization.The initialization of the
codesCachewithout a TTL works well here since the TTL is managed per entry when generating codes.
66-92: Secure implementation of code generation.The
GenerateUserCodefunction:
- Uses cryptographically secure random number generation via
crypto/rand- Has a proper retry mechanism to handle collisions
- Sets appropriate expiration time for the codes
- Returns both the code and its expiration time
This is a secure implementation that follows good practices for one-time code generation.
178-191: Efficient code verification implementation.The
AuthorizeUserByCodemethod properly retrieves and immediately deletes the code from the cache to ensure one-time use, then retrieves the associated user. The error handling is appropriate.
222-241: Good implementation of cache maintenance.The
Runandcleanmethods implement a regular cleanup mechanism for the caches, which helps prevent memory leaks from expired entries. The implementation properly handles context cancellation for clean shutdown.
Summary by CodeRabbit