microsoftgraph / msgraph-bicep-types

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

Microsoft.Graph/groups 'owners' property is altered to include user who triggered deployment even when 'owners' is explicitly defined in Bicep #167

Closed HARVS1789UK-VAIIE closed 2 months ago

HARVS1789UK-VAIIE commented 2 months ago

Bicep version

Bicep CLI version 0.29.47 (132ade51bc)

Resource and API version Microsoft.Graph/groups@v1.0

Auth flow

Interactive (performed manually via Azure Cli e.g. az deployment group create ...

Deployment details

Deployment correlation id: 8793c575-bf90-4842-b4de-8a48f87dcbd6 Graph client request id: 9a3b3ca3-00b4-4c0f-bcb3-aae04cb357f1 Graph request timestamp: 2024-08-28T16:05:24Z

Describe the bug

Attempting to create a new Entra ID Group whilst providing the owners property (as a string[] of valid, pre-existing, Entra ID User ID's) throws the following deployment error:

{
    "status": "Failed",
    "error": {
        "code": "DeploymentFailed",
        "target": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/my-resource-group/providers/Microsoft.Resources/deployments/MyDeployment_v1",
        "message": "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details.",
        "details": [
            {
                "code": "",
                "message": "{\"error\":{\"code\":\"BadRequest\",\"target\":\"/resources/myGroup\",\"message\":\"Request contains a property with duplicate values. Graph client request id: 00000000-0000-0000-0000-000000000000. Graph request timestamp: 2024-08-28T16:05:24Z.\"}}"
            }
        ]
    }
}

This appears to be related to the owners property as:

To Reproduce

Using the below template:

resource myGroup 'Microsoft.Graph/groups@v1.0' = {
  displayName: 'My Group'
  uniqueName: 'unique-identifier-for-my-group'
  description: 'A group to evidence the bug'
  securityEnabled: true
  mailEnabled: false
  mailNickname: '-'
  owners: [
    '10000000-0000-0000-0000-000000000000',
    '20000000-0000-0000-0000-000000000000',
    '30000000-0000-0000-0000-000000000000'
  ]
}

replace the owner GUID placeholders with actual Azure Entra ID User ID's from the tenant you intend to deploy to ensuring that one of them is the ID of the user who will be triggering this deployment, then attempt to create this new group. You should see the deployment fail with the error above:

Request contains a property with duplicate values.

Additional context

The Microsoft Graph Bicep > Group documentation states the following regarding the owners property:

The owners of the group. Limited to 100 owners. Nullable. If this property isn't specified when creating a Microsoft 365 group, the calling user is automatically assigned as the group owner

It is my strong suspicion that the 'duplicate value' the deployment error is due to the User ID of the currently logged in user who undertakes the deployment (the "calling user" in the above docs snippet) is added to the owners array at all times not just when the "property isn't specified when creating" as suggested above. e.g. if you ran the above whilst logged in as Entra ID User 20000000-0000-0000-0000-000000000000 you'd end up with the following array:

        "myGroup": {
            "import": "microsoftGraph",
            "type": "Microsoft.Graph/groups@v1.0",
            "properties": {
                "displayName": "My Group",
                "uniqueName": "unique-identifier-for-my-group",
                "description": "A group to evidence the bug",
                "securityEnabled": true,
                "mailEnabled": false,
                "mailNickname": "-",
                "owners": [
                  '10000000-0000-0000-0000-000000000000',
                  '20000000-0000-0000-0000-000000000000',
                  '30000000-0000-0000-0000-000000000000',
                  '20000000-0000-0000-0000-000000000000'
                ]
            }
        }

Note that the resulting ARM template from these failed deployments does not reference any alterations to the Bicep inputs, so this modification of the owners array must be undertaken at a later stage, probably by the Microsoft Graph Bicep Extension

HARVS1789UK-VAIIE commented 2 months ago

As a slight aside, the mailNickname property for groups is also required at all times, including when mailEnabled: false as per the docs:

The mail alias for the group, unique for Microsoft 365 groups in the organization. Maximum length is 64 characters. This property can contain only characters in the ASCII character set 0 - 127 except the following characters: @ () / [] ' ; : <> , SPACE. Required

As can be seen in my sample above, I am forced to provide this and use a placeholder/workaround value of - as an empty string or a space is not valid.

Surely this property should only be required if mailEnabled: true ?

dkershaw10 commented 2 months ago

@HARVS1789UK-VAIIE Yeah - that's a separate issue (mailNickname being a required property on creation for all use cases). It's actually a Microsoft Graph API issue (not an issue with the Graph Bicep extension - although we made the property required in the Bicep type, to mirror the Graph API. I think this property also has a uniqueness constraint (to make matters worse).

I can raise that as a platform issue with the team that owns the Groups API.

We'll investigate the owners issue separately and come back to you shortly, but thanks for reporting!

dkershaw10 commented 2 months ago

@HARVS1789UK-VAIIE Quick investigation on the duplicate values - it looks like this is caused by the Microsoft Graph Groups API - so yes it is adding the current signed-in user to the list of users to add as an owner. If you add that same user to the owners list, it errors.

Again, this isn't something that the Graph Bicep extension is doing.

I can raise this with the Groups API team - it seems like if the duplicate value is for the signed-in user, they should remove that duplicate value from the list and go ahead with the operation.

For now as a workaround please assume that the signed-in user is always added as an owner, so there's no need to add them to the list. I recognize that this may be a problem if multiple owners of the group need to rerun the deployment :(

HARVS1789UK-VAIIE commented 2 months ago

Thank you for the investigation and feedback @dkershaw10 and I appreciate your offer to raise both of these points with the Groups API team, I think that would be worthwhile if you would be willing to.

It sounds like outside of the above, the underlying issue(s) here are out of your control and not directly caused by the Graph Bicep extension.

Can I suggest that the one directly actionable outcome from the Graph Bicep side would be to update the owners property description below to make clear that the current user will be added as an owner of the group in all cases (due to the Groups API, as yo've identified):

From the Microsoft Graph Bicep > Group documentation:

The owners of the group. Limited to 100 owners. Nullable. If this property isn't specified when creating a Microsoft 365 group, the calling user is automatically assigned as the group owner

Thanks for the timely response 👍

dkershaw10 commented 2 months ago

I've filed bugs with the Groups team. For the documentation issue - I can ask one of our content writers to make an update to the Microsoft Graph REST API reference docs. The Graph Bicep reference docs source descriptions from there.

dkershaw10 commented 2 months ago

@HARVS1789UK-VAIIE The public Graph REST API docs have been updated here - see the owners relationship property description. It links to a new known issue entry for the issue you raised here. This will eventually find its way automatically into the Graph Bicep reference docs too.

While the Groups team agreed this issue is likely a bug, I have no ETA on a fix or whether they will fix this issue.

Is it OK if we close this issue, or would you like it to remain open to track the possible bug fix?

HARVS1789UK-VAIIE commented 2 months ago

@dkershaw10 Thanks for the update.

It would likely be worth updating the docs re: owners on the Graph Bicep Groups docs as well to match the Graph API Groups additions, but yes I am happy this can be closed.

For now, I am just excluding myself from my parameterised list of owners whenever it is me running the deployment.

dkershaw10 commented 2 months ago

@HARVS1789UK-VAIIE Thanks for the response. Yes - this will be updated in the Graph Bicep Groups docs, at our next update (as the descriptions from the REST API docs are pulled into the Graph Bicep reference topics).

Thanks for building in a workaround. It's kind of a shame that the Bicep environment() function doesn't return the calling principal's IDs (like appId, userId etc) - that way they could be excluded no matter who runs the deployment. That said, the proper fix would be in the Groups API.