Contd: Add layer on top of LDAP methods to catch server disconnects and attempt re-connection

[blizzz]

Continuation of #4992

@bline I signed-off your commits so this test passes :) Please test whether my changes still work for you.

For Nc 13 we should aim for a cleaner solution, but this requires architectural changes. Alas, we need to backport it, so let's get this is as an intermediate step.

Fixes LDAP Server caused timeout with long-running processes where there is a too big gap between LDAP calls.

[mention-bot]

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @LukasReschke and @Xenopathic to be potential reviewers.

[bline]

Thanks @blizzz, I'll test this tonight.

[blizzz]

Unfortunately the controlPagedResultResponse does not work around this way. With https://github.com/nextcloud/server/pull/5122/files you can use the IntegrationTestPaging and will see it fails. Likely invokeLDAPMethod() just does not handle this as reference.

[blizzz]

Possible solutions:

(1) don't use this method against the invoker, because it is fired very soon after an LDAP operation took place. An idleTimeout sould not be effective here.

(2) expect the command name, accept ALL attributes as references (invokeLDAPMethod($command, &...$arguments)). Downside: many places where value instead of variables are passed. Makes it cumbersome.

For simplicity I'd go with (1), although I first liked the second more at first. The final solution for 13 could adopt this.

[codecov]

Codecov Report

Merging #5104 into master will increase coverage by <.01%.
The diff coverage is 45%.

@@             Coverage Diff              @@
##             master    #5104      +/-   ##
============================================
+ Coverage     54.14%   54.15%   +<.01%     
- Complexity    22283    22289       +6     
============================================
  Files          1379     1379              
  Lines         85337    85361      +24     
  Branches       1322     1322              
============================================
+ Hits          46209    46227      +18     
- Misses        39128    39134       +6

Impacted Files	Coverage Δ	Complexity Δ
apps/user_ldap/lib/Access.php	18.42% <45%> (+1.12%)	301 <7> (+6)	⬆️
lib/private/Server.php	93.27% <0%> (-0.15%)	120% <0%> (ø)
core/js/js.js	61.78% <0%> (+0.56%)	0% <0%> (ø)	⬇️
lib/private/Security/CertificateManager.php	91.83% <0%> (+1.02%)	39% <0%> (ø)	⬇️

[blizzz]

I created a backport to stable11 #5128

stable12 upcoming, but this is far easier as the code base is pretty much the same as master

[blizzz]

Tests succeeds, only DB=postgresENABLE_REDIS=truePHP=5.6 runs into a timeout [sic!], but this is known from CI.

[blizzz]

Tested successfully by @bline as stated #5128 (comment)

Still need a reviewer @nextcloud/ldap :)

[rullzer]

Makes sense 👍

[blizzz]

@karlitschek btw, backport OK? Down to 11?

[blizzz]

missing backport to 12: #5210

[karlitschek]

nice. definitely backport to 12 and 11