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

Add proxito headers to redirect responses #7007

Merged
merged 7 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions dockerfiles/nginx/proxito.conf
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ server {
add_header X-RTD-Project-Method $rtd_project_method always;
set $rtd_redirect $upstream_http_x_rtd_redirect;
add_header X-RTD-Redirect $rtd_redirect always;
set $cache_tag $upstream_http_cache_tag;
add_header Cache-Tag $cache_tag always;
}

# Serve 404 pages here
Expand Down
35 changes: 35 additions & 0 deletions readthedocs/proxito/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,39 @@ class ProxitoMiddleware(MiddlewareMixin):

"""The actual middleware we'll be using in prod."""

def add_proxito_headers(self, request, response):
"""Add debugging headers to proxito responses."""

project_slug = getattr(request, 'host_project_slug', '')
version_slug = getattr(request, 'path_version_slug', '')
path = getattr(response, 'proxito_path', '')

response['X-RTD-Domain'] = request.get_host()
response['X-RTD-Project'] = project_slug

if version_slug:
response['X-RTD-Version'] = version_slug

if path:
response['X-RTD-Path'] = path

# Include the project & project-version so we can do larger purges if needed
response['Cache-Tag'] = f'{project_slug}'
if version_slug:
response['Cache-Tag'] += f',{project_slug}-{version_slug}'

if hasattr(request, 'rtdheader'):
response['X-RTD-Project-Method'] = 'rtdheader'
elif hasattr(request, 'subdomain'):
response['X-RTD-Project-Method'] = 'subdomain'
elif hasattr(request, 'cname'):
response['X-RTD-Project-Method'] = 'cname'

if hasattr(request, 'external_domain'):
response['X-RTD-Version-Method'] = 'domain'
else:
response['X-RTD-Version-Method'] = 'path'

def process_request(self, request): # noqa
if any([not settings.USE_SUBDOMAIN, 'localhost' in request.get_host(),
'testserver' in request.get_host()]):
Expand Down Expand Up @@ -144,6 +177,8 @@ def process_response(self, request, response): # noqa

hsts_header_values = []

self.add_proxito_headers(request, response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than adding a single-use mixin in a separate file, couldn't this method just be a method on this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I thought we'd have to use it more places, but we haven't yet, so that seems fine. 👍


if not request.is_secure():
# Only set the HSTS header if the request is over HTTPS
return response
Expand Down
67 changes: 67 additions & 0 deletions readthedocs/proxito/tests/test_headers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import pytest
from django.test import override_settings

import django_dynamic_fixture as fixture

from readthedocs.projects.models import Domain
from .base import BaseDocServing


@override_settings(
PUBLIC_DOMAIN='dev.readthedocs.io',
PUBLIC_DOMAIN_USES_HTTPS=True,
)
class ProxitoHeaderTests(BaseDocServing):

def test_redirect_headers(self):
r = self.client.get('', HTTP_HOST='project.dev.readthedocs.io')
self.assertEqual(r.status_code, 302)
self.assertEqual(r['X-RTD-Redirect'], 'system')
self.assertEqual(
r['Location'], 'https://project.dev.readthedocs.io/en/latest/',
)
self.assertEqual(r['Cache-Tag'], 'project')
self.assertEqual(r['X-RTD-Project'], 'project')
self.assertEqual(r['X-RTD-Project-Method'], 'subdomain')
self.assertEqual(r['X-RTD-Domain'], 'project.dev.readthedocs.io')
self.assertIsNone(r.get('X-RTD-Version'))
self.assertIsNone(r.get('X-RTD-Path'))

def test_serve_headers(self):
r = self.client.get('/en/latest/', HTTP_HOST='project.dev.readthedocs.io')
self.assertEqual(r.status_code, 200)
self.assertEqual(r['Cache-Tag'], 'project,project-latest')
self.assertEqual(r['X-RTD-Domain'], 'project.dev.readthedocs.io')
self.assertEqual(r['X-RTD-Project'], 'project')
self.assertEqual(r['X-RTD-Project-Method'], 'subdomain')
self.assertEqual(r['X-RTD-Version'], 'latest')
self.assertEqual(r['X-RTD-version-Method'], 'path')
self.assertEqual(r['X-RTD-Path'], '/proxito/media/html/project/latest/index.html')

def test_404_headers(self):
r = self.client.get('/foo/bar.html', HTTP_HOST='project.dev.readthedocs.io')
self.assertEqual(r.status_code, 404)
self.assertEqual(r['Cache-Tag'], 'project')
self.assertEqual(r['X-RTD-Domain'], 'project.dev.readthedocs.io')
self.assertEqual(r['X-RTD-Project'], 'project')
self.assertEqual(r['X-RTD-Project-Method'], 'subdomain')
self.assertEqual(r['X-RTD-version-Method'], 'path')
self.assertIsNone(r.get('X-RTD-Version'))
self.assertIsNone(r.get('X-RTD-Path'))

def test_custom_domain_headers(self):
hostname = 'docs.random.com'
self.domain = fixture.get(
Domain,
project=self.project,
domain=hostname,
)
r = self.client.get("/en/latest/", HTTP_HOST=hostname)
self.assertEqual(r.status_code, 200)
self.assertEqual(r['Cache-Tag'], 'project,project-latest')
self.assertEqual(r['X-RTD-Domain'], self.domain.domain)
self.assertEqual(r['X-RTD-Project'], self.project.slug)
self.assertEqual(r['X-RTD-Project-Method'], 'cname')
self.assertEqual(r['X-RTD-Version'], 'latest')
self.assertEqual(r['X-RTD-version-Method'], 'path')
self.assertEqual(r['X-RTD-Path'], '/proxito/media/html/project/latest/index.html')
1 change: 1 addition & 0 deletions readthedocs/proxito/tests/test_redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,4 @@ def test_proper_page_on_subdomain(self):
r['Location'],
'https://project.dev.readthedocs.io/en/latest/test.html',
)

20 changes: 1 addition & 19 deletions readthedocs/proxito/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,25 +120,7 @@ def _serve_docs_nginx(self, request, final_project, version_slug, path, download
filename = f'{domain}-{final_project.language}-{version_slug}.{filename_ext}'
response['Content-Disposition'] = f'filename={filename}'

# Add debugging headers to proxito responses
response['X-RTD-Domain'] = request.get_host()
response['X-RTD-Project'] = final_project.slug
response['X-RTD-Version'] = version_slug
# Needed to strip any GET args, etc.
response['X-RTD-Path'] = urlparse(path).path
# Include the project & project-version so we can do larger purges if needed
response['Cache-Tag'] = f'{final_project.slug}-{version_slug},{final_project.slug}'
if hasattr(request, 'rtdheader'):
response['X-RTD-Project-Method'] = 'rtdheader'
elif hasattr(request, 'subdomain'):
response['X-RTD-Project-Method'] = 'subdomain'
elif hasattr(request, 'cname'):
response['X-RTD-Project-Method'] = 'cname'
if hasattr(request, 'external_domain'):
response['X-RTD-Version-Method'] = 'domain'
else:
response['X-RTD-Version-Method'] = 'path'

response.proxito_path = urlparse(path).path
return response

def _serve_401(self, request, project):
Expand Down
3 changes: 3 additions & 0 deletions readthedocs/proxito/views/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,7 @@ def _get_project_data_from_request(
# * Subproject
# * Translations

# Set the version slug on the request so we can log it in middleware
request.path_version_slug = version_slug

return final_project, lang_slug, version_slug, filename