From be60ef0500a603e667598ef47bbab8204885aeea Mon Sep 17 00:00:00 2001 From: Karl Persson <23356117+kalleep@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:10:58 +0100 Subject: [PATCH] IDToken: cache invalidation (#100592) * Make org role part of id token cache key. This way we will always sign a new token when it changes * Remove calls to remove id token --- pkg/api/http_server.go | 4 +- pkg/api/org_users.go | 6 -- pkg/api/org_users_test.go | 74 ++++++------------- pkg/services/auth/idimpl/service.go | 8 +- pkg/services/auth/idimpl/service_test.go | 31 ++++++++ .../serviceaccounts/manager/service.go | 9 --- 6 files changed, 60 insertions(+), 72 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 850a4ea5fd5..21f6f2dcd56 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -207,7 +207,6 @@ type HTTPServer struct { tempUserService tempUser.Service loginAttemptService loginAttempt.Service orgService org.Service - idService auth.IDService orgDeletionService org.DeletionService TeamService team.Service accesscontrolService accesscontrol.Service @@ -273,7 +272,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi annotationRepo annotations.Repository, tagService tag.Service, searchv2HTTPService searchV2.SearchHTTPService, oauthTokenService oauthtoken.OAuthTokenService, statsService stats.Service, authnService authn.Service, pluginsCDNService *pluginscdn.Service, promGatherer prometheus.Gatherer, starApi *starApi.API, promRegister prometheus.Registerer, clientConfigProvider grafanaapiserver.DirectRestConfigProvider, anonService anonymous.Service, - userVerifier user.Verifier, pluginPreinstall plugininstaller.Preinstall, idService auth.IDService, + userVerifier user.Verifier, pluginPreinstall plugininstaller.Preinstall, ) (*HTTPServer, error) { web.Env = cfg.Env m := web.New() @@ -361,7 +360,6 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi tempUserService: tempUserService, loginAttemptService: loginAttemptService, orgService: orgService, - idService: idService, orgDeletionService: orgDeletionService, TeamService: teamService, navTreeService: navTreeService, diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index f19d4138bb8..1b83b4e3b77 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -7,11 +7,9 @@ import ( "net/http" "strconv" - claims "github.com/grafana/authlib/types" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/authn" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" @@ -434,10 +432,6 @@ func (hs *HTTPServer) updateOrgUserHelper(c *contextmodel.ReqContext, cmd org.Up } } - if err := hs.idService.RemoveIDToken(c.Req.Context(), &authn.Identity{ID: strconv.FormatInt(cmd.UserID, 10), Type: claims.TypeUser, OrgID: cmd.OrgID}); err != nil { - return response.Error(http.StatusInternalServerError, "Failed to invalidate the ID token cache", err) - } - if err := hs.orgService.UpdateOrgUser(c.Req.Context(), &cmd); err != nil { if errors.Is(err, org.ErrLastOrgAdmin) { return response.Error(http.StatusBadRequest, "Cannot change role so that there is no organization admin left", nil) diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 324e9da2b39..59bb47330d6 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -9,12 +9,9 @@ import ( "strings" "testing" - "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/services/auth/idtest" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/authn/authntest" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/api/dtos" @@ -205,12 +202,11 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { type testCase struct { - desc string - SkipOrgRoleSync bool - AuthEnabled bool - AuthModule string - shouldInvalidateIDToken bool - expectedCode int + desc string + SkipOrgRoleSync bool + AuthEnabled bool + AuthModule string + expectedCode int } permissions := []accesscontrol.Permission{ {Action: accesscontrol.ActionOrgUsersRead, Scope: "users:*"}, @@ -220,12 +216,11 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { } tests := []testCase{ { - desc: "should be able to change basicRole when skip_org_role_sync true", - SkipOrgRoleSync: true, - AuthEnabled: true, - AuthModule: login.LDAPAuthModule, - shouldInvalidateIDToken: true, - expectedCode: http.StatusOK, + desc: "should be able to change basicRole when skip_org_role_sync true", + SkipOrgRoleSync: true, + AuthEnabled: true, + AuthModule: login.LDAPAuthModule, + expectedCode: http.StatusOK, }, { desc: "should not be able to change basicRole when skip_org_role_sync false", @@ -242,20 +237,18 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { expectedCode: http.StatusForbidden, }, { - desc: "should be able to change basicRole with a basic Auth", - SkipOrgRoleSync: false, - AuthEnabled: false, - AuthModule: "", - shouldInvalidateIDToken: true, - expectedCode: http.StatusOK, + desc: "should be able to change basicRole with a basic Auth", + SkipOrgRoleSync: false, + AuthEnabled: false, + AuthModule: "", + expectedCode: http.StatusOK, }, { - desc: "should be able to change basicRole with a basic Auth", - SkipOrgRoleSync: true, - AuthEnabled: true, - AuthModule: "", - shouldInvalidateIDToken: true, - expectedCode: http.StatusOK, + desc: "should be able to change basicRole with a basic Auth", + SkipOrgRoleSync: true, + AuthEnabled: true, + AuthModule: "", + expectedCode: http.StatusOK, }, } @@ -286,11 +279,6 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { } hs.userService = &usertest.FakeUserService{ExpectedSignedInUser: userWithPermissions} hs.orgService = &orgtest.FakeOrgService{} - idService := &idtest.MockService{} - if tt.shouldInvalidateIDToken { - idService.On("RemoveIDToken", mock.Anything, mock.Anything).Return(nil) - } - hs.idService = idService hs.SocialService = &socialtest.FakeSocialService{ ExpectedAuthInfoProvider: &social.OAuthInfo{Enabled: tt.AuthEnabled, SkipOrgRoleSync: tt.SkipOrgRoleSync}, } @@ -627,7 +615,6 @@ func TestOrgUsersAPIEndpointWithSetPerms_AccessControl(t *testing.T) { ExpectedUser: &user.User{}, ExpectedSignedInUser: userWithPermissions(1, tt.permissions), } - hs.idService = &idtest.FakeService{} hs.accesscontrolService = &actest.FakeService{} }) @@ -650,24 +637,16 @@ func TestPatchOrgUsersAPIEndpoint_AccessControl(t *testing.T) { name string role org.RoleType permissions []accesscontrol.Permission - setup func(*testing.T, *idtest.MockService) input string expectedCode int } tests := []testCase{ { - name: "user with permissions can update org role", - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionOrgUsersWrite, Scope: "users:*"}}, - role: org.RoleAdmin, - input: `{"role": "Viewer"}`, - setup: func(t *testing.T, idService *idtest.MockService) { - idService.On("RemoveIDToken", mock.Anything, mock.MatchedBy(func(id *authn.Identity) bool { - return id.GetIdentityType() == types.TypeUser && - id.GetID() == "user:1" && - id.GetOrgID() == int64(1) - })).Return(nil) - }, + name: "user with permissions can update org role", + permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionOrgUsersWrite, Scope: "users:*"}}, + role: org.RoleAdmin, + input: `{"role": "Viewer"}`, expectedCode: http.StatusOK, }, { @@ -694,11 +673,6 @@ func TestPatchOrgUsersAPIEndpoint_AccessControl(t *testing.T) { AuthModule: "", }, } - idService := &idtest.MockService{} - if tt.setup != nil { - tt.setup(t, idService) - } - hs.idService = idService hs.accesscontrolService = &actest.FakeService{} hs.userService = &usertest.FakeUserService{ ExpectedUser: &user.User{}, diff --git a/pkg/services/auth/idimpl/service.go b/pkg/services/auth/idimpl/service.go index a4b89204d7b..5dec411fdee 100644 --- a/pkg/services/auth/idimpl/service.go +++ b/pkg/services/auth/idimpl/service.go @@ -63,7 +63,7 @@ func (s *Service) SignIdentity(ctx context.Context, id identity.Requester) (stri s.metrics.tokenSigningDurationHistogram.Observe(time.Since(t).Seconds()) }(time.Now()) - cacheKey := prefixCacheKey(id.GetCacheKey()) + cacheKey := getCacheKey(id) type resultType struct { token string @@ -140,7 +140,7 @@ func (s *Service) SignIdentity(ctx context.Context, id identity.Requester) (stri } func (s *Service) RemoveIDToken(ctx context.Context, id identity.Requester) error { - return s.cache.Delete(ctx, prefixCacheKey(id.GetCacheKey())) + return s.cache.Delete(ctx, getCacheKey(id)) } func (s *Service) hook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { @@ -181,8 +181,8 @@ func getAudience(orgID int64) jwt.Audience { return jwt.Audience{fmt.Sprintf("org:%d", orgID)} } -func prefixCacheKey(key string) string { - return fmt.Sprintf("%s-%s", cachePrefix, key) +func getCacheKey(ident identity.Requester) string { + return cachePrefix + ident.GetCacheKey() + string(ident.GetOrgRole()) } func shouldLogErr(err error) bool { diff --git a/pkg/services/auth/idimpl/service_test.go b/pkg/services/auth/idimpl/service_test.go index 0f3814968e3..bb7ee510e55 100644 --- a/pkg/services/auth/idimpl/service_test.go +++ b/pkg/services/auth/idimpl/service_test.go @@ -16,6 +16,7 @@ import ( "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/authn/authntest" "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/setting" ) @@ -101,4 +102,34 @@ func TestService_SignIdentity(t *testing.T) { assert.Equal(t, claims.TypeUser, gotClaims.Rest.Type) assert.Equal(t, "edpu3nnt61se8e", gotClaims.Rest.Identifier) }) + + t.Run("should sign new token if org role has changed", func(t *testing.T) { + s := ProvideService( + setting.NewCfg(), signer, remotecache.NewFakeCacheStorage(), + &authntest.FakeService{}, nil, + ) + + ident := &authn.Identity{ + ID: "1", + Type: claims.TypeUser, + AuthenticatedBy: login.AzureADAuthModule, + Login: "U1", + UID: "edpu3nnt61se8e", + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin}, + } + + first, _, err := s.SignIdentity(context.Background(), ident) + require.NoError(t, err) + + second, _, err := s.SignIdentity(context.Background(), ident) + require.NoError(t, err) + + assert.Equal(t, first, second) + + ident.OrgRoles[1] = org.RoleEditor + third, _, err := s.SignIdentity(context.Background(), ident) + require.NoError(t, err) + assert.NotEqual(t, first, third) + }) } diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index f53e7b72d9b..cde680f616d 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -17,8 +17,6 @@ import ( "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/apikey" - "github.com/grafana/grafana/pkg/services/auth" - "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" @@ -44,7 +42,6 @@ type ServiceAccountsService struct { secretScanService secretscan.Checker orgService org.Service serverLock *serverlock.ServerLockService - idService auth.IDService secretScanEnabled bool secretScanInterval time.Duration @@ -61,7 +58,6 @@ func ProvideServiceAccountsService( acService accesscontrol.Service, permissions accesscontrol.ServiceAccountPermissionsService, serverLockService *serverlock.ServerLockService, - idService auth.IDService, ) (*ServiceAccountsService, error) { serviceAccountsStore := database.ProvideServiceAccountsStore( cfg, @@ -81,7 +77,6 @@ func ProvideServiceAccountsService( backgroundLog: log.New("serviceaccounts.background"), orgService: orgService, serverLock: serverLockService, - idService: idService, } if err := RegisterRoles(acService); err != nil { @@ -271,10 +266,6 @@ func (sa *ServiceAccountsService) UpdateServiceAccount(ctx context.Context, orgI return nil, err } - if err := sa.idService.RemoveIDToken(ctx, &authn.Identity{ID: strconv.FormatInt(serviceAccountID, 10), Type: claims.TypeServiceAccount, OrgID: orgID}); err != nil { - return nil, err - } - return sa.store.UpdateServiceAccount(ctx, orgID, serviceAccountID, saForm) }