pnp
Introduce Permissions section in docs
Context
The idea is to introduce a new section to our docs called Permissions. Here we list all needed permissions to execute a specific command (both delegated and application permissions). This way it is easier for developers to use their own app registration with a few permissions instead of having the PnP one which has all permissions. It will also be easier to run the CLI with application permissions.
In the first stage we are going to list permissions only for Graph commands because these are well documented.
I suggest that we list all permissions that the command can possibly use in one table. Example: when using the command m365 planner plan get, if we provide the option ownerGroupName, we need the extra permission Group.Read.All.
I also suggest that we don't list all permissions e.g. to read group members we could use GroupMember.Read.All, GroupMember.ReadWrite.All, Group.Read.All and Group.ReadWrite.All. I suggest that we only list the most restrictive permission being GroupMember.Read.All in this case.
Right now we do have some admonitions in our remarks section stating which special roles you need. For example: To run this command you need the SharePoint Administrator or Global Administrator role.. I suggest that we also move this to the new Permissions section.
Where should we place it? Debatable, I'd say let's put it right before the response section.
How could it look like?
Still open for discussion (like everything else). Current design looks like:
I'm all for this idea! It's a good idea to split the different permission types into tabs. That way we don't clutter the docs with too many tables and all the necessary information is still available.
My only thought here is if we should wait with this feature until we changed our SSG (#4396). If we implement it now it will create a bit more work for us during the migration process.
Iām all for it.
Should we just add the scopes to the documentation? Or add them to the code and extract them to the documentation? The advantage would allowing us to verify the scopes when running commands as well (I started a POC on that a while back)
Agreed with all suggestions @milanholemans.
My only thought here is if we should wait with this feature until we changed our SSG (https://github.com/pnp/cli-microsoft365/issues/4396). If we implement it now it will create a bit more work for us during the migration process.
Are you looking into ways to automate document conversion or rather planning to do it manually @Jwaegebaert? If it's the former, then having an extra table with tabs shouldn't make a difference. If automating the migration is too hard, then I agree then we should wait until we migrate the docs to avoid unnecessarily doubling the effort.
Should we just add the scopes to the documentation? Or add them to the code and extract them to the documentation? The advantage would allowing us to verify the scopes when running commands as well (I started a POC on that a while back)
Another way @martinlingstuyl would be to have them in the docs and have the command extract them from the docs. For performance reasons, we could put it behind a config setting that's a guardrail of sorts and which is enabled by default, but can be disabled if you don't need our help with scopes and want your scripts to run as quickly as possible.
Another way @martinlingstuyl would be to have them in the docs and have the command extract them from the docs
But that would necessitate some quite complicated parsing @waldekmastykarz, or am I wrong? The other way around would be simpler (using a json object) and you could execute that on build, instead of on every command execution.
But that would necessitate some quite complicated parsing @waldekmastykarz, or am I wrong?
Yes, it would require some degree of parsing, which we'll need to implement no matter which way we go.
The other way around would be simpler (using a json object) and you could execute that on build, instead of on every command execution.
We'd need to plug it into the process of building docs, not the code. We'd need some code to load all commands, iterate through them, for each get its permissions, change them into an MD table, and inject in the right location of the corresponding MD file before passing MD files to SSG for building docs.
You're right that it would be better for CLI's performance to have that information readily available in commands. I still think that checking permissions should be hidden behind an option that folks can disable to increase performance.
Storing permissions on commands also means, that we'd need to change commands' implementation to allow defining permissions and need undefined as the default value (which we also need to consider when updating docs) until we implement it for all commands.
Thinking about it @waldekmastykarz, I think you're right. Parsing the docs would mean less changes in the entire codebase, it keeps it separate from the command implementations. Which might be more optimal.
Also: what do you think about just executing the checks when a command throws an error? (In the handleError functions) That's the only scenario when it's relevant anyway. And it draws less on general performance. What do you think about that?
We could add it as part of the printHelp function for example. In that case we have a config key in place already
Also: what do you think about just executing the checks when a command throws an error? (In the handleError functions) That's the only scenario when it's relevant anyway. And it draws less on general performance. What do you think about that?
That's a good alternative to consider: execute the check only when the request failed with 401. It won't give you an instantaneous feedback, like we could with having scopes on the command, but maybe it's acceptable in that case, because it's unlikely to occur in normal conditions.
We could add it as part of the printHelp function for example. In that case we have a config key in place already
The caveat here is, that we allow users to not print help on error, which would suppress this check. I think it should be a separate check that users can control independent of help.
Sorry for late reply @milanholemans I agree on all proposed. Are you planning on opening a PR with extended docs for one command that we may use as an example? @Jwaegebaert as @waldekmastykarz asked I am also wondering why would we need to wait for making the switch to SSG?
@milanholemans I agree on all proposed. Are you planning on opening a PR with extended docs for one command that we may use as an example?
When everything has been sorted out, this has to be something we should do indeed.
Are you looking into ways to automate document conversion or rather planning to do it manually @Jwaegebaert? If it's the former, then having an extra table with tabs shouldn't make a difference. If automating the migration is too hard, then I agree then we should wait until we migrate the docs to avoid unnecessarily doubling the effort.
At this moment, I was looking to do it manually but it could be worth pondering if automation would be faster. In both cases, I feel like it would be double the effort as we are working/discussing the new SSG. If this feature needs to be implemented as fast as possible, then I'm all for it. š If it's something that can wait then I would opt for putting on hold so we can implement it directly in the new format. Maybe another way about it could be if we set up a different branch during the migration process, that folks already apply these changes directly to that branch but that could make it more complex as well.
@pnp/cli-for-microsoft-365-maintainers what do you all think the best approach would be in this scenario?
We already have tabs in a lot of places, for the command responses, it seems to me that we would look into some migration scripting (if necessary) anyway, so putting it on hold is not necessary for my part @milanholemans, @Jwaegebaert.
There's no urgency behind this enhancement, and I suggest we put it on hold for now. If we're not over to the new SSG by March, let's revisit our stance.
It's March and I believe @Jwaegebaert is in the middle of bringing over docs to Docusaurus. Shall we wait with this work so that we not unnecessarily delay the migration?
I will add a few permission sections to the docs as example + will ensure that this table is also decently formatted in the console when using --help.
Hey @milanholemans, did you have a chance to look at this? Would be cool to do a couple commands and get the ball rolling. Let me know if I can help with anything.
Hey @milanholemans, did you have a chance to look at this? Would be cool to do a couple commands and get the ball rolling. Let me know if I can help with anything.
Yes, I'm halfway through. Normally I'll create the PR in the coming days.