Set-PnPGroupPermissions Case Sensitivity Inconsistency

[narbehm]

Set-PnPGroupPermissions -Ideneity "Group Name" <- "Group Name" is expected to be case sensitive. However,

Get-PnPGroup -Identity "Group Name" <- "Group Name" is not expected to be case sensitive.

The issue comes about when you use Get-PnPGroup to check to see if a group exists before you use Set-PnPGroupPermissions to set its permissions. Since the later is case sensitive, it could fail even though logically the group exists.

[martinlingstuyl]

Hi @veronicageek,
I'd like to pick this up if I may. It would be my first contribution to this repo and this looks like a simple issue to fix.

[gautamdsheth]

Hi @martinlingstuyl , welcome and please go ahead. Let me know if you need help, happy to do so :)

[martinlingstuyl]

Thanks!
I do need some help though. Can it be true that the contribution docs are a bit behind? :-)
It's talking about a post build script that should be on the VS solution and copy succesfully build files to the powershell folder. But I can't seem to find it. Succesful builds don't copy anything.

👇
https://github.com/pnp/powershell/blob/dev/CONTRIBUTING.md

also: I think cloning the PnP-Sites-Core is no longer necessary anymore. Am I correct?

[martinlingstuyl]

Never mind. I found the script in /build/build-debug.ps1 👍 succesfully debugging now!

[martinlingstuyl]

Okay @gautamdsheth, this is the story: one commandlet uses a CSOM function, the other uses PnP Core SDK. The Core SDK uses an OData call and $filter only supports case senstive. Hence the difference.

I'm not sure what the direction is this library is taking. Is it meant to be CSOM side by side with PnP Core SDK whichever one fits the requirements? Or is CSOM meant to be gradually replaced? What are the reasons why CSOM is sometimes picked over Core SDK for commandlets?

Hope to hear from you on this. My idea would be to change Get-PnPGroup to use PnP Core SDK as well. In that case everything becomes case-sensitive and is using the modern SDK.

[narbehm]

I’d like to chime in here. Are group names case sensitive in Sharepoint Online? If not then the PS commands should not either for consistency sake. Having one be case sensitive and the other not is probably worse than the current case. My assumption is since the GET command is case insensitive then they are also case insensitive for SPO

…

On Mar 31, 2022, at 12:48 PM, Martin Lingstuyl ***@***.***> wrote:  Okay @gautamdsheth, this is the story: one commandlet uses a CSOM function, the other uses PnP Core SDK. The Core SDK uses an OData call and $filter only supports case senstive. Hence the difference. I'm not sure what the direction is this library is taking. Is it meant to be CSOM side by side with PnP Core SDK whichever one fits the requirements? Or is CSOM meant to be gradually replaced? What are the reasons why CSOM is sometimes picked over Core SDK for commandlets? Hope to hear from you on this. My idea would be to change Get-PnPGroup to use PnP Core SDK as well. In that case everything becomes case-sensitive and is using the modern SDK. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[martinlingstuyl]

That's a good point @narbehm, SharePoint group names are case insensitive. But try to retrieve them using the REST Api (/_api/web/sitegroups?$filter=Title eq 'groupname') and you're bound to case sensitivity. So it seems to be a SharePoint thing.

[gautamdsheth]

@martinlingstuyl - thanks for investigating this, much appreciated !

I like the idea of replacing CSOM with PnP Core SDK, but that will be a breaking change and we cant do that unfortunately, at least not right now for 1.x versions. So, for now will have to keep using CSOM and PnP Core SDK side by side.

For 2.0 version, no ETA , we plan to replace CSOM with PnP Core SDK or plain HTTP REST requests where possible and keep CSOM for where we don't have a supported endpoint. So, yes we do plan to replace CSOM gradually where possible eventually.

There is no logic right now of picking CSOM over PnP Core, it is upto the developer who creates the commands.

@narbehm - for your particular issue, maybe you can use the Id of the group itself ? I believe Get-PnPGroup will get you the Id and then use that in your Set-PnPGroupPermissions command ?

