Skip to content

FM-1900 Add User defined type #68

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 1 commit into from
Feb 4, 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
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ script: "bundle exec rake spec SPEC_OPTS='--format documentation'"
rvm:
- 1.9.3
- 2.0.0
- 2.1.5
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

env:
matrix:
- PUPPET_GEM_VERSION="~> 3.7.1"
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ group :development, :test do
gem 'nokogiri'
gem 'mime-types', '<2.0', :require => false
gem 'rake', :require => false
gem 'rspec-puppet', :require => false
gem 'rspec-puppet', '~>1.0', :require => false
gem 'puppetlabs_spec_helper', :require => false
gem 'puppet-lint', :require => false
gem 'simplecov', :require => false
Expand Down
4 changes: 2 additions & 2 deletions lib/puppet/provider/sqlserver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ def self.run_authenticated_sqlcmd(query, opts)
#We want to not fail the exec but fail the overall process once we get the clean result back, otherwise we report the temp file which is meaningless
result = Puppet::Util::Execution.execute(['powershell.exe', '-noprofile', '-executionpolicy', 'unrestricted', temp_ps1.path], {:failonfail => false}) #We expect some things to fail in order to run as an only if
debug("Return result #{result}")
if opts[:failonfail] && result.match(/ERROR/)
fail(result)
if opts[:failonfail] && result.match(/THROW CAUGHT/)
fail(result.gsub('THROW CAUGHT:',''))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not fully following the change here. I should check the commit to see if it lends a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was refactored from ERROR to throw, this is a bug fix here.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end
return result
ensure
Expand Down
41 changes: 41 additions & 0 deletions manifests/user.pp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
##
#
#
#
##
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 reserved for comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have yet to do the comments on this defined type. Docs will be there, still not done with this defined type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right on. 👍

