Create-AadAppsForBC function now creating error when used along with -preAuthorizePowerShell

vvkjndl commented 2 years ago

Describe the issue Create-AadAppsForBC function now creating error when used along with -preAuthorizePowerShell.

Scripts used to create container and cause the issue

Create-AadAppsForBC -AadAdminCredential $aadadmincred -appIdUri "https://testaad.inecta.com/BC" -publicWebBaseUrl "https://testaad.inecta.com/BC" -preAuthorizePowerShell

Full output of scripts

Creating AAD App for WebClient
Create-AadAppsForBC Telemetry Correlation Id: aa61e9c8-c328-4c39-af0d-bd7ab2fe2f58
Get-AzureADMSApplication : Error occurred while executing GetMSApplications
Code: Request_ResourceNotFound
Message: Resource '45cb5db1-8218-45bc-a2b8-c755134c7760' does not exist or one of its queried reference-property objects are not present.
InnerError:
  RequestId: a3044585-fae3-4920-9593-6383ec67f754
  DateTimeStamp: Tue, 01 Feb 2022 12:53:22 GMT
HttpStatusCode: NotFound
HttpStatusDescription: Not Found
HttpResponseStatus: Completed
At C:\Program Files\WindowsPowerShell\Modules\BcContainerHelper\3.0.1\AzureAD\Create-AadAppsForNav.ps1:207 char:28
+ ... istration = Get-AzureADMSApplication -Filter "id eq '$($ssoAdApp.Obje ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Get-AzureADMSApplication], ApiException
    + FullyQualifiedErrorId : Microsoft.Open.MSGraphV10.Client.ApiException,Microsoft.Open.MSGraphV10.PowerShell.GetMSApplication

Screenshots N/A

Additional context

does it happen all the time? Yes
did it use to work? Yes

vvkjndl commented 2 years ago

After checking the Azure Portal, I can confirm that Object did create with Id "45cb5db1-8218-45bc-a2b8-c755134c7760".

ti-jalopez commented 2 years ago

Hi @freddydk , I've the same error. I'm testing your sample published on #2177, changing only containername and domain. It fails on https://github.com/microsoft/navcontainerhelper/blob/master/AzureAD/Create-AadAppsForNav.ps1#:~:text=%24appRegistration%20%3D%20Get%2DAzureADMSApplication%20%2DFilter%20%22id%20eq%20%27%24(%24ssoAdApp.ObjectId)%27%22 $appRegistration = Get-AzureADMSApplication -Filter "id eq '$($ssoAdApp.ObjectId)'"

After the error I've tested next command using the ID show in error, and the app have been created, but without API permisions and others.

freddydk commented 2 years ago

Do you have the AzureAD powershell module installed? (and what version)

freddydk commented 2 years ago

Apparently that function was added in AzureAD version 2.0.2.76 : https://www.powershellgallery.com/packages/AzureAD/2.0.2.76 Earlier Versions won't work. I will consider adding a version check to the function.

vvkjndl commented 2 years ago

@freddydk AzureAD version I am using is 2.0.2.140. This isn't a version problem.

Previously this function was working fine when I was doing testing with Run-TestsInBcContainer.

freddydk commented 2 years ago

Yeah, the function was changed to cope with OAuth v2.0 changes. Would it be possible to setup a troubleshooting session? If you could email me at freddyk at microsoft dot com and give me your timezone and a few timeslots where you are available, then we should be able to figure out why this fails.

Thanks

freddydk commented 2 years ago

This might be a timing/synchronization issue.

If you could modify the Create.AadAppsForNav.ps1 file and insert a Start-Sleep -seconds 10 before line 207 and then try again - then we will know whether this is the case.

Obviously the right fix is not just to wait 10 seconds - but it would be good to know.

Thanks

vvkjndl commented 2 years ago

Yeah, the function was changed to cope with OAuth v2.0 changes. Would it be possible to setup a troubleshooting session? If you could email me at freddyk at microsoft dot com and give me your timezone and a few timeslots where you are available, then we should be able to figure out why this fails.

Thanks

@freddydk Thanks for considering allocating a time slot for a troubleshooting session. Though I am in +5.30 timezone, you can decide any time slot as per your convenience.

We are very close to rolling out a complete migration to AAD authentication to our in-house containers, so I can make myself available at anytime given the priority.

In the meantime, I will try running tests again by adding some sleepy seconds.

ti-jalopez commented 2 years ago

Start-Sleep -seconds 10

Hello @freddydk , I've tested to add the sleep and the result is ok, no error. The script I've run is below.

Also I've a sugestion: in New-BcContainerHelper add a paremeter for setup the Excel Addin and it complete the requirements to create a container with Azure AD and Edit In Excel enabled. Nav instance parameter is ExcelAddInAzureActiveDirectoryClientId.

$containerName = "bc19v2ad"
$aadCredential = Get-Credential -Message 'Credenciales para AAD'
$aadCredential.UserName

$useSSL = $true
$params = @{ "useSSL" = $useSSL }
if ($useSSL) {
    $protocol = "https://"
    $params += @{
        "isolation" = "hyperv"
        "installCertificateOnHost" = $true
    }
}
else {
    $protocol = "http://"
}

$aadTenant = $aadCredential.UserName.Split('@')[1]
$appIdUri = "$protocol$containerName.$aadTenant/BC"

$AdProperties = Create-AadAppsForBC `
    -AadAdminCredential $aadCredential `
    -appIdUri $appIdUri `
    -publicWebBaseUrl "$protocol$containerName/BC" `
    -IncludeApiAccess `
    -IncludePowerBiAadApp `
    -IncludeExcelAadApp `
    -IncludeEmailAadApp `
    -PreAuthorizePowerShell

freddydk commented 2 years ago

The reason why this happens is that I use the "old" mechanism for creating the app and the "new" mechanism for pre-authorizing. I am working on changing the entire function to use new methods - for now - keep the sleep and when I checkin a new function test that.

On the excel add-in - you should be able to add

-additionalParameters** @("--env customNavSettings=ExcelAddInAzureActiveDirectoryClientId=$($excelapp.appid)")

to the container generation to get this added.

vvkjndl commented 2 years ago

@freddydk For me adding 20 seconds sleep fixed the problem.

freddydk commented 2 years ago

You can also try to grab the new version of the function here: https://github.com/microsoft/navcontainerhelper/blob/master/AzureAD/Create-AadAppsForNav.ps1 and see whether that solves the issue (this should be the right fix)

ti-jalopez commented 2 years ago

You can also try to grab the new version of the function here: https://github.com/microsoft/navcontainerhelper/blob/master/AzureAD/Create-AadAppsForNav.ps1 and see whether that solves the issue (this should be the right fix)

I have tested your new function Create-AadAppsForNav.ps1 using next code. The credential than I'm using is my office 365 email similar to: myemail@laberit.com MY SCRIPT

Write-Host "AzureAD version: $((Get-Module AzureAD).Version)"
$containerName = "bc19v2ad"
$aadCredential = Get-Credential -Message 'Credenciales para AAD'

$useSSL = $true
$params = @{ "useSSL" = $useSSL }
if ($useSSL) {
    $protocol = "https://"
    $params += @{
        "isolation" = "hyperv"
        "installCertificateOnHost" = $true
    }
}
else {
    $protocol = "http://"
}

$aadTenant = $aadCredential.UserName.Split('@')[1]
$appIdUri = "$protocol$containerName.$aadTenant/BC"

$AdProperties = Create-AadAppsForBC `
    -AadAdminCredential $aadCredential `
    -appIdUri $appIdUri `
    -publicWebBaseUrl "$protocol$containerName/BC" `
    -IncludeApiAccess `
    -IncludePowerBiAadApp `
    -IncludeExcelAadApp `
    -IncludeEmailAadApp `
    -PreAuthorizePowerShell

RESULT 1: first run

