pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
919 stars 325 forks source link

New command: 'aad oauth2grant scope add' to appending scope to existing OAuth2 grants #2889

Closed pkskelly closed 1 month ago

pkskelly commented 2 years ago

Usage

m365 aad oauth2grant scope add [options]

Description

Add scope to existing OAuth2 permission grant

Options

Option Description
-g, --grantId <grantId> The Id of the grant to append the new scope to
-s, --scopes <scopes> Space separated list of scopes to add to the existing grant.

Example:

m365 aad oauth2grant scope add --grantId 1eY...7y6MMXBDlyA9s_URDPg  --scopes "AuditLog.Read.All"

Additional Info

This enhancement is based on attempting to use 'm365 aad oauth2grant set' to add the "AuditLog.Read.All" scope and inadvertently overwriting all existing scopes. See Inventory Microsoft 365 Guest SignInActivity with CLI for M365 for details.

waldekmastykarz commented 2 years ago

Thank you for the suggestion @pkskelly! I just checked what we've already got and notice we have https://pnp.github.io/cli-microsoft365/cmd/aad/oauth2grant/oauth2grant-add/. Would this be sufficient for your scenario or would you see us extend it to be more suitable for your needs?

pkskelly commented 2 years ago

@waldekmastykarz Let me take a look. For some reason I thought I tried this without success. I will try this again and provide an update. Thanks for checking.

pkskelly commented 2 years ago

Here are some details of my research and findings.

First, I re-consented the PnP Office 365 Management Shell app permissions using https://login.microsoftonline.com/common/oauth2/authorize?client_id=<clientId>&response_type=code&prompt=admin_consent for the client_id of the PnP Office 365 Management Shell Enterprise app in my tenant.

An attempt to run m365 aad oauth2grant add results in the error below. Since the combination of --clientId (objectId of the Enterprise App) and the MS Graph service principal objectId (--resourceId) pair already exists, an expected error indicating the permission already exists occurs (--clientId and --resourceId pair already exists with permission scopes).

grpah-add-error

Since the intention is to add a scope to the existing MS Graph service principal + PnP app, and it already exists, the m365 aad oauth2grant add does not meet the need at this point. This is what I originally tried which led me to attempt to use m365 aad oauth2grant set.

However, using m365 aad oauth2grant set with the following succeeds, but removes all other scopes.

 m365 aad oauth2grant set --grantId  <grantId> --scope "AuditLog.Read.All"

post-set-scopes

Following this command, I re-consented again to restore the base permission scopes for MS Graph, and Audit Log scope is removed again.

The last action was to use m365 aad oauth2grant set again, but this time with all base scopes from the initial consent, and adding the Audit Log scope.

m365 aad oauth2grant set --grantId <grantId> --scope "AuditLog.Read.All Policy.Read.All AppCatalog.ReadWrite.All User.Invite.All Reports.Read.All Group.ReadWrite.All Directory.ReadWrite.All Directory.AccessAsUser.All Mail.ReadWrite Mail.Send IdentityProvider.ReadWrite.All ChannelMessage.Send TeamsApp.ReadWrite.All TeamsTab.ReadWrite.All ChannelSettings.ReadWrite.All TeamSettings.ReadWrite.All TeamMember.ReadWrite.All ChannelMember.ReadWrite.All ChannelMessage.Read.All TeamsAppInstallation.ReadWriteForUser Team.Create Tasks.ReadWrite"

The above command completes and the AuditLog.Read.All scope is included in the grant scopes, and the script call the Graph for signInActivity completes.

Based on all of this, I think the proposed m365 aad oauth2grant add --grantId --scope <additional_scope> still makes sense (I also corrected the original proposal Example to be correct since this had set instead of add).

Thoughts @waldekmastykarz and @garrytrinder?

waldekmastykarz commented 2 years ago

Thank you for the additional information @pkskelly. Like you mentioned, it would make the most sense to extend the existing aad oauth2grant add command to be able not only to add a new grant but also a new scope to an existing grant. One thing that we should consider is if the command's intent would be obvious: the command is named oauth2grant add which means add an OAuth2 grant rather than add a new scope to an existing OAuth2 grant. Having a command like aad oauth2grant scope add would be more obvious but the question is if it would be too granular. What do you think?

pkskelly commented 2 years ago

Good point on considering the commands intent @waldekmastykarz. I agree, clarifying to oauth2grant add scope makes the intent much more obvious and makes more sense from a command perspective. While this does deepen the command hierarchy, the granular use of a scope qualifier does have the added benefit of explicitly enabling an admin to increase scopes (and decrease scopes if oauth2grant remove scope were created).

If I had to pick, I would go with the more granular version since this makes the command explicit. Based on current definitions of the oauth2grant add and oauth2grant remove commands from help, this makes it obvious that the service principal associated with a specific grant are being increased (or decreased).

waldekmastykarz commented 2 years ago

Thank you for sharing your perspective @pkskelly. I think we'll go with it indeed. I'll create two new specs for the commands you mentioned and we'll then close this issue in favor of the new commands, ok?

waldekmastykarz commented 2 years ago

Thinking of it more, I think it would be clearer if it was a separate command named aad oauth2grant scope add. If we combined adding grants to service principals and adding scopes to existing grants, it could get confusing quickly. I'll adjust the spec accordingly

martinlingstuyl commented 9 months ago

Hi @pkskelly, @pnp/cli-for-microsoft-365-maintainers,

I think we should close this issue and make sure our new commands for adding permissions support this.

For app registrations: https://github.com/pnp/cli-microsoft365/issues/4922

For Enterprise applications / Service Principals: https://github.com/pnp/cli-microsoft365/issues/5777

Adam-it commented 1 month ago

Hi @pkskelly, @pnp/cli-for-microsoft-365-maintainers,

I think we should close this issue and make sure our new commands for adding permissions support this.

For app registrations: #4922

For Enterprise applications / Service Principals: #5777

I agree. Lets close it up, we may always reopen if needed. also the initial case was about the PnP Management Shell app which is not here anymore.