Skip to content
Jump to bottom

Automated Test: implement-device-limits #335

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

Automated Test: implement-device-limits #335

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;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add explicit type annotation for type consistency.

The property is initialized to undefined but lacks an explicit type annotation. TypeScript will infer the type as undefined rather than number | undefined, which could cause type errors when the actual numeric value is merged from boot data. This should match the interface definition in GrafanaConfig.

Proposed fix 
-  anonymousDeviceLimit = undefined;
+  anonymousDeviceLimit: number | undefined = undefined;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
anonymousDeviceLimit = undefined;
anonymousDeviceLimit: number | undefined = undefined;
🤖 Prompt for AI Agents 
In `@packages/grafana-runtime/src/config.ts` at line 97, The property
anonymousDeviceLimit is initialized to undefined without a type, so add an
explicit type annotation matching the GrafanaConfig interface (e.g.,
anonymousDeviceLimit: number | undefined = undefined) where anonymousDeviceLimit
is declared/initialized to ensure the inferred type allows merging numeric boot
data and stays consistent with GrafanaConfig.

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}
}
Comment on lines +52 to 54
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if wire.go still references ProvideAnonDBStore and how it's used

# Find wire bindings for ProvideAnonDBStore
echo "=== Wire bindings for ProvideAnonDBStore ==="
rg -n "ProvideAnonDBStore" --type go

# Check if there's any direct usage in wire sets
echo -e "\n=== Full context of wire usage ==="
rg -n -B5 -A5 "anonstore\.ProvideAnonDBStore|ProvideAnonDBStore" --type go -g '*wire*'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1633

Remove or update wire binding for new constructor signature.

The wire binding at pkg/server/wire.go:375 references anonstore.ProvideAnonDBStore as a bare function pointer without providing the now-required deviceLimit parameter. Since deviceLimit is a primitive type (int64), wire cannot auto-inject it. This will cause a compilation error. Either remove the wire binding if manual initialization in impl.go is sufficient, or update it to provide the deviceLimit parameter.

🤖 Prompt for AI Agents 
In `@pkg/services/anonymous/anonimpl/anonstore/database.go` around lines 52 - 54,
The wire binding references anonstore.ProvideAnonDBStore but the constructor now
requires a primitive deviceLimit (int64) that wire can't inject; update the DI
binding in pkg/server/wire.go to either remove the bare reference to
ProvideAnonDBStore and use manual initialization in impl.go, or change the
provider set to call ProvideAnonDBStore with a concrete deviceLimit value (or a
provider that returns int64) so the deviceLimit parameter is satisfied; locate
ProvideAnonDBStore and the wire binding entry for anonstore in wire.go and
ensure the provider set matches the new signature.


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)
}
}

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)
}

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