FM-2236 Add with_grant_option for user permissions #71
Conversation
|
FYI - minor typo in commit message. Looks like there should be 2 commits here ... one for moving the file / directory structure around, and another for the actual work. |
Iristyle
changed the title
| USE [<%= @database %>]; | ||
| <% if @with_grant_option == false %> | ||
| IF 'GRANT_WITH_GRANT_OPTION' = <%= scope.function_template(['sqlserver/snippets/user/permission/get_perm_state.sql.erb']) %> | ||
| REVOKE GRANT OPTION FOR <%= @_permission %> TO [<%= @user %>] CASCADE; |
There was a problem hiding this comment.
Should there be a note about the use of CASCADE here?
Docs say
A cascaded revocation of a permission granted WITH GRANT OPTION will revoke both GRANT and DENY of that permission.
Is this our intended behavior?
There was a problem hiding this comment.
When a user has been given GRANT OPTION they only way to remove that privilege is to CASCADE the changes.
REVOKE GRANT OPTION FOR SELECT FROM [user];
Msg 4611, Level 16, State 1, Line 9
To revoke or deny grantable privileges, specify the CASCADE option.
There was a problem hiding this comment.
Would you see a timing issue with this at all?
There was a problem hiding this comment.
Is there the possibility of unset (undefined) from the perspective of bringing on existing permissions? That way if they haven't explicitly set true/false - then you don't manage the grant/revoke.
There was a problem hiding this comment.
Not sure what you mean? Do you mean have three possible states for grant_with_option? It is difficult to manage as it reports back a state_desc of GRANT or GRANT_WITH_GRANT_OPTION depending on which it is. So to say we accept GRANT or GRANT_WITH_GRANT_OPTION if 'unset'.
|
@Iristyle agreed it should have probably been in two commits, however it would be a messy untangle to remove them at this point, noted for future commits however. |
|
Require #74 for shared function |
| SET @permission = '<%= permission %>' | ||
| <%= scope.function_template(['sqlserver/snippets/user/permission/exists.sql.erb']) %> | ||
| <% end %> | ||
| END |
There was a problem hiding this comment.
So this is going to set all perms, then check each individually after and fail on the first one that doesn't exist?
I'd almost prefer for it to be atomic. Set one permission, check one permission, fail there.
|
@ferventcoder updated to single loop |
| @@ -19,6 +19,9 @@ | |||
| # [state] | |||
There was a problem hiding this comment.
Permissions needs updated to show that it is an array.
|
The documentation needs updated to reflect permissions versus permission. This hasn't already shipped yet has it? If so this one will need a deprecation. |
|
Other than the documentation change and the question, the rest LGTM |
… of single permission
|
@ferventcoder Docs update for permissions instead of permission with accurate description. |
|
To answer #71 (comment) - the user manifest was not in the v1 release - see https://github.com/puppetlabs/puppetlabs-sqlserver/tree/1.0.0/manifests Therefore, no deprecation notice required. |
|
👍 |
No description provided.