Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: fix UserSQLRoles to account for global privileges #95258

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jan 15, 2023

Epic: None
Release note (bug fix): DB Console features that check for the
VIEWACTIVITYREDACTED privilege now also account for global privileges.

@rafiss rafiss requested review from maryliag, kpatron-cockroachlabs and a team January 15, 2023 00:00
@rafiss rafiss requested review from a team as code owners January 15, 2023 00:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Nice!
Can you also add a test on

func TestSQLRolesAPI(t *testing.T) {
that would have got this issue?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @kpatron-cockroachlabs)

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 18, 2023

@maryliag can you point me to where this endpoint is used in the frontend? i'm wondering since my change reduces the set of privileges that are returned in the map. e.g., it no longer includes VALID UNTIL or PASSWORD, and it doesn't include most of the NOXYZ options. the privileges it returns are now just these ones:

GlobalPrivileges = List{ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS}

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Basically we call refreshUser like this,
which then updates our redux layer, then we can use selectors to got some of the specific roles we care about, for now we only care about roles VIEWACTIVITYREDACTED and ADMIN,
which then can be used in places like here and here (this example is hiding statement diagnostics if the user has VIEWACTIVITYROLE)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @kpatron-cockroachlabs and @rafiss)


pkg/server/user_test.go line 114 at r2 (raw file):

	require.Equal(t, expRoles, res.Roles)

	// Remove one role from non-admin user.

can you also add an example with revoke global privilege?

Release note (bug fix): DB Console features that check for the
VIEWACTIVITYREDACTED privilege now also account for global privileges.
@rafiss rafiss requested a review from maryliag January 18, 2023 17:08
Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

i see, thanks for explaining! RFAL

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @kpatron-cockroachlabs and @maryliag)


pkg/server/user_test.go line 114 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you also add an example with revoke global privilege?

done

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kpatron-cockroachlabs)

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 18, 2023

tftr! flake was #95140 which was resolved in a later PR

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Jan 18, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants