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]>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request introduces an anonymous device limit feature across Grafana's configuration and backend services. A new configurable Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as TagDevice<br/>(Service)
participant Store as AnonDBStore
participant DB as Database
Client->>Service: TagDevice(ctx, deviceRef)
Service->>Store: CreateOrUpdateDevice(ctx, deviceRef)
alt Device Limit Enforced (limit > 0)
Store->>DB: Query device count within<br/>expiration window
DB-->>Store: count result
alt Count >= Limit
Store->>DB: UPDATE device (updateDevice)
DB-->>Store: rows affected
alt No rows affected
Store-->>Service: ErrDeviceLimitReached ❌
Service-->>Client: Error returned
else Rows updated
Store-->>Service: Success ✓
Service-->>Client: Device updated
end
else Count < Limit
Store->>DB: INSERT new device
DB-->>Store: device created
Store-->>Service: Success ✓
Service-->>Client: New device created
end
else No Limit (limit ≤ 0)
Store->>DB: INSERT or UPDATE device
DB-->>Store: Success
Store-->>Service: Success ✓
Service-->>Client: Device created/updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (5)
pkg/services/anonymous/anonimpl/anonstore/database.go (2)
16-16: Consider exporting the constant for consistency.The
anonymousDeviceExpirationconstant is also duplicated inpkg/services/anonymous/anonimpl/api/api.go(line 18). Consider exporting this constant from a single location to ensure consistency across the codebase.For example, export it from this file:
-const anonymousDeviceExpiration = 30 * 24 * time.Hour +const AnonymousDeviceExpiration = 30 * 24 * time.HourThen import and use it in api.go instead of redefining it.
18-18: Enhance error message with more context.The error message could be more helpful by including the device limit value or suggesting what users can do.
Consider creating a structured error type:
type ErrDeviceLimitReached struct { Limit int64 } func (e *ErrDeviceLimitReached) Error() string { return fmt.Sprintf("anonymous device limit of %d reached", e.Limit) }This would allow callers to extract the limit value if needed for user-facing messages.
pkg/services/anonymous/anonimpl/impl_test.go (2)
116-117: Note the reduced test flexibility.The test now passes the raw
db.DBto the service, which internally creates its own store. While acceptable for integration tests, this reduces the ability to inject mock stores for unit testing. Consider whether unit tests with mocked stores would be valuable.
124-124: Consider using the public API instead of accessing internal fields.The test directly accesses
anonService.anonStore.ListDevices(), but the service already exposes a publicListDevices()method (line 154 of impl.go). Using the public API would decouple the test from internal implementation details.Apply this diff to use the public API:
- devices, err := anonService.anonStore.ListDevices(context.Background(), nil, nil) + devices, err := anonService.ListDevices(context.Background(), nil, nil)pkg/services/anonymous/anonimpl/impl.go (1)
36-39: Constructor signature changed from dependency injection to internal construction.The service now receives
sqlStore db.DBinstead ofanonStore anonstore.AnonStore, shifting from dependency injection to internal store construction. While this provides better control over store configuration (e.g., passing the device limit), it reduces testability and flexibility. Consider whether this trade-off aligns with your testing strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/grafana-data/src/types/config.ts(1 hunks)packages/grafana-runtime/src/config.ts(1 hunks)pkg/api/dtos/frontend_settings.go(1 hunks)pkg/api/frontendsettings.go(1 hunks)pkg/services/anonymous/anonimpl/anonstore/database.go(3 hunks)pkg/services/anonymous/anonimpl/anonstore/database_test.go(2 hunks)pkg/services/anonymous/anonimpl/api/api.go(2 hunks)pkg/services/anonymous/anonimpl/client.go(2 hunks)pkg/services/anonymous/anonimpl/impl.go(4 hunks)pkg/services/anonymous/anonimpl/impl_test.go(2 hunks)pkg/setting/setting.go(2 hunks)
🔇 Additional comments (12)
pkg/services/anonymous/anonimpl/client.go (1)
44-50: Verify authentication behavior when device limit is reached.When
ErrDeviceLimitReachedis returned, the authentication will fail. This means that even existing anonymous users may be unable to authenticate if the device limit has been reached by other users. Consider whether the intended behavior is to:
- Block all anonymous access when the limit is reached (current implementation)
- Allow existing devices to continue but block new devices
- Provide a more graceful degradation
If the current behavior is intentional, consider adding user-friendly error messaging that explains why authentication failed.
pkg/services/anonymous/anonimpl/api/api.go (1)
18-18: LGTM - Constant renamed for consistency.The constant name now matches the one defined in
database.go, improving consistency. As noted in thedatabase.goreview, consider exporting and sharing a single constant definition to avoid duplication.packages/grafana-runtime/src/config.ts (1)
97-97: LGTM - Frontend config property added.The new
anonymousDeviceLimitproperty is correctly added with an appropriate default value. Thenumber | undefinedtype aligns well with the backend'sint64and the optional nature of this configuration.packages/grafana-data/src/types/config.ts (1)
200-200: LGTM - Type definition added correctly.The field is properly typed as
number | undefined, matching the optional nature of the configuration and aligning with the implementation inpackages/grafana-runtime/src/config.ts.pkg/api/frontendsettings.go (1)
198-198: LGTM - Frontend settings population added.The
AnonymousDeviceLimitis correctly populated from the configuration. This change is consistent with how other configuration values are exposed to the frontend.pkg/api/dtos/frontend_settings.go (1)
195-195: LGTM - DTO field added correctly.The
AnonymousDeviceLimitfield is properly defined with the correct type (int64) and JSON tag. The placement next toAnonymousEnabledis logical.pkg/services/anonymous/anonimpl/anonstore/database_test.go (2)
16-16: LGTM - Existing tests updated correctly.The existing test functions are properly updated to pass
deviceLimit=0to maintain their original behavior (no limit enforcement).Also applies to: 74-74
51-70: Test coverage added for device limit feature.The new test
TestIntegrationBeyondDeviceLimitcorrectly exercises the device limit enforcement:
- Creates a device successfully when under the limit
- Attempts to create a second device and expects
ErrDeviceLimitReachedHowever, note that this test may be sensitive to the race condition and time window logic issues identified in the
database.goreview. If those issues are addressed, this test may need adjustment.Consider adding additional test cases:
- Test updating an existing device when at the limit (should succeed per the current logic)
- Test with devices outside the expiration window (should not count toward limit)
- Test concurrent device creation to verify race condition behavior
pkg/setting/setting.go (2)
1650-1655: LGTM: Configuration reading is well-structured.The refactoring to reuse
anonSectionimproves efficiency by avoiding repeated section lookups. The configuration key naming and default value handling are consistent with the surrounding code.
375-375: AnonymousDeviceLimit = 0 correctly means "unlimited devices."The code at
pkg/services/anonymous/anonimpl/anonstore/database.go:109confirms the semantics: the device limit is only enforced whendeviceLimit > 0. When the default value is 0, the limit-checking block is skipped entirely, allowing new devices to be registered without restriction. This behavior is intentional and correct.pkg/services/anonymous/anonimpl/impl.go (2)
43-43: LGTM: Device limit configuration is properly wired.The store is correctly initialized with the device limit from configuration, ensuring consistent enforcement across the service.
144-148: Behavior change verified but partially mitigated: Error propagation incomplete in caller.The change is confirmed—
TagDevicenow returns errors fromtagDeviceUIinstead of swallowing them. However, verification shows:
- client.go line 44 already checks
if err := a.anonDeviceService.TagDevice(...); err != nil- Only
ErrDeviceLimitReachedis returned to upstream callers (lines 45-46)- Other errors are logged at line 49 but not returned—the function still returns success (line 52)
The original goroutine approach was also removed and replaced with synchronous execution, which is a separate change not covered in the original review.
Recommendation: The error is no longer completely swallowed at the
TagDevicelevel, but incomplete error handling persists inclient.go. Consider whether non-limit errors should propagate upstream or remain logged and suppressed.
| // 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 the time window logic and error semantics.
The updateDevice function has several concerns:
-
Ambiguous error: Returns
ErrDeviceLimitReachedwhen no rows are affected (line 95-96), but zero rows could mean:- Device doesn't exist in the database
- Device exists but was last updated outside the time window (older than 30 days or in the future)
This makes the error potentially misleading.
-
Unusual upper time bound: The WHERE clause checks
updated_at BETWEEN (now - 30 days) AND (now + 1 minute)(line 81). The upper bound ofnow + 1 minuteseems intended to handle clock skew, but:- It's inconsistent with the device expiration logic
- Consider using just
updated_at >= ?with a single lower bound - Or document why the upper bound is necessary
Consider refactoring to make the intent clearer:
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 ?`
+WHERE device_id = ? AND updated_at >= ?`
- args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID,
- device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute),
- }
+ args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID,
+ device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration),
+ }And potentially return a different error or add a comment explaining that ErrDeviceLimitReached in this context means "device not found or expired, cannot update."
🤖 Prompt for AI Agents
In pkg/services/anonymous/anonimpl/anonstore/database.go around lines 72 to 103,
the updateDevice function uses a BETWEEN window with a future upper bound and
returns ErrDeviceLimitReached when no rows are affected, which is ambiguous;
change the WHERE clause to use a single lower bound (e.g. updated_at >= ?) to
avoid the odd now+1min upper bound (or if you must keep the upper bound add a
clear comment explaining clock-skew handling), update the args to pass only the
lower bound (device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration)), and
replace or wrap the ErrDeviceLimitReached return with a clearer error (e.g.
ErrDeviceNotFoundOrExpired) so callers can distinguish "no matching/expired
device" from hitting a device limit; also add a brief comment above the query
describing the exact semantics (why the bound(s) are used) and ensure
tests/assertions reflect the new error semantics.
| // 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.
Race condition in device limit enforcement.
The device limit check has a race condition:
- Line 110:
CountDevicesis called to check the current count - Line 115-116: If the limit is reached,
updateDeviceis called - Lines 120-155: Otherwise, a new device is created
Between the count check and the insert/update, another concurrent request could create a device, causing the actual count to exceed the limit.
This race condition could allow the device limit to be exceeded under concurrent load. Consider one of these approaches:
-
Use database constraints: Add a mechanism to enforce the limit at the database level (though this is complex for row count limits)
-
Use locking: Acquire a lock before counting and hold it through the insert/update:
// Pseudo-code
s.lock.Lock()
defer s.lock.Unlock()
count, err := s.CountDevices(...)
if count >= s.deviceLimit {
return s.updateDevice(ctx, device)
}
// proceed with insert-
Accept eventual consistency: Document that the limit is approximate and may be briefly exceeded under high concurrency, which might be acceptable for this use case
-
Retry logic: After insert, check if count exceeded and remove the most recent device
Which approach is best depends on how strict the limit enforcement needs to be.
🤖 Prompt for AI Agents
In pkg/services/anonymous/anonimpl/anonstore/database.go around lines 108 to
118, the device limit check has a race where CountDevices and the subsequent
insert/update are not atomic, allowing concurrent requests to exceed the limit;
fix by serializing the critical section: add a mutex (or a named lock) on the
store struct, acquire the lock before calling CountDevices and hold it through
the decision and the eventual insert or update, defer unlock immediately after
acquiring, ensure the lock acquisition respects context/cancellation (or times
out) and return an error if it cannot be acquired, and keep the existing
updateDevice/insert logic inside that locked section so the count and
modification are atomic from the process perspective.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Test 1
Summary by CodeRabbit
Replicated from ai-code-review-evaluation/grafana-coderabbit#1