pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
920 stars 326 forks source link

Enhancement: list joined groups of user #5838

Open milanholemans opened 8 months ago

milanholemans commented 8 months ago

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

MathijsVerbeeck commented 8 months ago

Can I work on this?

martinlingstuyl commented 7 months ago

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 martin@contoso.com 

Returns all groups the user is a direct member of

m365 entra group list --userName martin@contoso.com --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..

milanholemans commented 7 months ago

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.

martinlingstuyl commented 7 months ago

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

milanholemans commented 7 months ago

"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 😊

martinlingstuyl commented 7 months ago

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)

waldekmastykarz commented 7 months ago

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.

martinlingstuyl commented 7 months ago

@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 martin@contoso.com 

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

m365 entra group list --userName martin@contoso.com --transitiveMemberOf

Is that what you mean?

milanholemans commented 7 months ago

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

martinlingstuyl commented 7 months ago

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

milanholemans commented 7 months ago

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.

martinlingstuyl commented 7 months ago

@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 martin@contoso.com 

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

m365 entra group list --userName martin@contoso.com --transitiveMemberOf

Is that what you mean?

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

milanholemans commented 7 months ago

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

martinlingstuyl commented 7 months ago

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

waldekmastykarz commented 7 months ago

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.

martinlingstuyl commented 7 months ago

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.

Adam-it commented 7 months ago

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 πŸ€”

waldekmastykarz commented 7 months ago

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

martinlingstuyl commented 7 months ago

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 commented 6 months ago

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?

waldekmastykarz commented 6 months ago

How about a shorter --withNestedMemberships?

martinlingstuyl commented 6 months ago

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

arjunumenon commented 6 months ago

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

martinlingstuyl commented 6 months ago

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 πŸ˜†

MathijsVerbeeck commented 6 months ago

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.

milanholemans commented 6 months ago

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

martinlingstuyl commented 5 months ago

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 ?

milanholemans commented 5 months ago

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