pnp / cli-microsoft365

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

Bug report: Can't remove `aadGroup` by displayname on the command `spo group member remove` #5227

Open nicodecleyre opened 1 year ago

nicodecleyre commented 1 year ago

Priority

(Low) Something is a little off

Description

When executing spo group member remove on a spo group where an aadgroup is member of with the aadGroupName option, we get an error stating that the aadgroup can't be found in the spo group. This is because the aadgroup gets te title 'aadGroupName members' where members is added to the groupName.

We should add an extra step where this command uses the util function aadGroup.getGroupIdByDisplayName when using the option aadGroupName. To find the aadgroup by it's unique id before removing.

While we're at it, let's also change the message that displays in the prompt so it says:

Steps to reproduce

execute on a spo group where the aad group is a member of:

m365 spo group member remove -u "https://contoso.sharepoint.com" --groupName "Contoso site Members" --aadGroupName "aad group displayname"

Expected results

The aadgroup should be removed successfully

Actual results

Error: The Azure AD group azure ad group name is not found in SharePoint group Contoso site Members

Diagnostics

No response

CLI for Microsoft 365 version

6.9.0

nodejs version

18.12.1

Operating system (environment)

Windows

Shell

PowerShell

cli doctor

No response

Additional Info

No response

milanholemans commented 1 year ago

Seems indeed that this is a pesky bug. Was able to repro it. Good find! The best approach is indeed to retrieve the AD group ID and then perform a match on the login name of the user. Just like we should do for the option aadGroupId.

Are you willing to work on it, or should we open it up for the community?

milanholemans commented 1 year ago

However, thinking of it, if we do this, it would be impossible to remove the AAD owner principal of the SP group. There's a Contoso Members and Contoso Owners principal.

nicodecleyre commented 1 year ago

However, thinking of it, if we do this, it would be impossible to remove the AAD owner principal of the SP group. There's a Contoso Members and Contoso Owners principal.

I don't really understand? Isn't an AAD O365 group always represented by it's name + "Members" in SP.. If I for instance want to grant permissions to a SPO object for an AAD O365 group by it's ID, it shows the group + "Members". Next to that I can't find a group in SPO where "Owner" is added to the group name for the AAD group?

For instance: image

Added by ID, it gives me this: image

Only finding the members group: image

milanholemans commented 1 year ago

Yes, correct. But it's also possible to target the group owner part. When you create a new team site, the group owners are automatically added to the SP owner group. It's not possible to target this owner group via the UI, but you can use APIs.

image

However it was probably never intended this way, I feel like there should be a way to remove this principal as well. Although it would still be possible via userId.

@pnp/cli-for-microsoft-365-maintainers any thoughts? Should we support removing O365 group owners?

waldekmastykarz commented 1 year ago

@milanholemans, isn't this a separate issue that what @nicodecleyre reported? If so, shall we take it up separately?

milanholemans commented 1 year ago

We can treat removing owners as an enhancement yes.

waldekmastykarz commented 1 year ago

Let's do that to avoid stalling this bug

waldekmastykarz commented 1 year ago

Hey @nicodecleyre, I can't repro it. For me, it's working just fine. Here's what I'm doing:

I've got a mail-enabled security group in Entra ID:

image

I created an SPO group, and added the Entra group to it:

image

Now if I remove the Entra group from the SPO group using the CLI:

m365 spo group member remove -u / --groupName "My group" --aadGroupName "Consent Request Approvers"

Everything works just fine:

image

What am I missing? Could it be, that by coincidence you've got an SPO and Entra group with the same name and added one SPO group into another rather than the Entra group?

milanholemans commented 1 year ago

Hi @waldekmastykarz, the issue only occurs with M365 groups. The point is that when you have a M365 group named Marketing for example, you cannot run spo group member remove ... --aadGroupName "Marketing". It only works if you run it like this: spo group member remove ... --aadGroupName "Marketing Members". I don't know if it was made with this intention.