microsoft / navcontainerhelper

Official Microsoft repository for BcContainerHelper, a PowerShell module, which makes it easier to work with Business Central Containers on Docker.
MIT License
384 stars 247 forks source link

New-AadAppsForBC - The property 'Id' cannot be found on this object. #3339

Closed DBiernat closed 8 months ago

DBiernat commented 8 months ago

Describe the issue

Using New-AadAppsForBc ends up in the following error Message: The property 'Id' cannot be found on this object.

Scripts used to the issue

New-AadAppsForBC -appIdUri $idUri -bcAuthContext $authContext

Full output of scripts

Creating AAD App for WebClient
The property 'Id' cannot be found on this object. Verify that the property exists.
At C:\Program Files\WindowsPowerShell\Modules\BcContainerHelper\6.0.5\AzureAD\New-AadAppsForBc.ps1:169 char:5
+     $admspwd = Add-MgApplicationPassword -ApplicationId $SsoAdApp.Id  ...
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : PropertyNotFoundStrict

Additional context There is a typo in Line 169: "$SsoAdApp.Id" has to be "$ssoAdApp.Id"

freddydk commented 8 months ago

Variable names in PowerShell are case insensitive, so while you are right that this is a type, it isn't the reason for the problem. Just tried this:

$bcAuthContext = New-BcAuthContext -includeDeviceLogin -scopes "https://graph.microsoft.com/.default"
$AdProperties = New-AadAppsForBc -appIdUri https://mycontainer.directionsemea2019demo.onmicrosoft.com -bcAuthContext $bcAuthContext

Logged in as my test account on tenant directionsemea2019demo.onmicrosoft.com and this works fine:

Not sure what is wrong though - typically you would get another error first, like: image

(and if erroractionpreference is stop - you would never get the second)

DBiernat commented 8 months ago

To be honest, it was new to me, that variable names are Case Insensitive. 😅

The first difference is, that I'm using an App rather than the Device Login.

$authContext = New-BcAuthContext -tenantID $tenantID -clientID $clientID -scopes "https://graph.microsoft.com/.default" -clientSecret $clientSecret

What I've done to resolve the issue is basically copy and renaming your function (also copied parse-JWTtoken) and changed the (now I know usless) variable name.

Using the renamed function worked like a charm. I'm running the script in the ISE on a Windows 2022 Standard Server.

freddydk commented 8 months ago

The App works as well if it has the right permissions and the tenant is correct. Not sure what has caused the failure then?

DBiernat commented 8 months ago

I think the permissions are correct. Otherwise the copied function should not work, right?!? May be you could give me a list with the correct permissions so that I can check this.

The call to the copied function was done with the same BcAuthContext variable. I can reproduce the failure also on my client machine.

Are there any informations I could share to point us in the right direction?

freddydk commented 8 months ago

If you have a copy of the function and the only thing changes was S -> s to make it work - then something else is wrong. Whether you are calling an old BcContainerHelper or something else, I don't know. You could try to remove and uninstall all prior versions - and then install the latest.

ronaldnl commented 8 months ago

I am having the same error on the creation of azure VM through ARM. My suspicion is that the following:

    $ssoAdApp = New-MgApplication `
        -DisplayName "WebClient for $publicWebBaseUrl" `
        -IdentifierUris $appIdUri `
        -Web @{ ImplicitGrantSettings = @{ EnableIdTokenIssuance = $true }; RedirectUris = $signInReplyUrls } `
        -SignInAudience $signInAudience `
        -Info @{ "LogoUrl" = $iconPath } `
        -RequiredResourceAccess $resourceAccessList

Fills the $ssoAdApp, but the property id is no longer present. Since the graph API normally doesn't return the id either. see line below: Update-MgApplication -ApplicationId $ssoAdApp.Id -Api @{PreAuthorizedApplications = $PreAuthorizedApplications}

Currently investigating further though, but most likely it is in graph / graph api Last week this still worked fine, so something changed there (both are using the same version of bc container helper which was the latest stable release 6.0.5.

ronaldnl commented 8 months ago

So after further investigation I found that the powershell code in uses the variable $ssoAdApp with and without capital inconsistently (see code below this came from line 161 to 169):

   $ssoAdApp = New-MgApplication `
        -DisplayName "WebClient for $publicWebBaseUrl" `
        -IdentifierUris $appIdUri `
        -Web @{ ImplicitGrantSettings = @{ EnableIdTokenIssuance = $true }; RedirectUris = $signInReplyUrls } `
        -SignInAudience $signInAudience `
        -Info @{ "LogoUrl" = $iconPath } `
        -RequiredResourceAccess $resourceAccessList

    $admspwd = Add-MgApplicationPassword -ApplicationId $SsoAdApp.Id -PasswordCredential @{ "DisplayName" = "Password" }
freddydk commented 8 months ago

https://locall.host/is-powershell-variable-case-sensitive/

PowerShell variables are not case sensitive, try this:

$myVariable = “Hello, World!”
Write-Host $myVariable # Output: Hello, World!
Write-Host $MyVariable # Output: Hello, World!
Write-Host $MYVARIABLE # Output: Hello, World!

If you are seeing different results - we need to figure out why? There might be many other places, where variables are used without case consistency.

ronaldnl commented 8 months ago

We did a check, and if we run the code in the script separate it indeed is case insensitive. But as soon as we run the procedure it fails with the following: image

Which doesn't make sense. In both cases the app registration gets made though. (so at least it is not the SDK that is failing)

It looks like when calling the code wrapped in a function suddenly the code in the function does become case sensitive where as if we put it in a standalone file it isn't.

Diving through more parts of the setup to get to the why.

freddydk commented 8 months ago

what PowerShell version are you using?

DBiernat commented 8 months ago

Server: -> PSVersion 5.1.20348.2110 Client: -> PSVersion 5.1.22621.2506

DBiernat commented 8 months ago

May be, is that a hint?

https://devblogs.microsoft.com/scripting/weekend-scripter-unexpected-case-sensitivity-in-powershell/#:~:text=External%20technologies

freddydk commented 8 months ago

Might be - I asked for one small change to the PR, then I will merge it

freddydk commented 8 months ago

If we ever run into stuff like this again - we will continue where we left off.

freddydk commented 8 months ago

Merged - will be in preview in ~1 hour - and in PROD tonight

ronaldnl commented 8 months ago

Issue with out VM creation is solved with the new version.

I still agree it is odd that in some cases it becomes case sensitive. I tested the same piece of code that threw the error with the varying as a script and it ran fine (even the property casing you mentioned in the pull request couldn't break it) But will leave that for when we run into it again.

DBiernat commented 8 months ago

Unfortunately the fix did not help. Therefore I digged in a bit deeper.

$ssoAdApp = New-MgApplication `
        -DisplayName "WebClient for $publicWebBaseUrl" `
        -IdentifierUris $appIdUri `
        -Web @{ ImplicitGrantSettings = @{ EnableIdTokenIssuance = $true }; RedirectUris = $signInReplyUrls } `
        -SignInAudience $signInAudience `
        -Info @{ "LogoUrl" = $iconPath } `
        -RequiredResourceAccess $resourceAccessList

The above command is returning an array and therefore accessing the id is not possible. $ssoAdApp[0].Id is working.

The version of the module (Microsoft.Graph.Applications) I was using is 2.13.1. After updating the module to the latest version, which is 2.14.1, the command is returing an object and in the end accessing the id through $ssoAdApp.Id is working.

Sorry for the confusing on my site! May be you should adjust the preconditions for the helper.

freddydk commented 8 months ago

Thanks for the info. As expected the problem was in a different place. This issue will be there for people to find if they experience the same.

Thanks