Automated Test: implement-device-limits #326
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>
📝 WalkthroughWalkthroughThis pull request introduces a device limit feature for anonymous access in Grafana. The changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Request
participant AuthBroker as Auth Service
participant AnonService as Anonymous Device Service
participant AnonStore as Anonymous Device Store
participant Database as Database
Client->>AuthBroker: Authenticate
AuthBroker->>AnonService: TagDevice
AnonService->>AnonStore: CreateOrUpdateDevice
alt Device count within limit or first device
AnonStore->>Database: Upsert device record
Database-->>AnonStore: Success
AnonStore-->>AnonService: Device created/updated
AnonService-->>AuthBroker: Success
AuthBroker-->>Client: Identity returned
else Device limit reached
AnonStore->>AnonStore: Check device count vs limit
AnonStore-->>AnonService: ErrDeviceLimitReached
AnonService-->>AuthBroker: ErrDeviceLimitReached
AuthBroker-->>Client: Error returned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/services/anonymous/anonimpl/anonstore/database.go`:
- Around line 108-118: The CountDevices check in database.go (using
s.CountDevices) and the subsequent insert/upsert (s.CreateOrUpdateDevice or
s.updateDevice) create a TOCTOU race under concurrent load; to fix, move the
count+insert logic into a single database transaction or enforce the limit with
a DB-level constraint: start a transaction, re-check the device count and
perform CreateOrUpdateDevice or updateDevice within that same transaction (or
add a unique/partial/index/constraint in the devices table and handle the
constraint error in CreateOrUpdateDevice), ensuring s.deviceLimit is enforced
atomically.
In `@pkg/services/anonymous/anonimpl/client.go`:
- Around line 44-50: The TagDevice call on anonDeviceService inside client.go is
now synchronous and blocks the authentication path; change it to run
asynchronously (fire-and-forget) so DB writes don't add request latency—invoke
a.anonDeviceService.TagDevice in a background goroutine and capture/log errors
there instead of blocking the response flow; additionally, if you must return
ErrDeviceLimitReached from TagDevice, wrap it in a clearer domain error or
sentinel (e.g., ErrAnonDeviceLimit or a descriptive error from the client
method) so the HTTP handler can map it to a 429/403 with a user-facing message,
and ensure existing log calls (a.log.Warn) remain for diagnostics.
🧹 Nitpick comments (5)
pkg/services/anonymous/anonimpl/anonstore/database.go (2)
72-103:updateDeviceconflates "device not found" with "device limit reached".When
rowsAffected == 0, it could mean the device doesn't exist or theupdated_atis outside theBETWEENwindow. Both cases returnErrDeviceLimitReached. This works in the current call path (only invoked when the limit is hit), but ifupdateDeviceis ever called independently or error messages are surfaced to the user, the ambiguity could be misleading. A comment clarifying this design intent would help.
16-16: Duplicate constantanonymousDeviceExpirationacross packages.The constant is defined identically in both
pkg/services/anonymous/anonimpl/anonstore/database.go(line 16) andpkg/services/anonymous/anonimpl/api/api.go(line 18). Export it from one package and reuse it to prevent drift if the value changes.pkg/services/anonymous/anonimpl/impl.go (1)
143-148: Error now propagated — non-limit errors will be logged twice.
tagDeviceUIfailures are logged atDebuglevel here (line 146), and then the returned error is logged again atWarnlevel inclient.go(line 49). ForErrDeviceLimitReachedthis is fine (it short-circuits in client.go), but other tagging errors get dual-logged. Consider removing theDebuglog here or only logging when you don't propagate.pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
51-70: Test covers the basic limit-exceeded path; consider adding the "update existing device at limit" case.The test correctly verifies that a new device is rejected when the limit is reached. However, the other key behavior — that an existing device can still be updated when the limit is hit — is untested. Adding a case where the second
CreateOrUpdateDevicecall reuses the first device's ID (expecting success) would strengthen confidence in theupdateDevicepath.💡 Example additional assertion
// After verifying ErrDeviceLimitReached for a new device, // updating the existing device should still succeed: anonDevice.DeviceID = "32mdo31deeqwes" // reuse original ID anonDevice.UserAgent = "updated-agent" err = anonDBStore.CreateOrUpdateDevice(context.Background(), anonDevice) require.NoError(t, err)packages/grafana-runtime/src/config.ts (1)
97-97: Add an explicit type annotation for type consistency and correctness.The property
anonymousDeviceLimit = undefined;lacks an explicit type annotation. TypeScript infers the type asundefinedrather thannumber | undefined, which doesn't match theGrafanaConfiginterface declaration. Other similar optional properties in this class (e.g.,loginError: string | undefined = undefined;) include explicit annotations. Adding the type annotation aligns with both the interface contract and the class's established style.♻️ Proposed fix
- anonymousDeviceLimit = undefined; + anonymousDeviceLimit: number | undefined = undefined;
| // 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.
TOCTOU race: count check and insert are not atomic.
CountDevices and the subsequent CreateOrUpdateDevice upsert execute in separate transactions. Under concurrent load, multiple requests can pass the count check simultaneously and all proceed to insert, overshooting the device limit.
For a soft/approximate limit this may be acceptable, but if strict enforcement is needed, the count + insert should occur within a single DB transaction or use a DB-level constraint.
🤖 Prompt for AI Agents
In `@pkg/services/anonymous/anonimpl/anonstore/database.go` around lines 108 -
118, The CountDevices check in database.go (using s.CountDevices) and the
subsequent insert/upsert (s.CreateOrUpdateDevice or s.updateDevice) create a
TOCTOU race under concurrent load; to fix, move the count+insert logic into a
single database transaction or enforce the limit with a DB-level constraint:
start a transaction, re-check the device count and perform CreateOrUpdateDevice
or updateDevice within that same transaction (or add a
unique/partial/index/constraint in the devices table and handle the constraint
error in CreateOrUpdateDevice), ensuring s.deviceLimit is enforced atomically.
| if err := a.anonDeviceService.TagDevice(ctx, httpReqCopy, anonymous.AnonDeviceUI); err != nil { | ||
| if errors.Is(err, anonstore.ErrDeviceLimitReached) { | ||
| return nil, err | ||
| } | ||
| }() | ||
|
|
||
| a.log.Warn("Failed to tag anonymous session", "error", err) | ||
| } |
There was a problem hiding this comment.
Synchronous DB call now sits on the critical authentication path.
TagDevice was previously asynchronous (per the summary). Making it synchronous means every anonymous request now blocks on a DB write (and potentially a CountDevices query when limits are enabled). Under high anonymous traffic this could add noticeable latency and increase DB load.
Also, returning the raw ErrDeviceLimitReached to the caller will eventually surface as a generic error to the anonymous user. Consider either:
- wrapping it in a more descriptive error for the HTTP layer, or
- ensuring the upstream HTTP handler translates it into an appropriate status (e.g.,
429 Too Many Requestsor403 Forbiddenwith a clear message).
🤖 Prompt for AI Agents
In `@pkg/services/anonymous/anonimpl/client.go` around lines 44 - 50, The
TagDevice call on anonDeviceService inside client.go is now synchronous and
blocks the authentication path; change it to run asynchronously
(fire-and-forget) so DB writes don't add request latency—invoke
a.anonDeviceService.TagDevice in a background goroutine and capture/log errors
there instead of blocking the response flow; additionally, if you must return
ErrDeviceLimitReached from TagDevice, wrap it in a clearer domain error or
sentinel (e.g., ErrAnonDeviceLimit or a descriptive error from the client
method) so the HTTP handler can map it to a 429/403 with a user-facing message,
and ensure existing log calls (a.log.Warn) remain for diagnostics.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes