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

@@ -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,

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)

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
+		desc            string
-		SkipOrgRoleSync         bool
+		SkipOrgRoleSync bool
-		AuthEnabled             bool
+		AuthEnabled     bool
-		AuthModule              string
+		AuthModule      string
-		shouldInvalidateIDToken bool
+		expectedCode    int
-		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",
+			desc:            "should be able to change basicRole when skip_org_role_sync true",
-			SkipOrgRoleSync:         true,
+			SkipOrgRoleSync: true,
-			AuthEnabled:             true,
+			AuthEnabled:     true,
-			AuthModule:              login.LDAPAuthModule,
+			AuthModule:      login.LDAPAuthModule,
-			shouldInvalidateIDToken: true,
+			expectedCode:    http.StatusOK,
-			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",
+			desc:            "should be able to change basicRole with a basic Auth",
-			SkipOrgRoleSync:         false,
+			SkipOrgRoleSync: false,
-			AuthEnabled:             false,
+			AuthEnabled:     false,
-			AuthModule:              "",
+			AuthModule:      "",
-			shouldInvalidateIDToken: true,
+			expectedCode:    http.StatusOK,
-			expectedCode:            http.StatusOK,
 		},
 		{
-			desc:                    "should be able to change basicRole with a basic Auth",
+			desc:            "should be able to change basicRole with a basic Auth",
-			SkipOrgRoleSync:         true,
+			SkipOrgRoleSync: true,
-			AuthEnabled:             true,
+			AuthEnabled:     true,
-			AuthModule:              "",
+			AuthModule:      "",
-			shouldInvalidateIDToken: true,
+			expectedCode:    http.StatusOK,
-			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",
+			name:         "user with permissions can update org role",
-			permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionOrgUsersWrite, Scope: "users:*"}},
+			permissions:  []accesscontrol.Permission{{Action: accesscontrol.ActionOrgUsersWrite, Scope: "users:*"}},
-			role:        org.RoleAdmin,
+			role:         org.RoleAdmin,
-			input:       `{"role": "Viewer"}`,
+			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)
-			},
 			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{},

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 {
+func getCacheKey(ident identity.Requester) string {
-	return fmt.Sprintf("%s-%s", cachePrefix, key)
+	return cachePrefix + ident.GetCacheKey() + string(ident.GetOrgRole())
 }
 
 func shouldLogErr(err error) bool {

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

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