sql: delegate SHOW SYSTEM GRANTS to new crdb_internal.kv_system_privileges vtable by ecwall · Pull Request #104565 · cockroachdb/cockroach

ecwall · 2023-06-08T00:30:05Z

Fixes #104412

SHOW SYSTEM GRANTS previously delegated to system.privileges which requires
the admin or root user privileges.

This change adds a new crdb_internal.kv_system_privileges vtable that is
accessible to anyone.

Release note (bug fix): Fixes issue requiring admin or root user privileges to
use SHOW SYSTEM GRANTS.

cockroach-teamcity · 2023-06-08T00:30:18Z

This change is

postamar · 2023-06-08T12:54:28Z

pkg/sql/crdb_internal.go

+		rows, err := p.ExecCfg().InternalDB.Executor().QueryIteratorEx(
+			ctx,
+			op,
+			nil,


style nit: decorate with comment

postamar · 2023-06-08T12:56:00Z

pkg/sql/crdb_internal.go

+			ctx,
+			op,
+			nil,
+			sessiondata.RootUserSessionDataOverride,


Could you make this a parameter of populateFromQuery instead of hard-coding it to the root user, please? I have another use-case in mind where populateFromQuery might be useful but it would be for the node user.

postamar · 2023-06-08T12:56:56Z

pkg/sql/crdb_internal.go

 	},
 }
+
+func populateFromQuery(query string, op string) populateFunc {


style nit: s/op/opName? also have it precede the query arg perhaps?

I replaced this func with virtualSchemaView{}.

postamar · 2023-06-08T12:59:09Z

pkg/sql/crdb_internal.go

+}
+
+var crdbInternalSystemPrivileges = virtualSchemaTable{
+	comment: `vtable backing SHOW SYSTEM GRANTS`,


nit: this comment deserves to be improved, how about something along the lines of "this vtable is a view on system.privileges with the root user and is used to back SHOW SYSTEM GRANTS"

postamar · 2023-06-08T13:00:05Z

pkg/sql/crdb_internal.go

+var crdbInternalSystemPrivileges = virtualSchemaTable{
+	comment: `vtable backing SHOW SYSTEM GRANTS`,
+	schema: `
+CREATE TABLE crdb_internal.system_privileges (


There's a naming scheme in force for this virtual schema which requires new tables to have a prefix depending on what they do, in this case this prefix is kv_.

Why do user privileges get a kv_ prefix?

According to https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/crdb_internal.go#L107 this vtable needs the kv_ prefix because it's populated (ultimately) by performing a KV scan on a physical table. That's my understanding, at least, which so far hasn't been challenged.

Ok sounds good to me. I never noticed that comment.

postamar · 2023-06-08T13:05:05Z

pkg/sql/crdb_internal.go

+		if err != nil {
+			return err
+		}
+		defer func() {


The motivation for the existence of this defer deserves a comment.

postamar · 2023-06-08T13:20:34Z

pkg/sql/crdb_internal.go

+	}
+}
+
+var crdbInternalSystemPrivileges = virtualSchemaTable{


I think (but I'm not sure) that you could define this as a virtualSchemaView and this way you would not need to introduce populateFromQuery at all!

I investigated this a bit. In master we already have crdb_internal.transaction_statistics_persisted which is the same thing for system.transaction_statistics; I set up a logictest in which testuser was non-admin and querying the latter would correctly raise an error however it totally can query the vtable.

Thanks I was looking for something like this, but did not know what to search for.

…s vtable Fixes #104412 SHOW SYSTEM GRANTS previously delegated to system.privileges which requires the admin or root user privileges. This change adds a new crdb_internal.kv_system_privileges vtable that is accessible to anyone. Release note (bug fix): Fixes issue requiring admin or root user privileges to use SHOW SYSTEM GRANTS.

rafiss

nice! glad the view works

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @postamar)

ecwall · 2023-06-08T16:36:38Z

bors r=rafiss

craig · 2023-06-08T17:05:01Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-06-08T17:05:10Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from f8c7d67 to blathers/backport-release-22.2-104565: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

error creating merge commit from f8c7d67 to blathers/backport-release-23.1-104565: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

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

ecwall requested a review from rafiss June 8, 2023 00:30

ecwall requested a review from a team as a code owner June 8, 2023 00:30

ecwall added backport-22.2.x backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only labels Jun 8, 2023

ecwall requested a review from a team as a code owner June 8, 2023 00:48

ecwall requested review from a team and DrewKimball June 8, 2023 00:48

ecwall requested a review from a team as a code owner June 8, 2023 02:21

postamar reviewed Jun 8, 2023

View reviewed changes

dhartunian removed the request for review from a team June 8, 2023 14:22

ecwall requested review from a team and postamar and removed request for a team and DrewKimball June 8, 2023 15:15

rafiss approved these changes Jun 8, 2023

View reviewed changes

craig bot merged commit 7fb4c16 into cockroachdb:master Jun 8, 2023

ecwall changed the title ~~sql: delegate SHOW SYSTEM GRANTS to new crdb_internal.system_privileges vtable~~ sql: delegate SHOW SYSTEM GRANTS to new crdb_internal.kv_system_privileges vtable Jun 8, 2023

This was referenced Jun 12, 2023

release-23.1: sql: delegate SHOW SYSTEM GRANTS to new crdb_internal.kv_system_privileges vtable #104732

Merged

release-22.2: sql: delegate SHOW SYSTEM GRANTS to new crdb_internal.kv_system_privileges vtable #104735

Merged

yuzefovich mentioned this pull request Oct 9, 2023

sql: separate "internal key" and "display name" for system privileges #111653

Merged

Conversation

ecwall commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

postamar Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall commented Jun 8, 2023

Uh oh!

craig bot commented Jun 8, 2023

Uh oh!

blathers-crl bot commented Jun 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ecwall commented Jun 8, 2023 •

edited

Loading

postamar Jun 8, 2023 •

edited

Loading