Anonymous: Add configurable device limit#2
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>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces an Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Service as AnonDeviceService
participant Store as AnonDBStore
participant DB as Database
Client->>Service: TagDevice(ctx, device)
Service->>Store: CreateOrUpdateDevice(ctx, device)
alt Device Limit Disabled (limit ≤ 0)
Store->>DB: INSERT new device
DB-->>Store: Success
else Device Limit Enabled (limit > 0)
Store->>DB: COUNT devices within expiration window
DB-->>Store: deviceCount
alt Count < Limit
Store->>DB: INSERT new device
DB-->>Store: Success
else Count ≥ Limit
Store->>Store: updateDevice(existing device)
Store->>DB: UPDATE device (if recently updated)
DB-->>Store: Result (0 or 1 rows)
alt Rows Updated = 0
Store-->>Service: ErrDeviceLimitReached
Service-->>Client: Error
else Rows Updated = 1
Store-->>Service: Success
Service-->>Client: Device Updated
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/grafana-runtime/src/config.ts (1)
97-97: Add explicit type annotation for clarity and interface compliance.The property is missing an explicit type annotation. While TypeScript will infer the type as
undefined, theGrafanaConfiginterface defines this asnumber | undefined. Adding the type annotation ensures type safety and makes the code more self-documenting.Suggested fix
- anonymousDeviceLimit = undefined; + anonymousDeviceLimit: number | undefined = undefined;pkg/services/anonymous/anonimpl/api/api.go (1)
18-18: Consider extracting the duplicated constant to a shared location.The
anonymousDeviceExpirationconstant is defined identically in both this file andpkg/services/anonymous/anonimpl/anonstore/database.go. This duplication risks inconsistency if one value is updated without the other.Consider moving this constant to a shared package (e.g., the
anonstorepackage) and importing it here.Suggested approach
In
anonstore/database.go, export the constant:-const anonymousDeviceExpiration = 30 * 24 * time.Hour +const AnonymousDeviceExpiration = 30 * 24 * time.HourThen in
api/api.go:-const anonymousDeviceExpiration = 30 * 24 * time.Hour +// Use the shared constant from anonstoreAnd reference
anonstore.AnonymousDeviceExpirationin both locations.pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
51-70: Test correctly verifies device limit enforcement.The test properly validates that creating a second device with a different
DeviceIDfails withErrDeviceLimitReachedwhen the limit is 1.Consider adding a test case to verify that updating an existing device (same
DeviceID) still succeeds after the limit is reached—this ensures the limit only blocks new devices, not updates to existing ones.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/grafana-data/src/types/config.tspackages/grafana-runtime/src/config.tspkg/api/dtos/frontend_settings.gopkg/api/frontendsettings.gopkg/services/anonymous/anonimpl/anonstore/database.gopkg/services/anonymous/anonimpl/anonstore/database_test.gopkg/services/anonymous/anonimpl/api/api.gopkg/services/anonymous/anonimpl/client.gopkg/services/anonymous/anonimpl/impl.gopkg/services/anonymous/anonimpl/impl_test.gopkg/setting/setting.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
pkg/services/anonymous/anonimpl/anonstore/database.go (3)
ProvideAnonDBStore(52-54)Device(26-33)ErrDeviceLimitReached(18-18)
pkg/api/frontendsettings.go (1)
pkg/setting/setting.go (1)
Cfg(149-578)
pkg/services/anonymous/anonimpl/impl.go (4)
pkg/setting/setting.go (1)
Cfg(149-578)pkg/infra/serverlock/serverlock.go (1)
ServerLockService(27-31)pkg/services/anonymous/anonimpl/anonstore/database.go (1)
ProvideAnonDBStore(52-54)pkg/services/anonymous/anonimpl/api/api.go (1)
NewAnonDeviceServiceAPI(34-47)
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)
🔇 Additional comments (14)
packages/grafana-data/src/types/config.ts (1)
199-201: LGTM!The new
anonymousDeviceLimitproperty is correctly typed asnumber | undefinedand logically placed alongside the relatedanonymousEnabledsetting.pkg/setting/setting.go (2)
371-375: LGTM!The
AnonymousDeviceLimitfield is correctly placed alongside other anonymous configuration fields and usesint64type consistent with similar settings in the codebase.
1650-1655: LGTM!The configuration parsing is clean. Refactoring to use the
anonSectionvariable improves readability, and the default value of0fordevice_limitcorrectly means "no limit" based on the enforcement logic in the store layer.pkg/services/anonymous/anonimpl/anonstore/database.go (1)
80-81: Verify the upper bound time window logic.The upper bound
device.UpdatedAt.UTC().Add(time.Minute)adds 1 minute to the device's update time. This appears to be a small buffer, but could you clarify the reasoning? Ifdevice.UpdatedAtis set totime.Now()before calling this method, the buffer may be unnecessary or could mask timing issues.pkg/api/frontendsettings.go (1)
197-198: LGTM!The
AnonymousDeviceLimitis correctly sourced from the configuration and positioned logically alongsideAnonymousEnabled.pkg/api/dtos/frontend_settings.go (1)
194-196: LGTM!The
AnonymousDeviceLimitfield is correctly typed asint64with proper JSON serialization tag, maintaining consistency with the backend configuration.pkg/services/anonymous/anonimpl/api/api.go (1)
70-72: LGTM!The usage of the renamed constant is correct and maintains the existing behavior.
pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
16-16: LGTM!Passing
0as the device limit preserves the original behavior (unlimited devices) for existing tests.pkg/services/anonymous/anonimpl/impl_test.go (2)
116-117: Wiring change aligns with the new service initialization pattern.The test now passes the DB store directly to
ProvideAnonymousDeviceService, which internally creates theanonStorewith the configured device limit fromcfg.AnonymousDeviceLimit. Sincesetting.NewCfg()defaultsAnonymousDeviceLimitto0(unlimited), existing test behavior is preserved.
124-124: Accessing internalanonStorefield for testing.This works for integration tests but couples the test to internal implementation details. If the
anonStorefield becomes unexported or renamed, tests will break.This is acceptable for now given the test's integration nature, but consider exposing a test helper or interface method if this access pattern expands.
pkg/services/anonymous/anonimpl/impl.go (3)
36-43: Good refactor to encapsulate store creation within the service.The signature change from accepting an
anonStoreto acceptingsqlStore db.DBand creating the store internally withcfg.AnonymousDeviceLimitimproves encapsulation. The device limit configuration is now properly wired from the Grafana config.
144-148: Error propagation change is essential for device limit enforcement.Previously, errors from
tagDeviceUIwere logged but swallowed. Now they're returned, which is necessary forErrDeviceLimitReachedto propagate up toclient.goand block authentication.Note: For non-limit errors (e.g., transient DB issues), this will now also block authentication where it previously allowed anonymous access to proceed. Verify this is the intended behavior—if not, consider only returning
ErrDeviceLimitReachedand continuing on other errors (similar to the pattern inclient.go).
61-62: API wiring correctly uses the internally created store.The
anonAPInow receivesa.anonStorewhich is the DB-backed store with the device limit configuration applied.pkg/services/anonymous/anonimpl/client.go (1)
44-50: Error handling for device limit is correct and properly propagated.The use of
errors.Is()for sentinel error comparison is idiomatic Go. The error is properly returned fromAuthenticate()and propagated up through the authentication middleware, where it blocks authentication as intended.The middleware in
pkg/services/accesscontrol/middleware.gohandlesLookupTokenErr(which includesErrDeviceLimitReached) by returning HTTP 401 Unauthorized for API requests. While 429 Too Many Requests would be semantically more precise for rate limiting, HTTP 401 is acceptable for blocking authentication. No additional error handling is needed.
| // updateDevice updates a device if it exists and has been updated between the given times. | ||
| func (s *AnonDBStore) updateDevice(ctx context.Context, device *Device) error { | ||
| const query = `UPDATE anon_device SET | ||
| client_ip = ?, | ||
| user_agent = ?, | ||
| updated_at = ? | ||
| WHERE device_id = ? AND updated_at BETWEEN ? AND ?` | ||
|
|
||
| args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID, | ||
| device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute), | ||
| } | ||
| err := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { | ||
| args = append([]interface{}{query}, args...) | ||
| result, err := dbSession.Exec(args...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| rowsAffected, err := result.RowsAffected() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if rowsAffected == 0 { | ||
| return ErrDeviceLimitReached | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
Clarify error semantics: rowsAffected == 0 may not always indicate limit reached.
The updateDevice method returns ErrDeviceLimitReached when no rows are affected, but this could also occur if the device doesn't exist or if updated_at falls outside the time window for reasons other than the limit being reached. This conflates different failure modes.
Consider returning a more specific error or adding a check to distinguish between "device not found" and "device limit reached".
Suggested approach
if rowsAffected == 0 {
- return ErrDeviceLimitReached
+ return ErrDeviceLimitReached // Note: This also triggers if device doesn't exist within the expiration window
}Alternatively, you could query for the device's existence first to provide a more accurate error, though this adds an extra query.
| // 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.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full function containing lines 108-118
cd /root/repo && sed -n '80,130p' pkg/services/anonymous/anonimpl/anonstore/database.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 143
🏁 Script executed:
# Let's also check the file size to ensure we can read it safely
wc -l pkg/services/anonymous/anonimpl/anonstore/database.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 140
🏁 Script executed:
# Search for related comments about device limit enforcement
rg -i "device.*limit" pkg/services/anonymous/ -A 2 -B 2Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 4253
🏁 Script executed:
# Look for transaction usage in this file
rg -i "transaction|begin|commit|rollback" pkg/services/anonymous/anonimpl/anonstore/database.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 65
🏁 Script executed:
cat -n pkg/services/anonymous/anonimpl/anonstore/database.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 7419
🏁 Script executed:
# Also check the test to understand expected behavior
cat -n pkg/services/anonymous/anonimpl/anonstore/database_test.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 3519
TOCTOU race condition between count check and insert—device limit can be exceeded under concurrent load.
The count check (line 110) and subsequent insert/update operations (lines 115-155) are not atomic. Multiple concurrent requests can both pass the count check simultaneously and then both proceed to insert, exceeding the device limit. For example, with a limit of 1: Request A counts 0 devices, Request B counts 0 devices, then both insert, resulting in 2 devices.
Since ErrDeviceLimitReached is returned as an explicit error, this appears to be intended as a strict limit. To properly enforce this, wrap the count check and insert in a database transaction with row locking (e.g., SELECT ... FOR UPDATE) or use a unique constraint on a computed "active device slot" to prevent overcounting at the database level.
|
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 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)*