-
Notifications
You must be signed in to change notification settings - Fork 341
Introduce Permissions section in docs #4399
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
Comments
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.
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.
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. |
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. |
Yes, it would require some degree of parsing, which we'll need to implement no matter which way we go.
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. |
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 |
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.
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 |
When everything has been sorted out, this has to be something we should do indeed. |
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 @pnp/cli-for-microsoft-365-maintainers what do you all think the best approach would be in this scenario? |
for me it's totally fine to put it on hold 👍 |
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 |
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. |
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 optionownerGroupName
, we need the extra permissionGroup.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
andGroup.ReadWrite.All
. I suggest that we only list the most restrictive permission beingGroupMember.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:
The text was updated successfully, but these errors were encountered: