Skip to content

Commit

Permalink
Addons: small improvements and privacy considerations (#11561)
Browse files Browse the repository at this point in the history
- We had a special case in a permissions class, I moved this into the view itself.
- We are allowing some requests to not have a version, so our normal permission check doesn't work in this case. In case that we don't have a version, we just check for the project permission.
- If we have a version, we are attaching its latest build, this code was duplicated, and wasn't taking into considerations permissions. In case of temporal sharing tokens, they don't have access to builds.
- Translations now take into consideration the current user permissions
- The list of active versions can now be overridden from .com to include versions granted by a temporal access.

Some notes:

- We are returning some custom 404 responses, but DRF already handles that when using get_object_or_404. Anyway, users won't see any of the messages, since our 404 handler is catching those in production.
- Returning the full serialized version of translations takes like 40 extra queries, probably it generates more per extra translation. From what I found, most of the queries are due to the serializer returning more related projects fully serialized (main translation, superproject).
- We are allowing users to override the whole response, we should just allow to override the `addons` key.
- This also suffers from readthedocs/readthedocs-corporate#1845

Since to really run the whole code from permissions checks organizations are needed, I'll add tests on .com. 

ref readthedocs/readthedocs-corporate#1773
  • Loading branch information
stsewd authored Oct 1, 2024
1 parent 70cefc0 commit 4834be4
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 100 deletions.
11 changes: 0 additions & 11 deletions readthedocs/api/v2/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,6 @@ class IsAuthorizedToViewVersion(permissions.BasePermission):
def has_permission(self, request, view):
project = view._get_project()
version = view._get_version()

# I had to add this condition here because I want to return a 400 when
# the `project-slug` or `version-slug` are not sent to the API
# endpoint. In those cases, we don't have a Project/Version and this
# function was failing.
#
# I think it's a valid use case when Project/Version is invalid to be
# able to return a good response from the API view.
if project is None or version is None:
return True

has_access = (
Version.objects.public(
user=request.user,
Expand Down
20 changes: 11 additions & 9 deletions readthedocs/proxito/tests/test_hosting.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
SINGLE_VERSION_WITHOUT_TRANSLATIONS,
)
from readthedocs.projects.models import AddonsConfig, Domain, Project
from readthedocs.proxito.views.hosting import ClientError


@override_settings(
Expand Down Expand Up @@ -284,6 +283,7 @@ def test_flyout_translations(self):
slug="translation",
main_language_project=self.project,
language="ja",
privacy_level=PUBLIC,
)
translation_ja.versions.update(
built=True,
Expand Down Expand Up @@ -530,20 +530,24 @@ def test_flyout_subproject_urls(self):
slug="translation",
language="es",
repo="https://github.com/readthedocs/subproject",
privacy_level=PUBLIC,
)
translation.versions.update(
built=True,
active=True,
)
subproject = fixture.get(
Project, slug="subproject", repo="https://github.com/readthedocs/subproject"
Project,
slug="subproject",
repo="https://github.com/readthedocs/subproject",
privacy_level=PUBLIC,
)
self.project.add_subproject(subproject)
subproject.translations.add(translation)
subproject.save()

fixture.get(Version, slug="v1", project=subproject)
fixture.get(Version, slug="v2.3", project=subproject)
fixture.get(Version, slug="v1", project=subproject, privacy_level=PUBLIC)
fixture.get(Version, slug="v2.3", project=subproject, privacy_level=PUBLIC)
subproject.versions.update(
privacy_level=PUBLIC,
built=True,
Expand Down Expand Up @@ -710,9 +714,7 @@ def test_non_existent_project(self):
},
)
assert r.status_code == 404
assert r.json() == {
"error": ClientError.PROJECT_NOT_FOUND,
}
assert r.json() == {"detail": "No Project matches the given query."}

def test_number_of_queries_project_version_slug(self):
# The number of queries should not increase too much, even if we change
Expand All @@ -734,7 +736,7 @@ def test_number_of_queries_project_version_slug(self):
active=True,
)

with self.assertNumQueries(23):
with self.assertNumQueries(22):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand Down Expand Up @@ -825,7 +827,7 @@ def test_number_of_queries_url_translations(self):
language=language,
)

with self.assertNumQueries(56):
with self.assertNumQueries(60):
r = self.client.get(
reverse("proxito_readthedocs_docs_addons"),
{
Expand Down
Loading

0 comments on commit 4834be4

Please sign in to comment.