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

Wire up admin privilege grant and revoke (#2). Fixes #2872 #3244

Merged
merged 1 commit into from
Jul 17, 2015

Conversation

gunnaraasen
Copy link
Member

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 with influxql.AdminPrivilege and influxql.NoAdminPrivilege to set an admin flag felt off.

With this change, the admin privilege is set when a setPrivilege is executed with influxql.AllPrivileges and no database name.

@otoolep
Copy link
Contributor

otoolep commented Jul 7, 2015

I think the CHANGELOG needs updating too, right?

@gunnaraasen
Copy link
Member Author

@otoolep thanks for the reminder. CHANGELOG has been updated.

@gunnaraasen gunnaraasen force-pushed the ga-admin-privilege-2 branch 4 times, most recently from f458ff7 to b1b3e9a Compare July 16, 2015 09:29
@gunnaraasen
Copy link
Member Author

@otoolep @toddboom This is ready for review. Turns out authorization on the query endpoint was not hooked up at all so the scope of this PR grew to fixing #3259.

@@ -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}}
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@otoolep
Copy link
Contributor

otoolep commented Jul 16, 2015

Makes general sense to me. You'll need to rebase.

@gunnaraasen gunnaraasen force-pushed the ga-admin-privilege-2 branch 2 times, most recently from 6295acf to f697fef Compare July 16, 2015 20:55
@gunnaraasen
Copy link
Member Author

Rebased onto master.

@otoolep
Copy link
Contributor

otoolep commented Jul 16, 2015

+1 from me.

@gunnaraasen gunnaraasen force-pushed the ga-admin-privilege-2 branch 2 times, most recently from bd099c0 to ac5e3d9 Compare July 17, 2015 18:28
@toddboom
Copy link
Contributor

+1

Adds GRANT and REVOKE statements for admin privilege. Adds authorization to the query endpoint.
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.

None yet

4 participants