Skip to content

FM-1556 Add ability to manage login server level permissions #72

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
52 changes: 52 additions & 0 deletions manifests/login/permission.pp
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
##
# == Define Resource Type: sqlserver::login::permission#
#
# === Requirement/Dependencies:
#
# Requires defined type {sqlserver::config} in order to execute against the SQL Server instance
#
#
# === Parameters
# [login]
# The login for which the permission will be manage.
#
# [permissions]
# An array of permissions you would like managed. i.e. ['SELECT', 'INSERT', 'UPDATE', 'DELETE']
#
# [state]
# 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.
#
# [instance]
# The name of the instance where the user and database exists. Defaults to 'MSSQLSERVER'
#
##
define sqlserver::login::permission (
$login,
$permissions,
$state = 'GRANT',
$with_grant_option = false,
$instance = 'MSSQLSERVER',
){
sqlserver_validate_instance_name($instance)

## Validate Permissions
sqlserver_validate_range($permissions, 4, 128, 'Permission must be between 4 and 128 characters')
validate_array($permissions)

sqlserver_validate_range($login, 1, 128, 'Login must be between 1 and 128 characters')

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

validate_bool($with_grant_option)
Copy link
Contributor

Choose a reason for hiding this comment

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

Refresh my memory - this is allowing the user to be able to grant that permission to others right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm almost prefer that we name this better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something like $allow_user_to_grant_permission_to_others or something that more succinctly says the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind on all of that, but please add the documentation on what with_grant_option is like you did with #71.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same language that they utilize in SQL documentation to match close with how a DBA would think about it, but I am open to changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the same validation for with grant option that you did over at https://github.com/puppetlabs/puppetlabs-sqlserver/pull/71/files#diff-48c6692022676df64485ceff46c5874bR49 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having the documentation state exactly what it is helps someone.

if $with_grant_option {
$grant_option = "-WITH_GRANT_OPTION"
}
sqlserver_tsql{ "login-permission-${instance}-${login}-${_state}${grant_option}":
instance => $instance,
command => template('sqlserver/create/login/permission.sql.erb'),
onlyif => template('sqlserver/query/login/permission_exists.sql.erb'),
require => Sqlserver::Config[$instance],
}
}
135 changes: 135 additions & 0 deletions spec/defines/login/permission_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
require 'spec_helper'
require File.expand_path(File.join(File.dirname(__FILE__), '..', 'manifest_shared_examples.rb'))

describe 'sqlserver::login::permission' do
let(:facts) { {:osfamily => 'windows'} }
context 'validation errors' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'login-permission-MSSQLSERVER-loggingUser-GRANT' }
end
context 'login =>' do
let(:params) { {
:permissions=> ['SELECT'],
} }
let(:raise_error_check) { 'Login must be between 1 and 128 characters' }
describe 'missing' do
let(:raise_error_check) { 'Must pass login to Sqlserver::Login::Permission[myTitle]' }
it_behaves_like 'validation error'
end
describe 'empty' do
let(:additional_params) { {:login => ''} }
it_behaves_like 'validation error'
end
describe 'over limit' do
let(:additional_params) { {:login => random_string_of_size(129)} }
it_behaves_like 'validation error'
end
end
context 'permission' do
let(:params) { {
:login => 'loggingUser',
} }
let(:raise_error_check) { 'Permission must be between 4 and 128 characters' }
describe 'empty' do
let(:additional_params) { {:permissions=> ['']} }
it_behaves_like 'validation error'
end
describe 'under limit' do
let(:additional_params) { {:permissions=> [random_string_of_size(3, false)]} }
it_behaves_like 'validation error'
end
describe 'over limit' do
let(:additional_params) { {:permissions=> [random_string_of_size(129, false)]} }
it_behaves_like 'validation error'
end
end
context 'state =>' do
let(:params) { {
:permissions=> ['SELECT'],
:login => 'loggingUser'
} }
describe 'invalid' do
let(:additional_params) { {:state => 'invalid'} }
let(:raise_error_check) { "State parameter can only be one of 'GRANT', 'REVOKE' or 'DENY', you passed a value of invalid" }
it_behaves_like 'validation error'
end
end
end
context 'successfully' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'login-permission-MSSQLSERVER-loggingUser-GRANT' }
let(:params) { {
:login => 'loggingUser',
:permissions=> ['SELECT'],
} }
end
%w(revoke grant deny).each do |state|
context "state => '#{state}'" do
let(:sqlserver_tsql_title) { "login-permission-MSSQLSERVER-loggingUser-#{state.upcase}" }
let(:should_contain_command) { ["#{state.upcase} SELECT TO [loggingUser];", 'USE [master];'] }
describe "lowercase #{state}" do
let(:additional_params) { {:state => state} }
it_behaves_like 'sqlserver_tsql command'
end
state.capitalize!
describe "capitalized #{state}" do
let(:additional_params) { {:state => state} }
it_behaves_like 'sqlserver_tsql command'
end
end
end

