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.
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.
Maybe this flag should just be require_https. I don't know what _domain signifies.
There was a problem hiding this comment.
You are right! I'm fixing this.
humitos
force-pushed
the
humitos/resolver/force-https
branch
from
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.
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.
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.
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.
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.
I mean, functionally, it works but from a logic perspective I think the above is better.
humitos
force-pushed
the
humitos/resolver/force-https
branch
from
d3a4e0c
to
301cd2f
Compare
|
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. |
Argument
require_https_domainadded 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.