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

"entra group user" renaming and enchancement #6088

Closed
3 tasks
dojcsakj opened this issue Jun 10, 2024 · 13 comments
Closed
3 tasks

"entra group user" renaming and enchancement #6088

dojcsakj opened this issue Jun 10, 2024 · 13 comments
Assignees
Labels
enhancement needs peer review Needs second pair of eyes to review the spec or PR

Comments

@dojcsakj
Copy link
Contributor

dojcsakj commented Jun 10, 2024

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, ...)
@milanholemans milanholemans added enhancement needs peer review Needs second pair of eyes to review the spec or PR labels Jun 13, 2024
@milanholemans
Copy link
Contributor

@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.

@Adam-it
Copy link
Member

Adam-it commented Jul 5, 2024

@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

@Jwaegebaert
Copy link
Contributor

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.

@milanholemans
Copy link
Contributor

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

@Adam-it
Copy link
Member

Adam-it commented Jul 15, 2024

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?

@milanholemans
Copy link
Contributor

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.

@Adam-it
Copy link
Member

Adam-it commented Jul 23, 2024

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 ?

@milanholemans
Copy link
Contributor

Looks good to me.

@Adam-it
Copy link
Member

Adam-it commented Jul 23, 2024

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 👍

@milanholemans
Copy link
Contributor

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

@Jwaegebaert
Copy link
Contributor

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

@dojcsakj dojcsakj changed the title entra group user list / list nested groups "entra group user" renaming and enchancement Jul 29, 2024
@milanholemans milanholemans self-assigned this Sep 7, 2024
@milanholemans
Copy link
Contributor

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

@milanholemans
Copy link
Contributor

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 milanholemans closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs peer review Needs second pair of eyes to review the spec or PR
Projects
None yet
Development

No branches or pull requests

4 participants