sql: refactor global privilege and legacy role option checking

[andyyang890]

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

Informs #103237

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[rafiss]

this looks good! just had some small nits, feel free to merge after

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

---

pkg/sql/authorization.go line 815 at r1 (raw file):

}

func privilegeToLegacyRoleOption(priv privilege.Kind) roleoption.Option {

can we make this get populated automatically, so we don't have to remember this in the future? i'm thinking about an initialization like this:

var privilegeToLegacyRoleOption map[privilege.Kind]roleoption.Option

func init() {
	privilegeToLegacyRoleOption = make(map[privilege.Kind]roleoption.Option)
	for _, priv := range privilege.GlobalPrivileges {
		if roleOpt, ok := roleoption.ByName[priv.String()]; ok {
			privilegeToLegacyRoleOption[priv] = roleOpt
		}
	}
}

---

pkg/sql/set_cluster_setting.go line 74 at r1 (raw file):

		return err
	}
	hasSqlModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)

can we still keep the previous logic that short-circuits, so we avoid checking them all?

[andyyang890]

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

---

pkg/sql/authorization.go line 815 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can we make this get populated automatically, so we don't have to remember this in the future? i'm thinking about an initialization like this:

var privilegeToLegacyRoleOption map[privilege.Kind]roleoption.Option

func init() {
	privilegeToLegacyRoleOption = make(map[privilege.Kind]roleoption.Option)
	for _, priv := range privilege.GlobalPrivileges {
		if roleOpt, ok := roleoption.ByName[priv.String()]; ok {
			privilegeToLegacyRoleOption[priv] = roleOpt
		}
	}
}

I considered this but it feels a little brittle since we're not guaranteed that the names are the same. e.g. CHANGEFEED maps to CONTROLCHANGEFEED.

I was thinking that a forgotten case should easily lead to a test failure since it would result in a failed privilege check when the user does not have the global privilege but does have the role option (i.e. is more restrictive). WDYT?

[andyyang890]

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

---

pkg/sql/set_cluster_setting.go line 74 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can we still keep the previous logic that short-circuits, so we avoid checking them all?

Done! I also took the liberty of reorganizing the code so that the privilege checks are closer to their actual usages.

[rafiss]

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

---

pkg/sql/authorization.go line 815 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

I considered this but it feels a little brittle since we're not guaranteed that the names are the same. e.g. CHANGEFEED maps to CONTROLCHANGEFEED.

I was thinking that a forgotten case should easily lead to a test failure since it would result in a failed privilege check when the user does not have the global privilege but does have the role option (i.e. is more restrictive). WDYT?

ah that's unfortunate.

i think we should do both then: do the automatic population with the loop, then for all the ones that don't have a matching name we add it explicitly to the map, with a comment explaining that it's because the names differ

that way, people only need to add boilerplate if the names differ, and not all the time.

---

pkg/sql/crdb_internal.go line 2352 at r3 (raw file):

			return err
		}
		hasView, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWCLUSTERSETTING)

nit: i see it was like this before already, but if it's easy enough can this be changed to avoid doing an unecessary privilege check?

[andyyang890]

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

---

pkg/sql/authorization.go line 815 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah that's unfortunate.

i think we should do both then: do the automatic population with the loop, then for all the ones that don't have a matching name we add it explicitly to the map, with a comment explaining that it's because the names differ

that way, people only need to add boilerplate if the names differ, and not all the time.

I realized that that CHANGEFEED is actually a table privilege, not a global privilege so we should be fine actually. I got rid of the map and moved the roleoption.ByName lookup directly into the function.

---

pkg/sql/crdb_internal.go line 2352 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i see it was like this before already, but if it's easy enough can this be changed to avoid doing an unecessary privilege check?

Done.

[andyyang890]

tftr!

bors r=rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)