-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Application permission check on certain commands #4054
Comments
@pnp/cli-for-microsoft-365-maintainers what do you think? |
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 |
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 However, we are now working with |
Good point! If we can use throw to break the code flow, it would suffice as well! |
Just not sure if we should centralise a single |
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. |
@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. |
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. |
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.
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? |
I'd say in these very edge cases, we just write our own logic in the command. |
@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. |
I think that all the |
Originating from #3894 (comment)
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.cli-microsoft365/src/m365/planner/commands/bucket/bucket-add.ts
Lines 108 to 111 in 1cd4e45
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
Let's check for other commands where this could be useful as well.
The text was updated successfully, but these errors were encountered: