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.

Sign up for GitHub

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

Jump to bottom

New-DbaDbUser: Refactor and added contained user creation #8827

Merged
merged 11 commits into from
Apr 1, 2023
Merged

New-DbaDbUser: Refactor and added contained user creation #8827

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
96a5fe0
Added contained user creation
sqlarticles Mar 28, 2023
f3741d5
Fixed tests (do New-DbaDbUser)
sqlarticles Mar 28, 2023
37c8341
Fixed Test (do New-DbaDbUser)
sqlarticles Mar 28, 2023
e21fed9
Fixed failing test (do New-DbaDbUser)
sqlarticles Mar 28, 2023
3aa827b
Code to revert changes done for testing
sqlarticles Mar 28, 2023
53383c2
Fix failing test and added db validation
sqlarticles Mar 30, 2023
87a306b
Reverted camel casing for user name parameter
sqlarticles Mar 30, 2023
dbeb961
Update public/New-DbaDbUser.ps1
potatoqualitee Apr 1, 2023
eb002aa
Update public/New-DbaDbUser.ps1
potatoqualitee Apr 1, 2023
9c335c3
Update public/New-DbaDbUser.ps1
potatoqualitee Apr 1, 2023
5a28d60
remove comment
potatoqualitee Apr 1, 2023
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
194 changes: 94 additions & 100 deletions public/New-DbaDbUser.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@ function New-DbaDbUser {
.PARAMETER Login
When specified, the user will be associated to this SQL login and have the same name as the Login.

.PARAMETER Username
.PARAMETER UserName
sqlarticles marked this conversation as resolved.
Show resolved Hide resolved
When specified, the user will have this name.

.PARAMETER Password
When specified, the user will be created as a contained user. Standalone databases partial containment should be turned on to succeed. By default, in Azure SQL databases this is turned on.

.PARAMETER DefaultSchema
The default database schema for the user. If not specified this value will default to dbo.

Expand Down Expand Up @@ -88,136 +91,127 @@ function New-DbaDbUser {

Creates a new sql user named 'claudio@********.onmicrosoft.com' mapped to Azure Active Directory (AAD) in the specified database.

.EXAMPLE
PS C:\> New-DbaDbUser -SqlInstance sqlserver2014 -Database DB1 -Username user1 -Password (ConvertTo-SecureString -String "DBATools" -AsPlainText)

Creates a new cointained sql user named user1 in the database given database with the password specified.
#>
[CmdletBinding(SupportsShouldProcess, ConfirmImpact = "Medium")]
param(
[parameter(Mandatory, Position = 1)]
[parameter(Mandatory = $True, Position = 1)]
potatoqualitee marked this conversation as resolved.
Show resolved Hide resolved
[DbaInstanceParameter[]]$SqlInstance,
[PSCredential]$SqlCredential,
[string[]]$Database,
[string[]]$ExcludeDatabase,
[switch]$IncludeSystem,
[switch]$IncludeSystem = $False,
[Parameter(ParameterSetName = "NoLogin", Mandatory = $False)]
[Parameter(ParameterSetName = "Login", Mandatory = $True)]
potatoqualitee marked this conversation as resolved.
Show resolved Hide resolved
[string]$Login,
[string]$Username = $Login,
[Parameter(ParameterSetName = "Login", Mandatory = $False)]
[Parameter(ParameterSetName = "NoLogin", Mandatory = $True)]
potatoqualitee marked this conversation as resolved.
Show resolved Hide resolved
[Parameter(ParameterSetName = "ContainedSQLUser", Mandatory = $True)]
[Parameter(ParameterSetName = "ContainedAADUser", Mandatory = $True)]
[string]$UserName,
[string]$DefaultSchema = 'dbo',
[Parameter(ParameterSetName = "ContainedSQLUser", Mandatory = $True)]
[securestring]$Password,
[Parameter(ParameterSetName = "ContainedAADUser", Mandatory = $True)]
[switch]$ExternalProvider,
[switch]$Force,
[switch]$EnableException
)

begin {
$connParam = @{ }
if ($SqlCredential) { $connParam.SqlCredential = $SqlCredential }

# is this required?
Copy link
Member

Choose a reason for hiding this comment

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

Please remove comment, it is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shame! I kept that comment. Apologies.

if ($Force) { $ConfirmPreference = 'none' }

function Remove-User {
param (
[Microsoft.SqlServer.Management.Smo.User]$User
)
if ($Force) {
if ($Pscmdlet.ShouldProcess($User, "Dropping existing user $($User.Name) in the database $($User.Parent.Name) on $instance because -Force was used")) {
try {
$User.Drop()
} catch {
Stop-Function -Message "Could not remove existing user $($User.Name) in the database $($User.Parent.Name) on $instance, skipping." -Target $User -ErrorRecord $_ -Continue
}
}
} else {
Stop-Function -Message "User $($User.Name) already exists in the database $($User.Parent.Name) on $instance and -Force was not specified, skipping." -Target $User -Continue
}
# When user is created from login and no user name is provided then login name will be used as the user name
if ($Login -and -not($UserName)) {
$UserName = $Login
}

#Set appropriate user type
#Removed SQLLogin user type. This is deprecated and the alternate is to map to SqlUser. Reference https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.usertype?view=sql-smo-160
Copy link
Member

Choose a reason for hiding this comment

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

Your link does not show that is deprecated. This will still work on lower 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 is mentioned in the fields section. https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.usertype?view=sql-smo-160#fields

SqlLogin 0 Specifies a SQLLogin user. This is deprecated, use SQLUser instead.

I might be wrong. I thought the smo library is part of dbatools or dbatools.library module as a dependency which should be the recent version. If this is the case then I will prefer to keep the changes. However, if the module uses smo library installed locally then it makes sense to revert it back. I was not clear, your inputs will be helpful.

if ($ExternalProvider) {
Write-Message -Level Verbose -Message "Using UserType: External"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::External
} elseif ($Password -or $Login) {
Write-Message -Level Verbose -Message "Using UserType: SqlUser"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::SqlUser
} else {
Write-Message -Level Verbose -Message "Using UserType: NoLogin"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::NoLogin
}
}

process {
if (-not $Login -and -not $Username) {
Stop-Function -Message "One of -Login or -Username is needed."
return
}

foreach ($instance in $SqlInstance) {
try {
$server = Connect-DbaInstance -SqlInstance $instance -SqlCredential $SqlCredential
} catch {
Stop-Function -Message "Failure" -Category ConnectionError -ErrorRecord $_ -Target $instance -Continue
#prepare parameter values
$connParam.SqlInstance = $instance
$getDbParam = $connParam.Clone()
$getDbParam.OnlyAccessible = $True
if ($Database) { $getDbParam.Database = $Database }
if ($ExcludeDatabase) { $getDbParam.ExcludeDatabase = $ExcludeDatabase }
if (-not ($IncludeSystem)) { $getDbParam.ExcludeSystem = $True }

# Is the login exist?
if ($Login -and (-not(Get-DbaLogin @connParam -Login $Login))) {
Stop-Function -Message "Invalid Login: $Login is not found on $instance, skipping." -Target $instance -Continue
}

# Does the given login exist?
if ($Login) {
$existingLogin = $server.Logins | Where-Object Name -eq $Login
if (-not $existingLogin) {
Stop-Function -Message "Invalid Login: $Login is not found on $instance, skipping." -Target $instance -Continue
}
}

$databases = $server.Databases | Where-Object IsAccessible -eq $true

if ($Database) {
$databases = $databases | Where-Object Name -In $Database
} else {
if (-not $IncludeSystem) {
$databases = $databases | Where-Object IsSystemObject -ne $true
}
}
if ($ExcludeDatabase) {
$databases = $databases | Where-Object Name -NotIn $ExcludeDatabase
}
$databases = Get-DbaDatabase @getDbParam
Copy link
Member

Choose a reason for hiding this comment

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

We tend to not use this in our commands internally because of performance impact. I would prefer this to be reverted to the original pattern as that is what is followed in most all of our commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didnt get it. You meant the hash table as parameters or using the Get-DbaDatabase function?

Copy link
Member

@wsmelton wsmelton Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My request is reverting the change back to the original pattern. Get-DbaDatabase incurs performance on many systems and use of that in any command can cause degradation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Before making this change I raised in the Slack channel and I was told there isn't any standard and suggested to use functions if there is one already exists. That's when I refactored to use the functions instead.

I'm happy to revert the changes to use SMO connection instead of the function. Since this PR is merged, do you guys want me to revert it and raise a new PR?

Also, this is my understanding. These are operational functions and not a real time application so it should be fine even if it add few ms to the execution. I know this module is used in wide scope so I dont want to mess up with the expectation. Like I said before, let me know if I need to revert it and create a new PR?

$getValidSchema = Get-DbaDbSchema -InputObject $databases -Schema $DefaultSchema -IncludeSystemSchemas

foreach ($db in $databases) {
Write-Message -Level Verbose -Message "Add user $Username to database $db on $instance"

# Does a schema exist with the given name?
$existingSchema = $db.Schemas | Where-Object Name -eq $DefaultSchema
if (-not $existingSchema) {
Stop-Function -Message "Invalid DefaultSchema: $DefaultSchema is not found in the database $db on $instance, skipping." -Continue
}

# Does a user exist with the given name?
$existingUser = $db.Users | Where-Object Name -eq $Username
if ($existingUser) {
Remove-User -User $existingUser
}

if ($ExternalProvider) {
Write-Message -Level Verbose -Message "Using UserType: External"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::External
} elseif ($Login) {
# Does a user exist with same login?
$existingUser = $db.Users | Where-Object Login -eq $Login
if ($existingUser) {
Remove-User -User $existingUser
}

Write-Message -Level Verbose -Message "Using UserType: SqlLogin"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::SqlLogin
} else {
Write-Message -Level Verbose -Message "Using UserType: NoLogin"
$userType = [Microsoft.SqlServer.Management.Smo.UserType]::NoLogin
}
#prepare user query param
$userParam = $connParam.Clone()
$userParam.Database = $db.name
$userParam.User = $UserName

#check if the schema exists
if ($db.Name -in ($getValidSchema).Parent.Name) {
if ($Pscmdlet.ShouldProcess($db, "Creating user $UserName")) {
Write-Message -Level Verbose -Message "Add user $UserName to database $db on $instance"

#smo param builder
$smoUser = New-Object Microsoft.SqlServer.Management.Smo.User
$smoUser.Parent = $db
$smoUser.Name = $UserName
if ($Login) { $smoUser.Login = $Login }
$smoUser.UserType = $userType
$smoUser.DefaultSchema = $DefaultSchema

#Check if the user exists already
$userExists = Get-DbaDbUser @userParam
if ($userExists -and -not($Force)) {
Stop-Function -Message "User $($User.Name) already exists in the database $($User.Parent.Name) on $instance and -Force was not specified, skipping." -Target $User -Continue
} elseif ($userExists -and $Force) {
try {
Write-Message -Level Verbose -Message "FORCE is used, user $UserName will be dropped in the database $($db.Name) on $instance"
Remove-DbaDbUser @userParam -Force
} catch {
Stop-Function -Message "Could not remove existing user $($User.Name) in the database $($User.Parent.Name) on $instance, skipping." -Target $User -ErrorRecord $_ -Continue
}
}

if ($Pscmdlet.ShouldProcess($db, "Creating user $Username")) {
try {
if ($ExternalProvider) {
# Due to a bug at the time of writing, the user is created using T-SQL
# More info at: https://github.com/microsoft/sqlmanagementobjects/issues/112
$sql = "CREATE USER [$Username] FROM EXTERNAL PROVIDER WITH DEFAULT_SCHEMA = [$DefaultSchema]"
$db.Query($sql)
# Refresh the user list otherwise won't appear in the list
$db.Users.Refresh()
} else {
$smoUser = New-Object Microsoft.SqlServer.Management.Smo.User
$smoUser.Parent = $db
$smoUser.Name = $Username
$smoUser.Login = $Login
$smoUser.UserType = $userType
$smoUser.DefaultSchema = $DefaultSchema

$smoUser.Create()
#Create the user
try {
if ($Password) {
$smoUser.Create($Password)
} else { $smoUser.Create() }
#Verfiy the user creation
Get-DbaDbUser @userParam
} catch {
Stop-Function -Message "Failed to add user $Username in $db to $instance" -Category InvalidOperation -ErrorRecord $_ -Target $instance -Continue
}
Write-Message -Level Verbose -Message "Successfully added $Username in $db to $instance."

# Display Results
Get-DbaDbUser -SqlInstance $server -Database $db.Name -User $Username
} catch {
Stop-Function -Message "Failed to add user $Username in $db to $instance" -Category InvalidOperation -ErrorRecord $_ -Target $instance -Continue
}
} else {
Stop-Function -Message "Invalid DefaultSchema: $DefaultSchema is not found in the database $db on $instance, skipping." -Continue
}
}
}
Expand Down
15 changes: 14 additions & 1 deletion tests/New-DbaDbUser.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Write-Host -Object "Running $PSCommandPath" -ForegroundColor Cyan
Describe "$CommandName Unit Tests" -Tag 'UnitTests' {
Context "Validate parameters" {
[object[]]$params = (Get-Command $CommandName).Parameters.Keys | Where-Object { $_ -notin ('whatif', 'confirm') }
[object[]]$knownParameters = 'SqlInstance', 'SqlCredential', 'Database', 'ExcludeDatabase', 'IncludeSystem', 'Login', 'Username', 'DefaultSchema', 'ExternalProvider', 'Force', 'EnableException'
[object[]]$knownParameters = 'SqlInstance', 'SqlCredential', 'Database', 'ExcludeDatabase', 'IncludeSystem', 'Login', 'Username', 'Password', 'DefaultSchema', 'ExternalProvider', 'Force', 'EnableException'
$knownParameters += [System.Management.Automation.PSCmdlet]::CommonParameters
It "Should only contain our specific parameters" {
(@(Compare-Object -ReferenceObject ($knownParameters | Where-Object { $_ }) -DifferenceObject $params).Count ) | Should Be 0
Expand All @@ -17,16 +17,21 @@ Describe "$CommandName Integration Tests" -Tag "IntegrationTests" {
BeforeAll {
$dbname = "dbatoolscidb_$(Get-Random)"
$userName = "dbatoolscidb_UserWithLogin"
$userNameWithPassword = "dbatoolscidb_UserWithPassword"
$userNameWithoutLogin = "dbatoolscidb_UserWithoutLogin"

$password = 'MyV3ry$ecur3P@ssw0rd'
$securePassword = ConvertTo-SecureString $password -AsPlainText -Force
$null = New-DbaLogin -SqlInstance $script:instance2 -Login $userName -Password $securePassword -Force
$null = New-DbaDatabase -SqlInstance $script:instance2 -Name $dbname
$dbContainmentSpValue = (Get-DbaSpConfigure -SqlInstance $script:instance2 -Name ContainmentEnabled).ConfiguredValue
$null = Set-DbaSpConfigure -SqlInstance $script:instance2 -Name ContainmentEnabled -Value 1
$null = Invoke-DbaQuery -SqlInstance $script:instance2 -Query "ALTER DATABASE [$dbname] SET CONTAINMENT = PARTIAL WITH NO_WAIT"
}
AfterAll {
$null = Remove-DbaDatabase -SqlInstance $script:instance2 -Database $dbname -Confirm:$false
$null = Remove-DbaLogin -SqlInstance $script:instance2 -Login $userName -Confirm:$false
$null = Set-DbaSpConfigure -SqlInstance $script:instance2 -Name ContainmentEnabled -Value $dbContainmentSpValue
}
Context "Test error handling" {
It "Tries to create the user with an invalid default schema" {
Expand All @@ -43,6 +48,14 @@ Describe "$CommandName Integration Tests" -Tag "IntegrationTests" {
$newDbUser.DefaultSchema | Should -Be 'guest'
}
}
Context "Should create the user with password" {
It "Creates the contained sql user and get it." {
New-DbaDbUser -SqlInstance $script:instance2 -Database $dbname -Username $userNameWithPassword -Password $securePassword -DefaultSchema guest
$newDbUser = Get-DbaDbUser -SqlInstance $script:instance2 -Database $dbname | Where-Object Name -eq $userNameWithPassword
$newDbUser.Name | Should -Be $userNameWithPassword
$newDbUser.DefaultSchema | Should -Be 'guest'
}
}
Context "Should create the user without login" {
It "Creates the user and get it. Login property is empty" {
New-DbaDbUser -SqlInstance $script:instance2 -Database $dbname -User $userNameWithoutLogin -DefaultSchema guest
Expand Down