Skip to content

FM-2236 Add with_grant_option for user permissions #71

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

Merged
merged 2 commits into from
Feb 27, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions manifests/user/permission.pp
Original file line number Diff line number Diff line change
Expand Up @@ -13,44 +13,54 @@
# [database]
# The databaser you would like the permission managed on.
#
# [permission]
# The permission you would like managed. i.e. 'SELECT', 'INSERT', 'UPDATE', 'DELETE'
# [permissions]
# An array of permissions you would like managed. i.e. ['SELECT', 'INSERT', 'UPDATE', 'DELETE']
#
# [state]
Copy link
Contributor

Choose a reason for hiding this comment

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

Permissions needs updated to show that it is an array.

# The state you would like the permission in. Accepts 'GRANT', 'DENY', 'REVOKE' Please note that REVOKE equates to absent and will default to database and system level permissions.
#
# [with_grant_option]
# Whether to give the user the option to grant this permission to other users, accepts true or false, defaults to false
#
# [instance]
# The name of the instance where the user and database exists. Defaults to 'MSSQLSERVER'
#
##
define sqlserver::user::permission (
$user,
$database,
$permission = $title,
$state = 'GRANT',
$instance = 'MSSQLSERVER',
$permissions,
$state = 'GRANT',
$with_grant_option = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this in #72 - not a fan of the name (even if that is what SQL server calls it). That's not a deal breaker and perhaps good to keep naming conventions for hardcore DBAs that immediately know what it is. It's good that you added the documentation to what it is here. You may have also in #72 but I didn't check for sure.

$instance = 'MSSQLSERVER',
){
sqlserver_validate_instance_name($instance)

## Validate Permissions
$_permission = upcase($permission)
sqlserver_validate_range($_permission, 4, 128, 'Permission must be between 4 and 128 characters')
validate_re($_permission, '^([A-Z]|\s)+$','Permissions must be alphabetic only')
sqlserver_validate_range($permissions, 4, 128, 'Permission must be between 4 and 128 characters')
validate_array($permissions)

## Validate state
$_state = upcase($state)
validate_re($_state,'^(GRANT|REVOKE|DENY)$',"State can only be of 'GRANT', 'REVOKE' or 'DENY' you passed ${state}")

validate_bool($with_grant_option)
if $with_grant_option and $_state != 'GRANT' {
fail("Can not use with_grant_option and state ${_state}, must be 'GRANT'")
}

sqlserver_validate_range($database, 1, 128, 'Database must be between 1 and 128 characters')

sqlserver_validate_range($user, 1, 128, 'User must be between 1 and 128 characters')

if $with_grant_option {
$grant_option = "-WITH_GRANT_OPTION"
}
sqlserver_tsql{
"user-permissions-${instance}-${database}-${user}-${$_state}-${_permission}":
"user-permissions-${instance}-${database}-${user}-${_state}${grant_option}":
instance => $instance,
command => template("sqlserver/create/user_permission.sql.erb"),
onlyif => template('sqlserver/query/user_permission_exists.sql.erb'),
command => template("sqlserver/create/user/permission.sql.erb"),
onlyif => template('sqlserver/query/user/permission_exists.sql.erb'),
require => Sqlserver::Config[$instance],
}

}
85 changes: 66 additions & 19 deletions spec/defines/user/permission_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
context 'validation errors' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-SELECT' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT' }
end
context 'user =>' do
let(:params) { {
:permission => 'SELECT',
:permissions => ['SELECT'],
:database => 'loggingDb',
} }
let(:raise_error_check) { 'User must be between 1 and 128 characters' }
Expand All @@ -24,30 +24,31 @@
end
describe 'over limit' do
let(:additional_params) { {:user => random_string_of_size(129)} }
it_behaves_like 'validation error'
end
end
context 'permission' do
context 'permissions' do
let(:params) { {
:user => 'loggingUser',
:database => 'loggingDb',
} }
let(:raise_error_check) { 'Permission must be between 4 and 128 characters' }
describe 'empty' do
let(:additional_params) { {:permission => ''} }
let(:additional_params) { {:permissions => ''} }
it_behaves_like 'validation error'
end
describe 'under limit' do
let(:additional_params) { {:permission => random_string_of_size(3, false)} }
let(:additional_params) { {:permissions => [random_string_of_size(3, false)]} }
it_behaves_like 'validation error'
end
describe 'over limit' do
let(:additional_params) { {:permission => random_string_of_size(129, false)} }
let(:additional_params) { {:permissions => [random_string_of_size(129, false)]} }
it_behaves_like 'validation error'
end
end
context 'state =>' do
let(:params) { {
:permission => 'SELECT',
:permissions => ['SELECT'],
:database => 'loggingDb',
:user => 'loggingUser'
} }
Expand All @@ -57,20 +58,38 @@
it_behaves_like 'validation error'
end
end
context 'with_grant_option => ' do
let(:params) { {
:permissions => ['SELECT'],
:database => 'loggingDb',
:user => 'loggingUser',

} }
describe 'true AND state => DENY' do
let(:additional_params) { {:with_grant_option => true, :state => 'DENY'} }
let(:raise_error_check) { "Can not use with_grant_option and state DENY, must be 'GRANT' " }
it_behaves_like 'validation error'
end
describe 'invalid' do
let(:additional_params) { {:with_grant_option => 'invalid'} }
let(:raise_error_check) { '"invalid" is not a boolean' }
it_behaves_like 'validation error'
end
end
end
context 'successfully' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-SELECT' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT' }
let(:params) { {
:user => 'loggingUser',
:permission => 'SELECT',
:permissions => ['SELECT'],
:database => 'loggingDb',
} }
end
%w(revoke grant deny).each do |state|
context "state => '#{state}'" do
let(:sqlserver_tsql_title) { "user-permissions-MSSQLSERVER-loggingDb-loggingUser-#{state.upcase}-SELECT" }
let(:sqlserver_tsql_title) { "user-permissions-MSSQLSERVER-loggingDb-loggingUser-#{state.upcase}" }
let(:should_contain_command) { ["#{state.upcase} SELECT TO [loggingUser];", 'USE [loggingDb];'] }
describe "lowercase #{state}" do
let(:additional_params) { {:state => state} }
Expand All @@ -87,15 +106,15 @@
context 'permission' do
describe 'upper limit' do
permission =random_string_of_size(128, false)
let(:additional_params) { {:permission => permission} }
let(:sqlserver_tsql_title) { "user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-#{permission.upcase}" }
let(:additional_params) { {:permissions => [permission]} }
let(:sqlserver_tsql_title) { "user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT" }
let(:should_contain_command) { ['USE [loggingDb];'] }
it_behaves_like 'sqlserver_tsql command'
end
describe 'alter' do
let(:additional_params) { {:permission => 'ALTER'} }
let(:additional_params) { {:permissions => ['ALTER']} }
let(:should_contain_command) { ['USE [loggingDb];', 'GRANT ALTER TO [loggingUser];'] }
let(:sqlserver_tsql_title) { "user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-ALTER" }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT' }
it_behaves_like 'sqlserver_tsql command'
end
end
Expand All @@ -110,26 +129,54 @@
it_behaves_like 'compile'
end

context 'with_grant_option =>' do
describe 'true' do
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-WITH_GRANT_OPTION' }
let(:additional_params) { {:with_grant_option => true} }
let(:should_contain_command) { [
"IF @perm_state != 'GRANT_WITH_GRANT_OPTION'",
'GRANT SELECT TO [loggingUser] WITH GRANT OPTION;',
] }
let(:should_not_contain_command) { [
'REVOKE GRANT OPTION FOR SELECT FROM [loggingUser];'] }
let(:should_contain_onlyif) { ["IF @perm_state != 'GRANT_WITH_GRANT_OPTION'",] }
it_behaves_like 'sqlserver_tsql command'
it_behaves_like 'sqlserver_tsql without_command'
it_behaves_like 'sqlserver_tsql onlyif'
end
describe 'false' do
let(:should_contain_command) { [
"IF @perm_state != 'GRANT'",
'GRANT SELECT TO [loggingUser];',
'REVOKE GRANT OPTION FOR SELECT TO [loggingUser] CASCADE;',
"IF 'GRANT_WITH_GRANT_OPTION' = ISNULL(",
] }
let(:should_contain_onlyif) { ["IF @perm_state != 'GRANT'",] }
it_behaves_like 'sqlserver_tsql command'
it_behaves_like 'sqlserver_tsql onlyif'
end
end
end

context 'command syntax' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT-SELECT' }
let(:sqlserver_tsql_title) { 'user-permissions-MSSQLSERVER-loggingDb-loggingUser-GRANT' }
let(:params) { {
:user => 'loggingUser',
:permission => 'SELECT',
:permissions => ['SELECT'],
:database => 'loggingDb',
} }
describe '' do
let(:should_contain_command) { [
'USE [loggingDb];',
'GRANT SELECT TO [loggingUser];',
/DECLARE @perm_state varchar\(250\)/,
/DECLARE @perm_state varchar\(250\), @error_msg varchar\(250\)/,
/SET @permission = 'SELECT'/,
/SET @perm_state = ISNULL\(\n\s+\(SELECT perm.state_desc FROM sys\.database_principals princ\n\s+JOIN sys\./,
/JOIN sys\.database_permissions perm ON perm\.grantee_principal_id = princ.principal_id\n\s+WHERE/,
/WHERE princ\.type in \('U','S','G'\) AND name = 'loggingUser' AND permission_name = 'SELECT' \),\n\s+'REVOKE'\);/,
/DECLARE @error_msg varchar\(250\);\nSET @error_msg = 'EXPECTED user \[loggingUser\] to have permission \[SELECT\] with GRANT but got ' \+ @perm_state;/,
/WHERE princ\.type in \('U','S','G'\) AND name = 'loggingUser' AND permission_name = @permission\),\n\s+'REVOKE'\)\s+;/,
/SET @error_msg = 'EXPECTED user \[loggingUser\] to have permission \[' \+ @permission \+ '\] with GRANT but got ' \+ @perm_state;/,
/IF @perm_state != 'GRANT'\n\s+THROW 51000, @error_msg, 10/
] }
it_behaves_like 'sqlserver_tsql command'
Expand Down
17 changes: 17 additions & 0 deletions templates/create/user/permission.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
USE [<%= @database %>];
DECLARE @perm_state varchar(250), @error_msg varchar(250), @permission varchar(250);
<% @permissions.each do |permission|
permission.upcase!
%>
SET @permission = '<%= permission %>'
BEGIN
<% 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;
<% end %>
<%= @_state %> <%= permission %> TO [<%= @user %>]<% if @with_grant_option == true %> WITH GRANT OPTION<% end %>;
END
BEGIN
<%= scope.function_template(['sqlserver/snippets/user/permission/exists.sql.erb']) %>
END
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or one loop.

<% end %>
7 changes: 0 additions & 7 deletions templates/create/user_permission.sql.erb

This file was deleted.

8 changes: 8 additions & 0 deletions templates/query/user/permission_exists.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
USE [<%= @database %>];

DECLARE @perm_state varchar(250), @error_msg varchar(250), @permission varchar(250);
<% @permissions.each do |permission|
permission.upcase! %>
SET @permission = '<%= permission %>'
<%= scope.function_template(['sqlserver/snippets/user/permission/exists.sql.erb']) %>
<% end %>
11 changes: 0 additions & 11 deletions templates/query/user_permission_exists.sql.erb

This file was deleted.

4 changes: 4 additions & 0 deletions templates/snippets/user/permission/exists.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SET @perm_state = <%= scope.function_template(['sqlserver/snippets/user/permission/get_perm_state.sql.erb']) %>;
SET @error_msg = 'EXPECTED user [<%= @user %>] to have permission [' + @permission + '] with <%= @_state %> but got ' + @perm_state;
IF @perm_state != '<% if @with_grant_option == true %>GRANT_WITH_GRANT_OPTION<% else %><%= @_state %><% end %>'
THROW 51000, @error_msg, 10
5 changes: 5 additions & 0 deletions templates/snippets/user/permission/get_perm_state.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
ISNULL(
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this refactor should be it's own commit as well IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

The file renames? I had to work to follow what actually changed there, but it would be helpful to rename the files in one commit and then adjust the values in another. It will make it easier to follow the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a thing that happened durnning development and having a, we may want to not have a growing folder of snippets/ and instead put them into logical folders.

(SELECT perm.state_desc FROM sys.database_principals princ
JOIN sys.database_permissions perm ON perm.grantee_principal_id = princ.principal_id
WHERE princ.type in ('U','S','G') AND name = '<%= @user %>' AND permission_name = @permission),
'REVOKE')