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

Enhancement: list joined groups of user

Open milanholemans opened this issue 1 year ago β€’ 28 comments

Just like teams team list, we should update entra group list with options joined and associated.

Options

Option Description
--withNestedMemberships Include groups that the user is an indirect member of, through a nested group membership.
--userId [userId] The ID of the user. Specify either userId or userName but not both.
--userName [userName] The user principal name of the user. Specify either userId or userName but not both.

API

Joined groups: https://learn.microsoft.com/en-us/graph/api/user-list-memberof?view=graph-rest-1.0&tabs=http Associated groups: https://learn.microsoft.com/en-us/graph/api/user-list-transitivememberof?view=graph-rest-1.0&tabs=http

milanholemans avatar Feb 11 '24 16:02 milanholemans

Can I work on this?

MathijsVerbeeck avatar Feb 16 '24 15:02 MathijsVerbeeck

I'm only seeing this now. I hate to break it open now, but I must say I find the new option names confusing, πŸ˜… as we are talking about membership of groups. "Joined" communicates an action on the part of the user, while he may have been added by someone else. Associated is not at all clear as well.

Also, when using the userName/userId options, we can assume the command should return groups the user is a member of, so we could remove either the joined or associated flag, and default to returning the groups for that user and use just one flag to adapt the behavior.

I'd say we could default to return ALL groups the user is a member of (either transitive or direct) and use a flag to only return the groups he is a direct member of.

examples

Returns all groups the user is a member of (directly or indirectly)

m365 entra group list --userName [email protected] 

Returns all groups the user is a direct member of

m365 entra group list --userName [email protected] --directMemberOf

The flag name is just an idea, I'm not yet content in that one.

What do you guys think @pnp/cli-for-microsoft-365-maintainers, @milanholemans ?

We apparently added these options on the teams team list command already. That may be a slightly different case..

martinlingstuyl avatar Apr 01 '24 14:04 martinlingstuyl

Hi @martinlingstuyl, yes I wanted to align it somewhat with teams team list.

"Joined" communicates an action on the part of the user, while he may have been added by someone else.

I don't quite get the issue here. Why would joined mean that the user joined the group himself and not by someone else? Joined just means that you are part of a group, isn't it?

If we output all groups, direct and indirect groups, that would make it really inconsistent with teams team list. Also, won't this be a breaking change since the amount of returned groups will suddenly change for people.

milanholemans avatar Apr 01 '24 14:04 milanholemans

"Joined" and "associated" just aren't words that I typically associate with group membership, I find them unclear in their meaning. The only place where the word "join" can be found in the context of groups as I remember it, is when you create a SharePoint group, one of the settings sounds like "allow people to join the group". I'm not sure if there are other places where join is used, but it's typically (in my mind) a verb in the connotation of an activity someone undertakes. You don't understand what I mean?

What would make sense to an Entra ID admin, that would be more interesting. Direct and indirect membership maybe?

(I think the Microsoft graph team struggled to find words for this as well, as they came up with the horrible wording of 'transitive members'.)

As for teams team list, I find it unclear there as well, so lets discuss what would be the optimal way to word it here, before we align it out of hand.

If we output all groups, direct and indirect groups, that would make it really inconsistent with teams team list. Also, won't this be a breaking change since the amount of returned groups will suddenly change for people.

As I said: consistency with teams team list is not my main object here. It seems that's a totally different command, and might need its own discussion on naming of these options.

If we run m365 entra group list without the user options, the command returns all groups, right? The user options are new in these specs, so why are you talking about a breaking change here? It seems logical to me to list all groups the selected user is a member of, whether that be directly or transitive. Two flags is just superfluous in my mind.

martinlingstuyl avatar Apr 01 '24 15:04 martinlingstuyl

"Joined" and "associated" just aren't words that I typically associate with group membership, I find them unclear in their meaning. The only place where the word "join" can be found in the context of groups as I remember it, is when you create a SharePoint group, one of the settings sounds like "allow people to join the group". I'm not sure if there are other places where join is used, but it's typically (in my mind) a verb in the connotation of an activity someone undertakes. You don't understand what I mean?

Joined is clear to me. Associated is quite clear to me, but that might be because I've used it quite frequently. Anyway, I have nothing against these words. What is your suggestion then?

