sql: add VIEWSCHEDULE and VIEWEVENTLOG system privileges#169679
sql: add VIEWSCHEDULE and VIEWEVENTLOG system privileges#169679souravcrl wants to merge 2 commits intocockroachdb:masterfrom
Conversation
|
Merging to
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 |
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
| if err := d.catalog.CheckPrivilege( | ||
| d.ctx, syntheticprivilege.GlobalPrivilegeObject, | ||
| d.catalog.GetCurrentUser(), privilege.VIEWSCHEDULE, | ||
| ); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Bug: This upfront guard exclusively requires VIEWSCHEDULE on GlobalPrivilegeObject (a synthetic privilege object), so the VIEWSYSTEMTABLE fallback in authorization.go:264-273 — which only applies to system table descriptors — is never reached. Any non-admin user who currently accesses SHOW SCHEDULES via VIEWSYSTEMTABLE or direct SELECT grants on system.scheduled_jobs/system.jobs will get "does not have VIEWSCHEDULE system privilege" after this change, even though they have sufficient access to the underlying data.
The simplest fix is to remove this upfront guard entirely: the delegated query (SELECT ... FROM system.scheduled_jobs) will naturally hit the table-level privilege checks in authorization.go, which already correctly handle VIEWSYSTEMTABLE, VIEWSCHEDULE, and direct SELECT grants. Alternatively, expand this check to also accept VIEWSYSTEMTABLE.
Note that the analogous SHOW JOBS command has no such upfront gate, confirming this is inconsistent.
There was a problem hiding this comment.
I agree with this comment. do we actually need the check here if there already is one in authorization.go?
We should be able to confirm this via a logictest.
There was a problem hiding this comment.
Good catch on the VIEWSYSTEMTABLE regression. I tested removing the upfront guard entirely and the behavior is:
- Without the guard, the error is
"user testuser does not have SELECT privilege on relation scheduled_jobs"— which doesn't tell the user what privilege they need. - With the guard, we get a clear
"does not have VIEWSCHEDULE system privilege".
So the guard serves a UX purpose (clear error message), but as written it blocks VIEWSYSTEMTABLE users which is a regression. I've updated the guard to also check VIEWSYSTEMTABLE — if the user has neither VIEWSCHEDULE nor VIEWSYSTEMTABLE, they get the clear error. If they have VIEWSYSTEMTABLE, the guard passes and the query executes with the implicit SELECT from authorization.go.
I've also added a logictest case confirming that VIEWSYSTEMTABLE still works for SHOW SCHEDULES.
AI Review: Potential Issue(s) DetectedAn inline comment has been added to Summary: The upfront If helpful: add |
DB Console Manual Test ResultsTested the Schedules and Events pages with a non-admin user ( Test Flow (recorded via Playwright):
All pages display inline error alerts when privilege is missing, and render data correctly when privilege is granted. |
rafiss
left a comment
There was a problem hiding this comment.
Nice work! I reviewed the privilege-related changes, and it looks good, with just a minor nit.
For changes in the frontend and RPC endpoints, please ask the o11y team.
This also seems to resolve #103341 -- i will add that issue linkage to this PR.
@rafiss made 3 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on kyle-a-wong and souravcrl).
pkg/server/admin.go line 1494 at r2 (raw file):
// Fall back to the existing VIEWCLUSTERMETADATA check. if err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx); err != nil { return nil, err
When a non-admin lacks both privileges, the user only sees "requires the VIEWCLUSTERMETADATA system privilege" — they're never told VIEWEVENTLOG would also work.
consider using errors.WithMessage so we can mention that information. another alternative would be to modify the privilegeChecker interface so both privileges can be checked with the same call.
| if err := d.catalog.CheckPrivilege( | ||
| d.ctx, syntheticprivilege.GlobalPrivilegeObject, | ||
| d.catalog.GetCurrentUser(), privilege.VIEWSCHEDULE, | ||
| ); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I agree with this comment. do we actually need the check here if there already is one in authorization.go?
We should be able to confirm this via a logictest.
Previously, `SHOW SCHEDULES` was implicitly gated by `SELECT` on `system.scheduled_jobs` and `system.jobs`, which only `admin` has by default. This made it impossible to grant schedule visibility to non-admin users without granting broad access to system tables. This change introduces a new `VIEWSCHEDULE` system privilege (following the pattern of `VIEWJOB`, `VIEWACTIVITY`, etc.) that explicitly gates `SHOW SCHEDULES`. Admin users satisfy this check implicitly through `ALL` privileges. The privilege also grants implicit `SELECT` on `system.scheduled_jobs` and `system.jobs` so the delegated query executes successfully. The delegator check is kept (rather than relying solely on the authorization.go fallback) because without it the user would see a confusing "does not have SELECT privilege on relation scheduled_jobs" error instead of a clear message about the VIEWSCHEDULE privilege. On the DB Console side, the schedules API now checks for SQL execution errors before accessing results, preventing runtime crashes when the user lacks the required privilege. Fixes: cockroachdb#169420 Epic: none Release note (sql change): Added a new `VIEWSCHEDULE` system privilege that controls access to `SHOW SCHEDULES`. Non-admin users can be granted schedule visibility via `GRANT SYSTEM VIEWSCHEDULE TO <user>` without needing direct `SELECT` on system tables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ca60e3f to
8b57f1a
Compare
Previously, viewing the event log required `admin` or `VIEWCLUSTERMETADATA`, both of which are overprivileged for users who only need to read cluster events (e.g. SREs and auditors correlating events during incidents). This change introduces a new `VIEWEVENTLOG` system privilege that grants read-only access to `system.eventlog`. The gRPC Events API now accepts either `VIEWEVENTLOG` or the existing `VIEWCLUSTERMETADATA` privilege, and the error message mentions both options. The privilege also grants implicit `SELECT` on `system.eventlog` so the DB Console SQL-based events page works correctly. Non-admin users can now be granted event log visibility via: ```sql GRANT SYSTEM VIEWEVENTLOG TO <user>; ``` Fixes: cockroachdb#169421 Epic: none Release note (sql change): Added a new `VIEWEVENTLOG` system privilege that grants read-only access to the event log. Non-admin users can be granted event log visibility via `GRANT SYSTEM VIEWEVENTLOG TO <user>` without needing `VIEWCLUSTERMETADATA` or the `admin` role. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8b57f1a to
7057256
Compare
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on kyle-a-wong and rafiss).
pkg/server/admin.go line 1494 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
When a non-admin lacks both privileges, the user only sees "requires the VIEWCLUSTERMETADATA system privilege" — they're never told VIEWEVENTLOG would also work.
consider using
errors.WithMessageso we can mention that information. another alternative would be to modify the privilegeChecker interface so both privileges can be checked with the same call.
done
Summary
VIEWSCHEDULE(Kind 45) andVIEWEVENTLOG(Kind 46) systemprivileges, following the pattern of
VIEWJOB,VIEWACTIVITY, etc.VIEWSCHEDULEexplicitly gatesSHOW SCHEDULESand grants implicitSELECTonsystem.scheduled_jobsandsystem.jobs.VIEWEVENTLOGgrants read-only access tosystem.eventlogand theDB Console Events page. The gRPC Events API now accepts either
VIEWEVENTLOGor the existingVIEWCLUSTERMETADATA.before accessing results, preventing runtime crashes on privilege errors.
Previously,
SHOW SCHEDULESwas implicitly gated bySELECTonsystem.scheduled_jobs(admin-only), and viewing the event log requiredadminorVIEWCLUSTERMETADATA— both overprivileged for users whoonly need schedule or event visibility. Non-admin users can now be
granted scoped access:
Fixes: #169420
Fixes: #169421
Epic: none
Release note (sql change): Added two new system privileges:
VIEWSCHEDULEcontrols access toSHOW SCHEDULES, andVIEWEVENTLOGgrants read-only access to the event log. Non-admin users can be granted
scoped visibility via
GRANT SYSTEM VIEWSCHEDULE TO <user>andGRANT SYSTEM VIEWEVENTLOG TO <user>without needing broad systemtable access or the admin role.
DB Console Demo
Individual Screenshots
Schedules — without VIEWSCHEDULE (error)
Events — without VIEWEVENTLOG (error)
Schedules — with VIEWSCHEDULE (working)
Events — with VIEWEVENTLOG (working)
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com