context 'permission' do
describe 'upper limit' do
permission =random_string_of_size(128, false)
let(:additional_params) { {:permissions => [permission]} }
let(:sqlserver_tsql_title) { "login-permission-MSSQLSERVER-loggingUser-GRANT" }
let(:should_contain_command) { ['USE [master];'] }
it_behaves_like 'sqlserver_tsql command'
end
describe 'alter' do
let(:additional_params) { {:permissions=> ['ALTER']} }
let(:should_contain_command) { ['USE [master];', 'GRANT ALTER TO [loggingUser];'] }
let(:sqlserver_tsql_title) { "login-permission-MSSQLSERVER-loggingUser-GRANT" }
it_behaves_like 'sqlserver_tsql command'
end
end

describe 'Minimal Params' do
let(:pre_condition) { <<-EOF
define sqlserver::config{}
sqlserver::config {'MSSQLSERVER': }
EOF
}
it_behaves_like 'compile'
end

end

context 'command syntax' do
include_context 'manifests' do
let(:title) { 'myTitle' }
let(:sqlserver_tsql_title) { 'login-permission-MSSQLSERVER-loggingUser-GRANT' }
let(:params) { {
:login => 'loggingUser',
:permissions => ['SELECT'],
} }
describe '' do
let(:should_contain_command) { [
'USE [master];',
'GRANT SELECT TO [loggingUser];',
/DECLARE @perm_state varchar\(250\)/,
/SET @perm_state = ISNULL\(\n\s+\(SELECT perm.state_desc FROM sys\.server_permissions perm\n\s+JOIN sys\./,
/JOIN sys\.server_principals princ ON princ.principal_id = perm\.grantee_principal_id\n\s+WHERE/,
/WHERE princ\.type IN \('U','S','G'\)\n\s+ AND princ\.name = 'loggingUser'\n\s+AND perm\.permission_name = @permission\),\n\s+'REVOKE'\)/,
/SET @error_msg = 'EXPECTED login \[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'
end
end
end

end
17 changes: 17 additions & 0 deletions templates/create/login/permission.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
USE [master];
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/login/get_perm_state.sql.erb']) %>
REVOKE GRANT OPTION FOR <%= permission %> TO [<%= @login %>] CASCADE;
<% end %>
<%= @_state %> <%= permission %> TO [<%= @login %>]<% if @with_grant_option == true %> WITH GRANT OPTION<% end %>;
END
BEGIN
<%= scope.function_template(['sqlserver/snippets/login/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.

RE: BEGIN / END. Not really seeing the benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure that we are within a transaction before we check to see if the changes took place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment from #71 about the double loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to single loop

<% end %>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on the loop

8 changes: 8 additions & 0 deletions templates/query/login/permission_exists.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
USE [master];
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/login/permission/exists.sql.erb']) %>
<% end %>
7 changes: 7 additions & 0 deletions templates/snippets/login/get_perm_state.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ISNULL(
(SELECT perm.state_desc FROM sys.server_permissions perm
JOIN sys.server_principals princ ON princ.principal_id = perm.grantee_principal_id
WHERE princ.type IN ('U','S','G')
AND princ.name = '<%= @login %>'
AND perm.permission_name = @permission),
'REVOKE')
4 changes: 4 additions & 0 deletions templates/snippets/login/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/login/get_perm_state.sql.erb']) %>;
SET @error_msg = 'EXPECTED login [<%= @login %>] 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;