Yvand / EntraCP

EntraCP (formerly AzureCP) is a claims provider that connects SharePoint to your Microsoft Entra ID tenant, in federated authentication
https://entracp.yvand.net/
Apache License 2.0
39 stars 8 forks source link

Show only members of Group #42

Closed chrisbrownie closed 7 months ago

chrisbrownie commented 6 years ago

Is there any way to limit the people picker to show only members of a particular Azure AD group?

We have a scenario where a SharePoint farm is deployed for a subset of users within an Active Directory forest. Users are granted access by membership of an Azure AD group, and we'd like to restrict the people picker to show only members of that group.

Yvand commented 5 years ago

Hi @chrisbrownie this is not possible currently, but I can add this to todo list for a future version

chrisbrownie commented 5 years ago

Hi @Yvand thanks! We've progressed with the full directory scoping, but very keen to see this implemented if and when you have time.

Thanks, Chris

Yvand commented 5 years ago

Hi @chrisbrownie this feature is actually more complex to develop than I thought: As you can see here, Microsoft Graph does not allow to create a filter on user and group in 1 query, so I cannot make a query like this: /users?$filter=(startswith(UserPrincipalName, 'aad') and memberOf eq 'GroupID')

So if I want to implement this feature, I need to completely change the logic, to get alll the members of the restricted group defined, and then for each user returned, check if he matches the input in the people picker.

Given the complexity, unless I get more feedback that this would be a very needed feature, I'm not going to implement it, sorry.

chrisbrownie commented 5 years ago

Hi @Yvand no worries! Thank you for taking the time to investigate it.

cblomart commented 5 years ago

This is actually a use case that would be relevant for us too.

