-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Force resolver to use PUBLIC_DOMAIN over HTTPS if not Domain.https #4579
Conversation
Argument ``require_https_domain`` added to force the resolver to use https URLs with PUBLIC_DOMAIN when the Domain is not https.
readthedocs/core/resolver.py
Outdated
domain = custom_domain.domain | ||
elif self._use_subdomain(): | ||
domain = self._get_project_subdomain(canonical_project) | ||
else: | ||
domain = getattr(settings, 'PRODUCTION_DOMAIN') | ||
|
||
protocol = 'http' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this logic is quite right. If require_https_domain=True
, the protocol should be https
. It doesn't matter whether there's a custom domain or the public domain is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this flag should just be require_https
. I don't know what _domain
signifies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! I'm fixing this.
5037210
to
1d7ac4a
Compare
readthedocs/core/resolver.py
Outdated
use_custom_domain = any([ | ||
(custom_domain and not require_https), | ||
(custom_domain and require_https and custom_domain.https), | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this logic is correct for .org (it is for .com). On .org, the canonical domain should be used regardless of HTTPS, correct? If the user specifies that their custom domain is canonical, all our links should use that I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aye, this logic seems to be for the .com case, where we require https if you use a custom domain. perhaps this needs to be another method on the resolver, similar to _use_subdomain()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In .org require_https
will be False
(the default). So this will enter because of the first condition. In that case, we will use the custom domain no matter what protocol we decide to use later in the next if
.
On the other hand, if you are referring to the canonical
attribute of the Domain
object, we weren't considering it on this resolver at least. Actually, I'm not sure if it should be considered or not. I'm not sure how that attribute works.
OK, I updated this PR and the one under .com with a method as @agjohnson suggested to decide whether to use the custom domain or not. |
Doing `settings.SITE_ROOT = '/path'` may cause problems. For this cases, `@override_settings` has to be used. This commit refactors the code around Symlink tests to use the decorator properly.
readthedocs/core/resolver.py
Outdated
|
||
use_https_protocol = any([ | ||
# Rely on the ``Domain.https`` field | ||
custom_domain and custom_domain.https, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the logic on this line should be self._use_custom_domain(custom_domain) and custom_domain.https
. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, functionally, it works but from a logic perspective I think the above is better.
d3a4e0c
to
301cd2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, but I don't see any tests for the new logic.
There shouldn't be changes in the logic. This PR only makes the resolver extensible from corporate site. After the changes in .org's resolver that David did around HTTPS some corporate tests start failing and when trying to fix them I realize that it was not possible with the current implementation of the resolver under .org. So, I refactored it to be able to extend it from corporate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We'll want to be very careful in QA pass and in deploying. Our resolver can be fragile and is used for a lot of not well tested cases.
Argument
require_https_domain
added to force the resolver to use https URLs with PUBLIC_DOMAIN when the Domain is not https.I found this while running tests on Corporate site since we don't serve documentation over plain HTTP there. So, we need a way to force the https protocol even when the project has a Domain object (but https=False). In this case, we force to use our PUBLIC_DOMAIN URL over HTTPS.