AuthN: Cleanup authn package (#63456)

* AuthN: Update comments for ClientParams

* AuthN: Update flag name from SyncTeamMembers to SyncTeams

* UserSync: rename function and fix order of parameters so it is correct

* UserSync: Fix so we skip check if no authModule or authID is passed

* UserSync: move quota check to create user function

* UserSync: Move FetchSyncedUserHook to UserSync

* UserSync: Move last seen user hook to user sync service

* ApiKey: Implement last seen hook as a client hook instead

pkg/services/authn/authnimpl/sync/apikey_last_seen_sync.go

@@ -1,38 +0,0 @@
-package sync
-
-import (
-	"context"
-
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/services/apikey"
-	"github.com/grafana/grafana/pkg/services/authn"
-)
-
-func ProvideAPIKeyLastSeenSync(service apikey.Service) *APIKeyLastSeenSync {
-	return &APIKeyLastSeenSync{log.New("apikeylastseen.sync"), service}
-}
-
-type APIKeyLastSeenSync struct {
-	log     log.Logger
-	service apikey.Service
-}
-
-func (s *APIKeyLastSeenSync) SyncLastSeen(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
-	namespace, id := identity.NamespacedID()
-	if namespace != authn.NamespaceAPIKey {
-		return nil
-	}
-
-	go func(apikeyID int64) {
-		defer func() {
-			if err := recover(); err != nil {
-				s.log.Error("panic during user last seen sync", "err", err)
-			}
-		}()
-		if err := s.service.UpdateAPIKeyLastUsedDate(context.Background(), apikeyID); err != nil {
-			s.log.Warn("failed to update last use date for api key", "id", apikeyID)
-		}
-	}(id)
-
-	return nil
-}

pkg/services/authn/authnimpl/sync/fetch_user_sync.go

@@ -1,56 +0,0 @@
-package sync
-
-import (
-	"context"
-
-	"github.com/grafana/grafana/pkg/services/authn"
-	"github.com/grafana/grafana/pkg/services/org"
-	"github.com/grafana/grafana/pkg/services/user"
-	"github.com/grafana/grafana/pkg/util/errutil"
-)
-
-var errFetchingSignedInUser = errutil.NewBase(errutil.StatusInternal, "user.sync.fetch", errutil.WithPublicMessage("Insufficient information to authenticate user"))
-
-func ProvideFetchUserSync(service user.Service) *FetchUserSync {
-	return &FetchUserSync{service}
-}
-
-type FetchUserSync struct {
-	userService user.Service
-}
-
-func (s *FetchUserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
-	if !identity.ClientParams.FetchSyncedUser {
-		return nil
-	}
-	namespace, id := identity.NamespacedID()
-	if namespace != authn.NamespaceUser {
-		return nil
-	}
-
-	usr, err := s.userService.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{
-		UserID: id,
-		OrgID:  r.OrgID,
-	})
-	if err != nil {
-		return errFetchingSignedInUser.Errorf("failed to resolve user: %w", err)
-	}
-
-	syncSignedInUserToIdentity(usr, identity)
-	return nil
-}
-
-func syncSignedInUserToIdentity(usr *user.SignedInUser, identity *authn.Identity) {
-	identity.Name = usr.Name
-	identity.Login = usr.Login
-	identity.Email = usr.Email
-	identity.OrgID = usr.OrgID
-	identity.OrgName = usr.OrgName
-	identity.OrgCount = usr.OrgCount
-	identity.OrgRoles = map[int64]org.RoleType{identity.OrgID: usr.OrgRole}
-	identity.HelpFlags1 = usr.HelpFlags1
-	identity.Teams = usr.Teams
-	identity.LastSeenAt = usr.LastSeenAt
-	identity.IsDisabled = usr.IsDisabled
-	identity.IsGrafanaAdmin = &usr.IsGrafanaAdmin
-}

pkg/services/authn/authnimpl/sync/fetch_user_sync_test.go

@@ -1,40 +0,0 @@
-package sync
-
-import (
-	"context"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-
-	"github.com/grafana/grafana/pkg/services/authn"
-)
-
-func TestFetchUserSync_FetchSyncedUserHook(t *testing.T) {
-	type testCase struct {
-		desc        string
-		req         *authn.Request
-		identity    *authn.Identity
-		expectedErr error
-	}
-
-	tests := []testCase{
-		{
-			desc:     "should skip hook when flag is not enabled",
-			req:      &authn.Request{},
-			identity: &authn.Identity{ClientParams: authn.ClientParams{FetchSyncedUser: false}},
-		},
-		{
-			desc:     "should skip hook when identity is not a user",
-			req:      &authn.Request{},
-			identity: &authn.Identity{ID: "apikey:1", ClientParams: authn.ClientParams{FetchSyncedUser: true}},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.desc, func(t *testing.T) {
-			s := ProvideFetchUserSync(nil)
-			err := s.FetchSyncedUserHook(context.Background(), tt.identity, tt.req)
-			require.ErrorIs(t, err, tt.expectedErr)
-		})
-	}
-}

