search

pnp / cli-microsoft365

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

Working with Entra ID permissions #5666

Open martinlingstuyl opened 1 year ago

martinlingstuyl commented 1 year ago

We've recently added app permission add for working with permissions of app registrations. But that command can only be used when you are working with a m365rc.json file.

We also have:

I think we need a couple more commands to be able to cover the entire permissions thing and to make the experience of working with permissions a little more consistent:

Working with App Registrations

Working with Service Principals

Service Principals can be instances of App Registrations, but they don't have to be. (as in the case of managed identities). We already support working with service principals, but the experience is not very consistent and clear. Bundling this in a single command and allowing users to work with resource URL's and scopes would be a great step forward in my opinion.

From this list, aad sp permission add would in time be able to replace aad approleassignment add and aad oauth2grant add.

Shared code

There's also quite some duplicated code among all these commands that's actually doing the same. I'd suggest we move some code to utils:

Fixes

We're also implementing a rename of entra serviceprincipal <verb> commands.

Other fixes

waldekmastykarz commented 1 year ago

What's the benefit of replacing oauth2grant/approleassignment commands with sp permission?

martinlingstuyl commented 1 year ago

I think it would benefit the end user.

In terms of language What's an oauth2grant? It does not make sense to me what that would do. oauth2 is a very technical term. approleassignment is slightly clearer, but still... We should be building commands that focus on fixing a user scenario. If I'm an IT-admin, my challenge would typically be: "I want to assign something permissions". I'm not interested in if that's using the oauth2 standard/protocol. I may be used to navigating the Entra ID portal, and the word used there is "permissions" as well. Even if I look into the manifest of entra Id App Registrations, the term cannot even be found there. what we're describing with approleassignment and oauth2grant here is called requiredResourceAccess there. And it's just one list with delegated and app-only permissions. In fact, it seems these words really are just used because we employ the Graph API endpoints /v1.0/oauth2PermissionGrants and /v1.0/servicePrincipals/${objectId}/appRoleAssignments.

It seems to me these commands were build with the Graph API in mind, while we should be building stuff with the end-user in mind.

A single command fits the purpose I'm assigning permissions, whether that be delegated or app only permissions. It seems to me it belongs together, just like it belongs together in the Azure Portal. As a user it's odd that for something that belongs together you have to use separate commands. And they even work differently, On the one you need to define resourceId's. on the other you may use resource names. It's just odd.

waldekmastykarz commented 1 year ago

Thanks for clarifying. You're right: oauth2grant and approleassignments are pretty technical. But the same applies to a service principal, which is yet another word that doesn't appear anywhere in the Azure Portal UI. Following the Azure Portal route, we've got App registrations and Enterprise applications (service principals). While we could align with them, we need to be mindful that UI labels tend do change, and the last thing we want is to keep breaking users each time the label changes. So it's a balancing act between clarity and stability.

I agree that we should aim to simplify things where possible. This might be one of those areas. If we can properly distinguish between app-only and delegated permissions without confusing the user and cluttering the code base, then let's try to write a spec so that we can understand how it would work, before we commit to it.

martinlingstuyl commented 1 year ago

Agreed.

service principal, which is yet another word that doesn't appear anywhere in the Azure Portal UI

Hmm, that's a very good point indeed! It would have been better to have command groups like aad appregistration <verb> VS aad app <verb>. But that would be one messy rename in the current setup 😀

martinlingstuyl commented 1 year ago

Although it may be a good rename for v8, when we also probably rename aad to entra

entra appregistration <verb>
entra appregistration permission <verb>
entra app <verb>
entra app permission <verb>
entra user <verb>
entra group <verb>

Where entra app will replace the current aad sp.

Etc...

(or entra application <verb> instead of entra app <verb>)

Would be very clear and understandable for everyone! Clear entities instead of multi-interpretable or too technical words...

waldekmastykarz commented 1 year ago

Personally, I'd find just app confusing. While appregistration is explicit and clear, app is ambiguous, until you see that we've got appregistration. I suggest we make it more explicit so that folks don't need to guess what it's for. serviceprincipal or enterpriseapp are both clear, but both come with a caveat: serviceprincipal is pretty internal and requires you to understand the internal workings of Entra, it's also expressed on Graph. enterpriseapp is in the UI, which could potentially change in the future. We could use all names as aliases, like serviceprincipal, sp, enterpriseapp to help folks find what they need.

martinlingstuyl commented 1 year ago

I see what you mean! I like entra enterpriseapp as well, though you're right about the potential caveat there. What do you think about entra application <verb>. Would that be clearer than app or just more of the same?

The pro of app or application would also be that it would be alphabetically sorted next to applicationregistration in the docs. Which would be convenient as they're related.

waldekmastykarz commented 1 year ago

To me app and application are interchangeable and equally vague. If only we called them app instances and app registrations, but if we used appinstance then it would be a new term that no one knows, which isn't any better.