New-dbaDBUser does not add user to databases if the -Database parameter is ommitted

[Shashtsoh]

Verified issue does not already exist?

I have searched and found no existing issue

What error did you receive?

No errors are returned. The function completes without errors or output; user is not added to any databases. If a database is explicitly specified using -Database, the function works as expected.

Steps to Reproduce

$reqLogin = 'TestLogin'
$conn = connect-dbainstance -sqlinstance MySQLServer -trustservercertificate

This fails

new-dbadbuser -sqlinstance $conn -login $reqLogin -username $reqLogin -Confirm:$false -ErrorAction Stop

This works

new-dbadbuser -sqlinstance $conn -login $reqLogin -username $reqLogin -Confirm:$false -ErrorAction Stop -Database mydb1, mydb2, mydb3

Please confirm that you are running the most recent version of dbatools

2.0.4

Other details or mentions

I pulled the code and was able to find the issue & fix. I also found another potential issue that I'll include here. Line numbers are based on the line numbers in the code as seen at https://github.com/dataplat/dbatools/blob/master/public/New-DbaDbUser.ps1\

For this particular bug, the issue is on line 172. Concerning the -Database input parameter, the docs say "...If unspecified, all user databases will be processed." Line 172 attempts to use the value of the -Database parameter to control all code adding users. If that parameter is not provided, there are no values to iterate through, and the function will exit immediately without doing any further work. The solution is to force this foreach loop to iterate over the internal $databases parameter created on line 169.

Either of the following two code blocks appears to resolve the issue. I prefer the second; YMMV
Lines 172-173:

foreach ($db in $databases.Name) { #We should be looking at $databases (internal) instead of $Databases (input). We need to reference the name property because this is now a collection of database objects
                $dbSmo = $databases | Where-Object Name -eq $db

Lines 172-173:

foreach ($dbSmo in $databases) { #We should be looking at $databases (internal) instead of $Databases (input). We need to reference the name property because this is now a collection of database objects
                #$dbSmo = $databases | Where-Object Name -eq $db #Commented out because $dbSmo is now assigned in the foreach statement

The output statements via stop-function and write-verbose inside the loop appear to use $dbSmo and $db interchangeably, but the content of those variables seems to be different. I'm looking into normalizing that now.

The second, unexpected, potential bug is on line 157:
} elseif (-not $IncludeSystem) {
This makes evaluation of -IncludeSystem dependent on the existence (or lack thereof) of -Databases. If I'm not mistaken, -IncludeSystem is independent of -Databases, similar to -ExcludeDatabases (which has its own if statement on line 160). If so, then the elseif on 157 should be converted to a standalone if statement.

I have run these fixes through a few tests locally; behavior thus far is as expected.

What PowerShell host was used when producing this error

Windows PowerShell (powershell.exe)

PowerShell Host Version

Name Value

---

PSVersion 5.1.22621.1778
PSEdition Desktop
PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...}
BuildVersion 10.0.22621.1778
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

SQL Server Edition and Build number

N/A

.NET Framework Version

.NET Framework 4.8.9167.0

[Shashtsoh]

On the potential messaging issue... I didn't catch that you were using an implied tostring when referencing $dbSmo in the strings. That checks out.
The only real quibble here then is the use of $db on 177, as it is inconsistent with the rest of the function. This shouldn't affect anything at all unless we do the second fix from above and get rid of $db.

[hkphooey777]

Just discovered the same issue.

[c0drs]

I too have verified this is an error. I wouldn't be surprised if there was a variable naming conventions mishap. The variable from the input parameter is $Database, and the internal variable is $databases. Bug appears to have been introduced ~5 months ago.

I concur with @Shashtosh's second proposal.

Line 172 with bug...
foreach ($db in $Database) {

Line 172 without bug...
foreach ($dbSmo in $databases) {

I also agree with the proposal for line 157.
Line 157 is...
} elseif (-not $IncludeSystem) {

That if elseif should be it's own if statment as follows...
if (-not $IncludeSystem) {

Great finds, thanks for sharing @Shastosh!

[wsmelton]

Likely means the test is not covering that line(s).

[sqlarticles]

The reason for using the input variable $Database is to return proper message to the end user when the database doesnt exists. If I switch to the $databases variable that will be lost.
It's a bug, let me fix it and raise a PR.

Thanks Shanthosh for raising the bug

[Shashtsoh]

@sqlarticles I'm not sure if it should go under a different bug or not, but I appear to be seeing the same issue with new-dbadbrole. I'm digging through the code right now to be sure

[Shashtsoh]

Nevermind. New-dbaDBRole wants the database parameter as a string or string array - not a dbatools db object or array of dbobjects. My fault :)