Skip to content
Jump to bottom

Automated Test: implement-device-limits #326

Closed
wants to merge 1 commit into from
+105 −37
Closed

Automated Test: implement-device-limits #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/grafana-data/src/types/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ export interface GrafanaConfig {
theme: GrafanaTheme;
theme2: GrafanaTheme2;
anonymousEnabled: boolean;
anonymousDeviceLimit: number | undefined;
featureToggles: FeatureToggles;
licenseInfo: LicenseInfo;
http2Enabled: boolean;
Expand Down
1 change: 1 addition & 0 deletions packages/grafana-runtime/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export class GrafanaBootConfig implements GrafanaConfig {
theme2: GrafanaTheme2;
featureToggles: FeatureToggles = {};
anonymousEnabled = false;
anonymousDeviceLimit = undefined;
licenseInfo: LicenseInfo = {} as LicenseInfo;
rendererAvailable = false;
rendererVersion = '';
Expand Down
1 change: 1 addition & 0 deletions pkg/api/dtos/frontend_settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ type FrontendSettingsDTO struct {

FeatureToggles map[string]bool `json:"featureToggles"`
AnonymousEnabled bool `json:"anonymousEnabled"`
AnonymousDeviceLimit int64 `json:"anonymousDeviceLimit"`
RendererAvailable bool `json:"rendererAvailable"`
RendererVersion string `json:"rendererVersion"`
SecretsManagerPluginEnabled bool `json:"secretsManagerPluginEnabled"`
Expand Down
1 change: 1 addition & 0 deletions pkg/api/frontendsettings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ func (hs *HTTPServer) getFrontendSettings(c *contextmodel.ReqContext) (*dtos.Fro

FeatureToggles: hs.Features.GetEnabled(c.Req.Context()),
AnonymousEnabled: hs.Cfg.AnonymousEnabled,
AnonymousDeviceLimit: hs.Cfg.AnonymousDeviceLimit,
RendererAvailable: hs.RenderService.IsAvailable(c.Req.Context()),
RendererVersion: hs.RenderService.Version(),
SecretsManagerPluginEnabled: secretsManagerPluginEnabled,
Expand Down
57 changes: 53 additions & 4 deletions pkg/services/anonymous/anonimpl/anonstore/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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),
}
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
}

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))
if err != nil {
return err
}

if count >= s.deviceLimit {
return s.updateDevice(ctx, device)
}
}
Comment on lines +108 to +118
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


args := []any{device.DeviceID, device.ClientIP, device.UserAgent,
device.CreatedAt.UTC(), device.UpdatedAt.UTC()}
switch s.sqlStore.GetDBType() {
Expand Down
25 changes: 23 additions & 2 deletions pkg/services/anonymous/anonimpl/anonstore/database_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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)
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{
Expand Down
6 changes: 2 additions & 4 deletions pkg/services/anonymous/anonimpl/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ import (
"github.com/grafana/grafana/pkg/util"
)

const (
thirtyDays = 30 * 24 * time.Hour
)
const anonymousDeviceExpiration = 30 * 24 * time.Hour

type deviceDTO struct {
anonstore.Device
Expand Down Expand Up @@ -70,7 +68,7 @@ func (api *AnonDeviceServiceAPI) RegisterAPIEndpoints() {
// 404: notFoundError
// 500: internalServerError
func (api *AnonDeviceServiceAPI) ListDevices(c *contextmodel.ReqContext) response.Response {
fromTime := time.Now().Add(-thirtyDays)
fromTime := time.Now().Add(-anonymousDeviceExpiration)
toTime := time.Now()

devices, err := api.store.ListDevices(c.Req.Context(), &fromTime, &toTime)
Expand Down
23 changes: 8 additions & 15 deletions pkg/services/anonymous/anonimpl/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,20 @@ package anonimpl

import (
"context"
"errors"
"net/http"
"strings"
"time"

"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/anonymous"
"github.com/grafana/grafana/pkg/services/anonymous/anonimpl/anonstore"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/setting"
)

var _ authn.ContextAwareClient = new(Anonymous)

const timeoutTag = 2 * time.Minute

type Anonymous struct {
cfg *setting.Cfg
log log.Logger
Expand All @@ -42,19 +41,13 @@ func (a *Anonymous) Authenticate(ctx context.Context, r *authn.Request) (*authn.
httpReqCopy.RemoteAddr = r.HTTPRequest.RemoteAddr
}

go func() {
defer func() {
if err := recover(); err != nil {
a.log.Warn("Tag anon session panic", "err", err)
}
}()

newCtx, cancel := context.WithTimeout(context.Background(), timeoutTag)
defer cancel()
if err := a.anonDeviceService.TagDevice(newCtx, httpReqCopy, anonymous.AnonDeviceUI); err != nil {
a.log.Warn("Failed to tag anonymous session", "error", err)
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)
}
Comment on lines +44 to +50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 Requests or 403 Forbidden with 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.


return &authn.Identity{
ID: authn.AnonymousNamespaceID,
Expand Down
8 changes: 5 additions & 3 deletions pkg/services/anonymous/anonimpl/impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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),
serverLock: serverLockService,
}

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 3 additions & 5 deletions pkg/services/anonymous/anonimpl/impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
require.NoError(t, err)
require.Len(t, devices, int(tc.expectedAnonUICount))
if tc.expectedDevice != nil {
Expand All @@ -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{
Expand Down
11 changes: 7 additions & 4 deletions pkg/setting/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ type Cfg struct {
AnonymousOrgName string
AnonymousOrgRole string
AnonymousHideVersion bool
AnonymousDeviceLimit int64

DateFormats DateFormats

Expand Down Expand Up @@ -1646,10 +1647,12 @@ func readAuthSettings(iniFile *ini.File, cfg *Cfg) (err error) {
readAuthGithubSettings(cfg)

// anonymous access
cfg.AnonymousEnabled = iniFile.Section("auth.anonymous").Key("enabled").MustBool(false)
cfg.AnonymousOrgName = valueAsString(iniFile.Section("auth.anonymous"), "org_name", "")
cfg.AnonymousOrgRole = valueAsString(iniFile.Section("auth.anonymous"), "org_role", "")
cfg.AnonymousHideVersion = iniFile.Section("auth.anonymous").Key("hide_version").MustBool(false)
anonSection := iniFile.Section("auth.anonymous")
cfg.AnonymousEnabled = anonSection.Key("enabled").MustBool(false)
cfg.AnonymousOrgName = valueAsString(anonSection, "org_name", "")
cfg.AnonymousOrgRole = valueAsString(anonSection, "org_role", "")
cfg.AnonymousHideVersion = anonSection.Key("hide_version").MustBool(false)
cfg.AnonymousDeviceLimit = anonSection.Key("device_limit").MustInt64(0)

// basic auth
authBasic := iniFile.Section("auth.basic")
Expand Down