pkg/services/authn/authnimpl/sync/oauth_token_sync.go

@@ -31,7 +31,7 @@ type OauthTokenSync struct {
 	sessionService auth.UserTokenService
 }
 
-func (s *OauthTokenSync) SyncOauthToken(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
+func (s *OauthTokenSync) SyncOauthTokenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
 	namespace, id := identity.NamespacedID()
 	// only perform oauth token check if identity is a user
 	if namespace != authn.NamespaceUser {

pkg/services/authn/authnimpl/sync/oauth_token_sync_test.go

@@ -17,7 +17,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/user"
 )
 
-func TestOauthTokenSync_SyncOauthToken(t *testing.T) {
+func TestOauthTokenSync_SyncOauthTokenHook(t *testing.T) {
 	type testCase struct {
 		desc     string
 		identity *authn.Identity
@@ -123,7 +123,7 @@ func TestOauthTokenSync_SyncOauthToken(t *testing.T) {
 				sessionService: sessionService,
 			}
 
-			err := sync.SyncOauthToken(context.Background(), tt.identity, nil)
+			err := sync.SyncOauthTokenHook(context.Background(), tt.identity, nil)
 			assert.ErrorIs(t, err, tt.expectedErr)
 			assert.Equal(t, tt.expectHasEntryCalled, hasEntryCalled)
 			assert.Equal(t, tt.expectTryRefreshTokenCalled, tryRefreshCalled)

pkg/services/authn/authnimpl/sync/org_sync.go

@@ -24,13 +24,13 @@ type OrgSync struct {
 	log log.Logger
 }
 
-func (s *OrgSync) SyncOrgUser(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
+func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
 	if !id.ClientParams.SyncUser {
 		return nil
 	}
 
 	namespace, userID := id.NamespacedID()
-	if namespace != "user" || userID <= 0 {
+	if namespace != authn.NamespaceUser || userID <= 0 {
 		s.log.Warn("invalid namespace %q for user ID %q", namespace, userID)
 		return nil
 	}

pkg/services/authn/authnimpl/sync/org_sync_test.go

@@ -18,7 +18,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/user/usertest"
 )
 
-func TestOrgSync_SyncOrgUser(t *testing.T) {
+func TestOrgSync_SyncOrgRolesHook(t *testing.T) {
 	orgService := &orgtest.FakeOrgService{ExpectedUserOrgDTO: []*org.UserOrgDTO{
 		{
 			OrgID: 1,
@@ -116,8 +116,8 @@ func TestOrgSync_SyncOrgUser(t *testing.T) {
 				accessControl: tt.fields.accessControl,
 				log:           tt.fields.log,
 			}
-			if err := s.SyncOrgUser(tt.args.ctx, tt.args.id, nil); (err != nil) != tt.wantErr {
-				t.Errorf("OrgSync.SyncOrgUser() error = %v, wantErr %v", err, tt.wantErr)
+			if err := s.SyncOrgRolesHook(tt.args.ctx, tt.args.id, nil); (err != nil) != tt.wantErr {
+				t.Errorf("OrgSync.SyncOrgRolesHook() error = %v, wantErr %v", err, tt.wantErr)
 			}
 
 			assert.EqualValues(t, tt.wantID, tt.args.id)

pkg/services/authn/authnimpl/sync/user_last_seen_sync.go

@@ -1,50 +0,0 @@
-package sync
-
-import (
-	"context"
-	"time"
-
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/services/authn"
-	"github.com/grafana/grafana/pkg/services/user"
-)
-
-func ProvideUserLastSeenSync(service user.Service) *UserLastSeenSync {
-	return &UserLastSeenSync{log.New("userlastseen.sync"), service}
-}
-
-type UserLastSeenSync struct {
-	log     log.Logger
-	service user.Service
-}
-
-func (s *UserLastSeenSync) SyncLastSeen(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
-	namespace, id := identity.NamespacedID()
-
-	if namespace != authn.NamespaceUser && namespace != authn.NamespaceServiceAccount {
-		// skip sync
-		return nil
-	}
-
-	if !shouldUpdateLastSeen(identity.LastSeenAt) {
-		return nil
-	}
-
-	go func(userID int64) {
-		defer func() {
-			if err := recover(); err != nil {
-				s.log.Error("panic during user last seen sync", "err", err)
-			}
-		}()
-
-		if err := s.service.UpdateLastSeenAt(context.Background(), &user.UpdateUserLastSeenAtCommand{UserID: userID}); err != nil {
-			s.log.Error("failed to update last_seen_at", "err", err, "userId", userID)
-		}
-	}(id)
-
-	return nil
-}
-
-func shouldUpdateLastSeen(t time.Time) bool {
-	return time.Since(t) > time.Minute*5
-}

pkg/services/authn/authnimpl/sync/user_sync.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/authn"
@@ -15,12 +16,26 @@ import (
 )
 
 var (
-	errSyncUserForbidden = errutil.NewBase(errutil.StatusForbidden,
-		"user.sync.forbidden", errutil.WithPublicMessage("User sync forbidden"))
-	errSyncUserInternal = errutil.NewBase(errutil.StatusInternal,
-		"user.sync.forbidden", errutil.WithPublicMessage("User sync failed"))
-	errUserProtection = errutil.NewBase(errutil.StatusForbidden,
-		"user.sync.protectedrole", errutil.WithPublicMessage("Unable to sync due to protected role"))
+	errSyncUserForbidden = errutil.NewBase(
+		errutil.StatusForbidden,
+		"user.sync.forbidden",
+		errutil.WithPublicMessage("User sync forbidden"),
+	)
+	errSyncUserInternal = errutil.NewBase(
+		errutil.StatusInternal,
+		"user.sync.forbidden",
+		errutil.WithPublicMessage("User sync failed"),
+	)
+	errUserProtection = errutil.NewBase(
+		errutil.StatusForbidden,
+		"user.sync.protected-role",
+		errutil.WithPublicMessage("Unable to sync due to protected role"),
+	)
+	errFetchingSignedInUser = errutil.NewBase(
+		errutil.StatusInternal,
+		"user.sync.fetch",
+		errutil.WithPublicMessage("Insufficient information to authenticate user"),
+	)
 )
 
 func ProvideUserSync(userService user.Service,
@@ -43,14 +58,14 @@ type UserSync struct {
 	log                   log.Logger
 }
 
-// SyncUser syncs a user with the database
-func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
+// SyncUserHook syncs a user with the database
+func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
 	if !id.ClientParams.SyncUser {
 		return nil
 	}
 
 	// Does user exist in the database?
-	usr, errUserInDB := s.UserInDB(ctx, &id.AuthModule, &id.AuthID, id.ClientParams.LookUpParams)
+	usr, errUserInDB := s.getUser(ctx, id.AuthModule, id.AuthID, id.ClientParams.LookUpParams)
 	if errUserInDB != nil && !errors.Is(errUserInDB, user.ErrUserNotFound) {
 		s.log.Error("error retrieving user", "error", errUserInDB,
 			"auth_module", id.AuthModule, "auth_id", id.AuthID,
@@ -66,20 +81,6 @@ func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Re
 			return errSyncUserForbidden.Errorf("%w", login.ErrSignupNotAllowed)
 		}
 
-		// quota check (FIXME: (jguer) this should be done in the user service)
-		// we may insert in both user and org_user tables
-		// therefore we need to query check quota for both user and org services
-		for _, srv := range []string{user.QuotaTargetSrv, org.QuotaTargetSrv} {
-			limitReached, errLimit := s.quotaService.CheckQuotaReached(ctx, quota.TargetSrv(srv), nil)
-			if errLimit != nil {
-				s.log.Error("error getting user quota", "error", errLimit)
-				return errSyncUserInternal.Errorf("%w", login.ErrGettingUserQuota)
-			}
-			if limitReached {
-				return errSyncUserForbidden.Errorf("%w", login.ErrUsersQuotaReached)
-			}
-		}
-
 		// create user
 		var errCreate error
 		usr, errCreate = s.createUser(ctx, id)
@@ -108,7 +109,7 @@ func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Re
 
 	syncUserToIdentity(usr, id)
 
-	// persist latest auth info token
+	// persist the latest auth info token
 	if errAuthInfo := s.updateAuthInfo(ctx, id); errAuthInfo != nil {
 		s.log.Error("error creating user", "error", errAuthInfo,
 			"auth_module", id.AuthModule, "auth_id", id.AuthID,
@@ -119,14 +120,52 @@ func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Re
 	return nil
 }
 
-// syncUserToIdentity syncs a user to an identity.
-// This is used to update the identity with the latest user information.
-func syncUserToIdentity(usr *user.User, id *authn.Identity) {
-	id.ID = fmt.Sprintf("user:%d", usr.ID)
-	id.Login = usr.Login
-	id.Email = usr.Email
-	id.Name = usr.Name
-	id.IsGrafanaAdmin = &usr.IsAdmin
+func (s *UserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
+	if !identity.ClientParams.FetchSyncedUser {
+		return nil
+	}
+	namespace, id := identity.NamespacedID()
+	if namespace != authn.NamespaceUser {
+		return nil
+	}
+
+	usr, err := s.userService.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{
+		UserID: id,
+		OrgID:  r.OrgID,
+	})
+	if err != nil {
+		return errFetchingSignedInUser.Errorf("failed to resolve user: %w", err)
+	}
+
+	syncSignedInUserToIdentity(usr, identity)
+	return nil
+}
+
+func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
+	namespace, id := identity.NamespacedID()
+
+	if namespace != authn.NamespaceUser && namespace != authn.NamespaceServiceAccount {
+		// skip sync
+		return nil
+	}
+
+	if !shouldUpdateLastSeen(identity.LastSeenAt) {
+		return nil
+	}
+
+	go func(userID int64) {
+		defer func() {
+			if err := recover(); err != nil {
+				s.log.Error("panic during user last seen sync", "err", err)
+			}
+		}()
+
+		if err := s.userService.UpdateLastSeenAt(context.Background(), &user.UpdateUserLastSeenAtCommand{UserID: userID}); err != nil {
+			s.log.Error("failed to update last_seen_at", "err", err, "userId", userID)
+		}
+	}(id)
+
+	return nil
 }
 
 func (s *UserSync) updateAuthInfo(ctx context.Context, id *authn.Identity) error {
@@ -135,7 +174,7 @@ func (s *UserSync) updateAuthInfo(ctx context.Context, id *authn.Identity) error
 	}
 
 	namespace, userID := id.NamespacedID()
-	if namespace != "user" && userID <= 0 { // FIXME: constant namespace
+	if namespace != authn.NamespaceUser && userID <= 0 {
 		return fmt.Errorf("invalid namespace %q for user ID %q", namespace, userID)
 	}
 
@@ -203,12 +242,25 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
 }
 
 func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.User, error) {
+	// quota check (FIXME: (jguer) this should be done in the user service)
+	// we may insert in both user and org_user tables
+	// therefore we need to query check quota for both user and org services
+	for _, srv := range []string{user.QuotaTargetSrv, org.QuotaTargetSrv} {
+		limitReached, errLimit := s.quotaService.CheckQuotaReached(ctx, quota.TargetSrv(srv), nil)
+		if errLimit != nil {
+			s.log.Error("error getting user quota", "error", errLimit)
+			return nil, errSyncUserInternal.Errorf("%w", login.ErrGettingUserQuota)
+		}
+		if limitReached {
+			return nil, errSyncUserForbidden.Errorf("%w", login.ErrUsersQuotaReached)
+		}
+	}
+
 	isAdmin := false
 	if id.IsGrafanaAdmin != nil {
 		isAdmin = *id.IsGrafanaAdmin
 	}
 
-	// TODO: add quota check
 	usr, errCreateUser := s.userService.Create(ctx, &user.CreateUserCommand{
 		Login:        id.Login,
 		Email:        id.Email,
@@ -234,18 +286,12 @@ func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.Us
 	return usr, nil
 }
 
-// Does user exist in the database?
-// Check first authinfo table, then user table
-// return user id if found, 0 if not found
-func (s *UserSync) UserInDB(ctx context.Context,
-	authID *string,
-	authModule *string,
-	params login.UserLookupParams) (*user.User, error) {
-	// Check authinfo table
-	if authID != nil && authModule != nil {
+func (s *UserSync) getUser(ctx context.Context, authModule, authID string, params login.UserLookupParams) (*user.User, error) {
+	// Check auth info fist
+	if authID != "" && authModule != "" {
 		query := &login.GetAuthInfoQuery{
-			AuthModule: *authModule,
-			AuthId:     *authID,
+			AuthModule: authModule,
+			AuthId:     authID,
 		}
 		errGetAuthInfo := s.authInfoService.GetAuthInfo(ctx, query)
 		if errGetAuthInfo == nil {
@@ -265,10 +311,10 @@ func (s *UserSync) UserInDB(ctx context.Context,
 	}
 
 	// Check user table to grab existing user
-	return s.LookupByOneOf(ctx, &params)
+	return s.lookupByOneOf(ctx, &params)
 }
 
-func (s *UserSync) LookupByOneOf(ctx context.Context, params *login.UserLookupParams) (*user.User, error) {
+func (s *UserSync) lookupByOneOf(ctx context.Context, params *login.UserLookupParams) (*user.User, error) {
 	var usr *user.User
 	var err error
 
@@ -302,3 +348,33 @@ func (s *UserSync) LookupByOneOf(ctx context.Context, params *login.UserLookupPa
 
 	return usr, nil
 }
+
+// syncUserToIdentity syncs a user to an identity.
+// This is used to update the identity with the latest user information.
+func syncUserToIdentity(usr *user.User, id *authn.Identity) {
+	id.ID = fmt.Sprintf("user:%d", usr.ID)
+	id.Login = usr.Login
+	id.Email = usr.Email
+	id.Name = usr.Name
+	id.IsGrafanaAdmin = &usr.IsAdmin
+}
+
+// syncSignedInUserToIdentity syncs a user to an identity.
+func syncSignedInUserToIdentity(usr *user.SignedInUser, identity *authn.Identity) {
+	identity.Name = usr.Name
+	identity.Login = usr.Login
+	identity.Email = usr.Email
+	identity.OrgID = usr.OrgID
+	identity.OrgName = usr.OrgName
+	identity.OrgCount = usr.OrgCount
+	identity.OrgRoles = map[int64]org.RoleType{identity.OrgID: usr.OrgRole}
+	identity.HelpFlags1 = usr.HelpFlags1
+	identity.Teams = usr.Teams
+	identity.LastSeenAt = usr.LastSeenAt
+	identity.IsDisabled = usr.IsDisabled
+	identity.IsGrafanaAdmin = &usr.IsGrafanaAdmin
+}
+
+func shouldUpdateLastSeen(t time.Time) bool {
+	return time.Since(t) > time.Minute*5
+}

pkg/services/authn/authnimpl/sync/user_sync_test.go

@@ -28,7 +28,7 @@ func ptrInt64(i int64) *int64 {
 	return &i
 }
 
-func TestUserSync_SyncUser(t *testing.T) {
+func TestUserSync_SyncUserHook(t *testing.T) {
 	userProtection := &authinfoservice.OSSUserProtectionImpl{}
 
 	authFakeNil := &logintest.AuthInfoServiceFake{
@@ -266,12 +266,13 @@ func TestUserSync_SyncUser(t *testing.T) {
 			},
 			args: args{
 				ctx: context.Background(),
-
 				id: &authn.Identity{
-					ID:    "",
-					Login: "test",
-					Name:  "test",
-					Email: "test",
+					ID:         "",
+					AuthID:     "2032",
+					AuthModule: "oauth",
+					Login:      "test",
+					Name:       "test",
+					Email:      "test",
 					ClientParams: authn.ClientParams{
 						SyncUser: true,
 						LookUpParams: login.UserLookupParams{
@@ -285,6 +286,8 @@ func TestUserSync_SyncUser(t *testing.T) {
 			wantErr: false,
 			wantID: &authn.Identity{
 				ID:             "user:1",
+				AuthID:         "2032",
+				AuthModule:     "oauth",
 				Login:          "test",
 				Name:           "test",
 				Email:          "test",
@@ -427,7 +430,7 @@ func TestUserSync_SyncUser(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService)
-			err := s.SyncUser(tt.args.ctx, tt.args.id, nil)
+			err := s.SyncUserHook(tt.args.ctx, tt.args.id, nil)
 			if tt.wantErr {
 				require.Error(t, err)
 				return
@@ -438,3 +441,33 @@ func TestUserSync_SyncUser(t *testing.T) {
 		})
 	}
 }
+
+func TestUserSync_FetchSyncedUserHook(t *testing.T) {
+	type testCase struct {
+		desc        string
+		req         *authn.Request
+		identity    *authn.Identity
+		expectedErr error
+	}
+
+	tests := []testCase{
+		{
+			desc:     "should skip hook when flag is not enabled",
+			req:      &authn.Request{},
+			identity: &authn.Identity{ClientParams: authn.ClientParams{FetchSyncedUser: false}},
+		},
+		{
+			desc:     "should skip hook when identity is not a user",
+			req:      &authn.Request{},
+			identity: &authn.Identity{ID: "apikey:1", ClientParams: authn.ClientParams{FetchSyncedUser: true}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			s := UserSync{}
+			err := s.FetchSyncedUserHook(context.Background(), tt.identity, tt.req)
+			require.ErrorIs(t, err, tt.expectedErr)
+		})
+	}
+}