sql: prevent auth failure when no roles are granted during sync

Previously, the EnsureUserOnlyBelongsToRoles function would construct
and execute a GRANT statement even if the list of roles to grant was
empty after filtering for roles that exist in the database.

This was inadequate because it caused authentication to fail for users
logging in via external providers (LDAP, JWT, OIDC). If a user
belonged to external groups that did not map to any existing roles in
CockroachDB, the function would generate an invalid SQL statement
(GRANT TO <user>), resulting in a syntax error that blocked the user's
session.

To address this, this patch modifies EnsureUserOnlyBelongsToRoles to
only build and execute the GRANT statement if at least one valid,
existing role is being granted. This prevents the syntax error and
allows users to log in successfully even if their external group
memberships do not result in any new role grants.

This commit also adds unit tests for `EnsureUserOnlyBelongsToRoles()`.

Fixes: #143878

Release note (bug fix): Fixed a bug where database login could fail
during LDAP, JWT, or OIDC authentication if the user's external group
memberships did not correspond to any existing roles in the
database. The login will now succeed, and no roles will be granted or
revoked in this scenario.

Author: shriramters

pkg/sql/authorization.go

@@ -592,21 +592,27 @@ func EnsureUserOnlyBelongsToRoles(
 			grantStmt := strings.Builder{}
 			grantStmt.WriteString("GRANT ")
 			addComma := false
+			rolesWereAdded := false // Flag to track if any roles are actually added
 			for _, role := range rolesToGrant {
 				if roleExists, _ := RoleExists(ctx, txn, role); roleExists {
 					if addComma {
 						grantStmt.WriteString(", ")
 					}
 					grantStmt.WriteString(role.SQLIdentifier())
 					addComma = true
+					rolesWereAdded = true // At least one role was added
 				}
 			}
-			grantStmt.WriteString(" TO ")
-			grantStmt.WriteString(user.SQLIdentifier())
-			if _, err := txn.Exec(
-				ctx, "EnsureUserOnlyBelongsToRoles-grant", txn.KV(), grantStmt.String(),
-			); err != nil {
-				return err
+
+			// Only execute the GRANT statement if at least one existing role was added.
+			if rolesWereAdded {
+				grantStmt.WriteString(" TO ")
+				grantStmt.WriteString(user.SQLIdentifier())
+				if _, err := txn.Exec(
+					ctx, "EnsureUserOnlyBelongsToRoles-grant", txn.KV(), grantStmt.String(),
+				); err != nil {
+					return err
+				}
 			}
 		}
 

pkg/sql/authorization_test.go

@@ -13,6 +13,8 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -163,3 +165,127 @@ COMMIT`,
 		})
 	}
 }
+
+func TestEnsureUserOnlyBelongsToRoles(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
+	tdb := sqlutils.MakeSQLRunner(db)
+
+	// Helper function to create a username.SQLUsername from a string.
+	makeRole := func(name string) username.SQLUsername {
+		r, err := username.MakeSQLUsernameFromUserInput(name, username.PurposeValidation)
+		require.NoError(t, err)
+		return r
+	}
+
+	// Helper function to get the current roles of a user and return them as a sorted string slice.
+	getRoles := func(user username.SQLUsername) []string {
+		rows := tdb.QueryStr(t, `SELECT role FROM system.role_members WHERE member = $1 ORDER BY role`, user.Normalized())
+		actualRoles := make([]string, len(rows))
+		for i, r := range rows {
+			actualRoles[i] = r[0]
+		}
+		return actualRoles
+	}
+
+	// Setup initial users and roles for the tests.
+	tdb.Exec(t, `
+		CREATE USER test_user;
+		CREATE ROLE role1;
+		CREATE ROLE role2;
+		CREATE ROLE role3;
+		CREATE ROLE role4;
+		GRANT role1, role2 TO test_user;
+	`)
+
+	testUser := makeRole("test_user")
+	role1 := makeRole("role1")
+	role2 := makeRole("role2")
+	role3 := makeRole("role3")
+	role4 := makeRole("role4")
+	nonExistentRole := makeRole("non_existent_role")
+
+	t.Run("no role is granted or revoked", func(t *testing.T) {
+		// The user currently has role1 and role2. Syncing with the same set should do nothing.
+		desiredRoles := []username.SQLUsername{role1, role2}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role1", "role2"}, getRoles(testUser))
+	})
+
+	t.Run("single role is granted", func(t *testing.T) {
+		// Grant role3. User should now have role1, role2, role3.
+		desiredRoles := []username.SQLUsername{role1, role2, role3}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role1", "role2", "role3"}, getRoles(testUser))
+	})
+
+	t.Run("single role is revoked", func(t *testing.T) {
+		// Revoke role1. User should now have role2, role3.
+		desiredRoles := []username.SQLUsername{role2, role3}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role2", "role3"}, getRoles(testUser))
+	})
+
+	t.Run("multiple roles are granted", func(t *testing.T) {
+		// Grant role1 and role4. User should now have role1, role2, role3, role4.
+		desiredRoles := []username.SQLUsername{role1, role2, role3, role4}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role1", "role2", "role3", "role4"}, getRoles(testUser))
+	})
+
+	t.Run("multiple roles are revoked", func(t *testing.T) {
+		// Revoke role3 and role4. User should now have role1, role2.
+		desiredRoles := []username.SQLUsername{role1, role2}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role1", "role2"}, getRoles(testUser))
+	})
+
+	t.Run("multiple roles are granted and revoked", func(t *testing.T) {
+		// Revoke role1, grant role3. User should now have role2, role3.
+		desiredRoles := []username.SQLUsername{role2, role3}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role2", "role3"}, getRoles(testUser))
+	})
+
+	t.Run("grant includes non-existent roles", func(t *testing.T) {
+		// Attempt to grant role4 and a non-existent role.
+		// User should end up with role2, role3, role4. The non-existent role should be ignored.
+		desiredRoles := []username.SQLUsername{role2, role3, role4, nonExistentRole}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Equal(t, []string{"role2", "role3", "role4"}, getRoles(testUser))
+	})
+
+	t.Run("all desired roles are non-existent", func(t *testing.T) {
+		// Attempt to sync with only a non-existent role.
+		// All existing roles (role2, role3, role4) should be revoked, and nothing new granted.
+		desiredRoles := []username.SQLUsername{nonExistentRole}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Empty(t, getRoles(testUser))
+	})
+
+	t.Run("empty desired roles list revokes all", func(t *testing.T) {
+		// First, grant some roles back to the user.
+		tdb.Exec(t, "GRANT role1, role2 TO test_user")
+		require.Equal(t, []string{"role1", "role2"}, getRoles(testUser))
+
+		// Now, sync with an empty list. All roles should be revoked.
+		desiredRoles := []username.SQLUsername{}
+		err := sql.EnsureUserOnlyBelongsToRoles(ctx, &execCfg, testUser, desiredRoles)
+		require.NoError(t, err)
+		require.Empty(t, getRoles(testUser))
+	})
+}