Added bool return value to RequireSuperUser function #38124
Conversation
|
This change is |
|
|
There was a problem hiding this comment.
Hi and thank you for you contribution!
The new interface doesn't really solve the problem. If I get false and an error, is it because I'm not superuser or is it because some other error was hit? I think the interface should return false and no error if we are not super user. (and the name should be changed to something like IsSuperUser). For the common use-case we can have a shared RequireSuperUser wrapper which only returns an error and generates the "insufficient privilege" error as needed.
Reviewable status:
complete! 0 of 0 LGTMs obtained
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/roleccl/role.go, line 74 at r1 (raw file):
} if _, err := p.RequireSuperUser(ctx, "grant role"); err != nil {
This is an example of what Radu explained: in this code block, the err object returned by RequireSuperUser is silently dropped. This code block (and others) should:
- test the error; if non-nil, always return the error
- if no error, test the boolean
- only if the boolean is true (the user is a super-user) do the privileged operation
Also, if the function merely tests whether the user is a super-user, the name RequireSuperUser is not appropriate any more. I think the name IsSuperUser would be better.
|
@mgm702: do you plan to continue on that task? otherwise I could take over |
38454: sql: require super user return status r=knz a=Gurio Resolves #32662 as discussed with @knz (hopefully correctly) First commit is taken from #38124 which helped to find all the necessary places where code should be changed I didn’t check if it builds with the last fix-commit, so please don’t review until TeamCity succeeds. Co-authored-by: Arseni Lapunov <re.stage00101@gmail.com>
Hello, I'm looking to make a contribution to your codebase. I was able to find a relatively simple issue that I can tackle (#32662). I've made the changes for the RequireSuperUser function and if this is still needed in the codebase I'd love to help with that.