Azure / azure-cli

Azure Command-Line Interface
MIT License
3.97k stars 2.95k forks source link

App roles update is broken after Azure CLI upgrade #22710

Open floushee opened 2 years ago

floushee commented 2 years ago

This is autogenerated. Please review and update as needed.

Describe the bug

Command Name az ad app update

Errors:

Expected property 'lang' is not present on resource of type 'AppRole'

To Reproduce:

Steps to reproduce the behavior. Note that argument values have been redacted, as they may contain sensitive information.

Expected Behavior

Environment Summary

Windows-10-10.0.22000-SP0
Python 3.10.4
Installer: MSI

azure-cli 2.37.0

Extensions:
kusto 0.5.0
resource-graph 2.1.0

Dependencies:
msal 1.18.0b1
azure-mgmt-resource 21.1.0b1

Additional Context

Today Azure DevOps rolled out the Azure CLI version 2.37, which forced me to migrate some of our Azure CLI steps as explained in this migration guide: https://docs.microsoft.com/en-gb/cli/azure/microsoft-graph-migration

The only thing that is still not working is the update of our app registration's roles. We are currently using the following json file for the roles deployment:

[ { "allowedMemberTypes": [ "User" ], "description": "Grafana read only Users", "displayName": "Grafana Viewer", "id": "....", "isEnabled": true, "lang": null, "origin": "Application", "value": "Viewer" }, { "allowedMemberTypes": [ "User" ], "description": "Grafana Editor Users", "displayName": "Grafana Editor", "id": "...", "isEnabled": true, "lang": null, "origin": "Application", "value": "Editor" } ]

It looks like this was already fixed in a different cli: https://github.com/pnp/cli-microsoft365/issues/3131

yonzhan commented 2 years ago

@jiasli for awareness

floushee commented 2 years ago

It is working when I remove the "lang" property. Until now it did not matter. The "lang"=null is also visible in manifest json in the portal and I have seen it in some blog posts etc like this. So I might not be the only one who gets confused by this new behaviour and it would make sense to fix it, or at least provide some error message.

jiasli commented 2 years ago

I saw the PR https://github.com/pnp/cli-microsoft365/pull/3137 just deletes lang property:

https://github.com/pnp/cli-microsoft365/pull/3137/files#diff-4bffc2f0d69d60fce428e83ed0b46c80a224eea3de200b3197330bdfb59a7fd0R381-R385

    if (graphManifest.appRoles) {
      graphManifest.appRoles.forEach((role: any) => {
        delete role.lang;
      });
    }

Actually, Azure CLI's az ad app update --id {} --app-roles {} doesn't make any special processing for appRole property. It sends the JSON to Update application API as-is:

https://github.com/Azure/azure-cli/blob/710c821b2920df504ae037e7b05763193b051a30/src/azure-cli/azure/cli/command_modules/role/custom.py#L715

The appRole resource type doesn't have lang property, so the Microsoft Graph service returned that error.

Indeed, we can add some special logic on the client side (Azure CLI) to remove the lang property here:

https://github.com/Azure/azure-cli/blob/710c821b2920df504ae037e7b05763193b051a30/src/azure-cli/azure/cli/command_modules/role/custom.py#L1570-L1579

but I think it would be more reasonable for the manifest to be consistent with Microsoft Graph API. I will work internally with Microsoft Graph team on this.