Add scripts for setting up and tearing down AAD environment for tests #97
Conversation
Add ConvertTo-FlattenedConfigurationHashtable.ps1
…to personal/brpoll/auth-aad-scripts
samples/scripts/PowerShell/FhirServer/Public/New-FhirServerClientApplicationRegistration.ps1
Show resolved
Hide resolved
|
As discussed, it probably makes sense to fold in ConvertTo-FlattenedConfigurationHashtable.ps1 to the module. |
...se/scripts/PowerShell/FhirServerRelease/Public/ConvertTo-FlattenedConfigurationHashtable.ps1
Outdated
Show resolved
Hide resolved
release/scripts/PowerShell/FhirServerRelease/Public/Add-TestAuthEnvironmentAad.ps1
Outdated
Show resolved
Hide resolved
release/scripts/PowerShell/FhirServerRelease/Public/Add-TestAuthEnvironmentAad.ps1
Outdated
Show resolved
Hide resolved
release/scripts/PowerShell/FhirServerRelease/Public/Add-TestAuthEnvironmentAad.ps1
Outdated
Show resolved
Hide resolved
release/scripts/PowerShell/FhirServerRelease/FhirServerRelease.psd1
Outdated
Show resolved
Hide resolved
release/scripts/PowerShell/FhirServerRelease/Public/Add-TestAuthEnvironmentAad.ps1
Outdated
Show resolved
Hide resolved
* Formatting scripts * Moving ConvertTo-FlattenedConfigurationHashtable.ps1 out of module * Changing `TestAuthorizationEnvironment` to `TestAuthEnvironment` * Adding timeout to sleep loop
| $appRolesToEnable = $false | ||
| $desiredAppRoles = @() | ||
|
|
||
| if ($RoleConfiguration) { |
There was a problem hiding this comment.
nit: don't need this check anymore since the validation is already happening above.
| Description = $role.name | ||
| DisplayName = $role.name | ||
| Id = $id | ||
| IsEnabled = "true" |
There was a problem hiding this comment.
should this be string "true" or boolean true?
| } | ||
| } | ||
|
|
||
| if (!($azureAdApplication.PsObject.Properties.Name -eq "AppRoles")) { |
There was a problem hiding this comment.
Would it be more readable if we use -contains? Name property is an array and -eq returns items matching the value but it "feels" a little odd ot use -eq on array? (maybe just me)
There was a problem hiding this comment.
although.. i am not sure what it would do if Name only contains 1 item.
| foreach ($diff in Compare-Object -ReferenceObject $desiredAppRoles -DifferenceObject $azureAdApplication.AppRoles -Property "Id") { | ||
| switch ($diff.SideIndicator) { | ||
| "<=" { | ||
| $appRolesToEnable = $true |
There was a problem hiding this comment.
Maybe a little bit of comment describing the logic would help here. The AppRole should be enabled if $auzreAdApplication.PsObject.Properties.Name contains "AppRoles" or one of the newly specified role is not in the application already?
| } | ||
| } | ||
|
|
||
| if ($appRolesToEnable -or $appRolesToDisable) { |
There was a problem hiding this comment.
nit: don't really need this line?
if ($appRolesToDisable) {
Write-Host "Disabling old appRoles"
Set-AzureADApplication -ObjectId $azureAdApplication.objectId -appRoles $azureAdApplication.AppRoles | Out-Null
}
if ($appRolesToEnable) {
# Update app roles
Write-Host "Updating appRoles"
Set-AzureADApplication -ObjectId $azureAdApplication.objectId -appRoles $desiredAppRoles | Out-Null
}
| foreach ($user in $UserConfiguration) { | ||
| $userId = $user.id | ||
| if ($UserNamePrefix) { | ||
| $userId = Get-UserId -EnvironmentName $UserNamePrefix -UserId $user.Id |
There was a problem hiding this comment.
Should -EnvironmentName be renamed to -Prefix? It's simply prepending the prefix to the username so -EnvironmentName feels a little weird?
| Set-AzureADUserPassword -ObjectId $aadUser.ObjectId -Password $passwordSecureString -EnforceChangePasswordPolicy $false -ForceChangePasswordNextLogin $false | ||
| } | ||
| else { | ||
| $PasswordProfile = New-Object -TypeName Microsoft.Open.AzureAD.Model.PasswordProfile |
There was a problem hiding this comment.
should be lowercase since it's a variable $passwordProfile
|
|
||
| $environmentUsers += @{ | ||
| upn = $userUpn | ||
| environmentId = $userId |
There was a problem hiding this comment.
nit: envrionmentId = $userId seems a little weird?
There was a problem hiding this comment.
Yea, it's not a good name. Basically I want to return the environment specific user id, so something like pr-123-john.
|
|
||
| if (!$keyVault) { | ||
| Write-Host "Creating keyvault with the name ${keyVaultName}" | ||
| New-AzureRmKeyVault -VaultName $keyVaultName -ResourceGroupName ${EnvironmentName} -Location 'East US' | Out-Null |
There was a problem hiding this comment.
should KeyVault always be created in East US?
release/scripts/PowerShell/FhirServerRelease/Public/Add-AadTestAuthEnvironment.ps1
Show resolved
Hide resolved
|
|
||
| Write-Host "Ensuring client application exists" | ||
| foreach ($clientApp in $TestAuthEnvironment.ClientApplications) { | ||
| $displayName = "${EnvironmentName}-$($clientApp.Id)" |
There was a problem hiding this comment.
nit: most of the other scripts are using
There was a problem hiding this comment.
Also, this format is used in both Add-AadTestAuthEnvironment and Remove-AadTestAuthEnvironment. Should it be extracted to shared module?
|
|
||
| Write-Host "Tearing down test authorization environment for AAD" | ||
|
|
||
| $TestAuthEnvironment = Get-Content -Raw -Path $TestAuthEnvironmentPath | ConvertFrom-Json |
There was a problem hiding this comment.
lowercase $testAuthEnvironment
|
|
||
| Write-Host "Setting up Test Authorization Environment for AAD" | ||
|
|
||
| $TestAuthEnvironment = Get-Content -Raw -Path $TestAuthEnvironmentPath | ConvertFrom-Json |
There was a problem hiding this comment.
lowercase $testAuthEnvironment
samples/scripts/PowerShell/FhirServer/Public/New-FhirServerClientApplicationRegistration.ps1
Show resolved
Hide resolved
…/Microsoft/fhir-server into personal/brpoll/auth-aad-scripts
…/Microsoft/fhir-server into personal/brpoll/auth-aad-scripts
No description provided.