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.

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

Easier tls #512

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Easier tls #512

merged 4 commits into from
Feb 28, 2020

Conversation

jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Feb 11, 2020

Make it easier to handle TLS support by using a checkbox instead of auto-detection.

Note that the CA certificate for the LDAP server must be accessible by the ownCloud's LDAP library (the default OS' certificate location should work)

Related: https://github.com/owncloud/enterprise/issues/3721

Quick list of changes:

  • Add TLS checkbox
  • Remove obsolete "detect port" button and TLS auto-detection
  • Include operation timeout option to prevent getting stuck if the CA certificate is missing (if TLS checkbox is enabled)

@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #512 into master will increase coverage by 0.8%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #512     +/-   ##
===========================================
+ Coverage     35.95%   36.75%   +0.8%     
+ Complexity     1332     1305     -27     
===========================================
  Files            31       31             
  Lines          3780     3700     -80     
===========================================
+ Hits           1359     1360      +1     
+ Misses         2421     2340     -81
Impacted Files Coverage Δ Complexity Δ
lib/Controller/WizardController.php 38.15% <ø> (-0.81%) 27 <0> (-1)
lib/Wizard.php 21.94% <100%> (+2.87%) 200 <0> (-26) ⬇️
lib/Connection.php 72.66% <100%> (+0.09%) 110 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce34cd5...f3cf582. Read the comment docs.

@jvillafanez jvillafanez requested a review from VicDeo February 12, 2020 12:51
@VicDeo
Copy link
Member

VicDeo commented Feb 13, 2020

Fixes #505 ?

Copy link
Member

@VicDeo VicDeo left a comment

Choose a reason for hiding this comment

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

Looks good

@jvillafanez
Copy link
Member Author

Fixes #505 ?

Yes, as long as he has the certificates to validate the LDAP server. He can also turn off the certificate validation although it's in the advanced tab.
Maybe we should move that "turn off certificate validation" checkbox....

@jvillafanez jvillafanez force-pushed the easier_tls branch 2 times, most recently from 5596f3d to b11d6dc Compare February 18, 2020 13:46
@micbar
Copy link
Contributor

micbar commented Feb 19, 2020

@enbrnz @pako81 @dercorn Please give feedback. There is a recording from JPs demo last friday.

@pako81
Copy link

pako81 commented Feb 19, 2020

@micbar had a look at the code as well as at the demo from JP. Change makes totally sense so 👍

@jvillafanez jvillafanez force-pushed the easier_tls branch 2 times, most recently from c0e817b to 74c6df0 Compare February 27, 2020 08:31
@micbar
Copy link
Contributor

micbar commented Feb 28, 2020

@jvillafanez please rebase and merge, seems fine

@jvillafanez jvillafanez merged commit b3288ec into master Feb 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the easier_tls branch February 28, 2020 11:35
@HanaGemela HanaGemela added this to the QA milestone Mar 9, 2020
@davitol davitol mentioned this pull request Mar 17, 2020
31 tasks
Sign up for free to 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.

5 participants