sql: RequireSuperUser() swallows errors

[knz]

Describe the problem

The function RequiresSuperUser() returns err both to indicate errors in the underlying logic / SQL queries, and to indicate the current user is not a superuser.

Subsequently, callers that which simply to know whether the current user is an admin will test the error return and silently drop it on the floor if non-nil.

Instead the function should return (bool, error) with the boolean indicating the admin status.

[shakeelrao]

Are you referring to this function? I'm not sure where

returns err both to indicate errors in the underlying logic / SQL queries

comes into play since action isn't validated.

[Gurio]

@knz: to summarize discussions in #38124:

1. current RequireSuperUser should be renamed to IsSuperUser and adapted so it returns false, nil or false, InsufficientPrivilege in the last line, if all checks have failed
2. there should be a wrapper RequireSuperUser with the same signature as now, which calls new IsSuperUser (?)
3. where possible RequireSuperUser should be replaced with new IsSuperUser, and caller should test both error and result. In case of error it should be returned immediately.

What is still unclear: if there is no error, but the result is false - what should normally happen? return no error, or same InsufficientPrivilege? for example here: https://github.com/cockroachdb/cockroach/pull/38124/files#diff-80ac101a13e49ceb76ced6f89317e67aL843

would it make sense to leave the same old RequireSuperUser function in most places, and replace it with IsSuperUser in cases like this: https://github.com/cockroachdb/cockroach/pull/38124/files#diff-c73296f0dedf1f73ff0826c8127df2d4L74 (where some additional action should take place for a super user)?

[knz]

current RequireSuperUser should be renamed to IsSuperUser and adapted so it returns false, nil or false, InsufficientPrivilege in the last line, if all checks have failed

that's not what I suggested. What I suggest is:

• func RequireSuperUser() error returns InsufficientPrivilege when the user is not superuser
• func IsSuperUser() (bool, error) returns (true, nil) if the user is a superuser, (false, nil) if the user is not a superuser (no error occurred), and (false, err) if there was an error running the query on the system.users table.

there should be a wrapper RequireSuperUser with the same signature as now, which calls new IsSuperUser (?)

yes, and it's simple:

func RequireSuperUser(n) error {
  ok, err := IsSuperUser(n)
  if err != nil {
    return err
  }
  if !ok {
    return InsufficientPrivilege
  }
  return nil
}

where possible RequireSuperUser should be replaced with new IsSuperUser, and caller should test both error and result. In case of error it should be returned immediately.

yes!

if there is no error, but the result is false - what should normally happen? return no error, or same InsufficientPrivilege? for example here: /pull/38124/files#diff-80ac101a13e49ceb76ced6f89317e67aL843

every time this code looks like this:

		if err := p.RequireSuperUser(ctx, "RESTORE"); err != nil {
			return err
		}

(i.e. the code returns the error produced by RequireSuperUser)

Then you would not change it -- the code is meant to fail.

What needs to change is code like this:

	if err := p.RequireSuperUser(ctx, "revoke role"); err != nil {
		// Not a superuser: check permissions on each role.
		allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())

(i.e. the code ignores the error)

In that case you'd change it to this:

	isSuperUser, err := p.IsSuperUser(ctx, "revoke role")
    if  err != nil {
        return err
    } else if !isSuperUser {
		// Not a superuser: check permissions on each role.
		allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())

does this make sense?

[Gurio]

@knz, makes perfect sense, I will do it (unless someone manages to do it faster)

[knz]

Go ahead -- Sent from my Android device with K-9 Mail. Please excuse my brevity.