Application permission check on certain commands

[milanholemans]

Originating from https://github.com/pnp/cli-microsoft365/issues/3894#issuecomment-1307805497

We could and maybe should add extra checks since some commands don't work with application permissions. Right now we are already doing this for planner commands.

https://github.com/pnp/cli-microsoft365/blob/1cd4e45a4596b6bee06b976a26ee95ee90dac81c/src/m365/planner/commands/bucket/bucket-add.ts#L108-L111

Should we maybe create a central function that checks this rather than copy-pasting these lines of code over and over again?

Commands to update

• [ ] All flow commands
• [ ] All Power Apps commands
• [ ] All Power Platform commands
• [ ] All Yammer commands
• [ ] teams chat send
• [ ] ...

Let's check for other commands where this could be useful as well.

[milanholemans]

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

[waldekmastykarz]

Good idea to add it to all commands where it's applicable. I wonder if we can centralize it though, since we need to break function execution by calling return on the main flow.

[milanholemans]

I wonder if we can centralize it though, since we need to break function execution by calling return on the main flow.

Good point, right we do a return indeed, but that was because we used to work with callbacks. When we passed an error to the callback function and had to add a return statement to prevent it from executing more of the commandAction.

However, we are now working with promise/await. We are now throwing errors, when we throw an error, the function will be terminated as well, so actually we can remove the return right now since it will never be hit anyway.

[waldekmastykarz]

Good point! If we can use throw to break the code flow, it would suffice as well!

[milanholemans]

Just not sure if we should centralise a single if with 1 line of code in the body. Or should we just copy/paste it. Might be overkill to centralise 3 lines of code.

[waldekmastykarz]

There's little overhead when it comes to centralizing it and offers us a great deal of consistency especially when it comes to the logic we use to decide if we're in app-only auth and the error message we return. I'm all for centralizing.

[MathijsVerbeeck]

@milanholemans I would love to give this a go. Would it be an idea to write this logic if all the commands of a single set require the check to add it in the command that it command that it extends, so for example for PowerPlatform, in the PowerPlatformCommand class?

I'll also do some research on other commands that require this check.

[milanholemans]

Not sure about this. While this would work for PowerPlatform commands, for example, we also have a bunch of commands that support both delegated & application permissions. Take a Graph command for example. Most commands support delegated and application permissions, but there are some commands that don't support application permissions. I think we should work uniformly and use the same logic everywhere. Maybe we could make a util class that checks if the command is running in app only mode and throws an error if this is the case. This will prevent us from copy-pasting the same code over and over again.

[MathijsVerbeeck]

Not sure about this. While this would work for PowerPlatform commands, for example, we also have a bunch of commands that support both delegated & application permissions. Take a Graph command for example. Most commands support delegated and application permissions, but there are some commands that don't support application permissions. I think we should work uniformly and use the same logic everywhere.

Of course I would only do this on where the entire set of commands use this, not where a Graph API is being used as this is command-specific.

Maybe we could make a util class that checks if the command is running in app only mode and throws an error if this is the case. This will prevent us from copy-pasting the same code over and over again.

Makes sense to write this in a util. Would we allow the user to pass a custom error message? Sometimes we will only block application permissions when updating specific properties, so we wouldn't have to throw a generic error. Or would we just retain the 'old way' for these cases?

[milanholemans]

Sometimes we will only block application permissions when updating specific properties, so we wouldn't have to throw a generic error.

I'd say in these very edge cases, we just write our own logic in the command.

[milanholemans]

@MathijsVerbeeck thinking about it some more. Stuff like Power Platform doesn't allow app permissions at all for any command, so maybe it does make sense to put it in the base command.

[MathijsVerbeeck]

I think that all the todo commands also require this check, as they call the me andpoint