Skip to content

Conversation

itsdkey
Copy link
Contributor

@itsdkey itsdkey commented Feb 13, 2021

Please describe your pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue. When linking to an issue, please use refs #... in the description of the pull request.

Hi there,

I was checking out bugs that are issued in DRF and I came across #5127 is somehow still valid so I started investigating.

With the sample code that was provided by @zemanel, I wasn't able to reproduce the error but that didn't stop me because according to @uliSchuster it is still valid.

But after my research, I am pretty sure that this isn't reproducible and it isn't a bug (in the newest DRF). I've checked it out on @uliSchuster requirements as well and I got the same result. And now I'm going to try to explain this.

To get this TypeError I had to remove the IsAuthenticated permission class from the view's permission classes. The response is built thanks to the rest_framework/views.py(497) method which calls the initial method.

In the initial method, we call check_permissions which iterates over the permission classes (in insertion order) and validates if the request can access the resource. But if we use the IsAuthenticated permission class then we will get a permission_denied. So to reproduce this error I needed to delete that particular class usage and now I get the error that was reported in the issue.

After getting the error I wanted I wrote two simple tests to reproduce this error (I'm not sure if I put the code in the right places - is it correct with the framework convention, please correct me if it's wrong) which are in this pull request.

After that, I can say that this bug isn't a valid bug at all because thanks to self.check_permissions we will get an error that will be passed as our data dictionary and used further while rendering our response (I've checked the flow after getting our 403 error and for safety, I've put a simple breakpoint in the get_queryset method which wasn't called at all).

We can reproduce this error if we change the permission_classes, but this ends up with an improperly configured view in my opinion. So after all of that, I think this can be closed.

But if in @uliSchuster opinion this is still valid, please give me the whole scenario in which it will be raised (by scenario I mean code :) )

PS. thanks @busla for your test, I based mine on yours from #6592

PS PS. I see that the builds don't pass but the issue is the same as on the master branch no module named 'six'. But even so, this Pull Requests doesn't need to be merged, because it just shows that the #5127 bug isn't reproducible

Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

you can just rebase to see working build

@itsdkey
Copy link
Contributor Author

itsdkey commented Feb 22, 2021

@auvipy - I've rebased it. Now the tests are working :)

@stale
Copy link

stale bot commented Apr 30, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 30, 2022
@lovelydinosaur lovelydinosaur merged commit e7af8d6 into encode:master Jun 8, 2022
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants