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.

Sign up for GitHub

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

Jump to bottom

Allow enforcing HTTPS for custom domains #4483

Merged
merged 3 commits into from
Aug 9, 2018
Merged

Allow enforcing HTTPS for custom domains #4483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0a2b26
Allow enforcing HTTPS for custom domains
davidfischer Aug 6, 2018
6d67a0e
Updates based on feedback
davidfischer Aug 8, 2018
570ce61
Show certificate issue status
davidfischer Aug 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions readthedocs/core/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def resolve_path(self, project, filename='', version_slug=None,
def resolve_domain(self, project, private=None):
# pylint: disable=unused-argument
canonical_project = self._get_canonical_project(project)
domain = canonical_project.domains.filter(canonical=True).first()
domain = self._get_project_custom_domain(canonical_project)
if domain:
return domain.domain
elif self._use_subdomain():
Expand All @@ -144,13 +144,24 @@ def resolve(self, project, protocol='http', filename='', private=None,
version_slug = project.get_default_version()
private = self._get_private(project, version_slug)

domain = self.resolve_domain(project, private=private)
canonical_project = self._get_canonical_project(project)
custom_domain = self._get_project_custom_domain(canonical_project)

# Use HTTPS if settings specify
public_domain = getattr(settings, 'PUBLIC_DOMAIN', None)
use_https = getattr(settings, 'PUBLIC_DOMAIN_USES_HTTPS', False)
if use_https and public_domain and public_domain in domain:
protocol = 'https'
if custom_domain:
domain = custom_domain.domain
elif self._use_subdomain():
domain = self._get_project_subdomain(canonical_project)
else:
domain = getattr(settings, 'PRODUCTION_DOMAIN')
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this if/elif/else the same code of what self.resolve_domain does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. This function needs access to the Domain object in order to read the .https property. That object is not exposed from the resolve_domain method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading that object again based on the hostname alone will require another database query and the resolver is already the largest single source of database load in Read the Docs.

Copy link
Member

Choose a reason for hiding this comment

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

I understand and I saw that you mentioned the issue to refactor this. So, I'm fine with "duplication" here.

Although, I think that adding a similar explanation written in these comments as a comment in the code would make it easier to refactor this in the future.


if custom_domain:
protocol = 'https' if custom_domain.https else 'http'
else:
# Use HTTPS if settings specify
public_domain = getattr(settings, 'PUBLIC_DOMAIN', None)
use_https = getattr(settings, 'PUBLIC_DOMAIN_USES_HTTPS', False)
if use_https and public_domain and public_domain in domain:
protocol = 'https'

return '{protocol}://{domain}{path}'.format(
protocol=protocol,
Expand Down Expand Up @@ -196,6 +207,9 @@ def _get_project_subdomain(self, project):
subdomain_slug = project.slug.replace('_', '-')
return "%s.%s" % (subdomain_slug, public_domain)

def _get_project_custom_domain(self, project):
return project.domains.filter(canonical=True).first()

def _get_private(self, project, version_slug):
from readthedocs.builds.models import Version
try:
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/projects/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class DomainAdmin(admin.ModelAdmin):
list_display = ('domain', 'project', 'https', 'count')
search_fields = ('domain', 'project__slug')
raw_id_fields = ('project',)
list_filter = ('canonical',)
list_filter = ('canonical', 'https')
model = Domain


Expand Down
2 changes: 1 addition & 1 deletion readthedocs/projects/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ class DomainForm(forms.ModelForm):

class Meta(object):
model = Domain
exclude = ['machine', 'cname', 'count', 'https']
exclude = ['machine', 'cname', 'count']

def __init__(self, *args, **kwargs):
self.project = kwargs.pop('project', None)
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ class Domain(models.Model):
https = models.BooleanField(
_('Use HTTPS'),
default=False,
help_text=_('SSL is enabled for this domain')
help_text=_('Always use HTTPS for this domain')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this will result in a migration. However, there's a bunch of other minor pending migrations on Project.

)
count = models.IntegerField(default=0, help_text=_(
'Number of times this domain has been hit'),)
Expand Down
8 changes: 4 additions & 4 deletions readthedocs/rtd_tests/tests/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,16 +745,16 @@ def test_subproject_with_translation_with_custom_domain(self):
)

url = resolve(self.superproject_en, filename='')
self.assertEqual(url, 'http://docs.example.com/en/latest/')
self.assertEqual(url, 'https://docs.example.com/en/latest/')

url = resolve(self.superproject_es, filename='')
self.assertEqual(url, 'http://docs.example.com/es/latest/')
self.assertEqual(url, 'https://docs.example.com/es/latest/')

# yapf: disable
url = resolve(self.subproject_en, filename='')
self.assertEqual(
url,
('http://docs.example.com/projects/'
('https://docs.example.com/projects/'
'{subproject.slug}/en/latest/').format(
subproject=self.subproject_en,
),
Expand All @@ -763,7 +763,7 @@ def test_subproject_with_translation_with_custom_domain(self):
url = resolve(self.subproject_es, filename='')
self.assertEqual(
url,
('http://docs.example.com/projects/'
('https://docs.example.com/projects/'
'{subproject.slug}/es/latest/').format(
subproject=self.subproject_en,
),
Expand Down