Skip to content

Commit 4416f36

Browse files
souravcrlclaude
andcommitted
sql: add e2e tests and fix privilege/NULL bugs for DROP PROVISIONED ROLES
Add comprehensive logic tests for DROP PROVISIONED ROLES and fix two bugs: 1. Privilege escalation: the internal query used `params.p.User()` which fails for non-admin CREATEROLE users who lack SELECT on system.users. Switched to `NodeUserSessionDataOverride` since the CREATEROLE authorization check already happened at plan time. 2. NULL filter panic: passing NULL as a SOURCE or LAST LOGIN BEFORE filter expression caused a server panic via tree.MustBeDString / tree.MustBeDTimestampTZ on tree.DNull. Added explicit DNull checks after eval.Expr, returning a proper InvalidParameterValue error instead of crashing the node. Follows the established pattern from set_zone_config.go. Refactors the main loop into two passes: the first pass filters candidates by checking permissions and dependencies, accumulating eligible users into a slice; the second pass performs the deletions. Tests cover all existing behavior plus: - NULL filter rejection for SOURCE and LAST LOGIN BEFORE - Negative test: no dedicated `drop_provisioned_role` audit event type (currently reuses generic `drop_role`) - Session revocation: web sessions for dropped users are revoked - EXPLAIN planning for various filter combinations Fixes: #170030 Fixes: #170031 Fixes: #170032 Fixes: #170048 Informs: #170033 Epic: CRDB-54682 Release note: None Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 33d214c commit 4416f36

4 files changed

Lines changed: 1007 additions & 11 deletions

File tree

pkg/sql/drop_provisioned_roles.go

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -129,11 +129,16 @@ func (n *DropProvisionedRolesNode) startExec(params runParams) error {
129129
}
130130
query, queryArgs := buildProvisionedRolesQuery(sourceVal, lastLoginVal, n.limitCount)
131131

132+
// Use NodeUserSessionDataOverride because the authorization check
133+
// (CREATEROLE) already happened at plan time. A non-admin CREATEROLE
134+
// user does not have SELECT on system.users, so the query must run
135+
// with elevated privileges. The query is hardcoded with only
136+
// parameterized filter values — no user-controlled SQL expressions.
132137
rows, err := params.p.InternalSQLTxn().QueryBufferedEx(
133138
params.ctx,
134139
"drop-provisioned-roles-find",
135140
params.p.txn,
136-
sessiondata.InternalExecutorOverride{User: params.p.User()},
141+
sessiondata.NodeUserSessionDataOverride,
137142
query,
138143
queryArgs...,
139144
)
@@ -149,10 +154,11 @@ func (n *DropProvisionedRolesNode) startExec(params runParams) error {
149154
return err
150155
}
151156

152-
var numDropped, numSkipped int
153-
var droppedNames []string
154-
var numRoleSettingsRowsDeleted int
155-
157+
// First pass: filter candidates by checking permissions and
158+
// dependencies. Accumulate eligible users into a separate slice
159+
// so that the deletion loop is cleanly separated from the
160+
// validation logic.
161+
var usersToDrop []username.SQLUsername
156162
for _, row := range rows {
157163
normalizedUsername := username.MakeSQLUsernameFromPreNormalizedString(
158164
string(tree.MustBeDString(row[0])),
@@ -179,7 +185,6 @@ func (n *DropProvisionedRolesNode) startExec(params runParams) error {
179185
params.ctx,
180186
pgnotice.Newf("skipping %q: must be superuser to drop superusers", normalizedUsername),
181187
)
182-
numSkipped++
183188
continue
184189
}
185190
}
@@ -195,22 +200,26 @@ func (n *DropProvisionedRolesNode) startExec(params runParams) error {
195200
params.ctx,
196201
pgnotice.Newf("skipping %q: role has dependent objects", normalizedUsername),
197202
)
198-
numSkipped++
199203
continue
200204
}
201205

202-
// Delete the role from all system tables.
206+
usersToDrop = append(usersToDrop, normalizedUsername)
207+
}
208+
209+
// Second pass: delete all eligible users from system tables.
210+
var droppedNames []string
211+
var numRoleSettingsRowsDeleted int
212+
for _, normalizedUsername := range usersToDrop {
203213
deleted, err := n.deleteRole(params, normalizedUsername, opName)
204214
if err != nil {
205215
return err
206216
}
207217
numRoleSettingsRowsDeleted += deleted
208-
numDropped++
209218
droppedNames = append(droppedNames, normalizedUsername.Normalized())
210219
}
211220

212221
// Bump table versions if anything was dropped.
213-
if numDropped > 0 {
222+
if len(droppedNames) > 0 {
214223
if sessioninit.CacheEnabled.Get(&params.p.ExecCfg().Settings.SV) {
215224
if err := params.p.bumpUsersTableVersion(params.ctx); err != nil {
216225
return err
@@ -252,6 +261,10 @@ func (n *DropProvisionedRolesNode) evalFilterExprs(
252261
if err != nil {
253262
return nil, nil, err
254263
}
264+
if d == tree.DNull {
265+
return nil, nil, pgerror.Newf(pgcode.InvalidParameterValue,
266+
"SOURCE filter cannot be NULL")
267+
}
255268
s := string(tree.MustBeDString(d))
256269
sourceVal = &s
257270
}
@@ -260,6 +273,10 @@ func (n *DropProvisionedRolesNode) evalFilterExprs(
260273
if err != nil {
261274
return nil, nil, err
262275
}
276+
if d == tree.DNull {
277+
return nil, nil, pgerror.Newf(pgcode.InvalidParameterValue,
278+
"LAST LOGIN BEFORE filter cannot be NULL")
279+
}
263280
v := tree.MustBeDTimestampTZ(d)
264281
lastLoginVal = &v
265282
}

0 commit comments

Comments
 (0)