Skip to content

LDAP Wizard: do not attempt to recognise cert issue by using LDAPTLS_REQCERT#5616

Merged
MorrisJobke merged 2 commits into
masterfrom
ldap-wizard-remove-LDAPTLS_REQCERT-attempt
Aug 10, 2017
Merged

LDAP Wizard: do not attempt to recognise cert issue by using LDAPTLS_REQCERT#5616
MorrisJobke merged 2 commits into
masterfrom
ldap-wizard-remove-LDAPTLS_REQCERT-attempt

Conversation

@blizzz

@blizzz blizzz commented Jul 5, 2017

Copy link
Copy Markdown
Member

first, it does not work (at least not everywhere/reliably), second if it
did it was not reset properly. Removes a bit of complexity (and useless debug output).

Might be worth to backport… there is no known bug about this, but it would not be straight forward to track it hereto.

please review @nextcloud/ldap

@mention-bot

Copy link
Copy Markdown

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

@codecov

codecov Bot commented Jul 5, 2017

Copy link
Copy Markdown

Codecov Report

Merging #5616 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #5616      +/-   ##
============================================
+ Coverage     53.82%   53.82%   +<.01%     
+ Complexity    22731    22726       -5     
============================================
  Files          1403     1403              
  Lines         86635    86627       -8     
  Branches       1328     1328              
============================================
- Hits          46630    46628       -2     
+ Misses        40005    39999       -6
Impacted Files Coverage Δ Complexity Δ
apps/user_ldap/lib/LDAP.php 25.43% <ø> (ø) 51 <0> (ø) ⬇️
apps/user_ldap/lib/Wizard.php 17.34% <0%> (+0.19%) 221 <8> (-5) ⬇️
apps/comments/lib/EventHandler.php 79.16% <0%> (-8.34%) 7% <0%> (ø)

@blizzz blizzz requested a review from LukasReschke July 10, 2017 08:53
blizzz added 2 commits July 21, 2017 14:55
first, it does not work (at least not everywhere/reliably), second if it
did it was not reset properly. Removes a bit of complexity.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@LukasReschke LukasReschke force-pushed the ldap-wizard-remove-LDAPTLS_REQCERT-attempt branch from 027ff99 to 2543991 Compare July 21, 2017 12:55
@LukasReschke

Copy link
Copy Markdown
Member

Rebased to retrigger test execution

@MorrisJobke

Copy link
Copy Markdown
Member

@rullzer Ping

@rullzer

rullzer commented Jul 22, 2017

Copy link
Copy Markdown
Member

@MorrisJobke well I have no idea how to test or what I'm testing :P

@MorrisJobke MorrisJobke merged commit 43facdb into master Aug 10, 2017
@MorrisJobke MorrisJobke deleted the ldap-wizard-remove-LDAPTLS_REQCERT-attempt branch August 10, 2017 19:06
@MorrisJobke

Copy link
Copy Markdown
Member

Might be worth to backport… there is no known bug about this, but it would not be straight forward to track it hereto.

I would not do that. Once we have a more clear idea if it fixes that specific bug, then sure, but until we have no clear issue, I would avoid to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants