cockroachdb / cockroachdb/cockroach

sql: DROP PROVISIONED ROLES and SHOW USERS/ROLES apply inconsistent filters, and DROP LIMIT lacks ORDER BY

Open
#173,322 1 comment 1 reaction 1 assignee Claimed by @shriramters View on GitHub
A-sql-privileges C-bug O-agent P-1 T-product-security
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

`DROP PROVISIONED ROLES` and its companion `SHOW USERS|ROLES ... LAST LOGIN BEFORE` have two related defects. The docs recommend previewing a `DROP` with `SHOW` using "the same clauses," but the two statements do not compute the same set, and `DROP` does not choose a deterministic subset under `LIMIT`.

**1. DROP and SHOW apply different filters**

*(a) NULL handling on `LAST LOGIN BEFORE` differs.* `DROP` includes never-recorded-login users (`estimated_last_login_time IS NULL`):
```go
"(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < $%d)"
```
`SHOW` excludes them (bare `<`; a code comment there notes NULLs are dropped):
```go
"u.estimated_last_login_time < (%s)::TIMESTAMPTZ"
```
Under SQL three-valued logic `NULL < ts` is `UNKNOWN`, so `SHOW` omits exactly the never-logged-in users that `DROP` deletes.

*(b) Provisioning-scope filter differs.* `DROP` always restricts to provisioned users via `EXISTS (... system.role_options ... option = 'PROVISIONSRC')`. `SHOW` only adds that restriction when a `SOURCE` clause is supplied; otherwise it lists all users.

Net: `SHOW ... LAST LOGIN BEFORE ` under-reports (hides NULL/never-logged-in) and, without `SOURCE`, over-reports (includes non-provisioned users). It is not a faithful preview of `DROP` in either direction.

**2. DROP's LIMIT has no ORDER BY (nondeterministic selection)**

`buildProvisionedRolesQuery` emits `SELECT u.username FROM system.users AS u WHERE ... LIMIT $N` with no `ORDER BY`. When more than `N` users match, which `N` get dropped is nondeterministic and may vary run to run. `SHOW` uses `ORDER BY 1`, so even a NULL-consistent `SHOW` could not tell an operator which `N` rows a `LIMIT`-bounded `DROP` will remove.

**Impact**

The documented "preview with `SHOW` before you `DROP`" workflow is unreliable: a destructive statement's official preview systematically omits part of what it deletes (never-logged-in provisioned users) and cannot indicate which rows `LIMIT` selects. Note `estimated_last_login_time` is nullable, not backfilled on upgrade, and populated best-effort/async, so NULL spans "never logged in" and "no recorded login yet" (upgraded clusters, PCR/standby, non-recording auth paths, the async suppression window). DROP-includes-NULL can therefore remove provisioned users that are active-but-unrecorded. Blast radius is bounded by PROVISIONSRC-only, dependency-skipping, `LIMIT`, and re-provisioning on next login.

**Repro sketch**

1. Provision N+1 users from one IDP source; have some never authenticate (so `estimated_last_login_time IS NULL`).
2. `SHOW USERS WITH LAST LOGIN BEFORE ` -- never-logged-in users don't appear; non-provisioned users do.
3. `DROP PROVISIONED ROLES WITH LAST LOGIN BEFORE LIMIT ` -- never-logged-in users are dropped; the specific N dropped is not determined by any ordering.

**Code references** (pinned to `8812064`)

- DROP predicate (includes NULL): [drop_provisioned_roles.go#L310](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/drop_provisioned_roles.go#L310)
- DROP query (no `ORDER BY`): [drop_provisioned_roles.go#L322](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/drop_provisioned_roles.go#L322) (`buildProvisionedRolesQuery`)
- SHOW predicate (excludes NULL): [show_roles.go#L70](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/delegate/show_roles.go#L70)
- SHOW query (`ORDER BY 1`): [show_roles.go#L96](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/delegate/show_roles.go#L96)

**Suggested resolution**

- Have both statements share one predicate builder so the filters cannot drift, and decide the NULL policy explicitly:
- *Preferred:* make `SHOW` include NULL to match `DROP` (preserves dormant-cleanup intent, makes the preview truthful); correct the docs; consider surfacing the "no recorded login" cohort distinctly.
- *Alternative:* make `DROP` exclude NULL to match `SHOW` if "never delete on unknown last-login state" is preferred, requiring `SOURCE` for the NULL cohort.
- Add a deterministic `ORDER BY` to the DROP selection query (and document it) so `LIMIT` is stable and previewable.

Jira issue: CRDB-66640

Epic CRDB-54682

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.