Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Filter users by group #243

Merged
merged 13 commits into from
Apr 9, 2024
Merged

Filter users by group #243

merged 13 commits into from
Apr 9, 2024

Conversation

Yvand
Copy link
Owner

@Yvand Yvand commented Apr 4, 2024

No description provided.

@Yvand Yvand linked an issue Apr 4, 2024 that may be closed by this pull request
Show only members of Group #42
Closed
@Yvand Yvand mentioned this pull request Apr 4, 2024
Closed
@Yvand Yvand added this to the Version 25 milestone Apr 4, 2024
KamilPacanek
KamilPacanek reviewed Apr 4, 2024
View reviewed changes
Copy link

@KamilPacanek KamilPacanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, I haven't study in details the tests and the logic itself :)

Yvand.EntraCP/Yvand.EntraClaimsProvider/Configuration/EntraIDProviderConfiguration.cs
@@ -69,6 +69,13 @@ public interface IEntraIDProviderSettings
/// Gets if only security-enabled groups should be returned
/// </summary>
bool FilterSecurityEnabledGroupsOnly { get; }

/// <summary>
/// Comma separated list of group IDs (max 18 values). If set, users must be member of any of these groups to be returned to SharePoint
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any checks for that (max 18 groups)? I see that setter only assigns the value.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in commit 368a5fe

Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraIDEntityProvider.cs
public EntraIDEntityProvider(string claimsProviderName, IClaimsProviderSettings Settings) : base(claimsProviderName)
{
this.Settings = Settings;

// Reset cache if config changes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when config changes the new instance of EntraIDEntityProvider is created, is that so?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. This happens here:

EntraCP/Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraCP.cs

Line 193 in ddb76a0

this.EntityProvider = new EntraIDEntityProvider(Name, this.Settings);

Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraIDEntityProvider.cs
@@ -180,7 +190,7 @@ protected virtual void BuildFilter(OperationContext currentContext, List<EntraID

List<string> userFilterBuilder = new List<string>();
List<string> groupFilterBuilder = new List<string>();
List<string> userSelectBuilder = new List<string> { "UserType", "Mail" }; // UserType and Mail are always needed to deal with Guest users
List<string> userSelectBuilder = new List<string> { "Id", "UserType", "Mail" }; // UserType and Mail are always needed to deal with Guest users
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Id was added because of the groups?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically, now I declare that I always want to get the user ID of each user returned by Graph

Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraIDEntityProvider.cs
// List of groups that users must be member of, to be returned to SharePoint
string[] groupsWhichUsersMustBeMemberOfRequestIds = null;
string groupsWhichUsersMustBeMemberOfAny = this.Settings.GroupsWhichUsersMustBeMemberOfAny;
//groupsWhichUsersMustBeMemberOfAny = "c9a94341-89b5-4109-a501-2a14027b5bf0"; // testEntraCPGroup_005 - everyone member
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should those comments be included in the master branch?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may or may not keep them, they can could be useful as hints for debugging in the future

Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraIDEntityProvider.cs
{
await cachedTenantData.WriteDataLock.WaitAsync().ConfigureAwait(false);
lockToWriteInCachedDataWasTaken = true;
if (cachedTenantData.UserIdsMembersOfAnyRequiredGroup != null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just checked in the line 468 that this is null.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here is the logic:

  1. Check if UserIdsMembersOfAnyRequiredGroup is null
  2. If so, wait for the lock, that is taken to fill UserIdsMembersOfAnyRequiredGroup
  3. Once lock is acquired, check if, in the meantime, UserIdsMembersOfAnyRequiredGroup was not already filled by another thread. If so, no need to fill it UserIdsMembersOfAnyRequiredGroup again

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right.

Yvand.EntraCP/Yvand.EntraClaimsProvider/EntraIDEntityProvider.cs
@@ -352,7 +362,7 @@ protected async Task<List<DirectoryObject>> QueryEntraIDTenantsAsync(OperationCo
return allResults;
}

protected virtual async Task<List<DirectoryObject>> QueryEntraIDTenantAsync(OperationContext currentContext, EntraIDTenant tenant)
protected virtual async Task<List<DirectoryObject>> QueryEntraIDTenantAsync(OperationContext currentContext, EntraIDTenant tenant, CachedEntraIDTenantData cachedTenantData)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could really use some refactoring into smaller ones :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you're right, but that would be for another day 😅

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, definitely.

@KamilPacanek
Copy link

That'll be it for me, thank you for taking care of that feature :)

@Yvand Yvand merged commit 130ed40 into dev Apr 9, 2024
4 checks passed
@Yvand Yvand deleted the filter-users-by-group branch April 9, 2024 08:38
@Yvand Yvand mentioned this pull request Apr 26, 2024
Closed
This was referenced May 3, 2024
Closed
Merged
Yvand added a commit that referenced this pull request May 3, 2024
@Yvand
Publish EntraCP v25.0 (#254)
98d9620 
* Update Yvand.EntraCP.Tests.csproj

* Filter users by group (#243)

* first implementation

* add tests

* improve tests

* update tests

* quick draft of caching authorized users for tests

* rename objects

* update caching

* add property TenantDataCacheLifetimeInMinutes

* validate the values set on the new properties

* add debug info to track random error

* Update CHANGELOG.md

* Update UnitTestsHelper.cs

* Update EntraIDProviderConfiguration.cs

* Set scope correctly for national clouds (#246)

* set scope correctly for national clouds

* add comments

* redesign management of national clouds

* Update CHANGELOG.md

* Update global config page to allow updating credentials of existing tenants (#248)

* update page

* Update GlobalSettings.ascx.cs

* implement feature

* Update CHANGELOG.md

* Update CHANGELOG.md

* Prompt user for confirmation before actually deleting a tenant

* improve the tenant list view

* Add controls in config page to set the groups list to restrict searchable users (#249)

* add controls to ui

* add plumbing

* update logging

* due to current design, minimum value for TenantDataCacheLifetimeInMinutes is 1 (not 0)

* rename mêmbers related to the tenant cache

* Bump all dependencies (#251)

* bump all deps

* Update CHANGELOG.md

* Update EntraIDProviderConfiguration.cs

* Update verify-prs-and-commits.yml

* call reusable workflows using new project name "EntraCP"

* make tenant name clickable to redirect to Entra portal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KamilPacanek KamilPacanek KamilPacanek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 25
Development

Successfully merging this pull request may close these issues.

Show only members of Group
2 participants
@Yvand @KamilPacanek