I did some work on group membership check earlier but it effectively requires an extra call to graph api (https://github.com/cblomart/AADIsMemberOf).

Yvand commented 5 years ago

@cblomart did you check by any chance if this was improved in beta endpoint v2?

cblomart commented 5 years ago

what do you mean by v2... for graph i know of "v1.0" or "beta"... the tests i made were on v1.0 if not mistaken... will check on v2 but i think this is still calling ismemberof separately.

cblomart commented 5 years ago

limited to 20 groups if my memory serves me well...

Yvand commented 5 years ago

Yes I was talking about the beta endpoint, I wonder if it makes this operations easier (possible with a single query)

cblomart commented 5 years ago

Allways an extra call...

If there is a list of groups to check, it looks like checkMamberGroups may be a good idea.

If all the groups the user is member of are of intrest transitiveMemberOf may be used (could be huge).

I need to plunge back in the code, but i am wondering how claims augmentation is done as it might require the last informations. for group claims.

cblomart commented 5 years ago

If i read well the request made to graph api is a request on the user endpoint with some odata filtering to get only the intresting object (1).

Assuming group membership whitelisting in this case: Maybe a way to do this would be to list members of whitelisted groups via transitiveMembers.

The first conclusion would be that application logic would then be different whether or not groups are given.

I think this could be solved by modifiying the object on which the user/group list are done. both the user endpoint and the group//transitiveMembers supports odata filtering.

Naturally one returns Users and the other directoryObjects: let's see the bright side, this may be a way to get users and groups in one request and use the directoryObject enpoint when no groups are provided.

1*: seems like membership filering is done after the request now. i will look if this was always so and eventually why it changed.

vargasfe commented 1 year ago

Hello Yvan, Was the ability to limit the search to a specific group/groups ever implemented? I am still on AzureCP v20.x Maybe is it possible to restrict the AppRegistration in Azure to return only memebers of a group?

Yvand commented 1 year ago

@vargasfe, no, but let me reopen this issue and investigate if this can be achieved. To be sure I understand the request, your expectation is that it returns all the groups, and only users members of the specified group(s)?

vargasfe commented 1 year ago

Yvan,

My specific use case is to define a tenant in AzureCP configuration that reads members of a specific group e.g. “Finance Members” or “HR Managers”. (https://graph.microsoft.com/v1.0/groups/{ https://graph.microsoft.com/v1.0/groups/%7bid%7d/members id}/members)

Could help though I understand this method does not support filtering which is what People-Picker needs.

If multiple groups are needed, either allow multiple group ids, or define multiple tenants using the same App Registration but a different single group each.

From: Yvan Duhamel @.> Sent: Monday, October 16, 2023 11:30 AM To: Yvand/EntraCP @.> Cc: vargasfe @.>; Mention @.> Subject: Re: [Yvand/EntraCP] Show only members of Group (#42)

@vargasfe https://github.com/vargasfe , no, but let me reopen this issue and investigate if this can be achieved. To be sure I understand the request, your expectation is that it returns all the groups, and only users members of the specified group(s)?

— Reply to this email directly, view it on GitHub https://github.com/Yvand/EntraCP/issues/42#issuecomment-1764745286 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUI3YEUY3YLJUMDJCAZNX3X7VHG5AVCNFSM4GF2VG62U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZWGQ3TINJSHA3A . You are receiving this because you were mentioned. https://github.com/notifications/beacon/ACUI3YHF2OYYXYYSYTK2NXDX7VHG5A5CNFSM4GF2VG62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGONEX6ARQ.gif Message ID: @. @.> >

Yvand commented 1 year ago

@vargasfe sorry for this very late reply. Technically it is possible to implement this but:

I realize there is no possibility to restrict the list of potential users that can be returned, but I will be more willing to imnplement this feature if more people request it

KamilPacanek commented 8 months ago

Hi @Yvand bumping the topic :). Two questions:

  1. I see that filtering by groups on Graph level is troublesome (as per MS Learn link you've provided) - but could be possible to filter those at later stage, after acquiring list of users, but before displaying them on pickers?
  2. As a workaround - do we have control over what is returned in people pickers from EntraCP (afaik there are some entities definition)? Like prefixing users from group X?
Yvand commented 8 months ago

Hi @KamilPacanek, everyone, so I took a fresh look at this, did a few tests and... I will implement it for the next version :)

The work is on this branch. The logic is that, for each group set in a new property (currently GroupsWhichUsersMustBeMemberOfAny, if you have a better name, please suggest), I add a request to the batch to get its members using this query: /v1.0/groups/{{groupId}}/members/microsoft.graph.user Then, for each user returned by the 'normal' query (part of same batch), I check if it is member of any of the groups above. If yes it is added, otherwise it is discarded. The nice part is that it does not require to send a new request to Graph, everything is done in the existing batch request that gets users and groups.

One limitation of this is that you cannot set more than 18 groups, because batch requests are limited to 20 individual requests, but I think it is more than enough. Also, I only verify the membership of the users, not the groups, because I believe it matches what everyone who contributed to this thread requested.

So far with my very limites tests, it works as expected. If you have feedback or suggestions on this, please shoot.

KamilPacanek commented 8 months ago

Hi @KamilPacanek, everyone, so I took a fresh look at this, did a few tests and... I will implement it for the next version :)

The work is on this branch. The logic is that, for each group set in a new property (currently GroupsWhichUsersMustBeMemberOfAny, if you have a better name, please suggest), I add a request to the batch to get its members using this query: /v1.0/groups/{{groupId}}/members/microsoft.graph.user Then, for each user returned by the 'normal' query (part of same batch), I check if it is member of any of the groups above. If yes it is added, otherwise it is discarded. The nice part is that it does not require to send a new request to Graph, everything is done in the existing batch request that gets users and groups.

One limitation of this is that you cannot set more than 18 groups, because batch requests are limited to 20 individual requests, but I think it is more than enough. Also, I only verify the membership of the users, not the groups, because I believe it matches what everyone who contributed to this thread requested.

So far with my very limites tests, it works as expected. If you have feedback or suggestions on this, please shoot.

Ahh so the other way around - instead of verifying if a user is in any given group, you ask for the group's members and see if that user is in this list.

How does it affect performance? Does the whole process repeat for each request (incremental search, when user types characters in to people picker)? If so, I would suggest caching the groups members (for some reasonable time) (on/off or configurable time). The least amount of work would be just storing the collection in some variable that will persist until iisreset/pool recycle or whenever EntraCP reloads. Cache can be forcefully invalidated, for example when cached groups IDs are different from those from EntraCP setting (GroupsWhichUsersMustBeMemberOfAny). Time ensures refreshing without user interaction, consistency check would give the possibility to force refresh the cache.

Yvand commented 8 months ago

Yes, this way I can do all the requests in a single batch request to Graph. Yet, today I wrote tests for this feature and there is definitely a cost in performance. To give you an idea: here are rough numbers for 50 search requests (simulate 50 sequential inputs from people picker):

So caching is very much necessary, I will work on it asap, and I like your ideas about it, thank you for the suggestions

KamilPacanek commented 8 months ago

Yes, this way I can do all the requests in a single batch request to Graph. Yet, today I wrote tests for this feature and there is definitely a cost in performance. To give you an idea: here are rough numbers for 50 search requests (simulate 50 sequential inputs from people picker):

  • No filtering (so a batch contains 1 request to search users and 1 request to search groups): Total time is around 17-20 secs
  • Filtering with 1 group (so a batch contains same as no filtering + 1 request to get group members): Total time is around 30-35 secs
  • Filtering with 18 groups (so a batch contains same as no filtering + 18 requests to get groups members): Total time is around 50-55 secs

Does it matter then how many members are in group? How many was in your tests? For how it is now, it seems that there is more costly to ask for groups members (almost double the response time) that it is to ask for one more group (~10% per group).

So caching is very much necessary, I will work on it asap, and I like your ideas about it, thank you for the suggestions

Yeah because we are aware of how it now looks regarding the response times it is now almost a must have :) ...yet I still don't know a better name for GroupsWhichUsersMustBeMemberOfAny ^^

Yvand commented 8 months ago

@KamilPacanek I did not spend time on analyzing the requests, to determine if the amount of members matters. In my test tenant I have 50 groups. A few of those have 53 members, and the other have 10 members. When my tests run, they pick randomly 18 of those groups

Yesterday I did a quick test with the caching, it works but now I need to think about a good implementation

Yvand commented 7 months ago

@KamilPacanek I made good progress on the feature and it looks good in my tests, are you interested in testing a private build, or do you have any comment/suggestion based on the code change? PR is https://github.com/Yvand/EntraCP/pull/243

KamilPacanek commented 7 months ago

Added some review comments - because you did created a PR, I will continue the discussion there 👍

Yvand commented 7 months ago

fixed by https://github.com/Yvand/EntraCP/pull/243