define sqlserver::user (
$database,
$ensure = 'present',
$user = $title,
$default_schema = undef,
$instance = 'MSSQLSERVER',
$login = undef,
$password = undef,
$force_delete = false,
)
{
sqlserver_validate_instance_name($instance)

$is_windows_user = sqlserver_is_domain_or_local_user($login)

if $password {
validate_re($password, '^.{1,128}$', 'Password must be equal or less than 128 characters')
if $is_windows_user and $login != undef{
fail('Can not provide password when using a Windows Login')
}
}
validate_re($database, '^.{1,128}$','Database name must be between 1 and 128 characters')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can validation be pushed to a function? I think something like this is helpful, but is helpful probably in many places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can but so simplistic to test if it is 1-128 characters is it worth the work of having the function and ensure that the function does not break?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well you've repeated the logic twice here. Better to push to a function taking minimum, maximum, field, and name

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 actually wanted to put that into stdlib entirely as the validate_slength only does max.


$create_delete = $ensure ? {
present => 'create',
absent => 'delete',
}

sqlserver_tsql{ "user-${instance}-${database}-${user}":
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 to believe the login is already a defined class (I kind of recall it but it's been a minute).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Login is yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will address string length validation method in next PR with User Delete

instance => $instance,
command => template("sqlserver/${create_delete}/user.sql.erb"),
onlyif => template('sqlserver/query/user_exists.sql.erb'),
require => Sqlserver::Config[$instance]
}

}
101 changes: 101 additions & 0 deletions spec/defines/user_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
require 'spec_helper'
require File.expand_path(File.join(File.dirname(__FILE__), 'manifest_shared_examples.rb'))

RSpec.describe 'sqlserver::user', :type => :define do
include_context 'manifests' do
let(:title) { 'loggingUser' }
let(:sqlserver_tsql_title) { 'user-MSSQLSERVER-myDatabase-loggingUser' }
let(:params) { {:user => 'loggingUser', :database => 'myDatabase'} }
end

describe 'should fail when password above 128 characters' do
o = [('a'..'z'), ('A'..'Z'), (0..9)].map { |i| i.to_a }.flatten
string = (0...129).map { o[rand(o.length)] }.join
let(:additional_params) { {:password => string} }
let(:raise_error_check) { 'Password must be equal or less than 128 characters' }
it_should_behave_like 'validation error'
end

describe 'should fail when database above 128 characters' do
o = [('a'..'z'), ('A'..'Z'), (0..9)].map { |i| i.to_a }.flatten
string = (0...129).map { o[rand(o.length)] }.join
let(:additional_params) { {:database => string} }
let(:raise_error_check) { 'Database name must be between 1 and 128 characters' }
let(:sqlserver_tsql_title) { "user-MSSQLSERVER-#{string}-loggingUser" }
it_should_behave_like 'validation error'
end

describe 'should contain correct sql syntax for check' do
let(:should_contain_onlyif) { [
"USE [myDatabase]",
"\nIF NOT EXISTS(SELECT name FROM sys.database_principals WHERE type in ('U','S','G') AND name = 'loggingUser')\n",
"THROW 51000, 'User [loggingUser] does not exist for database [myDatabase]', 10\n"
] }
let(:should_contain_command) { [
"USE [myDatabase]",
/CREATE USER \[loggingUser\]\n\s+FROM LOGIN \[mySysLogin\]/
] }
let(:should_not_contain_command) { [
'PASSWORD',
'DEFAULT_SCHEMA',
'WITH'
] }
let(:additional_params) { {:login => 'mySysLogin'} }
it_should_behave_like 'sqlserver_tsql onlyif'
it_should_behave_like 'sqlserver_tsql command'
it_should_behave_like 'sqlserver_tsql without_command'
end

describe 'when a password is specified' do
Copy link
Contributor

Choose a reason for hiding this comment

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

So I take it none of the acceptance tests actually install / modify SQL Server? And that our specs strictly verify that the commands look the way we hope that they do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have no acceptance tests currently because of an issue with Beaker and Cygwin. These are only spec tests. This also allows us to run our tests on non windows systems, I believe once we have the Acceptance tests worked out this will be a non issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh.

password = 'Pupp3t1@'
let(:additional_params) { {:password => password} }
let(:should_contain_command) { [
"USE [myDatabase];",
/CREATE USER \[loggingUser\]\n\s+WITH PASSWORD = '#{password}'/
] }
let(:should_not_contain_command) { [
'DEFAULT_SCHEMA',
] }
it_should_behave_like 'sqlserver_tsql onlyif'
it_should_behave_like 'sqlserver_tsql command'
it_should_behave_like 'sqlserver_tsql without_command'
end

describe 'when a default_schema is specified' do
let(:additional_params) { {:default_schema => 'dbo'} }
let(:should_contain_command) { [
"USE [myDatabase]",
/CREATE USER \[loggingUser\]\n\s+WITH\s+DEFAULT_SCHEMA = dbo/
] }
let(:should_not_contain_command) { [
'PASSWORD',
] }
it_should_behave_like 'sqlserver_tsql command'
it_should_behave_like 'sqlserver_tsql without_command'
end

describe 'when providing windows user' do
let(:additional_params) { {:user => 'myMachineName/myUser'} }
let(:sqlserver_tsql_title) { 'user-MSSQLSERVER-myDatabase-myMachineName/myUser' }
let(:should_contain_command) { [
"USE [myDatabase]",
'CREATE USER [myMachineName/myUser]'
] }
it_should_behave_like 'sqlserver_tsql command'
end

describe 'when providing a windows user and login' do
let(:additional_params) { {:user => 'myMachineName/myUser', :login => 'myMachineName/myUser'} }
let(:sqlserver_tsql_title) { 'user-MSSQLSERVER-myDatabase-myMachineName/myUser' }
let(:should_contain_command) { [
"USE [myDatabase]",
/CREATE USER \[myMachineName\/myUser\]\n\s+FROM LOGIN \[myMachineName\/myUser\]/
] }
it_should_behave_like 'sqlserver_tsql command'
end
describe 'have dependency on Sqlserver::Config[MSSQLSERVER]' do
it 'should require ::config' do
should contain_sqlserver_tsql(sqlserver_tsql_title).with_require('Sqlserver::Config[MSSQLSERVER]')
end
end
end
19 changes: 19 additions & 0 deletions templates/create/user.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- Need to use exec instead of use statement as this will trigger try catch
USE [<%= @database %>];
<% if @password %>
IF EXISTS(select containment from sys.databases WHERE name = '<%= @database %>' AND containment = 0)
THROW 51000, 'Database must be contained in order to use passwords', 0
<% end %>
CREATE USER [<%= @user %>]
<% if @login -%>
FROM LOGIN [<%= @login %>]
<% else -%>
<% if @password -%>
WITH PASSWORD = '<%= @password %>'
<% end -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it actually work to create a user without a login? I don't remember it working at all but it's been awhile. Usually if you create a user, there needs to be a login as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the username is actually a login already it will assume the FROM LOGIN so therefor a user does not need to provide the login

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where understanding containment comes in. Has that always been there? Or is that a newer thing with more recent versions of SQL server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been there since I believe 2008, if I am not mistaken. A tricky beast but used for isolation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to lookup containment myself when I first checked out this PR @ferventcoder as I wasn't familiar with it. It appears to be new to 2012+

https://msdn.microsoft.com/en-us/library/ff929071(v=sql.110).aspx

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 was sure it was older than 2012. Well good thing this only targets 2012 and 2014.

<% end -%>
<% if @default_schema -%>
<% if @password -%>,<% else -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of this. Would rather you set a @previous_option variable to true (as this may have more than just password and the branching logic could get crazy). Not just here necessarily But imagine how something like this could grow to be multiple ifs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In more complex templates I absolutely agree and do not use this pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

General comment if this does get to be any more complex. 👍

WITH <% end -%>
DEFAULT_SCHEMA = <%= @default_schema %>
<% end -%>
4 changes: 4 additions & 0 deletions templates/query/user_exists.sql.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- Need to use exec instead of use statement as this will trigger try catch
USE [<%= @database %>];
IF <% if @ensure == 'present' %>NOT<% end %> EXISTS(SELECT name FROM sys.database_principals WHERE type in ('U','S','G') AND name = '<%= @user %>')
THROW 51000, 'User [<%= @user %>] does not exist for database [<%= @database %>]', 10
Copy link
Contributor

Choose a reason for hiding this comment

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

So I see 50000 and 51000 used as the error codes thrown.

I know that 50000 is generally reserved for user defined error messages. I don't see any convention around using 51000 - any reason why you chose this number?

I also see that the last parameter is state - why is state set to 10 here? Note that RAISERROR uses the last parameter as severity, but for the THROW it's always set to 16 and is not user settable.

https://msdn.microsoft.com/en-us/library/ee677615.aspx

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 was generally staying with 51000 if there is something else it is because I forgot the 1. The last parameter is not optional and therefore needs to be set. I have never seen any conversations about only using 50000 for user messages but that THROWS by users must be higher than 50000.