Skip to content

sql: add e2e tests and fix privilege bug for DROP PROVISIONED ROLES#167003

Open
souravcrl wants to merge 1 commit intocockroachdb:masterfrom
souravcrl:fork/drop-provisioned-roles-refactor-tests
Open

sql: add e2e tests and fix privilege bug for DROP PROVISIONED ROLES#167003
souravcrl wants to merge 1 commit intocockroachdb:masterfrom
souravcrl:fork/drop-provisioned-roles-refactor-tests

Conversation

@souravcrl
Copy link
Copy Markdown
Contributor

@souravcrl souravcrl commented Mar 30, 2026

Add comprehensive logic tests for the DROP PROVISIONED ROLES statement
covering end-to-end behavior, LIMIT validation, admin-skip behavior
for non-admin CREATEROLE callers, skip-on-dependency contract for all
seven dependency branches, and expression evaluation.

These tests address the review feedback from @rafiss on #166980
(review)
which called out the lack of integration/logic tests, no coverage of
skip-on-dependency branches, no test of admin-skip behavior for
non-admin callers, and no test of the CREATEROLE-only caller path.

The execution layer enforces a mandatory LIMIT clause to prevent
accidentally dropping an unbounded number of provisioned roles in a
single transaction. The LIMIT must be a constant integer between 1
and 1024. This safety guard was added during implementation but
lacked test coverage until now.

Tests cover:

  • LIMIT is mandatory (error without it)
  • LIMIT validation (0, negative, >1024, subquery expression)
  • Provisioned users are dropped, non-provisioned remain untouched
  • root/admin users are never dropped
  • SOURCE filter drops only matching source
  • LIMIT caps the number of dropped users
  • LAST LOGIN BEFORE time-based filtering
  • Combined filters with LIMIT
  • Non-CREATEROLE user is rejected
  • Non-admin CREATEROLE caller: admin provisioned users are silently
    skipped while non-admin provisioned users are dropped (contrast
    with DROP ROLE which errors on admin users)
  • All seven dependency skip branches:
    1. Grants on objects
    2. Ownership of objects
    3. System privileges (e.g. VIEWCLUSTERMETADATA)
    4. Default privileges (explicit role)
    5. Default privileges (per-object grantee)
    6. Row-level security policies
    7. Scheduled jobs ownership
  • Empty match returns no error
  • Multiple sources filtered correctly
  • Role memberships cleaned up on drop
  • Parse roundtrip
  • Expression evaluation for SOURCE (concatenation) and LAST LOGIN
    BEFORE (now() - interval arithmetic) — verifies that non-literal
    expressions are properly type-checked and evaluated at execution
    time rather than being stringified

Also re-applies the NodeUserSessionDataOverride fix for the find
query. The find query originally used NodeUserSessionDataOverride
but was changed to params.p.User() during review to use lower
privileges. However, the AI reviewer on #166980 correctly
identified that a user with only CREATEROLE privilege cannot
directly read system.users, system.scheduled_jobs, or
system.privileges, so the dependency check and find queries would
get permission errors at runtime. The fix to
NodeUserSessionDataOverride was applied but got overwritten during
a force-push from a separate worktree. Authorization is already
checked at plan time via CheckGlobalPrivilegeOrRoleOption, and the
query is hardcoded with only parameterized filter values.

Fixes: #170030
Fixes: #170031
Fixes: #170032
Fixes: #170048
Epic: CRDB-54682
Release note: None

@souravcrl souravcrl requested a review from a team March 30, 2026 05:28
@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Mar 30, 2026

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Mar 30, 2026

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@souravcrl souravcrl force-pushed the fork/drop-provisioned-roles-refactor-tests branch from 43b4204 to 63c2d88 Compare April 25, 2026 14:09
@souravcrl souravcrl force-pushed the fork/drop-provisioned-roles-refactor-tests branch from 63c2d88 to 6754441 Compare May 10, 2026 08:11
@souravcrl souravcrl changed the title sql: add e2e tests for DROP PROVISIONED ROLES sql: add e2e tests and fix privilege bug for DROP PROVISIONED ROLES May 10, 2026
@souravcrl souravcrl marked this pull request as ready for review May 10, 2026 08:12
@souravcrl souravcrl requested a review from a team as a code owner May 10, 2026 08:12
@souravcrl souravcrl requested a review from rafiss May 10, 2026 08:12
@souravcrl souravcrl force-pushed the fork/drop-provisioned-roles-refactor-tests branch 2 times, most recently from 6cd4683 to a3ab299 Compare May 10, 2026 09:09
Add comprehensive logic tests for the DROP PROVISIONED ROLES statement
covering end-to-end behavior, LIMIT validation, admin-skip behavior
for non-admin CREATEROLE callers, skip-on-dependency contract for all
seven dependency branches, and expression evaluation.

