Anonymous: Add configurable device limit - #10
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>
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable device limit for anonymous authentication to prevent unlimited device registration. When the limit is reached, new device registration attempts will be rejected with an appropriate error.
Changes:
- Added
AnonymousDeviceLimitconfiguration field that can be set via theauth.anonymous.device_limitINI setting - Modified the anonymous device store to enforce the limit by only allowing updates (not new registrations) when the limit is reached
- Changed
TagDeviceerror handling from asynchronous/fire-and-forget to synchronous to properly propagate device limit errors
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/setting/setting.go | Added AnonymousDeviceLimit configuration field and refactored anonymous settings parsing to use a section variable |
| pkg/services/anonymous/anonimpl/impl_test.go | Updated test setup to pass db.DB directly instead of pre-instantiated AnonDBStore |
| pkg/services/anonymous/anonimpl/impl.go | Modified ProvideAnonymousDeviceService to instantiate AnonDBStore with the device limit and added error propagation in TagDevice |
| pkg/services/anonymous/anonimpl/client.go | Changed device tagging from async goroutine to synchronous call with device limit error handling |
| pkg/services/anonymous/anonimpl/api/api.go | Renamed constant from thirtyDays to anonymousDeviceExpiration for consistency |
| pkg/services/anonymous/anonimpl/anonstore/database_test.go | Updated all test instantiations of AnonDBStore to include device limit parameter and added test for limit enforcement |
| pkg/services/anonymous/anonimpl/anonstore/database.go | Added device limit enforcement logic with new updateDevice method and ErrDeviceLimitReached error |
| pkg/api/frontendsettings.go | Exposed AnonymousDeviceLimit to frontend settings |
| pkg/api/dtos/frontend_settings.go | Added AnonymousDeviceLimit field to frontend settings DTO |
| packages/grafana-runtime/src/config.ts | Added anonymousDeviceLimit to runtime configuration |
| packages/grafana-data/src/types/config.ts | Added anonymousDeviceLimit type definition to config interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if rowsAffected == 0 { | ||
| return ErrDeviceLimitReached |
There was a problem hiding this comment.
The updateDevice method returns ErrDeviceLimitReached when no rows are affected (line 96), but this is misleading. If rowsAffected == 0, it means the device doesn't exist or the time window condition wasn't met, not that the device limit was reached. This error should only be returned from the limit check in CreateOrUpdateDevice (lines 115-116). Consider returning a different error here (e.g., ErrDeviceNotFound or ErrDeviceUpdateFailed) or allowing the upsert logic to proceed to create the device.
| func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { | ||
| var query string | ||
|
|
||
| // 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.
This queries the database to count devices on every CreateOrUpdateDevice call when deviceLimit > 0, even for existing device updates. This could create a performance bottleneck under high load. Consider caching the count or only performing this check when attempting to create a new device (after checking if the device already exists).
| func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { | |
| var query string | |
| // 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) | |
| } | |
| func (s *AnonDBStore) deviceExists(ctx context.Context, deviceID string) (bool, error) { | |
| const query = `SELECT 1 FROM anon_device WHERE device_id = ? LIMIT 1` | |
| var exists bool | |
| err := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { | |
| rows, err := dbSession.Query(query, deviceID) | |
| if err != nil { | |
| return err | |
| } | |
| defer rows.Close() | |
| exists = rows.Next() | |
| return rows.Err() | |
| }) | |
| if err != nil { | |
| return false, err | |
| } | |
| return exists, nil | |
| } | |
| func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { | |
| var query string | |
| // if device limit is reached, only update devices; avoid counting on every update | |
| if s.deviceLimit > 0 { | |
| exists, err := s.deviceExists(ctx, device.DeviceID) | |
| if err != nil { | |
| return err | |
| } | |
| // Existing devices can be updated without counting all devices. | |
| if exists { | |
| return s.updateDevice(ctx, device) | |
| } | |
| // Only count devices when attempting to create a new device. | |
| now := time.Now().UTC() | |
| count, err := s.CountDevices(ctx, now.Add(-anonymousDeviceExpiration), now.Add(time.Minute)) | |
| if err != nil { | |
| return err | |
| } | |
| if count >= s.deviceLimit { | |
| return ErrDeviceLimitReached | |
| } |
| theme2: GrafanaTheme2; | ||
| featureToggles: FeatureToggles = {}; | ||
| anonymousEnabled = false; | ||
| anonymousDeviceLimit = undefined; |
There was a problem hiding this comment.
The default value is undefined, but the TypeScript type in config.ts is number | undefined. For consistency with the backend default of 0 (which means no limit), consider initializing this to 0 instead of undefined to avoid potential undefined checks in consuming code.
| anonymousDeviceLimit = undefined; | |
| anonymousDeviceLimit = 0; |
Test 1nn
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#1](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/1)*