Skip to content

LDAP Sync fixes: revert recursion resolution, fixed handling of pagingsize of 0 - #7796

Merged
MorrisJobke merged 4 commits into
masterfrom
ldap-sync-fixes
Jan 11, 2018
Merged

LDAP Sync fixes: revert recursion resolution, fixed handling of pagingsize of 0#7796
MorrisJobke merged 4 commits into
masterfrom
ldap-sync-fixes

Conversation

@blizzz

Copy link
Copy Markdown
Member

So, we still encountered to see a lot of log messages on our instances, and with some more debug output I could trace it back. The main issue here is that resolving a recursion during LDAP search (happens with an initial offset > 0) didn't work out as expected, due to some internal state issues. It was not obvious with low offset, but it can pile up unnicely.

Additionally to it comes some by catch with tests.

thus hasMoreResult should return false
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
The message is not helpful anyway for an admin, and oftentimes is just
valid (e.g. when searching with an offset beyond users in LDAP).
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
without recursion we have issues with internal states. paged search status
are set to false, cookies are not being set. In the end we have endless
requests which pile up enormously with a high initial offset.
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzzblizzz added this to the Nextcloud 13 milestone Jan 11, 2018
@MorrisJobkeMorrisJobke mentioned this pull request Jan 11, 2018
18 tasks

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🐘

@codecov

codecovBot commented Jan 11, 2018

Copy link
Copy Markdown

Codecov Report

Merging #7796 into master will increase coverage by <.01%.
The diff coverage is 42.85%.

@@ Coverage Diff @@## master #7796 +/- ##
============================================
+ Coverage 51.22% 51.23% +<.01% 
Complexity 24970 24970 ============================================
Files 1607 1607 Lines 95011 95008 -3 Branches 1376 1376 ============================================
- Hits 48674 48673 -1 + Misses 46337 46335 -2
Impacted FilesCoverage ΔComplexity Δ
apps/user_ldap/lib/Access.php35.61% <0%> (+0.13%)316 <0> (ø)⬇️
apps/user_ldap/lib/Jobs/Sync.php64.57% <100%> (+0.57%)46 <0> (ø)⬇️
apps/files_trashbin/lib/Expiration.php90.32% <0%> (-1.62%)29% <0%> (ø)
lib/private/Files/Cache/Propagator.php94.93% <0%> (-1.27%)16% <0%> (ø)

@MorrisJobke
MorrisJobke merged commit 9898ec9 into masterJan 11, 2018
@MorrisJobke
MorrisJobke deleted the ldap-sync-fixes branch January 11, 2018 16:22
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbugfeature: ldap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@blizzz@rullzer@MorrisJobke