Bug report: [`aad app add`] delegated and application permissions as options are skipped when using a manifest

[michaelmaillot]

Description

Like mentioned here:

If I refer to the command remarks regarding manifest usage, it says that options specified in the command will override manifest properties.

However, for now it's the opposite as the --apisDelegated / --apisApplication options are skipped. This is because the updateAppFromManifest method reads again the sumbitted manifest and if it contains requiredResourceAccess property, it overrides the options during transformation, before being patched.

Steps to reproduce

1. create an Azure AD app manifest file which will contain requiredResourceAccess property, and save it locally:

{
  "name": "My awesome app",
  "requiredResourceAccess": [
    {
      "resourceAppId": "00000003-0000-0000-c000-000000000000",
      "resourceAccess": [
        {
          "id": "62a82d76-70ea-41e2-9197-370581804d09",
          "type": "Role"
        }
      ]
    },
    {
      "resourceAppId": "00000003-0000-0ff1-ce00-000000000000",
      "resourceAccess": [
        {
          "id": "56680e0d-d2a3-4ae1-80d8-3c4f2100e3d0",
          "type": "Scope"
        }
      ]
    }
  ],
  "signInAudience": "AzureADMyOrg"
}

Permissions requested in the file:

Microsoft Graph - Group.ReadWrite.All (Application) SharePoint - AllSites.FullControl (Delegated)

2. run the following CLI command with the mentioned manifest:

m365 aad app add --manifest @C:\Temp\aad-manifest.json --apisApplication 'https://graph.microsoft.com/Directory.Read.All'

Expected results

The registered app should have the following configured permissions:

Actual results

The registered app have the following configured permissions:

Diagnostics

Creating Azure AD app registration... Existing access token ey... still valid. Returning... Request: { "url": "https://graph.microsoft.com/v1.0/myorganization/applications", "method": "post", "headers": { "common": { "Accept": "application/json, text/plain, /" }, "delete": {}, "get": {}, "head": {}, "post": { "Content-Type": "application/x-www-form-urlencoded" }, "put": { "Content-Type": "application/x-www-form-urlencoded" }, "patch": { "Content-Type": "application/x-www-form-urlencoded" }, "user-agent": "NONISV|SharePointPnP|CLIMicrosoft365/5.5.0", "accept-encoding": "gzip, deflate", "accept": "application/json;odata.metadata=none", "authorization": "Bearer ey..." }, "responseType": "json", "decompress": true, "data": { "displayName": "My awesome app", "signInAudience": "AzureADMyOrg", "requiredResourceAccess": [ { "resourceAppId": "00000003-0000-0000-c000-000000000000", "resourceAccess": [ { "id": "7ab1d382-f21e-4acd-a863-ba3e13f7da61", "type": "Role" } ] } ] } } Response: { "url": "https://graph.microsoft.com/v1.0/myorganization/applications", "status": 201, "statusText": "Created", "headers": { "cache-control": "no-cache", "transfer-encoding": "chunked", "content-type": "application/json;odata.metadata=none;odata.streaming=true;IEEE754Compatible=false;charset=utf-8", "location": "https://graph.microsoft.com/v2/1fdd85e0-9a94-6892-8ab0-5ad1b834475f/directoryObjects/cb821d93-4419-4874-8cdb-05d3d797a0c7/Microsoft.DirectoryServices.Application", "vary": "Accept-Encoding", "strict-transport-security": "max-age=31536000", "request-id": "65395b85-a860-4147-aa2f-4a1c23a9e6b6", "client-request-id": "65395b85-a860-4147-aa2f-4a1c23a9e6b6", "x-ms-ags-diagnostic": "{"ServerInfo":{"DataCenter":"France Central","Slice":"E","Ring":"5","ScaleUnit":"001","RoleInstance":"PA2PEPF00002694"}}", "x-ms-resource-unit": "1", "odata-version": "4.0", "date": "Wed, 03 Aug 2022 21:01:48 GMT", "connection": "close" }, "data": { "id": "cb821d93-4419-4874-8cdb-05d3d797a0c7", "deletedDateTime": null, "appId": "ff1e444a-2b3f-4359-91f1-55e07edc9701", "applicationTemplateId": null, "disabledByMicrosoftStatus": null, "createdDateTime": "2022-08-03T21:01:48.9597581Z", "displayName": "My awesome app", "description": null, "groupMembershipClaims": null, "identifierUris": [], "isDeviceOnlyAuthSupported": null, "isFallbackPublicClient": null, "notes": null, "publisherDomain": "contoso.onmicrosoft.com", "serviceManagementReference": null, "signInAudience": "AzureADMyOrg", "tags": [], "tokenEncryptionKeyId": null, "samlMetadataUrl": null, "defaultRedirectUri": null, "certification": null, "optionalClaims": null, "addIns": [], "api": { "acceptMappedClaims": null, "knownClientApplications": [], "requestedAccessTokenVersion": null, "oauth2PermissionScopes": [], "preAuthorizedApplications": [] }, "appRoles": [], "info": { "logoUrl": null, "marketingUrl": null, "privacyStatementUrl": null, "supportUrl": null, "termsOfServiceUrl": null }, "keyCredentials": [], "parentalControlSettings": { "countriesBlockedForMinors": [], "legalAgeGroupRule": "Allow" }, "passwordCredentials": [], "publicClient": { "redirectUris": [] }, "requiredResourceAccess": [ { "resourceAppId": "00000003-0000-0000-c000-000000000000", "resourceAccess": [ { "id": "7ab1d382-f21e-4acd-a863-ba3e13f7da61", "type": "Role" } ] } ], "verifiedPublisher": { "displayName": null, "verifiedPublisherId": null, "addedDateTime": null }, "web": { "homePageUrl": null, "logoutUrl": null, "redirectUris": [], "implicitGrantSettings": { "enableAccessTokenIssuance": false, "enableIdTokenIssuance": false } }, "spa": { "redirectUris": [] } } } Existing access token ey... still valid. Returning... Request: { "url": "https://graph.microsoft.com/v1.0/myorganization/applications/cb821d93-4419-4874-8cdb-05d3d797a0c7", "method": "patch", "headers": { "common": { "Accept": "application/json, text/plain, /" }, "delete": {}, "get": {}, "head": {}, "post": { "Content-Type": "application/x-www-form-urlencoded" }, "put": { "Content-Type": "application/x-www-form-urlencoded" }, "patch": { "Content-Type": "application/x-www-form-urlencoded" }, "user-agent": "NONISV|SharePointPnP|CLIMicrosoft365/5.5.0", "accept-encoding": "gzip, deflate", "content-type": "application/json", "authorization": "Bearer ey..." }, "responseType": "json", "decompress": true, "data": { "requiredResourceAccess": [ { "resourceAppId": "00000003-0000-0000-c000-000000000000", "resourceAccess": [ { "id": "62a82d76-70ea-41e2-9197-370581804d09", "type": "Role" } ] }, { "resourceAppId": "00000003-0000-0ff1-ce00-000000000000", "resourceAccess": [ { "id": "56680e0d-d2a3-4ae1-80d8-3c4f2100e3d0", "type": "Scope" } ] } ], "signInAudience": "AzureADMyOrg", "api": {}, "info": {}, "web": { "implicitGrantSettings": {}, "redirectUris": [] }, "spa": { "redirectUris": [] }, "displayName": "My awesome app" } } Response: { "url": "https://graph.microsoft.com/v1.0/myorganization/applications/cb821d93-4419-4874-8cdb-05d3d797a0c7", "status": 204, "statusText": "No Content", "headers": { "cache-control": "no-cache", "strict-transport-security": "max-age=31536000", "request-id": "6f07bc82-324e-48da-ae8d-e0b803259dc2", "client-request-id": "6f07bc82-324e-48da-ae8d-e0b803259dc2", "x-ms-ags-diagnostic": "{"ServerInfo":{"DataCenter":"France Central","Slice":"E","Ring":"5","ScaleUnit":"001","RoleInstance":"PA2PEPF00000F3C"}}", "x-ms-resource-unit": "1", "date": "Wed, 03 Aug 2022 21:01:48 GMT", "connection": "close" } } { "appId": "ff1e444a-2b3f-4359-91f1-55e07edc9701", "objectId": "cb821d93-4419-4874-8cdb-05d3d797a0c7", "tenantId": "1fdd85e0-9a94-6892-8ab0-5ad1b834475f" } DONE

CLI for Microsoft 365 version

5.5.0

nodejs version

16.15.0

Operating system (environment)

Windows

Shell

PowerShell

cli doctor

{ "os": { "platform": "win32", "version": "Windows 10 Pro", "release": "10.0.19043" }, "cliVersion": "5.5.0", "nodeVersion": "v16.15.0", "cliAadAppId": "31359c7f-bd7e-475c-86db-fdb8c937548e", "cliAadAppTenant": "common", "authMode": "DeviceCode", "cliEnvironment": "", "cliConfig": { "autoOpenBrowserOnLogin": true, "copyDeviceCodeToClipboard": true }, "roles": [], "scopes": [ "AllSites.FullControl", "AppCatalog.ReadWrite.All", "Directory.AccessAsUser.All", "Directory.ReadWrite.All", "Group.ReadWrite.All", "IdentityProvider.ReadWrite.All", "Mail.Send", "Reports.Read.All", "TermStore.ReadWrite.All", "User.Invite.All", "User.Read.All", "profile", "openid", "email" ] }

Additional Info

Right now, I see three update approaches:

• Be consistent according to the doc and kindly remove the requiredResourceAccess property in the manifest file, if --apisDelegated / --apisApplication options are submitted
• Update the doc, to be able to take both command options and manifest property
• Update the doc, to state that along with the name property, the --apisDelegated / --apisApplication options will be also overriden by the requiredResourceAccess manifest property if exists

[martinlingstuyl]

Thanks for raising this @michaelmaillot! Again: good catch 👏

I'm all for following what the docs currently state:

If --apisDelegated is specified, this should override the delegated scopes from the manifest.

If --apisApplication is specified, this should override the app only scopes from the manifest.

[martinlingstuyl]

By the way, does it mean that ALL options are overridden? Not only the scopes...

[michaelmaillot]

By the way, does it mean that ALL options are overridden? Not only the scopes...

Mind crossing @martinlingstuyl: I just though about that also, few hours after submitting the issue 😄

So I've tested all command options along with the manifest file, with or without related properties. Globally, the behavior described in the command documentation is wrong. Below my tests results:

--multitenant Skipped if mentioned in the manifest

--platform / --redirectUris Skipped even if not mentioned in the manifest

--implicitFlow Works as option, but doesn't work if specified in manifest

--uri Works as expected

--withSecret Creates two secrets if specified in both manifest and as an option

--scopeName, --scopeAdminConsentDescription, --scopeAdminConsentDisplayName & --scopeConsentBy Triggers an error if values in both manifest and as options: Error: Permission (scope or role) cannot be deleted or updated unless disabled first.

--certificateName Skipped if mentioned in the manifest

--certificateFile Skipped if mentioned in the manifest

--certificateBase64Encoded Skipped if mentioned in the manifest

[martinlingstuyl]

👏 Thanks for clearing that up @michaelmaillot 😅. Although probably not many people will stumble across this buggy behavior, it seems right to align it to the docs as fast as possible.

[martinlingstuyl]

Did you get to the bottom of the error with the scope options?

Error: Permission (scope or role) cannot be deleted or updated unless disabled first.

Suggests to me that it tries to update after the initial creation. Which seems to be close to what the docs specify as expected behavior.

[michaelmaillot]

Right, in fact there's a patch request called, in the configureUri() method once app created.

But if the manifest contains oauth2Permissions property, it's first updated with those.

Then the configureUri() method also try to update the app if the --scopeName command option is mentioned.

According to me, two possibilities:

• CLI documentation should warn about that to indicate that user can choose between setting property in the manifest or through command options, but not both.
• running configureUri() method before the updateAppFromManifest() one (but have to be sure that it doesn't trigger regressions first) and in that case: the options won't prevail on the manifest property

[martinlingstuyl]

Hi @michaelmaillot

If I execute your example (using the manifest and the apisApplication option) the scopes and roles are merged together. They are not overridden, but merged:

We could update the docs on this a bit. Add a remark about scopes being merged. What do you think @pnp/cli-for-microsoft-365-maintainers?

There are more issues here though, like with platform/redirectUris.

To the big question is: Should we merge options with manifest options when possible, or override them?

[martinlingstuyl]

Okay, so I went through this. What @michaelmaillot writes here is correct. The behavior is inconsistent.

Some options could be merged with the manifest, but some other options cannot be easily merged. I think I'd be for making the behavior consistent and like it's mentioned in the docs: if an option is used, it should always override the manifest, and not merge.

What do you think @pnp/cli-for-microsoft-365-maintainers?

[michaelmaillot]

Hi @michaelmaillot

If I execute your example (using the manifest and the apisApplication option) the scopes and roles are merged together. They are not overridden, but merged:

We could update the docs on this a bit. Add a remark about scopes being merged. What do you think @pnp/cli-for-microsoft-365-maintainers?

There are more issues here though, like with platform/redirectUris.

To the big question is: Should we merge options with manifest options when possible, or override them?

Yes, that's currently the behavior we wanted, as mentioned here. But I agree that there's a caveat as I should have updated the docs according to this, even if it was stated as "temporary".

Okay, so I went through this. What @michaelmaillot writes here is correct. The behavior is inconsistent.

Some options could be merged with the manifest, but some other options cannot be easily merged. I think I'd be for making the behavior consistent and like it's mentioned in the docs: if an option is used, it should always override the manifest, and not merge.

What do you think @pnp/cli-for-microsoft-365-maintainers?

Even if I'm not a maintainer, here's my two cents: options should override manifest when mentioned because according to me, if they're mentioned along with a manifest file, it's for a certain reason and it would make sense to consider them.

[waldekmastykarz]

If we look at our docs:

So our intention was to override not add, and if we don't override that's a bug.

If we want to revisit our stance, that's also okay, but it would be a breaking change that we need to postpone until the next major version.

[martinlingstuyl]

I'm of the opinion to keep the documented behavior.

[Adam-it]

+1 to override as well in order to be consistent with the docs. Additionally lets open a discussion and promote this behavior. If this should change to 'merge' kind of behavior we should reimplement for next major thanks @michaelmaillot for rising this up 👍 awesome catch 🤩 thanks @waldekmastykarz and @martinlingstuyl for the great feedback. The time you guys spend on managing issues is truly inspiring 🤩