Anonymous: Add configurable device limit #1
Conversation
* Anonymous: Add device limiter * break auth if limit reached * fix typo * refactored const to make it clearer with expiration * anon device limit for config --------- Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>
WalkthroughA device limit for anonymous users was introduced across multiple layers of the system. New configuration fields were added to backend and frontend config structures, device limit enforcement was implemented in the anonymous device database store, and related tests and service wiring were updated. Error handling and configuration reading logic were adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant AnonService
participant DB
User->>Frontend: Access anonymously
Frontend->>Backend: Request (with anonymousDeviceLimit in config)
Backend->>AnonService: Authenticate/TagDevice
AnonService->>DB: CreateOrUpdateDevice
alt Device limit not reached
DB-->>AnonService: Device created/updated
AnonService-->>Backend: Success
else Device limit reached
DB-->>AnonService: ErrDeviceLimitReached
AnonService-->>Backend: Error
end
Backend-->>Frontend: Response (with anonymousDeviceLimit)
Frontend-->>User: Rendered UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions 🔧 ESLint
yarn install v1.22.22 Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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/services/anonymous/anonimpl/anonstore/database.go (4)
18-18: Make error message more descriptiveThe error message could be more informative by including context about the anonymous device limit.
-var ErrDeviceLimitReached = fmt.Errorf("device limit reached") +var ErrDeviceLimitReached = fmt.Errorf("anonymous device limit reached")
52-54: Consider validating deviceLimit parameterThe constructor should validate that deviceLimit is not negative to prevent unexpected behavior.
func ProvideAnonDBStore(sqlStore db.DB, deviceLimit int64) *AnonDBStore { + if deviceLimit < 0 { + deviceLimit = 0 // Disable limit for negative values + } return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore"), deviceLimit: deviceLimit} }
108-119: Document device limit behavior and consider metricsWhen the device limit is reached, new anonymous devices cannot be created, which may impact legitimate users. Consider:
- Adding metrics to monitor when the limit is reached
- Documenting this behavior clearly for operators
// if device limit is reached, only update devices if s.deviceLimit > 0 { count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute)) if err != nil { return err } if count >= s.deviceLimit { + s.log.Warn("Anonymous device limit reached", "limit", s.deviceLimit, "count", count) return s.updateDevice(ctx, device) } }
109-113: Consider caching the device count for performanceThe
CountDevicesquery is executed on every device creation/update attempt when a limit is configured. For high-traffic instances, this could impact performance.Consider implementing a caching mechanism or using an approximate count to reduce database load, especially if this endpoint receives high traffic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/grafana-data/src/types/config.ts(1 hunks)packages/grafana-runtime/src/config.ts(1 hunks)pkg/api/dtos/frontend_settings.go(1 hunks)pkg/api/frontendsettings.go(1 hunks)pkg/services/anonymous/anonimpl/anonstore/database.go(3 hunks)pkg/services/anonymous/anonimpl/anonstore/database_test.go(2 hunks)pkg/services/anonymous/anonimpl/api/api.go(2 hunks)pkg/services/anonymous/anonimpl/client.go(2 hunks)pkg/services/anonymous/anonimpl/impl.go(4 hunks)pkg/services/anonymous/anonimpl/impl_test.go(2 hunks)pkg/setting/setting.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
pkg/services/anonymous/anonimpl/impl_test.go (4)
pkg/services/anonymous/anontest/fake.go (1)
FakeService(11-14)pkg/services/stats/statstest/stats.go (1)
FakeService(9-17)pkg/setting/setting.go (1)
NewCfg(985-997)pkg/services/org/orgtest/fake.go (1)
NewOrgServiceFake(25-27)
pkg/services/anonymous/anonimpl/client.go (2)
pkg/services/anonymous/service.go (1)
AnonDeviceUI(12-12)pkg/services/anonymous/anonimpl/anonstore/database.go (1)
ErrDeviceLimitReached(18-18)
pkg/api/frontendsettings.go (1)
pkg/setting/setting.go (1)
Cfg(149-578)
pkg/setting/setting.go (1)
pkg/login/social/connectors/common.go (1)
MustBool(178-198)
🔇 Additional comments (20)
packages/grafana-runtime/src/config.ts (1)
97-97: LGTM! Property addition is correctly implemented.The
anonymousDeviceLimitproperty is properly added to theGrafanaBootConfigclass with appropriate initialization and placement next to related anonymous configuration properties.pkg/services/anonymous/anonimpl/api/api.go (2)
18-18: Improved constant naming for better code clarity.The rename from
thirtyDaystoanonymousDeviceExpirationmakes the constant's purpose more explicit and self-documenting.
71-71: Consistent usage of the renamed constant.The updated reference to
anonymousDeviceExpirationmaintains functionality while using the improved naming.packages/grafana-data/src/types/config.ts (1)
200-200: Well-typed interface property addition.The
anonymousDeviceLimitproperty is correctly typed asnumber | undefinedto allow for optional configuration, and appropriately placed next to related anonymous settings.pkg/api/dtos/frontend_settings.go (1)
195-195: Correctly implemented DTO field addition.The
AnonymousDeviceLimitfield is properly added with appropriate int64 type and correct JSON tag following camelCase convention for frontend compatibility.pkg/api/frontendsettings.go (1)
198-198: Configuration properly propagated to frontend settings.The
AnonymousDeviceLimitfield assignment correctly transfers the backend configuration value to the frontend DTO, completing the configuration pipeline.pkg/services/anonymous/anonimpl/impl_test.go (3)
116-117: LGTM: Test correctly updated for new service construction pattern.The test properly adapts to the refactored
ProvideAnonymousDeviceServicefunction that now accepts a generic SQL store instead of a pre-constructed anonymous store.
124-124: LGTM: Test assertion correctly updated to access store through service.The change from
anonDBStore.ListDevicestoanonService.anonStore.ListDevicesproperly reflects the new service structure where the anonymous store is created internally.
151-152: LGTM: Consistent test pattern maintained.The second test function follows the same updated pattern as the first test, maintaining consistency in the test suite.
pkg/services/anonymous/anonimpl/anonstore/database_test.go (3)
16-16: LGTM: Test updated for new constructor signature.The addition of the device limit parameter (set to 0, likely meaning unlimited) maintains the existing test behavior while adapting to the new
ProvideAnonDBStoresignature.
51-70: LGTM: Excellent test coverage for device limit enforcement.This new test thoroughly validates the device limit functionality by:
- Setting a limit of 1 device
- Successfully creating the first device
- Verifying that creating a second device fails with
ErrDeviceLimitReachedThis provides essential coverage for the core feature being implemented.
74-74: LGTM: Consistent test pattern maintained.All existing tests are consistently updated to include the device limit parameter, maintaining uniformity across the test suite.
pkg/setting/setting.go (2)
375-375: LGTM: Appropriate field type and placement.The
AnonymousDeviceLimitfield is correctly typed asint64for handling device counts and is logically placed with other anonymous configuration fields.
1650-1655: LGTM: Clean refactoring with new configuration option.The refactoring to use a local
anonSectionvariable improves code readability and maintainability. The newAnonymousDeviceLimitconfiguration is properly read with a sensible default of 0 (likely unlimited).pkg/services/anonymous/anonimpl/client.go (2)
5-5: LGTM: Necessary imports for enhanced error handling.The
errorsandanonstoreimports are required for the new error comparison logic that specifically handles device limit exceeded scenarios.Also applies to: 11-11
44-50: LGTM: Proper synchronous error handling for device limits.The refactoring from asynchronous to synchronous
TagDevicecalling is appropriate for authentication flow. The specific handling ofErrDeviceLimitReachedcorrectly aborts authentication when the device limit is exceeded, while other errors are logged but don't prevent anonymous access - a sensible approach for non-critical device tagging functionality.pkg/services/anonymous/anonimpl/impl.go (4)
9-9: LGTM: Required import for new dependency injection pattern.The
dbimport is necessary for the updated service construction that accepts a generic SQL store interface.
36-43: LGTM: Improved service construction with device limit integration.The refactoring to accept a generic
db.DBparameter and internally create the anonymous store withcfg.AnonymousDeviceLimitis a clean design. This ensures the device limit configuration is properly propagated to the storage layer while simplifying dependency injection.
61-61: LGTM: Consistent usage of internally created store.The API initialization correctly uses the anonymous store from the service struct, maintaining consistency with the new construction pattern.
147-147: LGTM: Proper error propagation for device limit handling.Returning the error instead of just logging it allows callers to handle device limit scenarios appropriately, which is essential for the authentication flow to fail when limits are exceeded.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 1
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor