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

sql: change privilege check error message (superuser to admin role) #39211

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

Gurio
Copy link
Contributor

@Gurio Gurio commented Aug 1, 2019

This PR resolves #39099
change privilege check error message from "only superusers are allowed to" to "only users with the admin role are allowed to"

Open question: should IsSuperUser and RequireSuperUser method names stay the same?

@Gurio Gurio requested review from a team August 1, 2019 16:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Gurio Gurio force-pushed the change_privilege_check_err_msg branch from 82274e1 to 6884517 Compare August 1, 2019 16:54
@Gurio Gurio changed the title [DO NOT MERGE] sql: change privilege check error message (superuser to admin role) sql: change privilege check error message (superuser to admin role) Aug 1, 2019
@Gurio
Copy link
Contributor Author

Gurio commented Aug 2, 2019

@jordanlewis: not sure if you are the right person, but could you review, or point out who could review, this change?
Change actually looks very basic, shouldn't take much time. If only I didn't overlook something.

@jordanlewis jordanlewis requested a review from knz August 2, 2019 14:35
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Good work.
Feel free to rename the functions if you wish. This would make sense to me.

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

On the privileged operation failure, the following error was thrown:
"pq: only superusers are allowed to <OPERATION>"
Which is misleading, becuase superuser is not a concept in CRDB.
The error message should be:
"pq: only users with the admin role are allowed to <OPERATION>"

Resolves: cockroachdb#39099

Release note: None
@Gurio Gurio force-pushed the change_privilege_check_err_msg branch from 6884517 to 056afb3 Compare August 2, 2019 15:57
@Gurio Gurio requested review from a team as code owners August 2, 2019 15:57
@Gurio
Copy link
Contributor Author

Gurio commented Aug 2, 2019

ok, it builds, but lets see how the tests'll go

@Gurio
Copy link
Contributor Author

Gurio commented Aug 2, 2019

@knz: thanks for your review! I've renamed the functions as well, could you please re-review, and merge if it looks good?

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Very good, thank you very much for your contribution!

bors r+

Reviewed 21 of 21 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

craig bot pushed a commit that referenced this pull request Aug 2, 2019
39211: sql: change privilege check error message (superuser to admin role) r=knz a=Gurio

This PR resolves #39099
change privilege check error message from "only superusers are allowed to" to "only users with the admin role are allowed to"

Open question: should `IsSuperUser` and `RequireSuperUser` method names stay the same?
 

Co-authored-by: Arseni Lapunov <re.stage00101@gmail.com>
@craig
Copy link
Contributor

craig bot commented Aug 2, 2019

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.

CREATE DATABASE without admin role throws superuser error
3 participants