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

[waldekmastykarz]

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

• the AAD app will be created in the current directory
• the AAD app will be created using the Azure CLI Azure AD app
• the created AAD app will be single-tenant
• when configuring permissions, CLI automatically chooses the permissions to be granted based on the selected mode
• when mode selected to app-only, automatically generates a certificate valid for a year
• later, when we support persisting configuration New command: cli config set #1945, running this command should update the config with the ID of the newly created app
• returns the Azure AD app ID and, if the selected mode was daemon, the thumbprint, password and expiration date of the generated cert

[waldekmastykarz]

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

• is this spec oversimplified and are we missing some cases?
• are user and deamon clear enough or do we use different names to denote the different usage modes
• 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?

[garrytrinder]

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]

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:

• 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

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]

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]

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]

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]

Updated spec. Is it good to go now?

[garrytrinder]

Ship it 😊

[garrytrinder]

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]

Spec updated. Is it ready to go?

[garrytrinder]

Ship it (again) 🙂 🚀

[mkm17]

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

[milanholemans]

Looks still valid to me, thanks! 👍

[Adam-it]

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
#5870
Pointed out by @martinlingstuyl. Currently this is the last thing not covered yet

[milanholemans]

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]

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]

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]

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]

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]

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]

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]

@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]

Alternate idea: --permissionsDelegated / --permissionsApplication

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

[martinlingstuyl]

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

[waldekmastykarz]

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]

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

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

[waldekmastykarz]

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

[martinlingstuyl]

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]

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]

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]

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