search

pnp / cli-microsoft365

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

New command: create custom AAD app for use with the CLI #1963

Open waldekmastykarz opened 3 years ago

waldekmastykarz commented 3 years ago

Usage

cli app add

Description

Creates custom Azure AD app for use by CLI for Microsoft 365

Options

Option Description
-m, --mode <mode> Determines if CLI will be used by a user (delegated) or as a deamon process (appOnly). Allowed values delegated,appOnly
-p, --permissions [permissions] List of permissions to assign to the app. If not specified, will assign all permissions currently used by CLI for Microsoft 365. For the actual list see https://pnp.github.io/cli-microsoft365/concepts/authorization-tokens/#azure-ad-application-used-by-the-cli-for-microsoft-365. Use permissions or permissionSet but not both
--permissionSet [permissionSet] Predefined set of permissions to assign to the app. Allowed values ReadAll, SpoFull, SpoRead. Use permissions or permissionSet but not both

Additional Information

waldekmastykarz commented 3 years ago

@pnp/cli-for-microsoft-365-maintainers I'd love your feedback. In particular, I'd appreciate if you thought about:

garrytrinder commented 3 years ago

Great suggestion! πŸ‘ Some of my thoughts...

is this spec oversimplified and are we missing some cases?

I don't think it needs to be anymore than what it is.

are user and deamon clear enough or do we use different names to denote the different usage modes

I would prefer delegate and app, as this then makes it clear what type of permissions will be added, this is also what the Azure Portal refers to them as.

is it correct to assume that in app-only mode folks will be okay with using a secret or should we use a certificate instead? If cert, why and what expiration period should we choose?

Thinking from a best practice perspective, certificate is more secure than providing a password or secret, so I think we should promote this as such. See below statement on the Certificate & Secrets blade in the Azure Portal.

Credentials enable confidential applications to identify themselves to the authentication service when receiving tokens at a web addressable location (using an HTTPS scheme). For a higher level of assurance, we recommend using a certificate (instead of a client secret) as a credential.

Microsoft have provided some guidance on the validity period of certificates based on length in the past (https://techcommunity.microsoft.com/t5/configuration-manager-archive/recommendations-for-pki-key-lengths-and-validity-periods-with/ba-p/272758).

Key length of 1024:  Validity period = not greater than 6-12 months
Key length of 2048:  Validity period = not greater than 2 years
Key length of 4096:  Validity period = not greater than 16 years

I think that one year would be a good starting point, coincidentally when creating a secret in the Azure Portal the validity period defaults to one year also.

plamber commented 3 years ago

Hi, agree with @garrytrinder regarding delegated and app mode. I would extend the permissions concept by letting the use specify the service he/she wants to enable.

For example:

etc.

Not sure if we should have a separate parameter for this.

As return type I would also return the Azure AD Url to the app.

br, Patrick

garrytrinder commented 3 years ago

For example:

  • ReadWrite.All (enables all permissions of the CLI)

  • Read.All (enables all permissions of the CLI for read operations

  • ReadWrite.SPO (enables all read and write SPO permissions)

  • Read.SPO (enables all read SPO permissions

Love this, great idea πŸ‘πŸ»

waldekmastykarz commented 3 years ago

Thanks for your thoughts gents! Here are some thoughts:

I would prefer delegate and app, as this then makes it clear what type of permissions will be added, this is also what the Azure Portal refers to them as.

How about delegated and app-only? mode: delegate sounds weird and mode: app isn't quite self-explanatory.

I like the suggestion to use a cert instead of a secret. I thought first of using a secret, because it doesn't expire. Since there is no reminder on expiring certs, admins are often getting surprised when things stop working. If we were to use a cert, then we should return the expiration date of the generated cert.

As for permissions: there is something to say for using permission sets that you mentioned, but I would still allow users to specify their own permissions as listed in AAD. So perhaps these sets could be exposed in a separate option named --permissionSet? To make it clear that these are made up permissions, I'd also suggest using different names like ReadAll, SpoFull, SpoRead. I don't need if we need Full as that's the default setting.

garrytrinder commented 3 years ago

How about delegated and app-only? mode: delegate sounds weird and mode: app isn't quite self-explanatory.

Works for me πŸ‘πŸ»

If we were to use a cert, then we should return the expiration date of the generated cert.

Agreed.

So perhaps these sets could be exposed in a separate option named --permissionSet? To make it clear that these are made up permissions, I'd also suggest using different names like ReadAll, SpoFull, SpoRead. I don't need if we need Full as that's the default setting.

Agreed, I like the idea of permission sets.

waldekmastykarz commented 3 years ago

Updated spec. Is it good to go now?

garrytrinder commented 3 years ago

Ship it 😊

garrytrinder commented 3 years ago

Actually, can we change app-only to appOnly, removing the -?

Like you said in another issue, we don't use them in option names so using them in values doesn't seem right.

waldekmastykarz commented 3 years ago

Spec updated. Is it ready to go?

garrytrinder commented 3 years ago

Ship it (again) :slightly_smiling_face: :rocket:

mkm17 commented 7 months ago

Hi, if this idea/task is still valid then I would like to take care of it :)

milanholemans commented 7 months ago

Looks still valid to me, thanks! πŸ‘

Adam-it commented 7 months ago

One comment from my side which we already talked about internally is that we could reuse a lot of code from the app add command siΔ™ we may move some to utils methods. Also we could wait a bit for that improvement https://github.com/pnp/cli-microsoft365/issues/5870 Pointed out by @martinlingstuyl. Currently this is the last thing not covered yet

milanholemans commented 7 months ago

Also we could wait a bit for that improvement #5870 Pointed out by @martinlingstuyl. Currently this is the last thing not covered yet

Since we don't call sub-commands anymore, this issue doesn't really matter, does it?

Adam-it commented 7 months ago

Also we could wait a bit for that improvement #5870 Pointed out by @martinlingstuyl. Currently this is the last thing not covered yet

Since we don't call sub-commands anymore, this issue doesn't really matter, does it?

I think I wrote that one comment above πŸ˜‰

mkm17 commented 7 months ago

so if I understand correctly, I should wait for the change until @martinlingstuyl makes his adjustments. Regarding the util comment from @Adam-it, would it be included in Martin's change, or should I move it into my commits?

martinlingstuyl commented 7 months ago

Hi @mkm17, the other issue is still in help wanted mode, I'm currently not working on it.

Aside from that: you could also just start and build the feature here. We'll need some shared code anyway, and you could include the code to update the public client flows toggle along with what you're building.

Thoughts? @pnp/cli-for-microsoft-365-maintainers

milanholemans commented 7 months ago

Hi @mkm17, the other issue is still in help wanted mode, I'm currently not working on it.

Aside from that: you could also just start and build the feature here. We'll need some shared code anyway, and you could include the code to update the public client flows toggle along with what you're building.

Thoughts? @pnp/cli-for-microsoft-365-maintainers

Exactly what I was thinking.

Adam-it commented 7 months ago

yep. I would also not wait for this feature if you are ready to go πŸš€. I would:

  1. look for similarities with the app add command and extract that logic to some util methods to be reused in both commands
  2. add handling the public client flows toggle along with implementing this if possible
martinlingstuyl commented 7 months ago

Maybe a bit late on this, but shouldn't we align this issue with how we add permissions on applications? (using --apisDelegated and --apisApplication options) @pnp/cli-for-microsoft-365-maintainers @mkm17

mkm17 commented 7 months ago

@martinlingstuyl, as I understand, instead of choosing the --mode and assigning --permissions, with your approach, the mode would be chosen by defining the correct parameter --apisDelegated or --apisApplication. But then how to handle the --permissionsSet? By adding more options like: ReadAllDelegated, ReadAllApplication? What do you think @pnp/cli-for-microsoft-365-maintainers?

martinlingstuyl commented 7 months ago

Alternate idea: --permissionsDelegated / --permissionsApplication

It would then be an optionset @mkm17, specify either permissionsDelegated / permissionsApplication or permissionSet

martinlingstuyl commented 7 months ago

Ideas @pnp/cli-for-microsoft-365-maintainers ? Or shall we keep the specs as is?

waldekmastykarz commented 7 months ago

But then how to handle the --permissionsSet? By adding more options like: ReadAllDelegated, ReadAllApplication? What do you think @pnp/cli-for-microsoft-365-maintainers?

@mkm17 the idea was that the permissionSet defines the functional set of permissions and the mode dictates whether the granted permissions would be delegated or application.

It would then be an optionset @mkm17, specify either permissionsDelegated / permissionsApplication or permissionSet

In this approach, how would we differentiate between application and delegated permissions in the permissionSet @martinlingstuyl?

martinlingstuyl commented 7 months ago

We could make that two options as well: --permissionsSetDelegated and --permissionsSetApplication....

But maybe this does not make it clearer after all...

waldekmastykarz commented 7 months ago

I think it's too complicated. We can also drop permissions sets for now and start with just delegated and app-only permissions

martinlingstuyl commented 7 months ago

In that case: is your suggestion to keep --mode and --permissions? And what values would you expect for permissions? The full resource+scope urls I presume? @waldekmastykarz

waldekmastykarz commented 7 months ago

Yes, let's stick with --mode and --permissions. For permissions, the safest would be to get resource+scope, although I believe, that Entra defaults to Graph if all you specify is scope. We can follow the same route to make it more convenient for folks to specify their permissions.

mkm17 commented 6 months ago

Hi, I've paused a bit on this task as I've had some other topics at work. I'll come back to it after Easter. :)

mkm17 commented 5 months ago

Hello everyone, I've finally created a pull request (PR) for this task. However, I still have some concerns, so I'm eager to hear your opinions on certain aspects described in the PR comment. More information is available here: #5985