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

release-23.1: sql: add CREATEROLE, CREATELOGIN, CREATEDB, and CONTROLJOB system privileges #110359

Conversation

andyyang890
Copy link
Collaborator

@andyyang890 andyyang890 commented Sep 11, 2023

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

Release justification: Low risk, high benefit changes to existing functionality

This patch abstracts the logic for checking whether a user has a
global (system) privilege or the corresponding legacy role option
into a new function (`HasGlobalPrivilegeOrRoleOption`).

It also adds a wrapper function that returns an insufficient
privileges error when the user has neither the privilege nor
the legacy role option (`CheckGlobalPrivilegeOrRoleOption`).

Release note: None
This patch automates the process of generating the `AllPrivileges` list
so that any newly added privileges will automatically be included.

Release note: None
This patch adds a new `CREATEROLE` system privilege, which is analogous
to the existing `CREATEROLE` role option, but can also be inherited
by role membership.

Release note (sql change): There is now a `CREATEROLE` system privilege,
which is analogous to the existing `CREATEROLE` role option, but can
also be inherited by role membership.
This patch automates the process of generating the `ByName` map so that
any newly added privileges will automatically be included.

Release note: None
Release note (sql change): There is now a `CREATELOGIN` system privilege,
which is analogous to the existing `CREATELOGIN` role option, but can
also be inherited by role membership.
Release note (sql change): There is now a `CREATEDB` system privilege,
which is analogous to the existing `CREATEDB` role option, but can
also be inherited by role membership.
This patch updates the job authorization code to only check the VIEWJOB
privilege if the user does not have CONTROLJOB for ViewAccess.

It also updates the comment to reflect granting access when a user has
VIEWJOB and corrects a format specifier.

Release note: None
Release note (sql change): There is now a `CONTROLJOB` system privilege,
which is analogous to the existing `CONTROLJOB` role option, but can
also be inherited by role membership.
@andyyang890 andyyang890 requested review from a team as code owners September 11, 2023 18:04
@andyyang890 andyyang890 requested review from adityamaru and removed request for a team September 11, 2023 18:04
@blathers-crl
Copy link

blathers-crl bot commented Sep 11, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Sep 11, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@andyyang890 andyyang890 left a comment

Choose a reason for hiding this comment

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

I've left a comment on every file that had a merge conflict during the manual backport. It seems to be a pretty clean and straightforward backport overall.

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


pkg/sql/crdb_internal.go line 2079 at r8 (raw file):

		for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) {
			// If the user can only view sql.defaults settings, hide all other settings.
			if canViewSqlOnly && !strings.HasPrefix(k, "sql.defaults") {

Note: This file had a merge conflict. As far as I can tell, the only difference is that the type of k is string on release-23.1, while it is settings.InternalKey on master.


pkg/sql/grant_role.go line 71 at r8 (raw file):

	var err error
	if createRoleAllowsGrantRoleMembership.Get(&p.ExecCfg().Settings.SV) {
		allowedToGrantWithoutAdminOption, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)

Note: This file had a merge conflict since the createRoleAllowsGrantRoleMembership cluster setting doesn't exist on master anymore.


pkg/sql/revoke_role.go line 50 at r8 (raw file):

	var err error
	if createRoleAllowsGrantRoleMembership.Get(&p.ExecCfg().Settings.SV) {
		allowedToRevokeWithoutAdminOption, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)

Note: This file had a merge conflict since the createRoleAllowsGrantRoleMembership cluster setting doesn't exist on master anymore.


pkg/sql/set_cluster_setting.go line 66 at r8 (raw file):

}

func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, action string) error {

Note: This file had a merge conflict. As far as I can tell, the only difference is that name has type string on release-23.1, while it has type settings.SettingName on master.

Copy link
Collaborator

@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.

thanks for the detailed notes! made it very easy to review

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants