[Backport 5.4.5099] scim: Fix random suffix added to every user on resync (#63131)

In the current implementation, we check if there's a user in the
database already, and if so we add a random suffix to increase the
chances of the insert/update to succeed.

However, we didn't check if the user that already exists is _the
same user_. So for a first sync, usernames would look nice and tidy, but
then on a second sync from the SCIM provider, every user would get a
suffix.

This PR fixes that by adding a check for the user ID.

Test plan:

Added tests, verified SCIM sync doesn't modify usernames anymore
locally. <br> Backport 614375e from
#63122

Co-authored-by: Erik Seliger <[email protected]>

Author: sourcegraph-release-bot

CHANGELOG.md

@@ -34,6 +34,7 @@ All notable changes to Sourcegraph are documented in this file.
 - Pressing the numpad `Enter` key will now cycle through in-file search results [#62665](https://github.com/sourcegraph/sourcegraph/pull/62665)
 - Providing an access token via the [`SRC_ACCESS_TOKEN`](https://sourcegraph.com/docs/cli/how-tos/creating_an_access_token) environment variable is now mandatory for uploading SCIP indexes using [src-cli](https://sourcegraph.com/docs/cli). [#62573](https://github.com/sourcegraph/sourcegraph/pull/62573)
 - Fixed several conditions that could cause a repository being incorrectly marked as modified during code host syncing. For these cases, unnecessary git fetches were triggered. [#62837](https://github.com/sourcegraph/sourcegraph/pull/62837)
+- Fixed usernames getting random suffixes added during SCIM provisioning. [#63122](https://github.com/sourcegraph/sourcegraph/pull/63122)
 
 ## 5.4.2198
 

cmd/frontend/internal/auth/scim/BUILD.bazel

@@ -54,6 +54,7 @@ go_test(
         "user_get_test.go",
         "user_patch_test.go",
         "user_replace_test.go",
+        "user_test.go",
         "util_test.go",
     ],
     embed = [":scim"],

cmd/frontend/internal/auth/scim/user.go

@@ -228,7 +228,7 @@ func diffEmails(beforeUpdateUserData, afterUpdateUserData scim.ResourceAttribute
 // and adding a random suffix to make it unique in case there one without a suffix already exists in the DB.
 // This is meant to be done inside a transaction so that the user creation/update is guaranteed to be
 // coherent with the evaluation of this function.
-func getUniqueUsername(ctx context.Context, tx database.UserStore, requestedUsername string) (string, error) {
+func getUniqueUsername(ctx context.Context, tx database.UserStore, existingUserID int32, requestedUsername string) (string, error) {
 	// Process requested username
 	normalizedUsername, err := auth.NormalizeUsername(requestedUsername)
 	if err != nil {
@@ -238,15 +238,23 @@ func getUniqueUsername(ctx context.Context, tx database.UserStore, requestedUser
 			return "", scimerrors.ScimErrorBadParams([]string{"invalid username"})
 		}
 	}
-	_, err = tx.GetByUsername(ctx, normalizedUsername)
-	if err == nil { // Username exists, try to add random suffix
+	u, err := tx.GetByUsername(ctx, normalizedUsername)
+	if err != nil {
+		if !database.IsUserNotFoundErr(err) {
+			return "", scimerrors.ScimError{Status: http.StatusInternalServerError, Detail: errors.Wrap(err, "could not check if username exists").Error()}
+		}
+		// User doesn't exist, we're good to go
+		return normalizedUsername, nil
+	}
+
+	// Username exists and is not the same user, try to add random suffix to make it unique
+	if u.ID != existingUserID {
 		normalizedUsername, err = userpasswd.AddRandomSuffix(normalizedUsername)
 		if err != nil {
 			return "", scimerrors.ScimError{Status: http.StatusInternalServerError, Detail: errors.Wrap(err, "could not normalize username").Error()}
 		}
-	} else if !database.IsUserNotFoundErr(err) {
-		return "", scimerrors.ScimError{Status: http.StatusInternalServerError, Detail: errors.Wrap(err, "could not check if username exists").Error()}
 	}
+
 	return normalizedUsername, nil
 }
 

cmd/frontend/internal/auth/scim/user_service.go

@@ -185,7 +185,7 @@ func (u *UserSCIMService) Create(ctx context.Context, attributes scim.ResourceAt
 	// Make sure the username is unique, then create user with/without an external account ID
 	var user *types.User
 	err = u.db.WithTransact(ctx, func(tx database.DB) error {
-		uniqueUsername, err := getUniqueUsername(ctx, tx.Users(), extractStringAttribute(attributes, AttrUserName))
+		uniqueUsername, err := getUniqueUsername(ctx, tx.Users(), 0, extractStringAttribute(attributes, AttrUserName))
 		if err != nil {
 			return err
 		}

cmd/frontend/internal/auth/scim/user_test.go

@@ -0,0 +1,72 @@
+package scim
+
+import (
+	"context"
+	"testing"
+
+	"github.com/sourcegraph/sourcegraph/internal/database"
+	"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
+	"github.com/sourcegraph/sourcegraph/internal/types"
+)
+
+func TestGetUniqueUsername(t *testing.T) {
+	t.Parallel()
+
+	tx := dbmocks.NewMockUserStore()
+	tx.GetByUsernameFunc.SetDefaultReturn(nil, database.NewUserNotFoundError(1))
+	ctx := context.Background()
+
+	t.Run("valid username", func(t *testing.T) {
+		username, err := getUniqueUsername(ctx, tx, 0, "validusername")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if username != "validusername" {
+			t.Errorf("expected username 'validusername', got '%s'", username)
+		}
+	})
+
+	t.Run("invalid username", func(t *testing.T) {
+		username, err := getUniqueUsername(ctx, tx, 0, "invalid username")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if len(username) == 0 {
+			t.Error("expected non-empty username")
+		}
+	})
+
+	t.Run("existing username", func(t *testing.T) {
+		tx := dbmocks.NewMockUserStore()
+		tx.GetByUsernameFunc.SetDefaultReturn(&types.User{
+			ID: 1,
+		}, nil)
+
+		username, err := getUniqueUsername(ctx, tx, 1, "existinguser")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if username != "existinguser" {
+			t.Errorf("expected unaltered username, got '%s'", username)
+		}
+	})
+
+	t.Run("existing username for different user", func(t *testing.T) {
+		tx := dbmocks.NewMockUserStore()
+		tx.GetByUsernameFunc.SetDefaultReturn(&types.User{
+			ID: 1,
+		}, nil)
+
+		username, err := getUniqueUsername(ctx, tx, 2, "existinguser")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if username == "existinguser" {
+			t.Errorf("expected unique username, got '%s'", username)
+		}
+	})
+}

cmd/frontend/internal/auth/scim/user_update_action.go

@@ -66,7 +66,7 @@ func (u *updateUserProfileData) Update(ctx context.Context, before, after *scim.
 		return nil
 	}
 
-	usernameUpdate, err := getUniqueUsername(ctx, u.tx.Users(), extractStringAttribute(after.Attributes, AttrUserName))
+	usernameUpdate, err := getUniqueUsername(ctx, u.tx.Users(), u.userID, requestedUsername)
 	if err != nil {
 		return scimerrors.ScimError{Status: http.StatusBadRequest, Detail: errors.Wrap(err, "invalid username").Error()}
 	}