Add support for multiple devices for user #117
Conversation
|
@coderabbitai review |
WalkthroughThe changes replace legacy authentication middleware with a new integrated user authentication mechanism. Several HTTP handlers have been updated to remove the explicit basic or auth middleware, instead using the new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as userauth Middleware
participant AuthSvc as auth Service
participant Handler as Route Handler
Client->>Middleware: Send request with Authorization header
Middleware->>Middleware: Validate header, decode credentials
Middleware->>AuthSvc: Authorize credentials
AuthSvc-->>Middleware: Return user data or error
alt Successful Authentication
Middleware->>Handler: Pass request with user context
Handler-->>Client: Return response
else Authentication Fails
Middleware-->>Client: Return unauthorized response
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Nitpick comments (2)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
20-58: Consider adding rate limiting to the authentication middleware.While the basic authentication implementation is solid, consider adding rate limiting to prevent brute force attacks.
func New(authSvc *auth.Service) fiber.Handler { + limiter := rate.NewLimiter(rate.Every(1*time.Minute), 5) // 5 attempts per minute return func(c *fiber.Ctx) error { + if !limiter.Allow() { + return fiber.ErrTooManyRequests + } // Rest of the code... } }internal/sms-gateway/handlers/mobile.go (1)
225-240: Improve readability of authorization flow.The authorization flow could be more explicit with better comments and a cleaner structure.
Consider this implementation:
router.Post("/device", limiter.New(), userauth.New(h.authSvc), keyauth.New(keyauth.Config{ Next: func(c *fiber.Ctx) bool { - // skip server key authorization... - return h.authSvc.IsPublic() || // ...if public mode - userauth.HasUser(c) // ...if registration with existing user + // Skip server key authorization in the following cases: + // 1. Public mode is enabled - allowing open registration + // 2. User is already authenticated - allowing device registration for existing users + return h.authSvc.IsPublic() || userauth.HasUser(c) }, Validator: func(c *fiber.Ctx, token string) (bool, error) { err := h.authSvc.AuthorizeRegistration(token) return err == nil, err }, }), h.postDevice, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(2 hunks)internal/sms-gateway/handlers/logs/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(2 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(2 hunks)internal/sms-gateway/modules/auth/decorators.go(0 hunks)pkg/swagger/docs/local.http(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/requests.http(4 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/auth/decorators.go
✅ Files skipped from review due to trivial changes (1)
- pkg/swagger/docs/local.http
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (11)
pkg/swagger/docs/mobile.http (3)
12-13: Authentication method changed from Bearer to Basic.The change from Bearer token to Basic auth is a significant authentication change. Ensure all clients are updated and the transition period is properly managed.
Consider:
- Adding a deprecation notice for Bearer token authentication
- Implementing a transition period where both auth methods are supported
- Adding API versioning to manage breaking changes
18-18: Verify empty push token initialization.The push token is now initialized as an empty string. This might affect push notification functionality.
Is this intentional? If yes, please ensure:
- The server properly handles empty push tokens
- There's validation logic to require push tokens when needed
- Documentation reflects when push tokens are required vs optional
1-1: Verify API endpoint changes.The base URL has been updated and a new credentials variable has been added. Please ensure that:
- All client applications are updated to use the new base URL
- The
CLOUD__CREDENTIALSenvironment variable is properly documentedAlso applies to: 4-4
internal/sms-gateway/handlers/logs/3rdparty.go (1)
5-5: LGTM! Authentication middleware updated.The change from
authtouserauthmiddleware is consistent with the broader authentication refactoring.Ensure the new middleware maintains the same security level:
Also applies to: 43-43
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if userauth middleware implements all security checks from the old auth middleware ast-grep --pattern 'func WithUser($user) { $$$ }'Length of output: 53
Let's run another script to locate and inspect the definition of the
WithUserfunction in the newuserauthmiddleware. We'll search for its declaration and view some context to help assess if the security checks match the previous implementation.
🏁 Script executed:
#!/bin/bash # Search for the 'func WithUser' declaration within the userauth middleware directory with context. rg -n -A 15 -B 5 'func WithUser' internal/sms-gateway/handlers/middlewares/userauthLength of output: 1145
Authentication middleware security check confirmed.
- The
WithUserfunction ininternal/sms-gateway/handlers/middlewares/userauth/userauth.gocorrectly wraps the handler by retrieving the user (viaGetUser(c)) and panicking if the user is missing, which aligns with the security behavior of the previous middleware.- The change is applied consistently, including in the context of
internal/sms-gateway/handlers/logs/3rdparty.go(line 43).internal/sms-gateway/handlers/devices/3rdparty.go (1)
8-8: LGTM! Authentication middleware updated consistently.The change to use
userauthmiddleware is consistent with the application-wide authentication refactoring.Also applies to: 65-65
internal/sms-gateway/handlers/3rdparty.go (2)
54-54: Address TODO comments for deprecated endpoints.There are two TODO comments indicating endpoint deprecation dates:
/messageto be removed after 2025-12-31/deviceto be removed after 2025-07-11Consider:
- Adding deprecation notices in API responses
- Documenting the migration path to new endpoints
- Setting up monitoring for usage of deprecated endpoints
Also applies to: 57-57
49-52: Verify the authentication chain.The authentication middleware chain has been simplified and centralized. This is a good architectural change, but requires careful verification.
Please verify:
- The order of middleware execution is correct (
userauth.NewbeforeUserRequired)- All routes that previously had custom auth requirements are properly handled
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining auth middleware usage that might conflict rg -A 2 "basicauth.New|auth.WithUser"Length of output: 1298
Authentication chain verified: Order and route adjustments are correct.
- The middleware chain in
internal/sms-gateway/handlers/3rdparty.gois correctly ordered withuserauth.New(h.authSvc)executed beforeuserauth.UserRequired().- Searches confirm that no legacy middleware (e.g.,
basicauth.Neworauth.WithUserfrom the old implementation) is remaining. The endpoints in the other handler files (messages, webhooks, devices, logs) useuserauth.WithUser, which aligns with the new centralized authentication approach.pkg/swagger/docs/requests.http (1)
10-10: LGTM! API path standardization looks good.The removal of the
/apiprefix from the URLs aligns with API path standardization best practices, making the endpoints more concise and maintainable.Also applies to: 13-13, 28-28, 44-44, 59-59
internal/sms-gateway/handlers/webhooks/3rdparty.go (1)
107-109: LGTM! Middleware migration looks good.The replacement of the auth middleware with userauth middleware is consistent with the new authentication strategy.
internal/sms-gateway/handlers/messages/3rdparty.go (1)
166-169: LGTM! Middleware migration is consistent.The replacement of auth middleware with userauth middleware maintains consistency across the codebase.
internal/sms-gateway/handlers/mobile.go (1)
11-11: LGTM!The addition of the
userauthpackage import is appropriate for implementing the new user authentication functionality.
| func GetUser(c *fiber.Ctx) models.User { | ||
| return c.Locals(LocalsUser).(models.User) | ||
| } |
There was a problem hiding this comment.
Add error handling for type assertion.
The type assertion in GetUser could panic. Consider adding error handling.
func GetUser(c *fiber.Ctx) models.User {
- return c.Locals(LocalsUser).(models.User)
+ user, ok := c.Locals(LocalsUser).(models.User)
+ if !ok {
+ panic("user not found in context or invalid type")
+ }
+ return user
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetUser(c *fiber.Ctx) models.User { | |
| return c.Locals(LocalsUser).(models.User) | |
| } | |
| func GetUser(c *fiber.Ctx) models.User { | |
| user, ok := c.Locals(LocalsUser).(models.User) | |
| if !ok { | |
| panic("user not found in context or invalid type") | |
| } | |
| return user | |
| } |
| return c.Status(fiber.StatusCreated). | ||
| JSON(smsgateway.MobileRegisterResponse{ | ||
| Id: device.ID, | ||
| Token: device.AuthToken, | ||
| Login: login, | ||
| Password: password, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return appropriate response based on user context.
The response should be adjusted based on whether the user is new or existing.
Consider this implementation:
- return c.Status(fiber.StatusCreated).
- JSON(smsgateway.MobileRegisterResponse{
- Id: device.ID,
- Token: device.AuthToken,
- Login: login,
- Password: password,
- })
+ response := smsgateway.MobileRegisterResponse{
+ Id: device.ID,
+ Token: device.AuthToken,
+ }
+
+ if !userauth.HasUser(c) {
+ response.Login = credentials.login
+ response.Password = credentials.password
+ }
+
+ return c.Status(fiber.StatusCreated).JSON(response)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return c.Status(fiber.StatusCreated). | |
| JSON(smsgateway.MobileRegisterResponse{ | |
| Id: device.ID, | |
| Token: device.AuthToken, | |
| Login: login, | |
| Password: password, | |
| }) | |
| return c.Status(fiber.StatusCreated).JSON(func() smsgateway.MobileRegisterResponse { | |
| response := smsgateway.MobileRegisterResponse{ | |
| Id: device.ID, | |
| Token: device.AuthToken, | |
| } | |
| if !userauth.HasUser(c) { | |
| response.Login = credentials.login | |
| response.Password = credentials.password | |
| } | |
| return response | |
| }()) |
| return c.Status(fiber.StatusCreated). | |
| JSON(smsgateway.MobileRegisterResponse{ | |
| Id: device.ID, | |
| Token: device.AuthToken, | |
| Login: login, | |
| Password: password, | |
| }) | |
| response := smsgateway.MobileRegisterResponse{ | |
| Id: device.ID, | |
| Token: device.AuthToken, | |
| } | |
| if !userauth.HasUser(c) { | |
| response.Login = credentials.login | |
| response.Password = credentials.password | |
| } | |
| return c.Status(fiber.StatusCreated).JSON(response) |
| func (h *mobileHandler) postDevice(c *fiber.Ctx) (err error) { | ||
| req := smsgateway.MobileRegisterRequest{} | ||
|
|
||
| if err := h.BodyParserValidator(c, &req); err != nil { | ||
| if err = h.BodyParserValidator(c, &req); err != nil { | ||
| return fiber.NewError(fiber.StatusBadRequest, err.Error()) | ||
| } | ||
|
|
||
| id := h.idGen() | ||
| login := strings.ToUpper(id[:6]) | ||
| password := strings.ToLower(id[7:]) | ||
| var ( | ||
| user models.User | ||
| login string | ||
| password string | ||
| ) | ||
|
|
||
| if userauth.HasUser(c) { | ||
| user = userauth.GetUser(c) | ||
| } else { | ||
| id := h.idGen() | ||
| login = strings.ToUpper(id[:6]) | ||
| password = strings.ToLower(id[7:]) | ||
|
|
||
| user, err := h.authSvc.RegisterUser(login, password) | ||
| if err != nil { | ||
| return fmt.Errorf("can't create user: %w", err) | ||
| user, err = h.authSvc.RegisterUser(login, password) | ||
| if err != nil { | ||
| return fmt.Errorf("can't create user: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential nil return of credentials for existing users.
When a user already exists (userauth.HasUser(c)), the login and password variables remain empty but are still returned in the response. This could lead to confusion or errors on the client side.
Consider this implementation:
func (h *mobileHandler) postDevice(c *fiber.Ctx) (err error) {
req := smsgateway.MobileRegisterRequest{}
if err = h.BodyParserValidator(c, &req); err != nil {
return fiber.NewError(fiber.StatusBadRequest, err.Error())
}
- var (
- user models.User
- login string
- password string
- )
+ var user models.User
+ var credentials struct {
+ login string
+ password string
+ }
if userauth.HasUser(c) {
user = userauth.GetUser(c)
} else {
id := h.idGen()
- login = strings.ToUpper(id[:6])
- password = strings.ToLower(id[7:])
+ credentials.login = strings.ToUpper(id[:6])
+ credentials.password = strings.ToLower(id[7:])
- user, err = h.authSvc.RegisterUser(login, password)
+ user, err = h.authSvc.RegisterUser(credentials.login, credentials.password)
if err != nil {
return fmt.Errorf("can't create user: %w", err)
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/mobile.go (1)
88-99: Fix credential handling for existing users.The current implementation still returns empty login and password fields for existing users, which could lead to confusion on the client side.
🧹 Nitpick comments (7)
internal/sms-gateway/handlers/converters/devices.go (1)
9-18: LGTM! Consider adding documentation.The changes improve type safety by using value types and handle nil names cleanly with
anys.OrDefault. Consider adding a function comment to document the behavior.+// DeviceToDTO converts a models.Device to smsgateway.Device. +// If device.Name is nil, it returns an empty string. func DeviceToDTO(device models.Device) smsgateway.Device {internal/sms-gateway/handlers/converters/devices_test.go (1)
19-48: LGTM! Consider adding test case for nil name.The test cases are well-structured and cover basic scenarios. Consider adding a test case for a device with an explicit nil name to verify
anys.OrDefaultbehavior.tests := []struct { name string device models.Device expected smsgateway.Device }{ + { + name: "device with nil name", + device: models.Device{ + ID: "test-id", + Name: nil, + }, + expected: smsgateway.Device{ + ID: "test-id", + Name: "", + }, + },internal/sms-gateway/modules/devices/service.go (1)
60-66: Consider adding error validation for empty token.While the implementation is correct, it would be beneficial to validate the token parameter.
func (s *Service) GetByToken(token string) (models.Device, error) { + if token == "" { + return models.Device{}, fmt.Errorf("token cannot be empty") + } return s.devices.Get(WithToken(token)) }internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (2)
20-44: Consider adding rate limiting for failed authentication attempts.While the authentication logic is solid, adding rate limiting would enhance security against brute force attacks.
+const ( + maxFailedAttempts = 5 + blockDuration = 15 * time.Minute +) + func New(authSvc *auth.Service) fiber.Handler { + failedAttempts := make(map[string]int) + lastFailedTime := make(map[string]time.Time) + var mu sync.RWMutex + return func(c *fiber.Ctx) error { // Get authorization header auth := c.Get(fiber.HeaderAuthorization) + ip := c.IP() + + mu.RLock() + if time.Since(lastFailedTime[ip]) < blockDuration && failedAttempts[ip] >= maxFailedAttempts { + mu.RUnlock() + return fiber.NewError(fiber.StatusTooManyRequests, "Too many failed attempts") + } + mu.RUnlock() if len(auth) <= 7 || !strings.EqualFold(auth[:7], "Bearer ") { return c.Next() }
49-61: Consider using type assertion in HasDevice.The current implementation might panic if the value exists but isn't a Device.
func HasDevice(c *fiber.Ctx) bool { - return c.Locals(LocalsDevice) != nil + device, ok := c.Locals(LocalsDevice).(models.Device) + return ok && device != models.Device{} }internal/sms-gateway/modules/webhooks/service.go (2)
67-92: Consider adding webhook URL validation.While the implementation is solid, it would be beneficial to validate the webhook URL format and potentially check for common security issues.
func (s *Service) Replace(userID string, webhook smsgateway.Webhook) error { if !smsgateway.IsValidWebhookEvent(webhook.Event) { return newValidationError("event", string(webhook.Event), fmt.Errorf("enum value expected")) } + if _, err := url.Parse(webhook.URL); err != nil { + return newValidationError("url", webhook.URL, fmt.Errorf("invalid URL format")) + } + + // Check for common security issues + if strings.HasPrefix(webhook.URL, "file://") || strings.HasPrefix(webhook.URL, "ftp://") { + return newValidationError("url", webhook.URL, fmt.Errorf("unsupported URL scheme")) + }
107-128: Consider adding retry mechanism for push notifications.The notifyDevices method could benefit from a retry mechanism for failed push notifications.
+const ( + maxRetries = 3 + retryDelay = time.Second * 5 +) + func (s *Service) notifyDevices(userID string) { s.logger.Info("Notifying devices", zap.String("user_id", userID)) devices, err := s.devicesSvc.Select(userID) if err != nil { s.logger.Error("Failed to select devices", zap.String("user_id", userID), zap.Error(err)) return } for _, device := range devices { if device.PushToken == nil { continue } + var lastErr error + for i := 0; i < maxRetries; i++ { + if i > 0 { + time.Sleep(retryDelay) + } if err := s.pushSvc.Enqueue(*device.PushToken, push.NewWebhooksUpdatedEvent()); err != nil { - s.logger.Error("Failed to send push notification", zap.String("user_id", userID), zap.Error(err)) + lastErr = err + continue } + lastErr = nil + break } + if lastErr != nil { + s.logger.Error("Failed to send push notification after retries", + zap.String("user_id", userID), + zap.Error(lastErr)) + } }
📜 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 (32)
cmd/sms-gateway/main.go(1 hunks)go.mod(1 hunks)internal/sms-gateway/handlers/converters/devices.go(1 hunks)internal/sms-gateway/handlers/converters/devices_test.go(2 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(2 hunks)internal/sms-gateway/handlers/health.go(1 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(6 hunks)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go(1 hunks)internal/sms-gateway/handlers/mobile.go(5 hunks)internal/sms-gateway/handlers/upstream.go(2 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(3 hunks)internal/sms-gateway/handlers/webhooks/mobile.go(2 hunks)internal/sms-gateway/modules/auth/decorators.go(0 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/devices/errors.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(1 hunks)internal/sms-gateway/modules/devices/service.go(2 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/push/service.go(1 hunks)internal/sms-gateway/modules/webhooks/service.go(4 hunks)pkg/filters/phone.go(0 hunks)pkg/maps/map_values.go(0 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/requests.http(4 hunks)pkg/swagger/docs/swagger.json(5 hunks)pkg/swagger/docs/swagger.yaml(6 hunks)pkg/types/cache/cache.go(0 hunks)pkg/types/cache/cache_test.go(0 hunks)pkg/types/cache/config.go(0 hunks)pkg/types/cache/errors.go(0 hunks)pkg/types/types.go(0 hunks)pkg/types/types_test.go(0 hunks)
💤 Files with no reviewable changes (9)
- internal/sms-gateway/modules/auth/decorators.go
- pkg/types/cache/config.go
- pkg/types/cache/errors.go
- pkg/types/types.go
- pkg/types/types_test.go
- pkg/maps/map_values.go
- pkg/filters/phone.go
- pkg/types/cache/cache_test.go
- pkg/types/cache/cache.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/swagger/docs/mobile.http
- internal/sms-gateway/handlers/webhooks/3rdparty.go
- internal/sms-gateway/handlers/messages/3rdparty.go
- pkg/swagger/docs/requests.http
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
internal/sms-gateway/handlers/health.go (1)
8-8: Verify the external package dependency.The change introduces an external dependency
github.com/capcom6/go-helpers/mapsreplacing the internal maps package. Please ensure:
- The external package is stable, well-maintained, and has compatible behavior
- The
MapValuesfunction signature and behavior match the previous implementation- The package is secure and trustworthy
Let's verify the package:
Additionally, please verify:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify the external package # Check if the package exists and get its details go list -m -json github.com/capcom6/go-helpers@latest # Check for any known vulnerabilities govulncheck github.com/capcom6/go-helpers/maps # Verify the MapValues function signature ast-grep --pattern 'func MapValues($_, $_, $_)'Length of output: 621
🌐 Web query:
What is the latest version and status of github.com/capcom6/go-helpers?💡 Result:
Based on the provided search results, there is no direct mention or reference to a GitHub repository named github.com/capcom6/go-helpers. The search results focus on other repositories from the same developer (capcom6), such as:
- android-sms-gateway (latest version v1.27.2)
- android-sms-gateway-py (latest version v1.1.0)
- nginx-controller
- sms-gateway/server
The only "helpers" library mentioned is kiprotect/go-helpers ([6]), which provides utilities for form validation and settings management but belongs to a different organization (kiprotect).
To summarize:
- No evidence exists in the provided search results for a
capcom6/go-helpersrepository.- The developer’s public repositories focus on SMS gateway tools and infrastructure controllers rather than general-purpose Go helper libraries.
For accurate information about
capcom6/go-helpers, you would need to verify its existence directly on GitHub or through updated documentation not included in these search results.Citations:
- 1: https://github.com/seerge/g-helper/releases/
- 2: Building Your Own Independent SMS Gateway capcom6/android-sms-gateway#71
- 3: https://github.com/capcom6/android-sms-gateway-py/releases
- 4: https://github.com/capcom6/android-sms-gateway/releases
- 5: https://pkg.go.dev/github.com/capcom6/sms-gateway/pkg/smsgateway
- 6: https://github.com/kiprotect/go-helpers
- 7: Question. Energy saving by app capcom6/android-sms-gateway#17
- 8: https://github.com/capcom6/android-sms-gateway
- 9: https://github.com/capcom6/nginx-controller
- 10: https://github.com/android-sms-gateway/server
External Package Verification: Manual Follow-Up Required
- The module is available as version v0.1.1 per the local module cache.
- The external package's repository could not be conclusively identified through public search, raising concerns about its maintenance and transparency.
- The output for verifying the
MapValuessignature was inconclusive, and the vulnerability check was not executed due to tool limitations.Please confirm the source, maintenance status, and security reviews of
github.com/capcom6/go-helpersbefore proceeding with the merge.internal/sms-gateway/modules/devices/errors.go (1)
5-7: LGTM! Error definition follows Go conventions.The error variable is well-defined and appropriately scoped for handling invalid user scenarios in the devices module.
cmd/sms-gateway/main.go (1)
15-18: Verify Authorization header handling for different auth types.Both
MobileTokenandServerKeyuse the sameAuthorizationheader name. This could lead to confusion in request handling. Consider:
- Using different header names for different auth types
- Documenting how the server distinguishes between them
internal/sms-gateway/handlers/webhooks/mobile.go (1)
7-7: LGTM! Clean middleware transition.The change from
authtodeviceauthmiddleware aligns with the PR objective of supporting multiple devices. The API documentation remains accurate and up-to-date.Also applies to: 50-50
internal/sms-gateway/modules/devices/repository.go (1)
62-69: LGTM! Well-structured device removal implementation.The new
Removemethod follows the repository's established patterns with proper filter validation. This addition supports the multi-device functionality by enabling device removal.Please ensure test coverage for the new
Removemethod, particularly:
- Invalid filter case
- Valid filter with no matching records
- Valid filter with matching records
internal/sms-gateway/modules/push/service.go (1)
9-10: Verify the stability of the external cache package.The switch from internal to external cache package (
github.com/capcom6/go-helpers/cache) should be validated.❌ Verification inconclusive
🌐 Web query:
What is the latest version and stability status of github.com/capcom6/go-helpers/cache?💡 Result:
The GitHub repository
github.com/capcom6/go-helpers/cachedoes not appear in the provided search results[1-8]. The search results focus on:
Unrelated Go cache libraries:
eko/gocache: A comprehensive Go caching library supporting multiple backends like Redis, Memcache, and in-memory stores[1]pkg6/go-cache: A Go in-memory key:value store/cache similar to Python's dictionaries[4]Android SMS Gateway projects:
capcom6/android-sms-gateway: An SMS gateway application for Android (latest release v1.31.0)[3][5]- Server/client components for SMS gateway infrastructure[7][8]
No information exists in these results about any
capcom6/go-helpersrepository or its cache component. This suggests either:
- The repository is private/unreleased
- The path might contain a typo
- The project isn't documented in public sources matching this query
For authoritative information about this specific package, you would need to consult its official GitHub repository or package documentation directly.
Citations:
- 1: https://github.com/eko/gocache
- 2: https://gbatemp.net/threads/retroarch-wiiu-wip.447670/page-624
- 3: https://github.com/capcom6/android-sms-gateway/releases
- 4: https://pkg.go.dev/github.com/pkg6/go-cache
- 5: https://github.com/capcom6/android-sms-gateway
- 6: https://pkg.go.dev/github.com/android-sms-gateway/server/pkg/types/cache
- 7: https://github.com/android-sms-gateway/server
- 8: https://github.com/android-sms-gateway/client-py/releases
Cache Package Stability Verification Inconclusive
The web search returned no public version or stability details forgithub.com/capcom6/go-helpers/cache. This suggests the package might be private, underdocumented, or a potential typo. Please verify stability via internal tests, documentation, or by contacting the maintainers directly.internal/sms-gateway/handlers/upstream.go (1)
9-9: Verify the stability of the external anys package.The switch from internal types to external package (
github.com/capcom6/go-helpers/anys) should be validated.Also applies to: 72-72
❌ Verification inconclusive
🌐 Web query:
What is the latest version and stability status of github.com/capcom6/go-helpers/anys?💡 Result:
The specific GitHub repository
github.com/capcom6/go-helpers/anysis not mentioned in any of the provided search results[1][2][3][4][5][6][7][8][9][10]. None of the sources reference this exact Go package or provide information about its versioning or stability status.For software projects not covered in these results, you would typically:
- Check the repository's GitHub page directly for release tags and commit history
- Review its documentation for stability guarantees
- Look for CI/CD status badges indicating test/build success
- Verify dependency management files like
go.modWithout access to real-time GitHub data through these search results, I cannot provide current details about this specific package's status. You may want to verify the repository URL or share additional context from the project's documentation if available.
Citations:
- 1: https://github.com/ansys/ansys-tools-protoc-helper
- 2: https://www.cde.ca.gov/ds/ad/srinfo.asp
- 3: https://www.githubstatus.com
- 4: https://www.youtube.com/watch?v=oKUcvlOoFXY
- 5: https://www.hhs.nd.gov/applyforhelp/ndrh-housing-stability
- 6: https://innovationspace.ansys.com/forum/forums/topic/link-to-download-online-help-and-release-notes/
- 7: https://www.gopherguides.com/golang-fundamentals-book/07-testing/helpers
- 8: https://www.youtube.com/watch?v=4jlXWEYAQGk
- 9: https://www.ema.europa.eu/en/documents/scientific-guideline/ich-q-1-r2-stability-testing-new-drug-substances-and-products-step-5_en.pdf
- 10: https://www.ansys.com/products/release-highlights
Revalidate the external
anyspackage for stabilityThe web search did not reveal any published details (e.g., release tags, documentation, or CI/CD badges) for the package
github.com/capcom6/go-helpers/anys. Without this external confirmation, its stability cannot be independently confirmed. I recommend manually verifying the package by checking its GitHub repository for release history, commit logs, and related stability indicators (e.g., CI/CD status) to ensure that it meets your project’s requirements.internal/sms-gateway/modules/devices/service.go (4)
35-41: LGTM! The Insert method has been simplified.The change from accepting a
models.Userto justuserID stringfollows good design principles by:
- Reducing coupling with the User model
- Following interface segregation by only requiring the necessary data
43-48: LGTM! Well-documented method with clear user isolation.The Select method properly ensures user isolation by appending the userID filter.
50-58: LGTM! Comprehensive documentation and proper error handling.The Get method is well-documented with clear error cases and ensures proper user isolation.
76-82: LGTM! Proper user isolation in Remove method.The Remove method correctly ensures user isolation by appending the userID filter.
internal/sms-gateway/handlers/devices/3rdparty.go (3)
43-52: LGTM! Clean implementation with proper error handling.The get handler:
- Uses proper error wrapping
- Correctly maps devices to DTOs
- Ensures user isolation through userID
54-76: LGTM! Well-documented DELETE endpoint with proper status codes.The remove handler:
- Has comprehensive API documentation
- Returns appropriate status codes (204 for success)
- Properly handles errors
- Ensures user isolation
78-81: LGTM! Proper middleware usage.The router registration correctly uses userauth middleware for both endpoints.
internal/sms-gateway/modules/webhooks/service.go (1)
49-58: LGTM! Good separation of concerns.The split into _select and Select methods:
- Improves code organization
- Ensures user isolation
- Maintains backward compatibility
Also applies to: 59-66
internal/sms-gateway/modules/auth/service.go (2)
13-13: LGTM!The import statement has been updated to use the new
go-helpers/cachepackage, which aligns with the dependency updates in go.mod.
87-87: LGTM!The change improves type safety by explicitly passing the user ID instead of the entire user object, which aligns with the PR objective of supporting multiple devices per user.
internal/sms-gateway/handlers/mobile.go (3)
11-12: LGTM!The new middleware imports align with the PR objective of replacing legacy auth with a new integrated user authentication mechanism.
231-247: LGTM!The route registration has been improved with proper authentication middleware:
userauth.Newensures user authentication.- The server key authorization is skipped for public mode or authenticated users.
249-262: LGTM!The device routes are properly secured with the new device authentication middleware.
internal/sms-gateway/modules/messages/service.go (1)
15-15: LGTM!The changes consistently replace the
typespackage with the newanyspackage fromgo-helpers, maintaining the same functionality.Also applies to: 122-122, 209-209, 218-218
go.mod (1)
7-9: Verify compatibility with the updated dependencies.The significant version updates could introduce breaking changes:
android-sms-gateway/client-go: v1.2.1 -> v1.5.1capcom6/go-helpers: v0.0.0 -> v0.1.1✅ Verification successful
🌐 Web query:
What are the breaking changes in android-sms-gateway/client-go v1.5.1 and capcom6/go-helpers v0.1.1?💡 Result:
The provided search results do not contain specific information about breaking changes in android-sms-gateway/client-go v1.5.1 or capcom6/go-helpers v0.1.1. Here's what we can determine from the available sources:
For
android-sms-gateway/client-go
- The GitHub repository (source 1) provides general documentation but no explicit release notes for version 1.5.1.
- Key features include SMS sending via
Sendmethod and message state checks viaGetState, but no breaking changes are mentioned in the visible documentation.For
capcom6/go-helpers
- No references to this library appear in any of the provided search results.
- The
capcom6/android-sms-gatewayrepository (source 4) focuses on Android app updates rather than Go helper libraries.
Recommendation: Check GitHub release notes directly for these repositories or consult their official changelogs for details about specific versions like 1.5.1 and 0.1.1, as they aren’t documented in the provided sources[1][4].
Citations:
- 1: https://github.com/android-sms-gateway/client-go
- 2: https://github.com/android-sms-gateway/client-py/releases
- 3: https://www.twilio.com/docs/conversations/android/changelog
- 4: https://github.com/capcom6/android-sms-gateway/releases
- 5: https://developers.liveperson.com/mobile-app-messaging-sdk-for-android-release-notes-5-1-1.html
- 6: https://developers.google.com/maps/documentation/navigation/android-sdk/release-notes
- 7: https://developer.android.com/about/versions/10/behavior-changes-all
- 8: https://gist.github.com/luca020400/a9580ecac531da8748c9
- 9: https://developers.google.com/android/guides/releases?authuser=3
- 10: https://firebase.google.com/support/release-notes/android
Compatibility Check: No Documented Breaking Changes Found
Based on our web query for release notes and official documentation, there is no evidence of documented breaking changes for either
android-sms-gateway/client-go v1.5.1orcapcom6/go-helpers v0.1.1. While it's always recommended to run integration tests and verify behavior within your application, the current available sources do not indicate any compatibility issues.pkg/swagger/docs/swagger.yaml (5)
453-487: New DELETE Device Endpoint.
A new DELETE endpoint for/3rdparty/v1/devices/{id}has been introduced. The endpoint correctly defines the required path parameter (device ID), a comprehensive set of responses (including 204, 400, 401, 404, and 500), and is secured withApiAuth. Please ensure that the implementation in the handler aligns with these documented responses.
279-286: Clarified MobileRegisterResponse Fields.
TheMobileRegisterResponsedefinition now explicitly states in the descriptions for theloginandpasswordfields that these will be empty if the registration corresponds to an existing user. This clarification improves the API’s self-documentation.
591-592: Updated Enqueue Message Description.
The description for thePOST /3rdparty/v1/messagesendpoint now specifies that if multiple devices are registered, the message will be sent via a randomly selected device. Verify that the backend logic reflects this randomized behavior.
815-847: Enhanced Mobile Device Registration Endpoint.
The POST endpoint at/mobile/v1/devicenow requires bothApiAuthandServerKeysecurity tokens and clarifies that user credentials are returned only for new user registrations. This dual-authentication setup is pivotal for the new multi-device support functionality. Please double-check that the corresponding handler enforces these security requirements and behavior as expected.
1013-1017: New ServerKey Security Definition.
A new security definition,ServerKey, has been added to thesecurityDefinitionssection. It specifies that a private server API key is expected in the header (using the same header name as before forAuthorization). Ensure that this additional security layer is fully integrated into your authentication flow and that clients are informed of this requirement.pkg/swagger/docs/swagger.json (5)
63-117: New DELETE Device Endpoint in JSON.
The JSON specification now reflects the new DELETE endpoint at/3rdparty/v1/devices/{id}with all of its parameters, responses, and security settings. This update is consistent with the YAML documentation.
267-273: Updated Enqueue Message Description in JSON.
ThePOST /3rdparty/v1/messagesendpoint now documents that when there are multiple registered devices, a message will be dispatched using a random device. Please verify this description accurately mirrors the endpoint’s functionality.
563-626: Enhanced Mobile Device Registration Endpoint in JSON.
The JSON definition for the POST/mobile/v1/deviceendpoint now requires bothApiAuthandServerKeyfor authentication. In addition, the description has been updated to indicate that user credentials are returned only for new registrations. Confirm that the backend implementation supports this dual security check.
1277-1303: Clarified MobileRegisterResponse Fields in JSON.
The modifications within theMobileRegisterResponseschema, particularly for theloginandpasswordfields, now clearly state that these fields will be empty for existing users. This aligns with the documented API behavior in the YAML file.
1479-1484: New ServerKey Security Definition in JSON.
TheServerKeysecurity definition has been added to the JSON spec undersecurityDefinitions, mirroring the changes in the YAML file. Ensure that all endpoints requiring private server authentication reference this key where appropriate.
| // WithDevice is a decorator that provides the current device to the handler. | ||
| // It assumes that the device is stored in the Locals under the key LocalsDevice. | ||
| // If the device is not present, it will panic. | ||
| // | ||
| // It is a convenience function that wraps the call to GetDevice and calls the | ||
| // handler with the device as the first argument. | ||
| func WithDevice(handler func(models.Device, *fiber.Ctx) error) fiber.Handler { | ||
| return func(c *fiber.Ctx) error { | ||
| return handler(GetDevice(c), c) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add panic recovery to WithDevice handler.
Since the function can panic, it should include recovery middleware.
func WithDevice(handler func(models.Device, *fiber.Ctx) error) fiber.Handler {
return func(c *fiber.Ctx) error {
+ defer func() {
+ if r := recover(); r != nil {
+ c.Status(fiber.StatusInternalServerError)
+ _ = c.JSON(fiber.Map{"error": "Internal server error"})
+ }
+ }()
return handler(GetDevice(c), c)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WithDevice is a decorator that provides the current device to the handler. | |
| // It assumes that the device is stored in the Locals under the key LocalsDevice. | |
| // If the device is not present, it will panic. | |
| // | |
| // It is a convenience function that wraps the call to GetDevice and calls the | |
| // handler with the device as the first argument. | |
| func WithDevice(handler func(models.Device, *fiber.Ctx) error) fiber.Handler { | |
| return func(c *fiber.Ctx) error { | |
| return handler(GetDevice(c), c) | |
| } | |
| } | |
| // WithDevice is a decorator that provides the current device to the handler. | |
| // It assumes that the device is stored in the Locals under the key LocalsDevice. | |
| // If the device is not present, it will panic. | |
| // | |
| // It is a convenience function that wraps the call to GetDevice and calls the | |
| // handler with the device as the first argument. | |
| func WithDevice(handler func(models.Device, *fiber.Ctx) error) fiber.Handler { | |
| return func(c *fiber.Ctx) error { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| c.Status(fiber.StatusInternalServerError) | |
| _ = c.JSON(fiber.Map{"error": "Internal server error"}) | |
| } | |
| }() | |
| return handler(GetDevice(c), c) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/converters/devices_test.go (1)
24-58: Consider adding more test cases for multiple devices support.The test cases cover basic scenarios well, but given that this PR adds support for multiple devices, consider adding test cases for:
- Devices with duplicate names
- Maximum allowed devices per user
- Device name length limits
- Special characters in device names
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sms-gateway/handlers/converters/devices_test.go(3 hunks)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/handlers/converters/devices_test.go (3)
31-34: LGTM! Clean and clear test data setup.The test data setup is clear and demonstrates good practices by using explicit field names and the new
anys.AsPointerhelper.
21-22: Verify consistency with DeviceToDTO function signature.The test struct fields have been changed from pointer types to value types. This is a good simplification, but let's verify that it matches the actual
DeviceToDTOfunction signature.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check the DeviceToDTO function signature in the implementation file ast-grep --pattern 'func DeviceToDTO($_) $_'Length of output: 1151
DeviceToDTO signature consistency confirmed.
The ast-grep results show that the function accepts a value of typemodels.Deviceand returns a value of typesmsgateway.Device, matching the modifications in the test struct from pointer types to value types.
10-10: Verify the external dependency.The code introduces a new external dependency
github.com/capcom6/go-helpers/anys. Please ensure this dependency is properly vendored and its version is pinned in the go.mod file.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the dependency is properly vendored and its version is pinned grep -r "github.com/capcom6/go-helpers" . | grep "go.mod"Length of output: 433
Dependency Pinned and Vended Correctly
- The external dependency
github.com/capcom6/go-helpers/anysis pinned to version v0.1.1 in thego.modfile.- Corresponding integrity checks in the
go.sumfile confirm that the dependency is properly vendored.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation