-
Notifications
You must be signed in to change notification settings - Fork 0
Anonymous: Add configurable device limit #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: enhance-anonymous-access
Are you sure you want to change the base?
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 <[email protected]>
|
cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Early Cache Entry Creates False Success
The local cache is set before the database operation completes. If CreateOrUpdateDevice fails (e.g., with ErrDeviceLimitReached), the cache entry remains set, causing subsequent requests for the same device within 29 minutes to incorrectly return success without attempting the database operation, creating inconsistent behavior based on cache state.
pkg/services/anonymous/anonimpl/impl.go#L86-L94
cursor-grafana/pkg/services/anonymous/anonimpl/impl.go
Lines 86 to 94 in 3647ba7
| a.localCache.SetDefault(key, struct{}{}) | |
| if setting.Env == setting.Dev { | |
| a.log.Debug("Tagging device for UI", "deviceID", device.DeviceID, "device", device, "key", key) | |
| } | |
| if err := a.anonStore.CreateOrUpdateDevice(ctx, device); err != nil { | |
| return err |
|
|
||
| if count >= s.deviceLimit { | ||
| return s.updateDevice(ctx, device) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race Condition Bypasses Device Limit
Race condition in device limit enforcement: the check-then-act pattern between counting devices and inserting new ones allows multiple concurrent requests to bypass the limit. When the count is near the limit, concurrent requests can all pass the check and insert devices, exceeding the configured deviceLimit.
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Old Devices Cannot Update Due to Limit
The updateDevice function's WHERE clause restricts updates to devices modified within the last 30 days. When the device limit is reached, existing devices older than 30 days cannot be updated and incorrectly return ErrDeviceLimitReached, even though they should be updatable since they already exist in the database.
Test 1
Replicated from ai-code-review-evaluation/grafana-cursor#1
Note
Adds a configurable anonymous device limit (auth.anonymous.device_limit), surfaces it to the frontend, and enforces it in the anonymous device store with a 30‑day window.
auth.anonymous.device_limitinsetting.go; includeAnonymousDeviceLimitin frontend settings DTOs and server response.ProvideAnonymousDeviceServicenow constructsAnonDBStorewithsqlStoreandcfg.AnonymousDeviceLimit; API uses a shared 30‑dayanonymousDeviceExpiration.Authenticatenow surfacesErrDeviceLimitReachedfrom device tagging; otherwise logs warning.deviceLimit,ErrDeviceLimitReached, and 30‑day expiration.CreateOrUpdateDevice(update-only when at limit) via newupdateDevicemethod.ProvideAnonDBStore(sqlStore, deviceLimit)signature; minor query/CRUD adjustments.anonymousDeviceLimittoGrafanaConfigand default in runtime config.TestIntegrationBeyondDeviceLimit).Written by Cursor Bugbot for commit 3647ba7. Configure here.