-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Use our standard auth mixin for proxito downloads #6572
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
Conversation
This is the same logic as in the Proxito views, it just extends it to the downloads.
readthedocs/projects/views/public.py
Outdated
if not self.allowed_user(request, final_project, version_slug): | ||
return self.get_unauthed_response(request, final_project) | ||
|
||
version = final_project.versions.public(user=request.user).filter(slug=version_slug).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to raise a 404 here if version
is None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True -- I will re-add that logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
We should probably have a test case that would catch that no-404 issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code level perspective, changes look good. The logic around the same domain methods are foreign to me, but these changes seem to be working. This might be something to come back and refactor, but fixes the release issues we're having for now.
* Use our standard auth mixin for proxito downloads This is the same logic as in the Proxito views, it just extends it to the downloads. * re-add 404 logic * Add a bit of janky hacks * Remove functions to remove hacky isinstance * Fix lint
def allowed_user(self, *args, **kwargs): | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to default this to False
to avoid mistakes if it's not overwritten in the class that inherit from this. Forcing the class that inherit from this to re-define it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this worried me a bit, we should fix it in another PR.
This is the same logic as in the Proxito views,
it just extends it to the downloads.