If we run m365 entra group list without the user options, the command returns all groups, right? The user options are new in these specs, so why are you talking about a breaking change here? It seems logical to me to list all groups the selected user is a member of, whether that be directly or transitive. Two flags is just superfluous in my mind.

I was a bit too fast there, confused it with another command, sorry 😊

milanholemans avatar Apr 01 '24 15:04 milanholemans

Joined is clear to me. Associated is quite clear to me

Am I the only one who finds these words confusing in the context of group membership @pnp/cli-for-microsoft-365-maintainers?

Aside from the words though, I don't think we need two flags for this. Do you agree on that @milanholemans ?

It would make more sense to default to one behavior and use one flag to switch it, or maybe even use a type option (--membershipType direct VS --membershipType indirect/transitive/nested or whatever)

martinlingstuyl avatar Apr 01 '24 16:04 martinlingstuyl

Am I the only one who finds these words confusing in the context of group membership @pnp/cli-for-microsoft-365-maintainers?

Agreed: associated is unclear, especially for a non-native speaker. I suggest that we follow the API which uses the word 'transitive'. I like @martinlingstuyl's suggestion to use a flag, which is also clearer because it offers a single choice where only one option is allowed.

waldekmastykarz avatar Apr 01 '24 19:04 waldekmastykarz

@waldekmastykarz, --joined and --associated are both flags as well, in the current specs. But you mean you agree with me to move to just one flag? And such in the following manner?

Returns all groups the user is a direct member of

m365 entra group list --userName [email protected] 

Returns all groups the user is a direct or transitive member of

m365 entra group list --userName [email protected] --transitiveMemberOf

Is that what you mean?

martinlingstuyl avatar Apr 01 '24 19:04 martinlingstuyl

If you would like to use one flag. How can the user get:

  • All groups (direct membership + indirect membership)
  • Only direct membership
  • Only indirect membership

milanholemans avatar Apr 01 '24 20:04 milanholemans

Why would you want to retrieve only indirect membership? I believe the only really useful scenarios are: direct membership and all membership (including transitive)

The transitive endpoint returns both at the same time I believe...

martinlingstuyl avatar Apr 01 '24 20:04 martinlingstuyl

You never know if it might be useful for someone to list only indirect membership. But seems like you are right, the endpoint returns both (if I read the docs correctly.

milanholemans avatar Apr 01 '24 21:04 milanholemans

@waldekmastykarz, --joined and --associated are both flags as well, in the current specs. But you mean you agree with me to move to just one flag? And such in the following manner?

Returns all groups the user is a direct member of

m365 entra group list --userName [email protected] 

Returns all groups the user is a direct or transitive member of

m365 entra group list --userName [email protected] --transitiveMemberOf

Is that what you mean?

Do you guys agree on this new spec @pnp/cli-for-microsoft-365-maintainers?

martinlingstuyl avatar Apr 02 '24 19:04 martinlingstuyl

To be honest, I don't find transitiveMemberOf much clearer. I can live with --indirectMembership or something like that.

milanholemans avatar Apr 02 '24 20:04 milanholemans

I find that clearer as well, less API'ey. What about the rest @waldekmastykarz, @MathijsVerbeeck, @Jwaegebaert, @garrytrinder, @arjunumenon, @Adam-it, @appieschot?

martinlingstuyl avatar Apr 03 '24 16:04 martinlingstuyl

To be honest, I don't find transitiveMemberOf much clearer. I can live with --indirectMembership or something like that.

Will people who use this command reasonably know what indirect membership means? Have we looked at docs (not only API docs but also feature or support docs) for this feature to see what words they use? I suggest we align with what's commonly used and avoid introducing new vocabulary.

waldekmastykarz avatar Apr 04 '24 10:04 waldekmastykarz

True, What I sometimes see is the concept of "nested groups". Indirect membership means that you are part of a group that is nested in another group.

I also occasionally see the term "indirect group membership". But searching for it does not return much.

Both ideas are reasonably clear I guess. At least more clear than the term "transitive", which is strictly used with the Graph.

martinlingstuyl avatar Apr 04 '24 21:04 martinlingstuyl

sorry for being late. ok so to wrap my head around this we may use:

  1. transitiveMemberOf
  2. indirectMembership
  3. or use two flags instead of one which is joined and associated
  4. or we continue the discussion with finding a better name for it right?

so my opinion on each of those would be:

  1. πŸ‘Ž
  2. πŸ‘
  3. πŸ‘ - although it is bit confusing I agree, I like that it gives most possibilities and is aligned with m365 teams team list. We may always give more clarity and care to explain it properly in help/docs
  4. πŸ‘Žsince the implementation already started I would rather we make a decision on this ASAP not to give hard time for the contributor ❀️

@martinlingstuyl, @milanholemans am I missing something here πŸ€”

Adam-it avatar Apr 05 '24 22:04 Adam-it

I suggest that we try to align with existing feature docs and avoid introducing new terms that only further increase confusion.

waldekmastykarz avatar Apr 08 '24 17:04 waldekmastykarz

image

https://learn.microsoft.com/en-us/entra/fundamentals/how-to-manage-groups#add-or-remove-a-group-from-another-group

How about includeNestedGroupMembership?

martinlingstuyl avatar Apr 09 '24 17:04 martinlingstuyl

Ok guys,

We need to reach a conclusion here... The docs seem to talk about this practice as "nesting groups within groups"

So retrying:

By default the command lists groups where you are a direct member of.

Using a flag you can include nested group memberships

Option Description
-n, --includeNestedGroupMemberships Show all groups the user is a member of, including those where the user is a member of a nested group.

All in favor?

martinlingstuyl avatar Apr 21 '24 06:04 martinlingstuyl

How about a shorter --withNestedMemberships?

waldekmastykarz avatar Apr 21 '24 11:04 waldekmastykarz

Yeah, that's better... all in favor @pnp/cli-for-microsoft-365-maintainers ?

martinlingstuyl avatar Apr 21 '24 19:04 martinlingstuyl

Pardon for pithing with my opinion later. I think it makes --withNestedMemberships makes more sense and readable than transitiveMemberOf which is the Graph API convention. I would vote for --withNestedMemberships

arjunumenon avatar Apr 24 '24 04:04 arjunumenon

Hi @MathijsVerbeeck, after some discussion, we've decided to update the specs a bit. Could you update your pull request to align with this? In short: --joined and --associated are replaced by a single --withNestedMemberships flag.

Thanks in advance, and sorry for letting you wait this long πŸ˜†

martinlingstuyl avatar Apr 24 '24 20:04 martinlingstuyl

Hello everyone, I was reworking the command after all the feedback, and thinking of it now, for me it makes little sense that we add this to the existing command m365 entra group list, from which the main purpose is retrieving all the groups from Microsoft Entra ID.

If we want to list the groups specifically for a user, or for the currently signed in user, wouldn't it be better to create a new comand, named m365 entra user group list, and provide the option --withNestedMemberships in this command?

The reason being - If I want to implement the command as discussed here, and we would also like to retrieve the nested member ships of the currently signed in user, we would have to completely change the endpoint that we are calling, as we currently simply do

https://graph.microsoft.com/v1.0/groups

which simply lists all the groups --> makes sense, as this is the purpose of the command, but we would rework this so that it would become

https://graph.microsoft.com/v1.0/me/groups

Which would only list the groups that the currently signed in user is a member of. Which is wrong.

I know that I hopped in quite late in the conversation, but I feel like what I'm doing right now is wrong, as this changes the complete functionallity from the current command, and a new command would be better in my honest opinion.

MathijsVerbeeck avatar May 03 '24 21:05 MathijsVerbeeck

We also have to check that we don't create something overlapping with #5904

milanholemans avatar May 06 '24 20:05 milanholemans

We also have to check that we don't create something overlapping with #5904

This is actually a very good question @milanholemans, I had not seen that issue and I'm thinking it actually does overlap. We're actually building the same thing in two places at once now.

Should we take our losses and just cancel working on this one? What's your perspective @pnp/cli-for-microsoft-365-maintainers ?

martinlingstuyl avatar May 21 '24 20:05 martinlingstuyl

I'm wondering if that command can list nested groups and what the differences are between the two APIs.

milanholemans avatar May 21 '24 23:05 milanholemans