AzureAD version: 2.0.2.140
Creating AAD App for WebClient
Creating AAD App for API Access
Create-AadAppsForBC Telemetry Correlation Id: eecc631d-39c5-48c8-abc9-46ab1fc191f4
New-AzureADServicePrincipal : Error occurred while executing NewServicePrincipal 
Code: Request_BadRequest
Message: The appId '45cc17dc-7047-4e5f-adfe-438e5e9e4451' of the service principal does not reference a valid application object.
RequestId: 77fa7c15-9235-49d9-b9ed-ac304faa666e
DateTimeStamp: Thu, 03 Feb 2022 13:55:01 GMT
Details: PropertyName  - appId, PropertyErrorCode  - NoBackingApplicationObject
HttpStatusCode: BadRequest
HttpStatusDescription: Bad Request
HttpResponseStatus: Completed
En C:\Program Files\WindowsPowerShell\Modules\BcContainerHelper\3.0.1\AzureAD\Create-AadAppsForNav.ps1: 233 Carácter: 41
+ ... Principal = New-AzureADServicePrincipal -AppId $apiAdAppId -Tags @("W ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [New-AzureADServicePrincipal], ApiException
    + FullyQualifiedErrorId : Microsoft.Open.AzureAD16.Client.ApiException,Microsoft.Open.AzureAD16.PowerShell.NewServicePrincipal

RESULT 2,: second run

AzureAD version: 2.0.2.140
Creating AAD App for WebClient
Creating AAD App for API Access
Creating AAD App for Excel Add-in
Creating AAD App for PowerBI Service
Creating AAD App for EMail Service
Create-AadAppsForBC Telemetry Correlation Id: 1973d014-ddbc-4d3b-b747-584a49aeea2f
New-AzureADMSApplication : No se puede enlazar el parámetro porque se ha especificado más de una vez el parámetro 'Web'. Para proporcionar varios valores a los 
parámetros que pueden aceptar diversos valores, use la sintaxis de matriz. Por ejemplo, "-parameter valor1,valor2,valor3".
En C:\Program Files\WindowsPowerShell\Modules\BcContainerHelper\3.0.1\AzureAD\Create-AadAppsForNav.ps1: 314 Carácter: 13
+             -Web @{ "ImplicitGrantSettings" = @{ "EnableIdTokenIssuan ...
+             ~~~~
    + CategoryInfo          : InvalidArgument: (:) [New-AzureADMSApplication], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : ParameterAlreadyBound,Microsoft.Open.MSGraphV10.PowerShell.NewMSApplication

NEXT RESULTS: other runs Some times shows the same error as result 1 and others as result 2.

freddydk commented 2 years ago

no. 1 is another race condition, which I kind of assumed would come. no. 2 I have no idea what that is. let me fix no. 1 and then see if no. 2 goes away

freddydk commented 2 years ago

You can grab a new version of the function - I think both errors are fixed now.

vvkjndl commented 2 years ago

@freddydk We also need add one more reply URL to the application we create. Current code is creating problem when signing out from BC WebClient.

Currently assigned reply URLs:

https://abc.xyz.com/bc/SignIn https://abc.xyz.com/BC/SignIn

What we need:

https://abc.xyz.com/BC/SignIn https://abc.xyz.com/BC https://abc.xyz.com/bc/SignIn https://abc.xyz.com/bc

However, I am not sure if lowercase bc is needed.

freddydk commented 2 years ago

When would you need the one without SignIn?

freddydk commented 2 years ago

got it - on signout...

freddydk commented 2 years ago

Fixed this - and modified the apps from publicClient to Web - I think that is the right setup

freddydk commented 2 years ago

You can try the new version here: https://github.com/microsoft/navcontainerhelper/blob/master/AzureAD/Create-AadAppsForNav.ps1 (or wait 4-5 hours until the next pre-release ships)

ti-jalopez commented 2 years ago

You can grab a new version of the function - I think both errors are fixed now.

I've tested the new version and it run fine most cases, but I've detected a case with an small issue: After create an app registration using container name 'bc19v2ad', if you try to create the app using container name 'BC19V2AD' (the same name with all case characters), the error is:

Creating AAD App for WebClient
Create-AadAppsForBC Telemetry Correlation Id: efb18c03-84a7-4bad-83f0-8f7fc2fc0ac4
New-AzureADMSApplication : Error occurred while executing NewMSApplication 
Code: Request_BadRequest
Message: Another object with the same value for property identifierUris already exists.
InnerError:
  RequestId: e8652e6e-1762-49bf-979d-b8a446e5dfa6
  DateTimeStamp: Thu, 03 Feb 2022 20:27:36 GMT
HttpStatusCode: BadRequest
HttpStatusDescription: Bad Request
HttpResponseStatus: Completed
En C:\Program Files\WindowsPowerShell\Modules\BcContainerHelper\3.0.1\AzureAD\Create-AadAppsForNav.ps1: 139 Carácter: 17
+     $ssoAdApp = New-AzureADMSApplication `
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [New-AzureADMSApplication], ApiException
    + FullyQualifiedErrorId : Microsoft.Open.MSGraphV10.Client.ApiException,Microsoft.Open.MSGraphV10.PowerShell.NewMSApplication

freddydk commented 2 years ago

Thanks for your persistence on this matter :-) The fix for this is to replace .contains($xxx) with -contains $xxx

ti-jalopez commented 2 years ago

$appIdUri = "$protocol$containerName.$aadTenant/BC"

Hi @freddydk , I have a sugestion for next release of Create-AadAppsForBC:

If more than one in the same organization use your sample for the APP ID URI it is possible repeat the APP ID URI and when you executes Create-AadAppsForBC, it deletes de first app registration created. In this case the first container created is not accessible, because the app registration is deleted.
Maybe it should have a new param to allow delete previos app registration, and if not present this parent it trhows an error.

Thansk for all your aswers and quick resolution of issues or proposed improvements.

Best regads, José Antonio López

freddydk commented 2 years ago

The AppIdUri is container specific (because the replyto urls in the aad apps are container specific) If you create a second container, then the AAD apps won't have the reply urls for this container and cannot be reused. This is also the reasion why I remove the apps when AppIdUri is reused.

ti-jalopez commented 2 years ago

I'm using one registered azure app whith some containers, adding manually from azure portal all the URLs of my containers.

Best regards,

El vie., 4 feb. 2022 11:45, Freddy Kristiansen @.***> escribió:

The AppIdUri is container specific (because the replyto urls in the aad apps are container specific) If you create a second container, then the AAD apps won't have the reply urls for this container and cannot be reused. This is also the reasion why I remove the apps when AppIdUri is reused.

— Reply to this email directly, view it on GitHub https://github.com/microsoft/navcontainerhelper/issues/2287#issuecomment-1029855957, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQG3SNXTNOAOYTAF2CSUWFLUZOU6DANCNFSM5NJCGTJA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

freddydk commented 2 years ago

So you are talking about adding replyto addresses to existing apps instead of creating new ones?

ti-jalopez commented 2 years ago

The AppIdUri is container specific (because the replyto urls in the aad apps are container specific) If you create a second container, then the AAD apps won't have the reply urls for this container and cannot be reused. This is also the reasion why I remove the apps when AppIdUri is reused.

I have a sugestion for next release of Create-AadAppsForBC: If more than one in the same organization use your sample for the APP ID URI it is possible repeat the APP ID URI and when > you executes Create-AadAppsForBC, it deletes de first app registration created. In this case the first container created is not > > accessible, because the app registration is deleted. Maybe it should have a new param to allow delete previos app registration, and if not present this parent it trhows an error.

Hi @freddydk , So it is possible to create more than one registered app for different people in the same organization, it two people creates the same container name and using the appIdUri proposed ( $appIdUri = "$protocol$containerName.$aadTenant/BC") , when the second person creates de app registration, the command Create-AadAppsForBC deletes the first app registration without any warning or error. Because that, I think it should have a new param to allow delete the app registration if exists,

ti-jalopez commented 2 years ago

So you are talking about adding replyto addresses to existing apps instead of creating new ones?

That is what I do manually for share one app registration for more than one container:

First I create a registered app using a generic name for de appIdUri. Example: devarea, etc..
Second, for each container that I need to use this app registratios, I manually add the reply URLs.
Third, I create the container using the data the app registration created in the first steep, manually assigning the parameters (AadAppId, AadAppIdUri and AadTenant).

In any case, I am not requesting to automate this process from bccontainerhelper, I'm doing it manually without any problem.

Best regards,

freddydk commented 2 years ago

Got it. The big problem here is that if I do not create the AAD app, I won't be able to return the app Secrets (cannot get to them after creating the app) Recommendations would be to add a username or like to the appiduri when people are sharing azure subscriptions ($appIdUri = "$protocol$username.$containerName.$aadTenant/BC")

ti-jalopez commented 2 years ago

The big problem here is that if I do not create the AAD app, I won't be able to return the app Secrets (cannot get to them after creating the app)

Well, nowadays I don't use the secrets for anythink. When a need to add more reply URLs then I do that from Azure portal. And, In any case, when I've create the app registration the first time a can save the secrets returned with the creation.

However, I currently do this manually and am not requesting to create additional reply URLS from bccontainerhelper.

Recommendations would be to add a username or like to the appiduri when people are sharing azure subscriptions ($appIdUri = "$protocol$username.$containerName.$aadTenant/BC")

Ok, this is a good idea; but it is still easy to repeat apiIdURI in companies that share Azure Active Directory (same domain) with many people in the domain,

It's just an idea anyway, and certainly with the latest enhancements to managing containers with AAD credentials it's possible to do it much more easily. Thanks a lot

freddydk commented 2 years ago

I will definitely consider this

freddydk commented 2 years ago

BTW - please note that v20 changes from using wsfed to use OpenIdConnect - latest changes in insider BCContainerHelper adds support for this

freddydk commented 2 years ago

Closing this, but keeping the idea...

microsoft / navcontainerhelper

Create-AadAppsForBC function now creating error when used along with -preAuthorizePowerShell #2287