Skip to content

Allow ldap_dynamic_group_member_url to be a filter - #8230

Closed
Cybso wants to merge 3 commits into
nextcloud:masterfrom
Cybso:ldap-dynamic-groups-as-filter
Closed

Allow ldap_dynamic_group_member_url to be a filter#8230
Cybso wants to merge 3 commits into
nextcloud:masterfrom
Cybso:ldap-dynamic-groups-as-filter

Conversation

@Cybso

@CybsoCybso commented Feb 7, 2018

Copy link
Copy Markdown

This change allows dynamic_group_member_url of the user_ldap module to
be the filter itself instead of an attribute where the filter can
be read from. The value is assumed to be a filter when it starts with
a round bracket. The substring '%gid' will be replaced by the group's
DN.

This allows group members to be resolved by LDAP_MATCHING_RULE_IN_CHAIN
on Active Directory servers. Example:

 (&(objectClass=person)(memberOf:1.2.840.113556.1.4.1941:=%gid))

Returns all persons that are either direct members of %gid or members
of another group that is member of %gid.

An alternative implementation would be to implement this as a dedicated settings field, but this "simple" solution works for me ;-)

Signed-off-by: Roland Tapken roland@bitarbeiter.net

This change allows dynamic_group_member_url of the user_ldap module to
be the filter itself instead of an attribute where the filter can
be read from. The value is assumed to be a filter when it starts with
a round bracket. The substring '%gid' will be replaced by the group's
DN.
This allows group members to be resolved by LDAP_MATCHING_RULE_IN_CHAIN
on Active Directory servers. Example:
(&(objectClass=person)(memberOf:1.2.840.113556.1.4.1941:=%gid))
Returns all persons that are either direct members of %gid or members
of another group that is member of %gid.
Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@Cybso

Cybso commented Feb 8, 2018

Copy link
Copy Markdown
Author

Why has the jsunit test being aborted?! However, since this patch did not change the UI it shouldn't matter.

fetchListOfGroups() returns an array of strings when called
with a single attribute to fetch. This patch makes the result
compatible with the rest of the method.
Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@codecov

codecovBot commented Feb 8, 2018

Copy link
Copy Markdown

Codecov Report

Merging #8230 into master will increase coverage by 0.06%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #8230 +/- ##
============================================
+ Coverage 51.73% 51.79% +0.06% - Complexity 25366 25371 +5 
============================================
Files 1599 1607 +8 Lines 95064 95160 +96 Branches 1376 1378 +2 ============================================
+ Hits 49180 49290 +110 + Misses 45884 45870 -14
Impacted FilesCoverage ΔComplexity Δ
apps/user_ldap/lib/Group_LDAP.php61.14% <0%> (-1.78%)180 <0> (+4)
apps/dav/lib/Files/FileSearchBackend.php42.61% <0%> (-29.27%)71% <0%> (+22%)
apps/updatenotification/appinfo/routes.php11.11% <0%> (-28.89%)0% <0%> (ø)
lib/private/Files/Cache/HomePropagator.php77.77% <0%> (-11.12%)3% <0%> (ø)
lib/private/Settings/Admin/Sharing.php84% <0%> (-8.86%)6% <0%> (+1%)
settings/Middleware/SubadminMiddleware.php87.5% <0%> (-5.36%)6% <0%> (ø)
lib/private/Http/Client/Client.php95.08% <0%> (-4.92%)21% <0%> (+5%)
apps/updatenotification/lib/Settings/Admin.php73.58% <0%> (-3.92%)11% <0%> (+3%)
apps/user_ldap/lib/Connection.php54.74% <0%> (-3.33%)122% <0%> (+3%)
...pps/encryption/lib/Controller/StatusController.php74.28% <0%> (-3.22%)6% <0%> (-1%)
... and 242 more

@MorrisJobke

Copy link
Copy Markdown
Member

@nextcloud/ldap Please have a look at this as well.

@blizzz

Copy link
Copy Markdown
Member

The substring '%gid' will be replaced by the group's DN.

Please don't use %gid as it is misleading. In nextcloud it is used to represent the group id, apart from that the DN is not really an ID. For instance, use %gdn .

Would it be feasable to write some unit tests for it?

Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@Cybso

Cybso commented Mar 7, 2018

Copy link
Copy Markdown
Author

@blizzz I've replaced the string. Is there any tutorial on how to setup a test environment where I can execute the unit tests myself before pushing them into the repository?

@blizzz

Copy link
Copy Markdown
Member

@Cybso does this help? https://docs.nextcloud.com/server/13/developer_manual/core/unit-testing.html

[sudo] ./autotest.sh sqlite $TESTFILE is my to-go approach, too.

@Cybso

Cybso commented Mar 7, 2018

Copy link
Copy Markdown
Author

@blizzz Thanks, I will try

@blizzz

Copy link
Copy Markdown
Member

@Cybso seems you did not have a chance yet, did you?

@Cybso

Copy link
Copy Markdown
Author

@blizzz Yes, you're right, I'm very busy at the moment, but I haven't forgot this ticket. Holidays are coming in a few weeks and I think I will then find the time to write this test case.

@MorrisJobkeMorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018
@MorrisJobkeMorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 9, 2018
@nextcloud-botnextcloud-bot removed the stale Ticket or PR with no recent activity label Jul 9, 2018
@ChristophWurst

Copy link
Copy Markdown
Member

@Cybso any updates? This is conflicting with other changes of that file that happened in the meantime.

@Cybso

Copy link
Copy Markdown
Author

I am very sorry, but at the moment I am extremely busy both professionally and privately, so I did not have the time to do that and probably will not find an opportunity to get into the test framework in the foreseeable future.

But since we still have to do the upgrade to Nextcloud 15 within my company I think I will at least be able to solve the merge conflicts this week.

@MorrisJobkeMorrisJobke added this to the Nextcloud 17 milestone Mar 20, 2019
@MorrisJobkeMorrisJobke mentioned this pull request Jul 16, 2019
28 tasks
@rullzer

Copy link
Copy Markdown
Member

I'm going to close this.
@blizzz feel free to reopen and rebase

@rullzerrullzer closed this Sep 15, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Cybso@MorrisJobke@blizzz@ChristophWurst@rullzer@nextcloud-bot