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.

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

Allow -UseDeviceAuthentication switch in Deploy_SHM.ps1 #1378

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

craddm
Copy link
Contributor

@craddm craddm commented Feb 2, 2023

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the develop branch.
  • Your branch is up-to-date with the develop branch (you probably started your branch from develop but it may have changed since then).
  • If-and-only-if your changes are not yet ready to merge, you have marked this pull request as a draft pull request and added '[WIP]' to the title.
  • If-and-only-if you have changed any Powershell code, you have run the code formatter. You can do this with ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory>.

⤴️ Summary

Adds a switch for Deploy_SHM.ps1 to allow use of device authentication.

🌂 Related issues

Closes #1377

🔬 Tests

Tested on a local deployment.

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

Have you checked for all places where authentication is required (e.g. switching accounts)?

A quick search

rg Connect-Az
Deploy_SHM.ps1
16:Connect-AzAccount -ErrorAction Stop | Out-Nullrg Connect-Mg
Deploy_SHM.ps1
28:Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process | Out-Null

Setup_SHM_Key_Vault_And_Emergency_Admin.ps1
27:    Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process

Setup_SHM_AAD_Domain.ps1
25:    Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process

@craddm
Copy link
Contributor Author

craddm commented Feb 3, 2023

Have you checked for all places where authentication is required (e.g. switching accounts)?

A quick search

❯ rg Connect-Az
Deploy_SHM.ps1
16:Connect-AzAccount -ErrorAction Stop | Out-Null
❯ rg Connect-Mg
Deploy_SHM.ps1
28:Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process | Out-Null

Setup_SHM_Key_Vault_And_Emergency_Admin.ps1
27:    Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process

Setup_SHM_AAD_Domain.ps1
25:    Connect-MgGraph -TenantId $config.azureAdTenantId -Scopes "User.ReadWrite.All", "UserAuthenticationMethod.ReadWrite.All", "Directory.AccessAsUser.All", "RoleManagement.ReadWrite.Directory" -ErrorAction Stop -ContextScope Process

Those in Deploy_SHM.ps1 are dealt with in the PR.
Deploy_SHM.ps1 and Deploy_SRE.ps1 actively force the tokens to be refreshed by disconnecting the user from existing logins and making the user login as part of the script. The others don't pose the same problem, as they only come into play if the user has not already authenticated with Microsoft Graph. The other scripts don't force a token refresh and will simply use the existing token if the user is already authenticated, so the user can simply run Connect-MgGraph or Connect-AzAccount themselves if the login imposed by the script fails.

I could change the behaviour or all scripts to add a -UseDeviceAuthentication switch, but my intention was to make Deploy_SHM.ps1 behave the same way as Deploy_SRE.ps1 on the assumption that forcing a token refresh when starting from the beginning is desirable, but less so when running individual steps directly with scripts like Setup_SHM_AAD_Domain.ps1 (as then you'd have to log back in for every step, even when running Deploy_SRE.ps1).

@JimMadge JimMadge merged commit 8794bf7 into alan-turing-institute:develop Feb 3, 2023
@craddm craddm deleted the device-code-deploy branch February 3, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive authentication required when deploying an SHM
2 participants