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

Allow enforcing HTTPS for custom domains #4483

Merged
merged 3 commits into from Aug 9, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer 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 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')
Copy link
Contributor Author

@davidfischer davidfischer Aug 7, 2018

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.

Loading

Copy link
Member

@humitos humitos left a comment

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?

Loading

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

@humitos humitos Aug 7, 2018

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?

Loading

Copy link
Contributor Author

@davidfischer davidfischer Aug 7, 2018

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.

Loading

Copy link
Contributor Author

@davidfischer davidfischer Aug 7, 2018

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.

Loading

Copy link
Member

@humitos humitos Aug 7, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer 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.

Loading

davidfischer added 2 commits Aug 8, 2018
- add resolve HTTPS/HTTP test
- add comment detailing duplication
@davidfischer
Copy link
Contributor Author

@davidfischer 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.

Loading

Copy link
Member

@ericholscher ericholscher left a comment

Looks good to me, once the build is passing.

Loading

@davidfischer davidfischer merged commit 5d8becd into master Aug 9, 2018
1 check passed
Loading
@davidfischer davidfischer deleted the davidfischer/custom-domains-use-https branch Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants