-
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 (#2). Fixes #2872 #3244
Conversation
661b6e9
to
c041a9f
Compare
I think the CHANGELOG needs updating too, right? |
@otoolep thanks for the reminder. |
f458ff7
to
b1b3e9a
Compare
@@ -555,7 +592,7 @@ func (s *CreateRetentionPolicyStatement) String() string { | |||
|
|||
// RequiredPrivileges returns the privilege required to execute a CreateRetentionPolicyStatement. | |||
func (s *CreateRetentionPolicyStatement) RequiredPrivileges() ExecutionPrivileges { | |||
return ExecutionPrivileges{{Name: "", Privilege: AllPrivileges}} | |||
return ExecutionPrivileges{{Admin: true, Name: "", Privilege: AllPrivileges}} |
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.
Is this correct? You need to be an admin to create an RP on a database?
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 made any statement that previously required AllPrivileges
a statement that requires admin access. My assumption was that no additional privileges should be granted for AllPrivileges
that were not already allowed by the Read
and Write
privileges. Therefore, all statements that were previously AllPrivileges
now require admin privilege.
RP creation seems like something that could only require the Write
privilege, since there should be no way to delete or change existing data by creating a new RP.
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.
Seems like a good default for now. Let's make a matrix of operations and permissions and see what shakes out.
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.
Here's a proposal for permissions by GRANT type per opertation: https://docs.google.com/a/errplane.com/spreadsheets/d/1V1acjK8PS8QCQ3MagsvIMhT3giy8dJArzBOVanl_Vz0/edit?usp=sharing
Makes general sense to me. You'll need to rebase. |
6295acf
to
f697fef
Compare
Rebased onto master. |
+1 from me. |
bd099c0
to
ac5e3d9
Compare
+1 |
Adds GRANT and REVOKE statements for admin privilege. Adds authorization to the query endpoint.
ac5e3d9
to
9ba3732
Compare
This is an alternative to #3233 for setting the admin privilege on an existing user.
There does not seem to be a clean way to set/unset the admin privilege flag without making wider reaching changes to the way user privileges are stored.
influxql.Privilege
is used for per database privilege settings, e.g.map[string]Privilege
so extending it withinfluxql.AdminPrivilege
andinfluxql.NoAdminPrivilege
to set an admin flag felt off.With this change, the admin privilege is set when a
setPrivilege
is executed withinfluxql.AllPrivileges
and no database name.