-
Notifications
You must be signed in to change notification settings - Fork 9
Anonymous: Add configurable device limit #1
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,14 @@ import ( | |
| ) | ||
|
|
||
| const cacheKeyPrefix = "anon-device" | ||
| const anonymousDeviceExpiration = 30 * 24 * time.Hour | ||
|
|
||
| var ErrDeviceLimitReached = fmt.Errorf("device limit reached") | ||
|
|
||
| type AnonDBStore struct { | ||
| sqlStore db.DB | ||
| log log.Logger | ||
| sqlStore db.DB | ||
| log log.Logger | ||
| deviceLimit int64 | ||
| } | ||
|
|
||
| type Device struct { | ||
|
|
@@ -45,8 +49,8 @@ type AnonStore interface { | |
| DeleteDevicesOlderThan(ctx context.Context, olderThan time.Time) error | ||
| } | ||
|
|
||
| func ProvideAnonDBStore(sqlStore db.DB) *AnonDBStore { | ||
| return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore")} | ||
| func ProvideAnonDBStore(sqlStore db.DB, deviceLimit int64) *AnonDBStore { | ||
| return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore"), deviceLimit: deviceLimit} | ||
| } | ||
|
|
||
| func (s *AnonDBStore) ListDevices(ctx context.Context, from *time.Time, to *time.Time) ([]*Device, error) { | ||
|
|
@@ -65,9 +69,54 @@ func (s *AnonDBStore) ListDevices(ctx context.Context, from *time.Time, to *time | |
| return devices, err | ||
| } | ||
|
|
||
| // 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), | ||
| } | ||
|
Comment on lines
+80
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Time window calculation inconsistency: Using |
||
| 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 | ||
| } | ||
|
Comment on lines
+95
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Misleading error: Returning |
||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| 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)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Inconsistent time windows: The count query uses |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if count >= s.deviceLimit { | ||
| return s.updateDevice(ctx, device) | ||
| } | ||
| } | ||
|
Comment on lines
+108
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Race condition: Multiple concurrent requests could pass the device count check simultaneously and create devices beyond the limit. Consider using a database transaction or lock. |
||
|
|
||
| args := []any{device.DeviceID, device.ClientIP, device.UserAgent, | ||
| device.CreatedAt.UTC(), device.UpdatedAt.UTC()} | ||
| switch s.sqlStore.GetDBType() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import ( | |
|
|
||
| func TestIntegrationAnonStore_DeleteDevicesOlderThan(t *testing.T) { | ||
| store := db.InitTestDB(t) | ||
| anonDBStore := ProvideAnonDBStore(store) | ||
| anonDBStore := ProvideAnonDBStore(store, 0) | ||
| const keepFor = time.Hour * 24 * 61 | ||
|
|
||
| anonDevice := &Device{ | ||
|
|
@@ -48,9 +48,30 @@ func TestIntegrationAnonStore_DeleteDevicesOlderThan(t *testing.T) { | |
| assert.Equal(t, "keep", devices[0].DeviceID) | ||
| } | ||
|
|
||
| func TestIntegrationBeyondDeviceLimit(t *testing.T) { | ||
| store := db.InitTestDB(t) | ||
| anonDBStore := ProvideAnonDBStore(store, 1) | ||
|
|
||
| anonDevice := &Device{ | ||
| DeviceID: "32mdo31deeqwes", | ||
| ClientIP: "10.30.30.2", | ||
| UserAgent: "test", | ||
| UpdatedAt: time.Now().Add(-time.Hour), | ||
| } | ||
|
|
||
| err := anonDBStore.CreateOrUpdateDevice(context.Background(), anonDevice) | ||
| require.NoError(t, err) | ||
|
|
||
| anonDevice.DeviceID = "keep" | ||
| anonDevice.UpdatedAt = time.Now().Add(-time.Hour) | ||
|
|
||
| err = anonDBStore.CreateOrUpdateDevice(context.Background(), anonDevice) | ||
|
Comment on lines
+55
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Test may not accurately validate device limit: both devices use the same ClientIP and UserAgent. The database implementation might treat this as an update rather than a new device creation, making the test inconclusive. |
||
| require.ErrorIs(t, err, ErrDeviceLimitReached) | ||
| } | ||
|
|
||
| func TestIntegrationAnonStore_DeleteDevice(t *testing.T) { | ||
| store := db.InitTestDB(t) | ||
| anonDBStore := ProvideAnonDBStore(store) | ||
| anonDBStore := ProvideAnonDBStore(store, 0) | ||
| const keepFor = time.Hour * 24 * 61 | ||
|
|
||
| anonDevice := &Device{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import ( | |
| "time" | ||
|
|
||
| "github.com/grafana/grafana/pkg/api/routing" | ||
| "github.com/grafana/grafana/pkg/infra/db" | ||
| "github.com/grafana/grafana/pkg/infra/localcache" | ||
| "github.com/grafana/grafana/pkg/infra/log" | ||
| "github.com/grafana/grafana/pkg/infra/network" | ||
|
|
@@ -33,13 +34,13 @@ type AnonDeviceService struct { | |
| } | ||
|
|
||
| func ProvideAnonymousDeviceService(usageStats usagestats.Service, authBroker authn.Service, | ||
| anonStore anonstore.AnonStore, cfg *setting.Cfg, orgService org.Service, | ||
| sqlStore db.DB, cfg *setting.Cfg, orgService org.Service, | ||
| serverLockService *serverlock.ServerLockService, accesscontrol accesscontrol.AccessControl, routeRegister routing.RouteRegister, | ||
| ) *AnonDeviceService { | ||
| a := &AnonDeviceService{ | ||
| log: log.New("anonymous-session-service"), | ||
| localCache: localcache.New(29*time.Minute, 15*time.Minute), | ||
| anonStore: anonStore, | ||
| anonStore: anonstore.ProvideAnonDBStore(sqlStore, cfg.AnonymousDeviceLimit), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Store creation moved into service constructor - this tightly couples the service to the specific store implementation rather than using dependency injection |
||
| serverLock: serverLockService, | ||
| } | ||
|
|
||
|
|
@@ -57,7 +58,7 @@ func ProvideAnonymousDeviceService(usageStats usagestats.Service, authBroker aut | |
| authBroker.RegisterPostLoginHook(a.untagDevice, 100) | ||
| } | ||
|
|
||
| anonAPI := api.NewAnonDeviceServiceAPI(cfg, anonStore, accesscontrol, routeRegister) | ||
| anonAPI := api.NewAnonDeviceServiceAPI(cfg, a.anonStore, accesscontrol, routeRegister) | ||
| anonAPI.RegisterAPIEndpoints() | ||
|
|
||
| return a | ||
|
|
@@ -143,6 +144,7 @@ func (a *AnonDeviceService) TagDevice(ctx context.Context, httpReq *http.Request | |
| err = a.tagDeviceUI(ctx, httpReq, taggedDevice) | ||
| if err != nil { | ||
| a.log.Debug("Failed to tag device for UI", "error", err) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,16 +113,15 @@ func TestIntegrationDeviceService_tag(t *testing.T) { | |
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| store := db.InitTestDB(t) | ||
| anonDBStore := anonstore.ProvideAnonDBStore(store) | ||
| anonService := ProvideAnonymousDeviceService(&usagestats.UsageStatsMock{}, | ||
| &authntest.FakeService{}, anonDBStore, setting.NewCfg(), orgtest.NewOrgServiceFake(), nil, actest.FakeAccessControl{}, &routing.RouteRegisterImpl{}) | ||
| &authntest.FakeService{}, store, setting.NewCfg(), orgtest.NewOrgServiceFake(), nil, actest.FakeAccessControl{}, &routing.RouteRegisterImpl{}) | ||
|
|
||
| for _, req := range tc.req { | ||
| err := anonService.TagDevice(context.Background(), req.httpReq, req.kind) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| devices, err := anonDBStore.ListDevices(context.Background(), nil, nil) | ||
| devices, err := anonService.anonStore.ListDevices(context.Background(), nil, nil) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: accessing internal field |
||
| require.NoError(t, err) | ||
| require.Len(t, devices, int(tc.expectedAnonUICount)) | ||
| if tc.expectedDevice != nil { | ||
|
|
@@ -149,9 +148,8 @@ func TestIntegrationDeviceService_tag(t *testing.T) { | |
| // Ensure that the local cache prevents request from being tagged | ||
| func TestIntegrationAnonDeviceService_localCacheSafety(t *testing.T) { | ||
| store := db.InitTestDB(t) | ||
| anonDBStore := anonstore.ProvideAnonDBStore(store) | ||
| anonService := ProvideAnonymousDeviceService(&usagestats.UsageStatsMock{}, | ||
| &authntest.FakeService{}, anonDBStore, setting.NewCfg(), orgtest.NewOrgServiceFake(), nil, actest.FakeAccessControl{}, &routing.RouteRegisterImpl{}) | ||
| &authntest.FakeService{}, store, setting.NewCfg(), orgtest.NewOrgServiceFake(), nil, actest.FakeAccessControl{}, &routing.RouteRegisterImpl{}) | ||
|
|
||
| req := &http.Request{ | ||
| Header: http.Header{ | ||
|
|
||
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.
style: Consider using a more specific type like
number | undefinedinstead of justundefinedto match the backend type definition