These tests address the review feedback from @rafiss on cockroachdb#166980
(cockroachdb#166980 (review))
which called out the lack of integration/logic tests, no coverage of
skip-on-dependency branches, no test of admin-skip behavior for
non-admin callers, and no test of the CREATEROLE-only caller path.

The execution layer enforces a mandatory LIMIT clause to prevent
accidentally dropping an unbounded number of provisioned roles in a
single transaction. The LIMIT must be a constant integer between 1
and 1024. This safety guard was added during implementation but
lacked test coverage until now.

Tests cover:
- LIMIT is mandatory (error without it)
- LIMIT validation (0, negative, >1024, subquery expression)
- Provisioned users are dropped, non-provisioned remain untouched
- root/admin users are never dropped
- SOURCE filter drops only matching source
- LIMIT caps the number of dropped users
- LAST LOGIN BEFORE time-based filtering
- Combined filters with LIMIT
- Non-CREATEROLE user is rejected
- Non-admin CREATEROLE caller: admin provisioned users are silently
  skipped while non-admin provisioned users are dropped (contrast
  with DROP ROLE which errors on admin users)
- All seven dependency skip branches:
  1. Grants on objects
  2. Ownership of objects
  3. System privileges (e.g. VIEWCLUSTERMETADATA)
  4. Default privileges (explicit role)
  5. Default privileges (per-object grantee)
  6. Row-level security policies
  7. Scheduled jobs ownership
- Empty match returns no error
- Multiple sources filtered correctly
- Role memberships cleaned up on drop
- Parse roundtrip
- Expression evaluation for SOURCE (concatenation) and LAST LOGIN
  BEFORE (now() - interval arithmetic) — verifies that non-literal
  expressions are properly type-checked and evaluated at execution
  time rather than being stringified

Also re-applies the NodeUserSessionDataOverride fix for the find
query. The find query originally used NodeUserSessionDataOverride
but was changed to params.p.User() during review to use lower
privileges. However, the AI reviewer on cockroachdb#166980 correctly
identified that a user with only CREATEROLE privilege cannot
directly read system.users, system.scheduled_jobs, or
system.privileges, so the dependency check and find queries would
get permission errors at runtime. The fix to
NodeUserSessionDataOverride was applied but got overwritten during
a force-push from a separate worktree. Authorization is already
checked at plan time via CheckGlobalPrivilegeOrRoleOption, and the
query is hardcoded with only parameterized filter values.

Fixes: cockroachdb#170030
Fixes: cockroachdb#170031
Fixes: cockroachdb#170032
Fixes: cockroachdb#170048
Epic: CRDB-54682
Release note: None

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@souravcrl souravcrl force-pushed the fork/drop-provisioned-roles-refactor-tests branch from a3ab299 to 9516498 Compare May 10, 2026 09:17
sessiondata.NodeUserSessionDataOverride,
query,
queryArgs...,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Pre-existing panic on NULL filter expressions. The evalFilterExprs method (lines 255-271) uses tree.MustBeDString and tree.MustBeDTimestampTZ on the result of eval.Expr, which returns tree.DNull when the user passes NULL. Since the grammar accepts a_expr and TypeCheckAndRequire allows types.UnknownFamily, NULL passes all checks and causes a server panic at runtime. Any CREATEROLE user can crash the node with DROP PROVISIONED ROLES WITH SOURCE = NULL LIMIT 10;. This bug predates this PR but should be fixed — add a d == tree.DNull check after each eval.Expr call in evalFilterExprs and return pgerror.Newf(pgcode.InvalidParameterValue, "SOURCE/LAST LOGIN BEFORE filter cannot be NULL").

@github-actions
Copy link
Copy Markdown
Contributor

AI Review: Potential Issue(s) Detected

An inline comment has been added to the relevant line in the diff.

Bug: Server panic on NULL filter expressions in DROP PROVISIONED ROLES (High severity)

In pkg/sql/drop_provisioned_roles.go:255-271, the evalFilterExprs method calls tree.MustBeDString (line 260) and tree.MustBeDTimestampTZ (line 268) on the result of eval.Expr, which returns tree.DNull when the user passes NULL. The grammar accepts a_expr (including NULL) for both SOURCE and LAST LOGIN BEFORE, and TypeCheckAndRequire allows types.UnknownFamily through. This means any user with CREATEROLE privilege can crash the server node with:

DROP PROVISIONED ROLES WITH SOURCE = NULL LIMIT 10;
DROP PROVISIONED ROLES WITH LAST LOGIN BEFORE NULL LIMIT 10;

Suggested fix: After each eval.Expr call in evalFilterExprs, check for d == tree.DNull and return a user-facing error (e.g. pgerror.Newf(pgcode.InvalidParameterValue, "SOURCE filter cannot be NULL")).

Minor: SHOW vs DROP inconsistency for NULL estimated_last_login_time (Low severity)

The DROP query (drop_provisioned_roles.go:297-299) uses IS NULL OR in its filter, matching users who never logged in, while SHOW ROLES ... LAST LOGIN BEFORE intentionally excludes them. This means a SHOW preview won't match what DROP actually deletes.


View full analysis

If helpful: add O-AI-Review-Real-Issue-Found label.
If not helpful: add O-AI-Review-Not-Helpful label.

@github-actions github-actions Bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label May 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only.

Projects

None yet

2 participants