-
Notifications
You must be signed in to change notification settings - Fork 326
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
Deploy SA with 3 separate app registrations, Fixes #686 #698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets review when you get a chance please
$ISADMTApplicationIDProvided = ($ADMTApplicationIDAdmin && $ADMTApplicationIDPortal) | ||
|
||
if($ISADMTApplicationIDProvided -eq $null){ | ||
Write-Host "🔑 Multi-Tenant App Registrations provided." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can call it , "Login App Registration.."
{ | ||
"requestedAccessTokenVersion" : 2 | ||
}, | ||
"signInAudience" : "AzureADandPersonalMicrosoftAccount", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be single Tenant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good way to restrict users further. That said, sometimes the individual responsible for actions on the admin portal may not even be part of the tenant hosting the app registrations. In those cases, partner will need to invite the user to the tenant before being able to manage subscriptions.
@@ -96,7 +97,7 @@ public void ConfigureServices(IServiceCollection services) | |||
.AddOpenIdConnect(options => | |||
{ | |||
options.Authority = $"{config.AdAuthenticationEndPoint}/common/v2.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should change to /tenantid/ not common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make this change too if you no longer want multitenant access to the admin portal (comment above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--sign-in-audience AzureADMYOrg
Fixes #686
Creates unique App Registrations for both landing page and admin portal
Associates appropriate App registrations and URI redirects
Added both app registrations to web app config
Added additional Client ID to supporting interface