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

@davidfischer davidfischer commented Apr 20, 2018

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 Apr 20, 2018
Copy link
Member

@ericholscher ericholscher left a comment

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

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 2, 2018

👍

Loading

@rmzelle
Copy link

@rmzelle 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)?

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 2, 2018

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.

Loading

@rmzelle
Copy link

@rmzelle rmzelle commented May 2, 2018

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

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 2, 2018

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.

Loading

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

@humitos humitos May 3, 2018

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

Loading

Copy link
Contributor Author

@davidfischer davidfischer May 3, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 3, 2018

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 15, 2018

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.

Loading

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

@ericholscher ericholscher commented May 18, 2018

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

Loading

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.

None yet

4 participants