AuthN: Fix user sync with multiple client (#63615)

* AuthN: Fix user sync to handle auth connections to multiple providers

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

@@ -65,7 +65,7 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth
 	}
 
 	// Does user exist in the database?
-	usr, errUserInDB := s.getUser(ctx, id.AuthModule, id.AuthID, id.ClientParams.LookUpParams)
+	usr, userAuth, errUserInDB := s.getUser(ctx, id)
 	if errUserInDB != nil && !errors.Is(errUserInDB, user.ErrUserNotFound) {
 		s.log.Error("error retrieving user", "error", errUserInDB,
 			"auth_module", id.AuthModule, "auth_id", id.AuthID,
@@ -98,8 +98,8 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth
 	}
 
 	// update user
-	if errUpdate := s.updateUserAttributes(ctx, usr, id); errUpdate != nil {
+	if errUpdate := s.updateUserAttributes(ctx, usr, id, userAuth); errUpdate != nil {
-		s.log.Error("error creating user", "error", errUpdate,
+		s.log.Error("error updating user", "error", errUpdate,
 			"auth_module", id.AuthModule, "auth_id", id.AuthID,
 			"login", usr.Login, "email", usr.Email,
 			"id_login", id.Login, "id_email", id.Email,
@@ -108,15 +108,6 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth
 	}
 
 	syncUserToIdentity(usr, id)
-
-	// 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,
-		)
-		return errSyncUserInternal.Errorf("unable to update auth info")
-	}
-
 	return nil
 }
 
@@ -168,28 +159,33 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit
 	return nil
 }
 
-func (s *UserSync) updateAuthInfo(ctx context.Context, id *authn.Identity) error {
+func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, identity *authn.Identity, createConnection bool) error {
-	if id.AuthModule != "" && id.OAuthToken != nil && id.AuthID != "" {
+	if identity.AuthModule == "" {
 		return nil
 	}
 
-	namespace, userID := id.NamespacedID()
+	// If a user does not a connection to a specific auth module, create it.
-	if namespace != authn.NamespaceUser && userID <= 0 {
+	// This can happen when: using multiple auth client where the same user exists in several or
-		return fmt.Errorf("invalid namespace %q for user ID %q", namespace, userID)
+	// changing to new auth client
-	}
+	if createConnection {
-
+		return s.authInfoService.SetAuthInfo(ctx, &login.SetAuthInfoCommand{
-	updateCmd := &login.UpdateAuthInfoCommand{
+			UserId:     userID,
-		AuthModule: id.AuthModule,
+			AuthModule: identity.AuthModule,
-		AuthId:     id.AuthID,
+			AuthId:     identity.AuthID,
-		UserId:     userID,
+			OAuthToken: identity.OAuthToken,
-		OAuthToken: id.OAuthToken,
+		})
 	}
 
 	s.log.Debug("Updating user_auth info", "user_id", userID)
-	return s.authInfoService.UpdateAuthInfo(ctx, updateCmd)
+	return s.authInfoService.UpdateAuthInfo(ctx, &login.UpdateAuthInfoCommand{
+		UserId:     userID,
+		AuthId:     identity.AuthID,
+		AuthModule: identity.AuthModule,
+		OAuthToken: identity.OAuthToken,
+	})
 }
 
-func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id *authn.Identity) error {
+func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id *authn.Identity, userAuth *login.UserAuth) error {
 	// sync user info
 	updateCmd := &user.UpdateUserCommand{
 		UserID: usr.ID,
@@ -221,11 +217,13 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
 		}
 	}
 
+	// FIXME(kalleep): Should this be its own hook?
 	if usr.IsDisabled && id.ClientParams.EnableDisabledUsers {
 		usr.IsDisabled = false
-		if errDisableUser := s.userService.Disable(ctx,
+		if errDisableUser := s.userService.Disable(
-			&user.DisableUserCommand{
+			ctx,
-				UserID: usr.ID, IsDisabled: false}); errDisableUser != nil {
+			&user.DisableUserCommand{UserID: usr.ID, IsDisabled: false},
+		); errDisableUser != nil {
 			return errDisableUser
 		}
 	}
@@ -238,12 +236,12 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
 		}
 	}
 
-	return nil
+	return s.upsertAuthConnection(ctx, usr.ID, id, userAuth == nil)
 }
 
 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)
+	// FIXME(jguer): this should be done in the user service
-	// we may insert in both user and org_user tables
+	// quota check: we can have quotas on both global and org level
 	// 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)
@@ -272,49 +270,57 @@ func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.Us
 		return nil, errCreateUser
 	}
 
-	if id.AuthModule != "" && id.AuthID != "" {
+	err := s.upsertAuthConnection(ctx, usr.ID, id, true)
-		if errSetAuth := s.authInfoService.SetAuthInfo(ctx, &login.SetAuthInfoCommand{
+	if err != nil {
-			UserId:     usr.ID,
+		return nil, err
-			AuthModule: id.AuthModule,
-			AuthId:     id.AuthID,
-			OAuthToken: id.OAuthToken,
-		}); errSetAuth != nil {
-			return nil, errSetAuth
-		}
 	}
 
 	return usr, nil
 }
 
-func (s *UserSync) getUser(ctx context.Context, authModule, authID string, params login.UserLookupParams) (*user.User, error) {
+func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user.User, *login.UserAuth, error) {
 	// Check auth info fist
-	if authID != "" && authModule != "" {
+	if identity.AuthID != "" && identity.AuthModule != "" {
-		query := &login.GetAuthInfoQuery{
+		query := &login.GetAuthInfoQuery{AuthId: identity.AuthID, AuthModule: identity.AuthModule}
-			AuthModule: authModule,
-			AuthId:     authID,
-		}
 		errGetAuthInfo := s.authInfoService.GetAuthInfo(ctx, query)
 		if errGetAuthInfo == nil {
 			usr, errGetByID := s.userService.GetByID(ctx, &user.GetUserByIDQuery{ID: query.Result.UserId})
 			if errGetByID == nil {
-				return usr, nil
+				return usr, query.Result, nil
 			}
 
 			if !errors.Is(errGetByID, user.ErrUserNotFound) {
-				return nil, errGetByID
+				return nil, nil, errGetByID
 			}
 		}
 
 		if !errors.Is(errGetAuthInfo, user.ErrUserNotFound) {
-			return nil, errGetAuthInfo
+			return nil, nil, errGetAuthInfo
 		}
 	}
 
 	// Check user table to grab existing user
-	return s.lookupByOneOf(ctx, &params)
+	usr, err := s.lookupByOneOf(ctx, identity.ClientParams.LookUpParams)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	var userAuth *login.UserAuth
+	// Special case for generic oauth: generic oauth does not store authID,
+	// so we need to find the user first then check for the userAuth connection by module and userID
+	if identity.AuthModule == login.GenericOAuthModule {
+		query := &login.GetAuthInfoQuery{AuthModule: identity.AuthModule, UserId: usr.ID}
+		err := s.authInfoService.GetAuthInfo(ctx, query)
+		if err != nil && !errors.Is(err, user.ErrUserNotFound) {
+			return nil, nil, err
+		}
+		userAuth = query.Result
+	}
+
+	return usr, userAuth, nil
 }
 
-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