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 <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a configurable device limit feature for anonymous access in Grafana. The changes add a new configuration option anonymousDeviceLimit that allows administrators to control the maximum number of anonymous devices that can be tracked simultaneously.
The implementation spans both frontend and backend components:
Frontend Changes:
- Adds
anonymousDeviceLimitproperty to theGrafanaConfiginterface in TypeScript definitions - Exposes the configuration through the boot config system to make it available to client-side code
Backend Changes:
- Adds
AnonymousDeviceLimitfield to the main configuration struct, loaded from theauth.anonymous.device_limitINI setting with a default of 0 (no limit) - Exposes the limit to the frontend via the
FrontendSettingsDTOAPI response - Modifies the anonymous device store to enforce limits by checking device count before allowing new device creation
- When the limit is reached, the system falls back to update-only mode for existing devices and returns
ErrDeviceLimitReachedfor new device attempts - Refactors the authentication client to handle device limit errors synchronously, causing authentication to fail immediately when limits are exceeded
- Updates the dependency injection pattern to pass the device limit configuration properly to the database store
The feature integrates with Grafana's existing anonymous device tracking system, which maintains device records with 30-day expiration. The limit is enforced at the database level and prevents resource exhaustion from unlimited anonymous device registrations while allowing existing users to continue accessing the system.
Confidence score: 2/5
- This PR has significant implementation issues that could cause production problems, particularly race conditions in device counting logic
- The score is low due to potential race conditions in the device limit enforcement and inconsistent time window calculations in the database layer
- Files that need more attention:
pkg/services/anonymous/anonimpl/anonstore/database.go(race condition in device counting),pkg/services/anonymous/anonimpl/client.go(synchronous authentication changes), and the overall architectural changes inpkg/services/anonymous/anonimpl/impl.go
11 files reviewed, 6 comments
| } | ||
|
|
||
| devices, err := anonDBStore.ListDevices(context.Background(), nil, nil) | ||
| devices, err := anonService.anonStore.ListDevices(context.Background(), nil, nil) |
There was a problem hiding this comment.
style: accessing internal field anonStore - consider exposing a public method on the service instead of accessing private fields in tests
| theme2: GrafanaTheme2; | ||
| featureToggles: FeatureToggles = {}; | ||
| anonymousEnabled = false; | ||
| anonymousDeviceLimit = undefined; |
There was a problem hiding this comment.
style: Consider using a more specific type like number | undefined instead of just undefined to match the backend type definition
| // 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 { | ||
| return s.updateDevice(ctx, device) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: Race condition: Multiple concurrent requests could pass the device count check simultaneously and create devices beyond the limit. Consider using a database transaction or lock.
| args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID, | ||
| device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute), | ||
| } |
There was a problem hiding this comment.
logic: Time window calculation inconsistency: Using device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration) as the lower bound but device.UpdatedAt as the current time may not match the intended logic. Consider using time.Now().UTC() consistently.
| if rowsAffected == 0 { | ||
| return ErrDeviceLimitReached | ||
| } |
There was a problem hiding this comment.
logic: Misleading error: Returning ErrDeviceLimitReached when no rows are affected in updateDevice is confusing since the device might not exist rather than limit being reached.
|
|
||
| // 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)) |
There was a problem hiding this comment.
logic: Inconsistent time windows: The count query uses time.Now().UTC().Add(time.Minute) as upper bound, but updateDevice uses device.UpdatedAt.UTC().Add(time.Minute). This could cause timing issues.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
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! |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
2 participants
Test 1