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

Connectivity check: test https OR ELSE http #31426

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

julien-nc
Copy link
Member

Instead of https AND http.

As tested domains can be set by admins with connectivity_check_domains config value, we can't know for sure those domains are reachable with both http and https.

This avoids the connectivity warning in admin setting's overview section when a domain is only accessible via either http or https.

@julien-nc julien-nc requested review from a team, blizzz, skjnldsv and come-nc and removed request for a team March 3, 2022 14:18
@come-nc
Copy link
Contributor

come-nc commented Mar 3, 2022

That means that an SSL expiration problem for instance would not be spotted if the http version works?

I understand that more and more domains are not available by http, but I feel like https should always work?

@julien-nc julien-nc added this to the Nextcloud 24 milestone Mar 3, 2022
@julien-nc
Copy link
Member Author

@come-nc I agree. How about only testing https? It feels a bit harsh...

Another (heavy) solution would be to have a config setting to choose which protocol is tested in this connectivity check.

@come-nc
Copy link
Contributor

come-nc commented Mar 3, 2022

I did not understand the context at first, but this is only used to test whether the server has internet access.
So testing https only seems reasonnable to me.

Other solution would be to allow putting URIs with protocol instead of just domains in the config, but I would say that is overkill for this feature.

@julien-nc
Copy link
Member Author

😁 The context is someone who does not have proper internet access but who needs this test to pass to use a custom appstore URL. So they provide custom connectivity_check_domains and some of them are accessible via http only, some others are accessible via https only.

@julien-nc julien-nc force-pushed the enh/noid/make-connectivity-check-softer branch from 79e0584 to e147127 Compare March 4, 2022 09:29
@julien-nc
Copy link
Member Author

@come-nc Rebased on master, added method signature types.

It would be awesome to backport this to stable23 and stable22.

@come-nc
Copy link
Contributor

come-nc commented Mar 4, 2022

My fear is that http is a lot less useful check, you may be on a network where all external accesses are redirected to a blocker page. With https this would be detected.

@julien-nc
Copy link
Member Author

@come-nc How about this instead? 3b6e463
If no protocol is set, then we keep the same behaviour (test both http and https), if a protocol is set, just try this one.
If you agree I'll push it in this PR's branch.

@come-nc
Copy link
Contributor

come-nc commented Mar 7, 2022

@come-nc How about this instead? 3b6e463 If no protocol is set, then we keep the same behaviour (test both http and https), if a protocol is set, just try this one. If you agree I'll push it in this PR's branch.

👍 The regex is missing a ^ at the start, otherwise it’s good for me.

@julien-nc julien-nc force-pushed the enh/noid/make-connectivity-check-softer branch from e147127 to 135fcbe Compare March 7, 2022 13:12
@julien-nc julien-nc force-pushed the enh/noid/make-connectivity-check-softer branch from 135fcbe to d5574cf Compare March 7, 2022 13:13
@julien-nc
Copy link
Member Author

@come-nc Great, nice catch, thanks for the feedback.
Rebased on master.

@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Mar 7, 2022
@julien-nc
Copy link
Member Author

Drone failure seems unrelated.

@julien-nc julien-nc merged commit 9be87e6 into master Mar 7, 2022
@julien-nc julien-nc deleted the enh/noid/make-connectivity-check-softer branch March 7, 2022 15:05
@julien-nc
Copy link
Member Author

/backport to stable23

@julien-nc
Copy link
Member Author

/backport to stable22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: settings pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants