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

Allow enforcing HTTPS for custom domains #4483

Merged
merged 3 commits into from Aug 9, 2018

Conversation

Projects
None yet
3 participants
@davidfischer
Contributor

davidfischer commented Aug 6, 2018

This allows users to mark a domain as HTTPS and then all links to its documentation will be HTTPS links instead of HTTP links.

Personally I don't love these changes to the resolver but I believe the resolver needs a larger refactor for performance reasons anyway (#3712). If there's a better way to add this functionality without adding more SQL queries and degrading performance of a very performance sensitive bit of code, please let me know or feel free to add to this.

Fixes #2236

Screenshot

screen shot 2018-08-06 at 3 17 06 pm

@davidfischer davidfischer requested a review from rtfd/core Aug 6, 2018

@@ -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')

This comment has been minimized.

@davidfischer

davidfischer Aug 7, 2018

Contributor

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

@davidfischer

davidfischer Aug 7, 2018

Contributor

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

@humitos

I think these changes are good.

I got a little lost on the review (as usual when taking a look at the resolver) but seems smooth. I left a refactor comment, but not 100% sure that I'm correct.

Besides the tests you changed, shouldn't we have a specific test that uses a Domain(https=False) and check that it resolves to http?

elif self._use_subdomain():
domain = self._get_project_subdomain(canonical_project)
else:
domain = getattr(settings, 'PRODUCTION_DOMAIN')

This comment has been minimized.

@humitos

humitos Aug 7, 2018

Member

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

@humitos

humitos Aug 7, 2018

Member

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

This comment has been minimized.

@davidfischer

davidfischer Aug 7, 2018

Contributor

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.

@davidfischer

davidfischer Aug 7, 2018

Contributor

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.

This comment has been minimized.

@davidfischer

davidfischer Aug 7, 2018

Contributor

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.

@davidfischer

davidfischer Aug 7, 2018

Contributor

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.

This comment has been minimized.

@humitos

humitos Aug 7, 2018

Member

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.

@humitos

humitos Aug 7, 2018

Member

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.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 7, 2018

Contributor

Besides the tests you changed, shouldn't we have a specific test that uses a Domain(https=False) and check that it resolves to http?

Probably yes.

Contributor

davidfischer commented Aug 7, 2018

Besides the tests you changed, shouldn't we have a specific test that uses a Domain(https=False) and check that it resolves to http?

Probably yes.

davidfischer added some commits Aug 8, 2018

Updates based on feedback
- add resolve HTTPS/HTTP test
- add comment detailing duplication
@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 8, 2018

Contributor

I updated based on the feedback to add another resolver test and to comment on the duplication.

I also added a UI element for showing the status of issuing a certificate.

Contributor

davidfischer commented Aug 8, 2018

I updated based on the feedback to add another resolver test and to comment on the duplication.

I also added a UI element for showing the status of issuing a certificate.

@ericholscher

ericholscher approved these changes Aug 8, 2018 edited

Looks good to me, once the build is passing.

@davidfischer davidfischer merged commit 5d8becd into master Aug 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidfischer davidfischer deleted the davidfischer/custom-domains-use-https branch Aug 9, 2018

@Juanlu001 Juanlu001 referenced this pull request Aug 14, 2018

Closed

SSL for docs #386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment