From 544b5f905ca4b69ca40da7cca35d851a66a14b88 Mon Sep 17 00:00:00 2001 From: Jo Date: Fri, 4 Oct 2024 15:20:55 +0200 Subject: [PATCH] Anonymous: Fix anonymous cache ignoring device limit evaluation (#94218) * ensure cache contains the evaluation result for device limit * add device limit errors and warnings * fix lint --- pkg/services/anonymous/anonimpl/client.go | 7 +- pkg/services/anonymous/anonimpl/impl.go | 18 +++- pkg/services/anonymous/anonimpl/impl_test.go | 96 ++++++++++++++++++++ 3 files changed, 114 insertions(+), 7 deletions(-) diff --git a/pkg/services/anonymous/anonimpl/client.go b/pkg/services/anonymous/anonimpl/client.go index 57b4c9873fb..43c1d8107c2 100644 --- a/pkg/services/anonymous/anonimpl/client.go +++ b/pkg/services/anonymous/anonimpl/client.go @@ -17,8 +17,9 @@ import ( ) var ( - errInvalidOrg = errutil.Unauthorized("anonymous.invalid-org") - errInvalidID = errutil.Unauthorized("anonymous.invalid-id") + errInvalidOrg = errutil.Unauthorized("anonymous.invalid-org") + errInvalidID = errutil.Unauthorized("anonymous.invalid-id") + errDeviceLimit = errutil.Unauthorized("anonymous.device-limit-reached", errutil.WithPublicMessage("Anonymous device limit reached. Contact Administrator")) ) var _ authn.ContextAwareClient = new(Anonymous) @@ -51,7 +52,7 @@ func (a *Anonymous) Authenticate(ctx context.Context, r *authn.Request) (*authn. if err := a.anonDeviceService.TagDevice(ctx, httpReqCopy, anonymous.AnonDeviceUI); err != nil { if errors.Is(err, anonstore.ErrDeviceLimitReached) { - return nil, err + return nil, errDeviceLimit.Errorf("limit reached for anonymous devices: %w", err) } a.log.Warn("Failed to tag anonymous session", "error", err) diff --git a/pkg/services/anonymous/anonimpl/impl.go b/pkg/services/anonymous/anonimpl/impl.go index b34048be407..c0105441d62 100644 --- a/pkg/services/anonymous/anonimpl/impl.go +++ b/pkg/services/anonymous/anonimpl/impl.go @@ -2,6 +2,7 @@ package anonimpl import ( "context" + "errors" "net/http" "time" @@ -79,20 +80,29 @@ func (a *AnonDeviceService) usageStatFn(ctx context.Context) (map[string]any, er }, nil } -func (a *AnonDeviceService) tagDeviceUI(ctx context.Context, httpReq *http.Request, device *anonstore.Device) error { +func (a *AnonDeviceService) tagDeviceUI(ctx context.Context, device *anonstore.Device) error { key := device.CacheKey() - if _, ok := a.localCache.Get(key); ok { + if val, ok := a.localCache.Get(key); ok { + if boolVal, ok := val.(bool); ok && !boolVal { + return anonstore.ErrDeviceLimitReached + } return nil } - a.localCache.SetDefault(key, struct{}{}) + a.localCache.SetDefault(key, true) if a.cfg.Env == setting.Dev { a.log.Debug("Tagging device for UI", "deviceID", device.DeviceID, "device", device, "key", key) } if err := a.anonStore.CreateOrUpdateDevice(ctx, device); err != nil { + if errors.Is(err, anonstore.ErrDeviceLimitReached) { + a.localCache.SetDefault(key, false) + return err + } + // invalidate cache if there is an error + a.localCache.Delete(key) return err } @@ -142,7 +152,7 @@ func (a *AnonDeviceService) TagDevice(ctx context.Context, httpReq *http.Request UpdatedAt: time.Now(), } - err = a.tagDeviceUI(ctx, httpReq, taggedDevice) + err = a.tagDeviceUI(ctx, taggedDevice) if err != nil { a.log.Debug("Failed to tag device for UI", "error", err) return err diff --git a/pkg/services/anonymous/anonimpl/impl_test.go b/pkg/services/anonymous/anonimpl/impl_test.go index a84e913f3b1..b193d22edb6 100644 --- a/pkg/services/anonymous/anonimpl/impl_test.go +++ b/pkg/services/anonymous/anonimpl/impl_test.go @@ -26,6 +26,10 @@ func TestMain(m *testing.M) { } func TestIntegrationDeviceService_tag(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + type tagReq struct { httpReq *http.Request kind anonymous.DeviceKind @@ -152,6 +156,9 @@ func TestIntegrationDeviceService_tag(t *testing.T) { // Ensure that the local cache prevents request from being tagged func TestIntegrationAnonDeviceService_localCacheSafety(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } store := db.InitTestDB(t) anonService := ProvideAnonymousDeviceService(&usagestats.UsageStatsMock{}, &authntest.FakeService{}, store, setting.NewCfg(), orgtest.NewOrgServiceFake(), nil, actest.FakeAccessControl{}, &routing.RouteRegisterImpl{}) @@ -184,6 +191,10 @@ func TestIntegrationAnonDeviceService_localCacheSafety(t *testing.T) { } func TestIntegrationDeviceService_SearchDevice(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + fixedTime := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC) // Fixed timestamp for testing testCases := []struct { @@ -271,3 +282,88 @@ func TestIntegrationDeviceService_SearchDevice(t *testing.T) { }) } } + +func TestIntegrationAnonDeviceService_DeviceLimitWithCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + // Setup test environment + store := db.InitTestDB(t) + cfg := setting.NewCfg() + cfg.AnonymousDeviceLimit = 1 // Set device limit to 1 for testing + anonService := ProvideAnonymousDeviceService( + &usagestats.UsageStatsMock{}, + &authntest.FakeService{}, + store, + cfg, + orgtest.NewOrgServiceFake(), + nil, + actest.FakeAccessControl{}, + &routing.RouteRegisterImpl{}, + ) + + // Define test cases + testCases := []struct { + name string + httpReq *http.Request + expectedErr error + }{ + { + name: "first request should succeed", + httpReq: &http.Request{ + Header: http.Header{ + "User-Agent": []string{"test"}, + "X-Forwarded-For": []string{"10.30.30.1"}, + http.CanonicalHeaderKey(deviceIDHeader): []string{"device1"}, + }, + }, + expectedErr: nil, + }, + { + name: "second request should fail due to device limit", + httpReq: &http.Request{ + Header: http.Header{ + "User-Agent": []string{"test"}, + "X-Forwarded-For": []string{"10.30.30.2"}, + http.CanonicalHeaderKey(deviceIDHeader): []string{"device2"}, + }, + }, + expectedErr: anonstore.ErrDeviceLimitReached, + }, + { + name: "repeat request should hit cache and succeed", + httpReq: &http.Request{ + Header: http.Header{ + "User-Agent": []string{"test"}, + "X-Forwarded-For": []string{"10.30.30.1"}, + http.CanonicalHeaderKey(deviceIDHeader): []string{"device1"}, + }, + }, + expectedErr: nil, + }, + { + name: "third request should hit cache and fail due to device limit", + httpReq: &http.Request{ + Header: http.Header{ + "User-Agent": []string{"test"}, + "X-Forwarded-For": []string{"10.30.30.2"}, + http.CanonicalHeaderKey(deviceIDHeader): []string{"device2"}, + }, + }, + expectedErr: anonstore.ErrDeviceLimitReached, + }, + } + + // Run test cases + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := anonService.TagDevice(context.Background(), tc.httpReq, anonymous.AnonDeviceUI) + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr, err) + } else { + require.NoError(t, err) + } + }) + } +}