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

Serve non-html at documentation domain though El Proxito #6419

Merged
merged 39 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
6c2b187
Serve non-html at documentation domain
humitos Nov 28, 2019
d4b1275
Remove "Downloads" link to dashboard from footer response
humitos Nov 28, 2019
33735f4
Add /media/ (storage bucket) to proxito test URLs (X-Accel-Redirect)
humitos Nov 28, 2019
97b5667
Serve robots.txt using the storage backend
humitos Nov 28, 2019
1a82333
Use a custom storage for tests
humitos Nov 28, 2019
65567e9
Adapt all APIv3 tests for the new URLs
humitos Dec 2, 2019
d622579
Update documentation with the new URLs
humitos Dec 2, 2019
3f669bf
Use BuildMediaFileSystemStorageTest on required tests only
humitos Dec 2, 2019
b6fd7fd
Re-add URL to serve non-html files from dashboard
humitos Dec 2, 2019
6b97b56
Do not define /_/downloads/... URL in main site
humitos Dec 2, 2019
284286b
Remove name= attribute from "project_download_media" URL
humitos Dec 2, 2019
44acf5b
Move test for download pdf file to el proxito
humitos Dec 2, 2019
7bc611d
Skip test for URL without name= attribute defined
humitos Dec 2, 2019
bd5829d
Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…
humitos Dec 2, 2019
809e4df
Style
humitos Dec 2, 2019
7d09d9c
Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…
humitos Dec 4, 2019
f4d1842
Proxy to the storage directly
humitos Dec 4, 2019
7926e57
Revert "Skip test for URL without name= attribute defined"
humitos Dec 4, 2019
f5a2fc4
Do not register `/_/downloads/` URL in web instance
humitos Dec 4, 2019
2805709
Update note about URLconf
humitos Dec 4, 2019
3d4e897
Lint
humitos Dec 4, 2019
b40aad2
Re-add comment about nginx
humitos Dec 4, 2019
43ff8b8
Skip no named URL on tests
humitos Dec 5, 2019
ec8cdb0
Make all non-html fields true when testing APIv3 responses
humitos Dec 5, 2019
6c9a5b6
Revert "Remove "Downloads" link to dashboard from footer response"
humitos Dec 5, 2019
f0ca138
Refactor get_production_media_url
humitos Dec 5, 2019
081cbe1
Fix tests
humitos Dec 5, 2019
0ed5185
Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…
ericholscher Dec 16, 2019
3d73a36
Merge branch 'master' of github.com:readthedocs/readthedocs.org into …
humitos Jan 13, 2020
bc2e06c
Use final discussed URLs for download files
humitos Jan 13, 2020
5653a23
Add missing self in the method definition
humitos Jan 13, 2020
3502bad
Remove old test
humitos Jan 13, 2020
014643a
Remove old comment
humitos Jan 13, 2020
5e9b6c4
Add some docstrings
humitos Jan 14, 2020
be6cf14
Split URL with optional <alias> in two
humitos Jan 14, 2020
c4da1b1
Adapt tests to match the new download URLs
humitos Jan 14, 2020
e657ba4
Use same permission logic in both URLs
humitos Jan 14, 2020
6414fbd
Lint
humitos Jan 14, 2020
36ec761
More lint
humitos Jan 14, 2020
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
14 changes: 0 additions & 14 deletions dockerfiles/nginx/web.conf
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,4 @@ server {
location / {
proxy_pass http://web:8000/;
}

# Sendfile support to serve non-html files under "/dashboard/<project_slug>/downloads/<type>/<version_slug>"
location /proxito/ {
internal;
# Nginx will strip the `/proxito/` and pass just the `<type>/$proj/$ver/$filename`
# So we need to make sure we pass it to `media/` for the proper URL
proxy_pass http://storage:10000/;
proxy_set_header Host storage:10000;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Host $host;

add_header X-Served Nginx-Proxito-Sendfile always;
}
}
6 changes: 3 additions & 3 deletions docs/api/v3.rst
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,9 @@ Version detail
"type": "tag",
"last_build": "{BUILD}",
"downloads": {
"pdf": "https://readthedocs.org/projects/pip/downloads/pdf/stable/",
"htmlzip": "https://readthedocs.org/projects/pip/downloads/htmlzip/stable/",
"epub": "https://readthedocs.org/projects/pip/downloads/epub/stable/"
"pdf": "https://pip.readthedocs.io/_/downloads/pdf/pip/stable/",
"htmlzip": "https://pip.readthedocs.io/_/downloads/htmlzip/pip/stable/",
"epub": "https://pip.readthedocs.io/_/downloads/epub/pip/stable/"
},
"urls": {
"documentation": "https://pip.pypa.io/en/stable/",
Expand Down
7 changes: 6 additions & 1 deletion readthedocs/api/v3/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,12 @@ def get_downloads(self, obj):
data = {}

for k, v in downloads.items():
if k in ('htmlzip', 'pdf', 'epub'):
if k in ('html', 'pdf', 'epub'):

# Keep backward compatibility
if k == 'html':
k = 'htmlzip'
humitos marked this conversation as resolved.
Show resolved Hide resolved

data[k] = ('http:' if settings.DEBUG else 'https:') + v

return data
Expand Down
14 changes: 14 additions & 0 deletions readthedocs/api/v3/tests/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.contrib.auth.models import User
from django.core.cache import cache
from django.test import TestCase
from django.test.utils import override_settings
from django.utils.timezone import make_aware
from rest_framework.authtoken.models import Token
from rest_framework.test import APIClient
Expand All @@ -16,6 +17,12 @@
from readthedocs.redirects.models import Redirect


@override_settings(
PUBLIC_DOMAIN='readthedocs.io',
PRODUCTION_DOMAIN='readthedocs.org',
USE_SUBDOMAIN=True,
RTD_BUILD_MEDIA_STORAGE='readthedocs.rtd_tests.storage.BuildMediaFileSystemStorageTest',
)
class APIEndpointMixin(TestCase):

fixtures = []
Expand Down Expand Up @@ -97,6 +104,13 @@ def setUp(self):
versions=[],
)

