microsoftgraph / msgraph-bicep-types

Repo contains Microsoft Graph resource types to integrate with bicep templates.
MIT License
46 stars 7 forks source link

Cannot declare an AD/Entra-ID app registration in bicep that was previously created using `New-AzADApplication` #196

Open christianacca opened 4 days ago

christianacca commented 4 days ago

Bicep version Run bicep --version via the Bicep CLI, az bicep version via the AZ CLI

0.31.92 (b06509316a) for both

Resource and API version Which Microsoft.Graph resource and API version has the issue? Microsoft.Graph/applications@v1.0

Auth flow Is the deployment interactive (e.g. with a signed in user) or automated (e.g. with an application)?

service principal

Deployment details If it's related to deployment failures, please provide the deployment correlation id, Microsoft Graph client request id, and deployment timestamp if applicable.

Deployment Correlation ID: 542657bf-070a-4149-8471-9df6b0e69f0b Graph client request id: ad124473-0944-4c96-9761-1551b3c14cb2 Graph request timestamp: 2024-11-29T09:49:53Z

Describe the bug

It seems impossible to use the Microsoft.Graph/applications@v1.0 resource for an existing app registration that was created using New-AzADApplication.

When I try and define that same app registration using bicep using Microsoft.Graph/applications@v1.0 resource, I always get the error:

Another object with the same value for property identifierUris already exists. Graph client request id: xxxx-xxxx-xxxx. Graph request timestamp: xxxx

The only way to resolve this is to delete (then permanently delete) the existing app registration. That way, when the bicep declaration of this app registration runs, it gets created using the resource declaration in the bicep. Thereafter the bicep continues to work.

What should happen is the existing app registration should be recognised and any updates (if required) to match the bicep resource definition be made.

The above wouldn't be so bad if we could permanently delete the app registration from powershell, but after searching for a while now I couldn't find a way to achieve that even via the MS-Graph REST api. And so now, requires manual intervention and a potentially extended amount of downtime for the app that uses that registration. Not to mention the risk that, after manually permanently deleting the app registration, that the infra-as-code declaration is indeed successful to build out the desired state.

This in practice is a show stopper, and so unusable for greenfield infra-as-code.

To Reproduce Steps to reproduce the behavior:

Additional context

Here's an extract from bicep that I've been using:

var roleName = 'app_only'
var roleId = guid(roleName, functionAppName)
resource appReg 'Microsoft.Graph/applications@v1.0' = {
  displayName: functionAppName
  uniqueName: functionAppName
  identifierUris: [
    'api://${functionAppName}'
  ]
  appRoles: [
    {
      allowedMemberTypes: [
        'Application'
      ]
      description: 'Service-to-Service access'
      displayName: roleName
      id: roleId
      isEnabled: true
      value: 'app_only_access'
    }
  ]
}

resource appRegServicePrincipal 'Microsoft.Graph/servicePrincipals@v1.0' = {
  appId: appReg.appId
  appRoleAssignmentRequired: true
}

resource appRoleAssignments 'Microsoft.Graph/appRoleAssignedTo@v1.0' = [for principalId in allowedPrincipalIds: {
  appRoleId: roleId
  principalId: principalId
  resourceId: appRegServicePrincipal.id
}]

Full listing: internal-api.bicep

Also see: https://github.com/christianacca/web-api-starter/pull/22

dkershaw10 commented 3 days ago

Thanks for reporting @christianacca.

Please see https://learn.microsoft.com/graph/templates/how-to-reference-existing-resources?tabs=CLI for how to reference an existing application object.

Also uniquely named resource names and resource keys conceptual topic may help.

Let me know if this addresses your problem, and if it does, if there's a better way to surface this information (as you aren't the first person to raise the same issue).

christianacca commented 3 days ago

Oh boy! I've even read those articles before, and understood about the need to backfill the client provided key.

When I read those articles originally, I had in mind the scenario explicitly referenced in these doc links: referencing an existing resource using the existing keyword. I think the reason I didn't "connect the dots" for my use case of "onboarding" an existing resource as a bicep desired state declaration, is that I wasn't really referencing an existing resource, as in using the keyword existing.

I don't think the docs really help spell out that it's not just when you want to reference an existing resource using the existing keyword, that you need to back fill the id, but when declaring the desired state of an existing resource ie you are onboarding an existing resource into bicep.

dkershaw10 commented 3 days ago

Thanks for this feedback. This really helps, to get your perspective. I'll review the docs and see if we can make the "onboarding an existing resource into Bicep" pop more.

christianacca commented 3 days ago

also @dkershaw10, will the service principal that is running the bicep (already an owner of the app registration) have permission to also run the necessary az cli commands to backfill keys?

dkershaw10 commented 3 days ago

