Cli-microsoft365: New command: teams conversationmember list

[plamber]

Usage

teams conversationmember list [options]

Description

List all conversational members of a channel.

Options

| Option | Description |
| ----------------------- | ----------------------------------------- |
| --teamId <teamId> | The Team's ID |
| --teamName <teamName> | The DisplayName of the Team |
| --channelId <channelId> | The channel's ID |
| --channelName <channelName> | The Channel's name |

Additional Information

https://docs.microsoft.com/en-us/graph/api/team-list-members?view=graph-rest-1.0&tabs=http

I can pick this up.

[waldekmastykarz]

Looking at the spec, let's allow referring to team and channel by their name as well to make it easier to use.

You've got 4 issues assigned to you. Shall we let others help? If you're done with the other 4 and no one has picked up this one, you can still do it 🙂

[sebastienlevert]

Actually this one is really interesting! If you are already @plamber on other things, I'm willing to take on this one! Should this also support "simply" listing the member of a team if --channelId is not specified? Both options are available on the endpoint. It starts to be conflicting with other commands though... Should we look at realigning all these commands to host them under "conversationMembers"? Have a look here : https://pnp.github.io/cli-microsoft365/cmd/aad/o365group/o365group-user-list/

[plamber]

@sebastienlevert: you can take this up. Let us define the specifications before.

1879 #1878 are somewhat related and should follow the same logic as here.

I worked with this endpoint in the last weeks a lot and there are some important aspects to consider.

