-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Wire up admin privilege grant and revoke. Fixes #2872 #3233
Conversation
5f2a8fb
to
305953c
Compare
This is ready for review. @otoolep I'm lost on why circle tests are failing. |
5d812db
to
bb7e121
Compare
@@ -26,7 +26,7 @@ type StatementExecutor struct { | |||
CreateUser(name, password string, admin bool) (*UserInfo, error) | |||
UpdateUser(name, password string) error | |||
DropUser(name string) error | |||
SetPrivilege(username, database string, p influxql.Privilege) error | |||
SetPrivilege(username, database string, p influxql.Privilege, admin bool) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering -- we couldn't extend the meaning of influxql.Privilege
to include an admin level, could we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a new PR to see if extending influxql.Privilege
is a better way to pass along the admin privilege (I suspect it is).
Adding the admin flag seemed cleaner when I initially started this PR, but it means the only way to tell if the admin privilege should be set is by checking if the database is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that the admin flag should be checked if no database is specified? What if a database is specified and the user has admin privileges?
Kicked the build the couple more times, one reason for the CI failures were build issues, which is rather strange. |
Closing in favor of #3244 |
No description provided.