[martinlingstuyl]

Thanks for the explanation @gautamdsheth!

If the general direction is PnP Core SDK and HTTP REST anyway, we might eventually run into this casing-issue for all commands that filter something by name or title... I think. But well, as we cannot really fix the SharePoint REST API... 😁

We could at least ensure that both commands do the same thing, get the group using Pnp Core SDK. What I could do is update Get-PnPGroup to use the Core SDK when using the Name parameter. That's no breaking change I guess (or is it? And Why), but I would need to ensure that the same group properties are loaded.

Is that a good course of action?

[gautamdsheth]

@martinlingstuyl - It will be a breaking change. PnP Core SDK returns its own objects and not the CSOM ones which are being returned currently. So, current scripts which rely on CSOM returned objects will break.

Example:

$group = Get-PnPGroup -Identity "Some group"

$group.Users  --> this will break due if we change to use PnP Core , because we might need to explicitly load it
$group.Context  --> this will change because, PnP Core usually doesnt return the context. Even if it does, it will be PnPContext and not ClientContext

[martinlingstuyl]

I see! In that case there's nothing to be done for now I guess.

The only alternative would be to query for all groups and filter clientside. Which is not exactly ideal in other respects. Although the amount of sitegroups is probably always more or less limited. If we $select only the title the amount of data crossing the wire is rather small...after which we can get the details of the one with a matching name.

I'm not sure what you would think about such a solution. But I agree with @narbehm that this case sensitivity is rather unwanted.

[gautamdsheth]

@martinlingstuyl , @narbehm : Sorry for responding a bit late, have been doing some thinking around this and I think I may have found a way.

How about we change the Get-PnPGroup cmdlet to somethin like below:

var pnpGroup = Identity.GetGroup(PnPContext);
if(pnpGroup.Id != -1)
{
	var csomGroup = CurrentWeb.SiteGroups.GetById(pnpGroup.Id);
	ClientContext.Load(csomGroup);
	ClientContext.Load(csomGroup.Users);
	ClientContext.ExecuteQueryRetry();

	WriteObject(csomGroup);
}
else
{
	WriteWarning("Site group not found");
}

In this way, I think it would be a bit more accurate.
What we are doing is essentially using PnPCoreSDK to retrieve the Group. PnP Core returns the group Id.
Using this Id, we use CSOM to get the Site group by its Id and send it to the user.
This way, there are no breaking changes and it will be a bit more accurate. Yes, there will 2 requests, but I think we will be ok with that as it will make the cmdlet more reliable.

Let me know what you think about this approach @martinlingstuyl :)

[martinlingstuyl]

hi @gautamdsheth
I agree that would make things more consistent. I would be happy to implement it.
It still might break some scripts though, as the Get-PnPGroup script will change from being case insensitive to case sensitive.

[narbehm]

Here’s an idea. With Set-PnPGroupOermissions, Remove the ability of using the group name as the identity. This will force the use of the group Id instead.

…

On Apr 26, 2022, at 4:33 AM, Martin Lingstuyl ***@***.***> wrote:  hi @gautamdsheth I agree that would make things more consistent. I would be happy to implement it. It still might break some scripts though, as the Get-PnPGroup script will change from being case insensitive to case sensitive. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[gautamdsheth]

hi @martinlingstuyl , go for it !!
We can mention it in the docs, Get-PnPGroup.md about this , agree it is a breaking one, a smaller one, but eventually our code will be better and consistent after this !

@narbehm - no can't do that, but we will fix the Get-PnPGroup cmdlet for a consistent behavior

[martinlingstuyl]

Ok @gautamdsheth, @narbehm , so I fixed it and created a pull request. This is my first pull request in this repo. I hope it meets the requirements!

[gautamdsheth]

@narbehm am closing this issue since @martinlingstuyl fixed it. Now both Get-PnPGroup and Set-PnPGroupPermissions will behave in a more consistent way.

The fix will be available in tomorrow's nightly and later on in a major release whenever that happens.
Thanks for raising the issue and thanks @martinlingstuyl for fixing it :)