If all you need to do is update the app reg that the SP is an owner of (with a uniqueName) then that should work fine. That might not work if you need to update other resources though.

Having looked at the how-to topic - I definitely concur with your feedback. I'll make some changes and might even rename the topic (if the content team allow it :) )

christianacca commented 3 days ago

@dkershaw10, when you at improving the docs, I'd suggest that you mention that the identity that's already the owner of the graph resource (eg app registration) will have permissions to backfill the key.

christianacca commented 3 days ago

@dkershaw10, there's another issue. I have patched the uniqueName. When I run the bicep definition for the app registration I'm now getting:

{"error":{"code":"BadRequest","target":"/resources/appReg","message":"Permission (scope or role) cannot be deleted or updated unless disabled first. Graph client request id: 546082c6-f28f-4b07-b9ff-0bdcea83a41a. Graph request timestamp: 2024-11-29T17:56:16Z."}} (Code:)   CorrelationId: 51eb5547-7f22-4762-b076-42d4ddb86d2a

Where do I go from here... is this another blocker for getting an existing app registration into bicep?

As clarification: the bicep definition works fine if the app registration is created from scratch.

christianacca commented 3 days ago

@dkershaw10, I'm pretty sure I know what's going on: I need to remove the existing app role registrations as well before "onboarding" these into the bicep definition.

The error above, should in theory, not happen once I extend the fixup script to not only backfill the app registration, but also to delete the existing app role registrations first. The app registrations will then get created via the bicep definitions, and all should be well.

christianacca commented 3 days ago

@dkershaw10, the plot thickens. even remove the existing app role assignments is insufficient. I also need to delete the app role itself I believe.

I think the problem here is the id of the app role is different in my bicep definition compared to the existing app role resource.

The challenge is finding the right voodoo magic to actually delete approles. I've tried adding an empty appRoles array element to the patch request that is backfilling uniqueName, but it seems to have no affect - I still see the original app roles

christianacca commented 3 days ago

The challenge is finding the right voodoo magic to actually delete approles. I've tried adding an empty appRoles array element to the patch request that is backfilling uniqueName, but it seems to have no affect - I still see the original app roles

That was the final part of the puzzle. Turns out to be real fiddly to remove existing appRoles! For reference here's the pwsh that did the trick for me:

Write-Information '7.0.2. Remove original app roles...'
$targetAppAdRegistration = Get-AzADApplication -DisplayName $funcAppName -EA Stop
if ($targetAppAdRegistration.AppRole) {
    $appRegUrl = "https://graph.microsoft.com/v1.0/applications/$($targetAppAdRegistration.Id)"
    # note: `id` field is case sensitive in REST endpoint
    $appRoles = $targetAppAdRegistration.AppRole | Select-Object `
        @{n='allowedMemberTypes'; e={@(, $_.AllowedMemberType)}}, DisplayName, Description, `
        @{n='id'; e={$_.Id}}, @{n='isEnabled'; e={$false}}, Origin, Value

    $appRegJson = @{ appRoles = @($appRoles) } | ConvertTo-Json -Compress -Depth 100
    Write-Information "  Disable app role for '$funcAppName' to allow for it be deleted..."
    { Invoke-AzRestMethod -Method PATCH -Uri $appRegUrl -Payload $appRegJson -EA Stop } |
        Invoke-EnsureHttpSuccess | Out-Null

    $appRegJson = @{ appRoles = @() } | ConvertTo-Json -Compress
    Write-Information '  ...delete app role'
    { Invoke-AzRestMethod -Method PATCH -Uri $appRegUrl -Payload $appRegJson -EA Stop } |
        Invoke-EnsureHttpSuccess | Out-Null
}
dkershaw10 commented 1 day ago

@christianacca I see you've been busy and discovered that to delete an app role you first have to disable it... It is documented here - https://learn.microsoft.com/graph/templates/reference/applications?view=graph-bicep-1.0#microsoftgraphapprole

I will say that It's not particularly intuitive in Bicep, because this is not exactly what I would call desired state configuration. I think the expectation would be that if the appRoles array/collection is changed (to remove records from the collection) then this operation should succeed. And it would, as long as the records to be deleted are first disabled, which is gnarly. I think at the time, the team wanted to have safeguards against accidental record deletion.

Unfortunately this API behaviour predates Bicep.

We could explore doing something in the Graph Bicep extension to make the operation appear to conform to desired state configuration and not require explicitly disabling records in the collection before removing them from the collection in a subsequent update.

christianacca commented 1 day ago

We could explore doing something in the Graph Bicep extension to make the operation appear to conform to desired state configuration and not require explicitly disabling records in the collection before removing them from the collection in a subsequent update.

Yes please :-)

dkershaw10 commented 22 hours ago

Thanks @christianacca. I've filed #197 to track this ask.