Skip to content

Use ServeDocsPermissionsMixin to isolate this methods and be more clear #6408

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
35 changes: 35 additions & 0 deletions readthedocs/proxito/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,38 @@ def get_redirect_response(self, request, redirect_path, http_status):
return HttpResponsePermanentRedirect(new_path)

return HttpResponseRedirect(new_path)


class ServeDocsPermissionsMixin:

"""
Mixin to handle documentation permissions.

To use this mixin, the method that serve the docs should call
``allowed_user`` first to check if the user requesting to read the page is
allowed. If allowed, the flow should continue.

On the other hand, if the user is not allowed, the serve docs method MUST
break the flow and return a call to ``get_unauthed_response``.
"""

def allowed_user(self, request, project, version_slug):
"""
Return whether the user is allowed to read this documentation's version.

:returns: ``True`` if allowed. ``False`` otherwise.
:rtype: bool
"""
return False

def get_unauthed_response(self, request, project):
"""
Return response for an unauthed hit at a documentation's version.

When a user does not have access to read documentation, we need to
return a response showing the proper message. This method produces that
reponse.

:rtype: django.http.HttpResponse
"""
return self._serve_401(request, project)
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand what this is meant to be doing. Are we using these additional methods, or are they just meant to be overwritten?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are overriding these methods from corporate. They are meant to be overwritten.

This PR just move them outside the main class and put them into a mixin with better docstrings. Which is easier to understand where they come from. That's all.

I could move the docstrings to the Base class, otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

@humitos I think I ended up doing most of this PR in #6572 -- we should get this merged and combined with that one 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@ericholscher see https://github.com/readthedocs/readthedocs.org/pull/6572/files#r371770894

I'm fine with either. I think defaulting to False and force re-define it, it's safer, though.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it should default False 👍

Copy link
Member

Choose a reason for hiding this comment

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

This _serve_401 method doesn't exist on this class. It shouldn't be called if we aren't implementing it.

15 changes: 12 additions & 3 deletions readthedocs/proxito/views/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@
from readthedocs.projects import constants
from readthedocs.projects.templatetags.projects_tags import sort_version_aware

from .mixins import ServeDocsMixin, ServeRedirectMixin
from .mixins import (
ServeDocsMixin,
ServeRedirectMixin,
ServeDocsPermissionsMixin,
)

from .decorators import map_project_slug
from .redirects import redirect_project_slug
Expand All @@ -30,7 +34,12 @@
log = logging.getLogger(__name__) # noqa


class ServeDocsBase(ServeRedirectMixin, ServeDocsMixin, View):
class ServeDocsBase(
ServeRedirectMixin,
ServeDocsPermissionsMixin,
ServeDocsMixin,
View,
):

def get(self,
request,
Expand Down Expand Up @@ -108,7 +117,7 @@ def get(self,
path=path,
)

def allowed_user(self, *args, **kwargs):
def allowed_user(self, *args, **kwargs): # noqa
return True


Expand Down