Merge pull request #149747 from shriramters/blathers/backport-release-25.2-149638

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

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