• It works for private channels only, in all other situations this command is going to fail
• It is Teams only (merging it with the https://pnp.github.io/cli-microsoft365/cmd/aad/o365group/o365group-user-list/ feels messy to me).
• Adding and updating is performed with one command #1878. This command accepts the AAD id of the user. Removing the command #1879 was accepting only the conversationmemberid, which you get if you retrieve all conversationmembers before

@waldekmastykarz suggested to use the channelname as additional parameter together with the channelid. Since channelnames are unique, this is indeed a good point. It will however cost additional calls to graph to get all channels and perform the filtering.

On the other hand, I would not provide a filtering by Teamname (Displayname). This is not unique and might provide some undesired behavior. If we want to provide an additional option we could think of having the mailnickname option as filter. This would be unique but I do not believe this will be the standard case of handling things in scripts.

With all these points it feels right to me to have it separated. @sebastienlevert and @waldekmastykarz: what do you think about this?

[waldekmastykarz]

I've updated the command name to teams conversationmember list (singular) to match our naming convention.

Good points @plamber 👍

Since channelnames are unique, this is indeed a good point. It will however cost additional calls to graph to get all channels and perform the filtering.

This is true and a tradeoff for the bit of convenience. Otherwise, you'd have the same hit when having to lookup the ID yourself, not to mention having to run two commands and copy-paste the ID

On the other hand, I would not provide a filtering by Teamname (Displayname). This is not unique and might provide some undesired behavior.

We have this implemented in other teams commands, where if we find multiple results, we prompt the user to disambiguate them using an ID. Not sure how common it is, but again, it's a convenience that we should consider offering for non-automated scenarios.

[plamber]

@sebastienlevert and @waldekmastykarz: what about the updated command definition? Is this fine for you?

[sebastienlevert]

What is the --owner parameter? I would not expect a list command to alter any of the data and here it seems like it. Copy / Paste issue?

[plamber]

@sebastienlevert good catch. Cleared it up

[sebastienlevert]

Makes sense to me! Let me know if there is any "guidance" you want to provide to utilize the endpoint or if I can start with it!

[plamber]

You can start with it from my point of view. I am doing the review afterwards

[sebastienlevert]

@waldekmastykarz I'm realizing that we will need a new scope in the AAD app, especially for listing members. The following permissions are documented on the Graph : Chat.ReadBasic, Chat.Read, Chat.ReadWrite (https://docs.microsoft.com/en-us/graph/api/conversationmember-list?view=graph-rest-beta&tabs=http). Interestingly enough, this won't be required for the add / remove as it relies on the Group.ReadWrite.All (https://docs.microsoft.com/en-us/graph/api/conversationmember-add?view=graph-rest-beta&tabs=http).

I believe Chat.Read should be enough, in our case. For now, I would be "stuck" behind this before I move on with any further development. I'm already done with a majority of the implementation, but I want to make sure it works with the PnP Management Shell app and not only with a personal AAD app.

[plamber]

Hi,
we already have Group.ReadWrite.All. This should suffice for most operations. I already opened the issue asking why the permissions are so messed up compared to the beta endpoint. If we want to have this working temporarily Chat.Read might be required. I hope that it is getting fixed in production soon.

Here the issue reference.

br,

[sebastienlevert]

The list endpoint is referencing very different scopes. I get a 403 with the currently consented scopes, so I feel this is a required scope. https://docs.microsoft.com/en-us/graph/api/conversationmember-list?view=graph-rest-1.0&tabs=http#permissions

Especially the list seems to only allow GET on the /beta endpoint. It's not rolled out yet in the v1.0 versions, at least, according to my tenant... In the Graph Explorer, you can see the Permissions tab and see it really required those Chat.* scopes...

V1.0

https://developer.microsoft.com/en-us/graph/graph-explorer?request=chats%2F19%3A586a8b9e36c4479bbbd378e439a96df2%40thread.skype%2Fmembers&method=GET&version=v1.0&GraphUrl=https://graph.microsoft.com

Beta

https://developer.microsoft.com/en-us/graph/graph-explorer?request=chats%2F19%3A586a8b9e36c4479bbbd378e439a96df2%40thread.skype%2Fmembers&method=GET&version=beta&GraphUrl=https://graph.microsoft.com

[waldekmastykarz]

The way we handled that in the past, is that during dev we used a custom AAD app and when we had a PR open, we extended the scopes on the PnP Management Shell app so that when the PR is merged all is up & running.

[sebastienlevert]

I was wondering, and this might be a way bigger topic than it is... But...

Let's say, in this command, that we want to validate the teamId + channelId. Not only that they are valid GUIDs, but that they are actual team IDs and/or channel IDs... Should this be part of the command of we should expect "valid" and "existing" ids? It would make the command a lot more robust, but with a ton more code...

So far, in the CLI, it seems like a lot of that validity is "assumed" and it's definitely OK. Just wondering if we should, or not ;)

[sebastienlevert]

@plamber : Been poking around and indeeed, this endpoint is having some interesting behaviors! I'm actually looking at this one and feel it would be the right one to hit : https://docs.microsoft.com/en-us/graph/api/conversationmember-add?view=graph-rest-beta&tabs=http.

From a list perspective, running it on the same end point with a GET gets me the members of the private channel, but only in beta. Are we on the same page with the endpoint to leverage?

GET /teams/{id}/channels/{id}/members

[plamber]

The enpoint works only for private channels. In v1.0 it requires different permissions as in beta.

The engineers in ms stated they are investigating. Not sure how long it will take.

My suggestion is to work with the v1.0 enpoint and updated permissions. Consider only the private channel usecase. All other channels in theory throw an exception

[waldekmastykarz]

Let's say, in this command, that we want to validate the teamId + channelId. Not only that they are valid GUIDs, but that they are actual team IDs and/or channel IDs... Should this be part of the command of we should expect "valid" and "existing" ids? It would make the command a lot more robust, but with a ton more code...

They way the CLI is built at the moment is that we have a separate place to validate arguments, but at that stage, we don't have authentication initialized yet, so we can't execute any requests towards M365. So how we're handling it instead is, that we validate the input's structure but let M365 handle the validity of the actual resources.

[sebastienlevert]

So more findings today with this interesting endpoint!

@plamber I don't know if you've been through this already, but it's definitely interesting...

It works great with Private Channels and I can get all of the conversation members (well... The API does not support paging for now. So this will be definitely documented as we might be missing "some" users). All of this is based on the following call :

https://graph.microsoft.com/beta/teams/47d6625d-a540-4b59-a4ab-19b787e40593/channels/19:[email protected]/members

The interesting thing, is that if we pass a "somewhat" valid ID that would not exist, it returns the members of the team instead :

https://graph.microsoft.com/beta/teams/47d6625d-a540-4b59-a4ab-19b787e40593/channels/19:[email protected]/members

Should we "catch" these (we already have a better validation on the channelId but if you change a simple character, it would still return the team members, and not the non-existing channel members) or should we simply let it be, like it is right now? It would add a validation call on the existence of the provided channelId and not only "assume" that the ID is valid.

Also, as this seems to be considered "members" in the Graph, should we rename the command? And maybe even make it part of the channel part? m365 teams channel members list. It feels this would be more user friendly for users to understand what it does.

[plamber]

HI @sebastienlevert,
I think we shouldn't fix the API if it does not behave as it should but provide the feedback directly to the Graph team. Lets work on the minimum viable solution and then list down the issues we identified.

Afterwards, I am getting in touch with some colleagues and will report the problem. I could imagine that they are happy to get the feedback.

br,
Patrick

[sebastienlevert]

Alright, makes a lot of sense. PR incoming tomorrow as it's ready!

[waldekmastykarz]

@sebastienlevert @plamber I think there is something to say for the somewhat odd API behavior and us trying to help users work with it more reliably. I'd suggest we follow both of your suggestions: let's absolutely report it back to the Graph team, but also, let's check if the specified ID is valid. There is nothing worse than thinking you have the right data while it's not the case.

[sebastienlevert]

Totally agree and I just implemented a validation based on the id. If /channels?$filter=id eq '19:[email protected]' returns nothing, we consider it invalid or not existing.

[waldekmastykarz]

Awesome @sebastienlevert 👍