Skip to content
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

Enhancement: extend spo list get with --withPermissions #2796

Closed
Adam-it opened this issue Oct 27, 2021 · 15 comments
Closed

Enhancement: extend spo list get with --withPermissions #2796

Adam-it opened this issue Oct 27, 2021 · 15 comments

Comments

@Adam-it
Copy link
Contributor

Adam-it commented Oct 27, 2021

We need to extend the command spo list get with additional option --withPermissions. When the option is passed, the result would include permission information along with other details.

Specs are updated based on this conversation.

Additional Info

I came across this when I wanted to add a CLI equivalent of and existing PnP PowerShell script pnp/script-samples#110
I was able to retrieve all list information similar to PnP PowerShell except permission info 🤔. I wonder maybe I did something wrong 😅 or maybe there is no such functionality so we could ad one 🤔?
please let me know what do you think

@waldekmastykarz
Copy link
Member

Thanks for the suggestion, @Adam-it. If I look at the sample you linked, there is an equivalent version based on the CLI. Are we still missing something or has the gap been filled already?

@Adam-it
Copy link
Contributor Author

Adam-it commented Oct 28, 2021

@waldekmastykarz yes we are missing some information which I could not retrieve (this is the unique permissions and members info -what group and what permission). I already added and example as 80% of the list info is the same as PnP Powershell and it got approved, but this info is missing.
Please check this screen
image
I marked the columns which were present using PnPPowershell and which we don't have when I did the same using CLI

@waldekmastykarz
Copy link
Member

Ah, understood. Have you checked how we currently handle that for webs, which also are securable objects? It would be good to build it in a consistent way across the different spo commands.

@Adam-it
Copy link
Contributor Author

Adam-it commented Oct 30, 2021

not yet.. but I will do so in the coming weekend and let you know 👍

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 1, 2021

ok so I checked and for example in web in CLI we have an argument --withGroups when this is used we add and expand option to the rest call to get the additional info

if (args.options.withGroups) {
      url += '?$expand=AssociatedMemberGroup,AssociatedOwnerGroup,AssociatedVisitorGroup';
}

we could do similar thing for the list command 🤔. So we could add and additional argument option --withGroups or other like --permissionInfoOne (😉😋) and when this is used we may add
.../roleassignments?$expand=Member/users,RoleDefinitionBindings
for example like
https://tenanttocheck.sharepoint.com/sites/demomarketing/_api/web/lists/GetByTitle('sampleList')/roleassignments?$expand=Member/users,RoleDefinitionBindings

also we could use this rest call to check the additional info if the permission inheritance is broken or not
...(ListRestApiCall)/HasUniqueRoleAssignments

@waldekmastykarz
Copy link
Member

Thanks for the research @Adam-it. What if we extended the existing spo list get command with a new option named --withPermissions that would include the additional information you mentioned? @pnp/cli-for-microsoft-365-maintainers any opinions about this approach?

@arjunumenon
Copy link
Member

I am with the idea of adding the param --withPermissions which will give the permission details only as and when needed. I am with that.

If that is the case, do you think it make sense to update the specs @waldekmastykarz so that it would be clearer?

@waldekmastykarz
Copy link
Member

Yes, good idea @arjunumenon 👍

@arjunumenon arjunumenon changed the title New command: get list permission info Enhancement: extend spo list get with --withPermissions Nov 8, 2021
@arjunumenon
Copy link
Member

Hello @Adam-it - Thanks for all the suggestion and all findings which you have made. We have updated the specs based on the conversations.
Do you want to take a stab in updating the command?

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 8, 2021

@arjunumenon wow that's cool 👍 thanks a lot for the help and fast changes 👍 you're awesome 🤩
TBH I would really love to be assigned to it but currently I have different issue assigned in this repo and also a list of issues in script-samples repo were I also try to increase number of samples with CLI version... and some other as well 🙄😋... so all in all I will leave this open for any other candidate which would be willing to contribute 👍. If in some time I will be free for anything new, and this still will be open, I will let you know to get assigned 😉

@arjunumenon
Copy link
Member

Thanks for the kind words @Adam-it ❤.

Totally understand your schedule and appreciate all the help in script-sample areas as well. You have already done a great job in identifying endpoints needed for enhancing the command.

Based on your suggestion, We will keep the issue open to others so that they can contribute it. Appreciate your help 👍.

@Abderahman88
Copy link
Contributor

Volunteer for this one ✋

Do we want to show all the information for the roles and permissions?

if (args.options.withPermissions) {
      requestUrl += '?$expand=HasUniqueRoleAssignments,RoleAssignments/Member,RoleAssignments/RoleDefinitionBindings';
    }

image

image

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 11, 2021

@Abderahman88 this is way cool 😎. I think I am not in any position to make any kind of final decision here 😋 but yes I would suggest to include both (HasUniqueRoleAssignments and RoleAssignments) information in the json response 👍.
TBH I was thinking that this parameter --withPermissions would return similar information to PnP Powershell Get-PnPList -Includes HasUniqueRoleAssignments,RoleAssignments
🤔 What do you think/suggest? Also @waldekmastykarz, @arjunumenon do you agree?

@arjunumenon
Copy link
Member

Thanks @Abderahman88 for looking into that and volunteering. It is all yours.

I would also second @Adam-it recommendation to add HasUniqueRoleAssignments,RoleAssignments as a staring point.
Then in future, if needed, let us enhance it further.

@waldekmastykarz
Copy link
Member

+1 on the suggestion from @Abderahman88 and @Adam-it 👏

@waldekmastykarz waldekmastykarz added this to the v4.2 milestone Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment