From 55a138f04560b4e9ea0cf150e3e0f56845a0da01 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Mon, 18 Nov 2024 16:24:24 -0500 Subject: [PATCH] sql: fix statement generation when ensuring roles Since we started skipping over non-existent roles in 35f723e3812a29f4b295c26885be706cc49ddd67, we need to make sure we only add commas at the appropriate point. The test update demonstrates that there was a bug before this patch, as it would fail with: ``` expected: ok defaultdb found: ERROR: LDAP authorization: error assigning roles to user ldap_user: EnsureUserOnlyBelongsToRoles-grant: at or near ",": syntax error (SQLSTATE 42601) HINT: try \h GRANT DETAIL: source SQL: GRANT , "ldap-parent-synced" TO ldap_user ``` Release note: None --- pkg/ccl/testccl/authccl/testdata/ldap | 2 +- pkg/sql/authorization.go | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/ccl/testccl/authccl/testdata/ldap b/pkg/ccl/testccl/authccl/testdata/ldap index 8ed9d1df12cf..1f5537dbe07e 100644 --- a/pkg/ccl/testccl/authccl/testdata/ldap +++ b/pkg/ccl/testccl/authccl/testdata/ldap @@ -352,7 +352,7 @@ CREATE ROLE "ldap-parent-synced"; ---- ok -ldap_mock set_groups=(ldap_user,cn=ldap-parent-synced,cn=ldap-parent-unsynced) +ldap_mock set_groups=(ldap_user,cn=ldap-parent-unsynced,cn=ldap-parent-synced) ---- connect user=ldap_user password="ldap_pwd" diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index c12be52b389c..daea0309f22e 100644 --- a/pkg/sql/authorization.go +++ b/pkg/sql/authorization.go @@ -739,11 +739,13 @@ func EnsureUserOnlyBelongsToRoles( if len(rolesToRevoke) > 0 { revokeStmt := strings.Builder{} revokeStmt.WriteString("REVOKE ") - for i, role := range rolesToRevoke { - if i > 0 { + addComma := false + for _, role := range rolesToRevoke { + if addComma { revokeStmt.WriteString(", ") } revokeStmt.WriteString(role.SQLIdentifier()) + addComma = true } revokeStmt.WriteString(" FROM ") revokeStmt.WriteString(user.SQLIdentifier()) @@ -757,12 +759,14 @@ func EnsureUserOnlyBelongsToRoles( if len(rolesToGrant) > 0 { grantStmt := strings.Builder{} grantStmt.WriteString("GRANT ") - for i, role := range rolesToGrant { + addComma := false + for _, role := range rolesToGrant { if roleExists, _ := RoleExists(ctx, txn, role); roleExists { - if i > 0 { + if addComma { grantStmt.WriteString(", ") } grantStmt.WriteString(role.SQLIdentifier()) + addComma = true } } grantStmt.WriteString(" TO ")