cli-microsoft365 icon indicating copy to clipboard operation
cli-microsoft365 copied to clipboard

"entra group user" renaming and enchancement

Open dojcsakj opened this issue 1 year ago • 12 comments

entra group user list does not show nested security groups. It shows only member users but does not show member groups.

It would be useful to be able to get groups or to get users+groups.

Filtering happens because of the microsoft.graph.user part in group-user-list.ts:184:

const endpoint: string = `${this.resource}/v1.0/groups/${groupId}/${role}/microsoft.graph.user?$select=${selectParam}${expandParam}`;

To get groups microsoft.graph.group can be used:

const endpoint: string = `${this.resource}/v1.0/groups/${groupId}/${role}/microsoft.graph.group?$select=${selectParam}${expandParam}`;

To get user + groups no microsoft.graph.* is needed:

const endpoint: string = `${this.resource}/v1.0/groups/${groupId}/${role}?$select=${selectParam}${expandParam}`;

Todo:

  • [ ] entra group user ... commands should be marked as deprecated
  • [ ] move all this logic to ęntra group member ...
  • [ ] add subgroup support to all ęntra group member ... commands (e.g. add, remove, list, ...)

dojcsakj avatar Jun 10 '24 16:06 dojcsakj

@pnp/cli-for-microsoft-365-maintainers what is our stance on this? In my opinion, it's not really ideal that we only return users and not groups.

milanholemans avatar Jun 29 '24 12:06 milanholemans

@milanholemans, @dojcsakj I totally agree. I think it would be a great improvement to show always all members of a group, so users and groups. I only wonder if we should create a new command for it like entra group member list that would list out groups and users and in the current command, so entra group user list we mark is as deprecated and suggest using the ..member list command and for v8 we could remove it. 🤔 @pnp/cli-for-microsoft-365-maintainers what do you think? it would also align with what we have in spo so spo group member list

Adam-it avatar Jul 05 '24 10:07 Adam-it

Fully agree that this kind of command should return all the members including the associated groups.

Good point @Adam-it, if we do apply this change, the command would be much clearer as entra group member list instead of user.

Jwaegebaert avatar Jul 09 '24 11:07 Jwaegebaert

In that case, shouldn't we move all group user commands to group member?

milanholemans avatar Jul 10 '24 15:07 milanholemans

In that case, shouldn't we move all group user commands to group member?

@milanholemans but that would mean we need to add support for subgroups in the other commands as well right?

Adam-it avatar Jul 15 '24 00:07 Adam-it

Yes, that would mean we have to add support for this to entra group user set and entra group user add. entra group user remove is still an open PR of me, so I can easily adapt that one.

milanholemans avatar Jul 16 '24 08:07 milanholemans

Yes, that would mean we have to add support for this to entra group user set and entra group user add. entra group user remove is still an open PR of me, so I can easily adapt that one.

yes lets do that. So to sum up:

  • we will deprecate (and in v9 if possible remove) the entra group user.... commands in favor of entra group member... commands
  • we will add support for subgroups to those commands as well

am I right? if so lets update the initial message of this post to align naming and open it up and create a new epic issue to update the other commands as well @pnp/cli-for-microsoft-365-maintainers ?

Adam-it avatar Jul 23 '24 08:07 Adam-it

Looks good to me.

milanholemans avatar Jul 23 '24 08:07 milanholemans

Looks good to me.

@milanholemans do you want to take the lead on the related epic or should I do that? @dojcsakj lets update the naming in your initial issue request. Lets mention that the ... group user list should be marked as deprecated and we should move this logic to ...group member list command which will now support subgroups as well. After that lets open this up 👍

Adam-it avatar Jul 23 '24 08:07 Adam-it

If you have time, go ahead creating the issue 😊 If not, I can put it on my todo list.

milanholemans avatar Jul 23 '24 08:07 milanholemans

@milanholemans, do you still have space on your todo list?

Jwaegebaert avatar Jul 23 '24 09:07 Jwaegebaert

V10 issue has been made, will create other issues to enhance current commands (with group options) soon.

milanholemans avatar Sep 28 '24 22:09 milanholemans

In the last major release, we've updated entra group user list to entra group member list which will also return sub-groups now. I also just created #6480 to introduce support for adding sub groups. If that issue has been reviewed, I will update my PR #5844 for the remove command with support for sub-groups as well. I think, for now, we can close this issue. Thank you once again for pointing this out @dojcsakj!

milanholemans avatar Nov 08 '24 23:11 milanholemans