# Make all non-html true so responses are complete
self.project.versions.update(
has_pdf=True,
has_epub=True,
has_htmlzip=True,
)

self.client = APIClient()

def tearDown(self):
Expand Down
10 changes: 7 additions & 3 deletions readthedocs/api/v3/tests/responses/projects-detail.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
{
"active": true,
"built": true,
"downloads": {},
"downloads": {
"epub": "https://project.readthedocs.io/_/downloads/epub/project/v1.0/",
"htmlzip": "https://project.readthedocs.io/_/downloads/htmlzip/project/v1.0/",
"pdf": "https://project.readthedocs.io/_/downloads/pdf/project/v1.0/"
},
"id": 2,
"identifier": "a1b2c3",
"last_build": {
Expand Down Expand Up @@ -38,7 +42,7 @@
"slug": "v1.0",
"type": "tag",
"urls": {
"documentation": "http://readthedocs.org/docs/project/en/v1.0/",
"documentation": "http://project.readthedocs.io/en/v1.0/",
"vcs": "https://github.com/rtfd/project/tree/v1.0/"
},
"verbose_name": "v1.0"
Expand Down Expand Up @@ -83,7 +87,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://readthedocs.org/docs/project/en/latest/",
"documentation": "http://project.readthedocs.io/en/latest/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/api/v3/tests/responses/projects-list.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://readthedocs.org/docs/project/en/latest/",
"documentation": "http://project.readthedocs.io/en/latest/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/api/v3/tests/responses/projects-list_POST.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/test-project/builds/",
"documentation": "http://readthedocs.org/docs/test-project/en/latest/",
"documentation": "http://test-project.readthedocs.io/en/latest/",
"home": "https://readthedocs.org/projects/test-project/",
"versions": "https://readthedocs.org/projects/test-project/versions/"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/subproject/builds/",
"documentation": "http://readthedocs.org/docs/project/projects/subproject/en/latest/",
"documentation": "http://project.readthedocs.io/projects/subproject/en/latest/",
"home": "https://readthedocs.org/projects/subproject/",
"versions": "https://readthedocs.org/projects/subproject/versions/"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/subproject/builds/",
"documentation": "http://readthedocs.org/docs/project/projects/subproject/en/latest/",
"documentation": "http://project.readthedocs.io/projects/subproject/en/latest/",
"home": "https://readthedocs.org/projects/subproject/",
"versions": "https://readthedocs.org/projects/subproject/versions/"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/new-project/builds/",
"documentation": "http://readthedocs.org/docs/project/projects/subproject-alias/en/latest/",
"documentation": "http://project.readthedocs.io/projects/subproject-alias/en/latest/",
"home": "https://readthedocs.org/projects/new-project/",
"versions": "https://readthedocs.org/projects/new-project/versions/"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://readthedocs.org/docs/project/en/latest/",
"documentation": "http://project.readthedocs.io/en/latest/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"translation_of": null,
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://readthedocs.org/docs/project/en/latest/",
"documentation": "http://project.readthedocs.io/en/latest/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand All @@ -73,7 +73,11 @@
"version": {
"active": true,
"built": true,
"downloads": {},
"downloads": {
"epub": "https://project.readthedocs.io/_/downloads/epub/project/v1.0/",
"htmlzip": "https://project.readthedocs.io/_/downloads/htmlzip/project/v1.0/",
"pdf": "https://project.readthedocs.io/_/downloads/pdf/project/v1.0/"
},
"id": 2,
"identifier": "a1b2c3",
"_links": {
Expand All @@ -85,7 +89,7 @@
"slug": "v1.0",
"type": "tag",
"urls": {
"documentation": "http://readthedocs.org/docs/project/en/v1.0/",
"documentation": "http://project.readthedocs.io/en/v1.0/",
"vcs": "https://github.com/rtfd/project/tree/v1.0/"
},
"verbose_name": "v1.0"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
{
"active": true,
"built": true,
"downloads": {},
"downloads": {
"epub": "https://project.readthedocs.io/_/downloads/epub/project/v1.0/",
"htmlzip": "https://project.readthedocs.io/_/downloads/htmlzip/project/v1.0/",
"pdf": "https://project.readthedocs.io/_/downloads/pdf/project/v1.0/"
},
"id": 2,
"identifier": "a1b2c3",
"_links": {
Expand All @@ -13,7 +17,7 @@
"slug": "v1.0",
"type": "tag",
"urls": {
"documentation": "http://readthedocs.org/docs/project/en/v1.0/",
"documentation": "http://project.readthedocs.io/en/v1.0/",
"vcs": "https://github.com/rtfd/project/tree/v1.0/"
},
"verbose_name": "v1.0"
Expand Down
42 changes: 28 additions & 14 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@


log = logging.getLogger(__name__)
DOC_PATH_PREFIX = getattr(settings, 'DOC_PATH_PREFIX', '')


class ProjectRelationship(models.Model):
Expand Down Expand Up @@ -607,23 +608,36 @@ def get_production_media_path(self, type_, version_slug, include_file=True):
)
return path

def get_production_media_url(self, type_, version_slug, full_path=True):
def get_production_media_url(self, type_, version_slug):
"""Get the URL for downloading a specific media file."""
try:
path = reverse(
'project_download_media',
kwargs={
'project_slug': self.slug,
'type_': type_,
'version_slug': version_slug,
},
)
except NoReverseMatch:
return ''
if full_path:
path = '//{}{}'.format(settings.PRODUCTION_DOMAIN, path)
# Use project domain for full path --same domain as docs
# (project-slug.{PUBLIC_DOMAIN} or docs.project.com)
domain = self.subdomain()

# NOTE: we can't use ``reverse('project_download_media')`` here
# because this URL only exists in El Proxito and this method is
# accessed from Web instance

if self.is_subproject:
# docs.example.com/_/downloads/<alias>/<lang>/<ver>/pdf/
path = f'//{domain}/{DOC_PATH_PREFIX}downloads/{self.alias}/{self.language}/{version_slug}/{type_}/'
else:
# docs.example.com/_/downloads/<lang>/<ver>/pdf/
path = f'//{domain}/{DOC_PATH_PREFIX}downloads/{self.language}/{version_slug}/{type_}/'

return path
humitos marked this conversation as resolved.
Show resolved Hide resolved

@property
def is_subproject(self):
"""Return whether or not this project is a subproject."""
return self.superprojects.exists()
humitos marked this conversation as resolved.
Show resolved Hide resolved

@property
def alias(self):
"""Return the alias (as subproject) for this project if it's a subproject."""
if self.is_subproject:
return self.superprojects.first().alias
humitos marked this conversation as resolved.
Show resolved Hide resolved

def subdomain(self):
"""Get project subdomain from resolver."""
return resolve_domain(self)
Expand Down
7 changes: 6 additions & 1 deletion readthedocs/projects/urls/public.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,19 @@
public.project_downloads,
name='project_downloads',
),

# NOTE: this URL is kept here only for backward compatibility to serve
# non-html files from the dashboard. The ``name=`` is removed to avoid
# generating an invalid URL by mistake (we should manually generate it
# pointing to the right place: "docs.domain.org/_/downloads/")
url(
(
r'^(?P<project_slug>{project_slug})/downloads/(?P<type_>[-\w]+)/'
r'(?P<version_slug>{version_slug})/$'.format(**pattern_opts)
),
public.ProjectDownloadMedia.as_view(),
name='project_download_media',
humitos marked this conversation as resolved.
Show resolved Hide resolved
),
humitos marked this conversation as resolved.
Show resolved Hide resolved

url(
r'^(?P<project_slug>{project_slug})/badge/$'.format(**pattern_opts),
public.project_badge,
Expand Down
32 changes: 26 additions & 6 deletions readthedocs/projects/views/public.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from readthedocs.projects.models import Project
from readthedocs.projects.templatetags.projects_tags import sort_version_aware
from readthedocs.proxito.views.mixins import ServeDocsMixin
from readthedocs.proxito.views.utils import _get_project_data_from_request

from .base import ProjectOnboardMixin
from ..constants import PRIVATE
Expand Down Expand Up @@ -269,7 +270,9 @@ def project_downloads(request, project_slug):

class ProjectDownloadMedia(ServeDocsMixin, View):

def get(self, request, project_slug, type_, version_slug):
same_domain_url = False

def get(self, request, project_slug=None, type_=None, version_slug=None, lang_slug=None, subproject_slug=None):
"""
Download a specific piece of media.

Expand All @@ -279,11 +282,10 @@ def get(self, request, project_slug, type_, version_slug):
It should only care about the Version permissions,
not the actual Project permissions.
"""
version = get_object_or_404(
Version.objects.public(user=request.user),
project__slug=project_slug,
slug=version_slug,
)
if self.same_domain_url:
version = self._version_same_domain_url(request, type_, lang_slug, version_slug, subproject_slug)
else:
version = self._version_dashboard_url(request, project_slug, type_, version_slug)

# Send media download to analytics - sensitive data is anonymized
analytics_event.delay(
Expand Down Expand Up @@ -312,6 +314,24 @@ def get(self, request, project_slug, type_, version_slug):
download=True,
)

def _version_same_domain_url(self, request, type_, lang_slug, version_slug, subproject_slug=None):
final_project, lang_slug, version_slug, filename = _get_project_data_from_request(
request,
project_slug=None,
subproject_slug=subproject_slug,
lang_slug=lang_slug,
version_slug=version_slug,
)
return final_project.versions.get(slug=version_slug)

def _version_dashboard_url(self, request, project_slug, type_, version_slug):
version = get_object_or_404(
Version.objects.public(user=request.user),
project__slug=project_slug,
slug=version_slug,
)
return version
humitos marked this conversation as resolved.
Show resolved Hide resolved


def project_versions(request, project_slug):
"""
Expand Down
19 changes: 19 additions & 0 deletions readthedocs/proxito/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

from readthedocs.constants import pattern_opts
from readthedocs.builds.constants import EXTERNAL
from readthedocs.projects.views.public import ProjectDownloadMedia
from readthedocs.proxito.views.serve import (
ServeDocs,
ServeError404,
Expand All @@ -44,8 +45,26 @@
from readthedocs.proxito.views.redirects import redirect_page_with_filename
from readthedocs.proxito.views.utils import fast_404

DOC_PATH_PREFIX = getattr(settings, 'DOC_PATH_PREFIX', '')

urlpatterns = [
# Serve downloads
url(
(
# /_/downloads/<lang>/<ver>/<type>/
# /_/downloads/<alias>/<lang>/<ver>/<type>/
r'^{DOC_PATH_PREFIX}downloads/'
r'(?:(?P<subproject_slug>{project_slug})/)?'
humitos marked this conversation as resolved.
Show resolved Hide resolved
r'(?P<lang_slug>{lang_slug})/'
r'(?P<version_slug>{version_slug})/'
r'(?P<type_>[-\w]+)/$'.format(
DOC_PATH_PREFIX=DOC_PATH_PREFIX,
**pattern_opts)
),
ProjectDownloadMedia.as_view(same_domain_url=True),
name='project_download_media',
),

# Serve custom 404 pages
url(
r'^_proxito_404_(?P<proxito_path>.*)$',
Expand Down
Loading