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

Subdomains use HTTPS if settings specify #3987

Merged
merged 2 commits into from
May 18, 2018

Conversation

davidfischer
Copy link
Contributor

This PR adds an option PUBLIC_DOMAIN_USES_HTTPS which if set will make all "view docs" links and everywhere that uses the resolver (eg. the version selector menu) default to HTTPS for *.readthedocs.iodomains. We know there is a valid certificate for these and so our generated links should use HTTPS.

This is step 1/5 on on getting to HTTPS everywhere:
#3282 (comment)

@davidfischer davidfischer requested a review from agjohnson April 20, 2018 20:16
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good. This should be coupled w/ a PR to our ops repo that uses this setting!

@davidfischer
Copy link
Contributor Author

👍

@rmzelle
Copy link

rmzelle commented May 2, 2018

Would it be possible to have PUBLIC_DOMAIN_USES_HTTPS default to True for new RTD projects (and set to False for existing projects)?

@davidfischer
Copy link
Contributor Author

I think there's some confusion about what PUBLIC_DOMAIN_USES_HTTPS will actually do. When we set this to true in production, that means anywhere where somebody clicks "view docs" and the link goes to *.readthedocs.io, the link will be HTTPS. This will apply to all projects where the domain for the docs is *.readthedocs.io both old and new.

Some people run the readthedocs open source project on their own infrastructure. They may or may not have SSL certificates. Likewise if this code is run locally, it shouldn't generate a HTTPS link to localhost. That's why this will default to false. However, in production, this will be true.

@rmzelle
Copy link

rmzelle commented May 2, 2018

Ah, sorry, I thought it was a per-project setting. Never mind!

@davidfischer
Copy link
Contributor Author

Ah, sorry, I thought it was a per-project setting. Never mind!

The per project part of it is that it won't apply to people using custom domains until we can support Let's Encrypt. Then we'll just change this to always use HTTPS in production.

@override_settings(
PRODUCTION_DOMAIN='readthedocs.org',
PUBLIC_DOMAIN='readthedocs.io',
PUBLIC_DOMAIN_USES_HTTPS=True,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of override this outside the method you may use with inside the method and test the same cases you are testing now, but using PUBLIC_DOMAIN_USES_HTTPS=False and test that in that case both are http

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pretty good idea.

@davidfischer
Copy link
Contributor Author

I'm wondering if I should somehow make the setting generic enough where it can later apply to custom domains or whether that will be a separate setting.

@davidfischer
Copy link
Contributor Author

I'm wondering if I should somehow make the setting generic enough where it can later apply to custom domains or whether that will be a separate setting.

I don't want to overengineer this for now. I think we'll decide that when we add Let's Encrypt support. Whether we need another setting or we slightly tweak the purpose of this one, I think it'll be fine.

@ericholscher ericholscher merged commit f74d4fb into readthedocs:master May 18, 2018
@ericholscher
Copy link
Member

This should go out in a deploy early next week. 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants