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

Ensure request.user is available to response middleware. #2155

Merged
merged 3 commits into from Dec 17, 2014
Merged

Ensure request.user is available to response middleware. #2155

merged 3 commits into from Dec 17, 2014

Conversation

martinmaillard
Copy link
Contributor

@martinmaillard martinmaillard commented Nov 28, 2014

Because of the way DRF handles authentication, middleware classes don't have access to the currently authenticated user. If we set the user on the wrapped request during authentication, middleware classes can at least access it in the process_response method.

I'm not aware of all the implications of doing that, so this PR is mostly meant to start a discussion.

I use DRF in several projects and I met this issue at least twice while trying to integrate with other libraries. I realize there is a reason why the authentication is not handled in DRF as it is in django, but I think it is worth exploring possible solutions to mitigate the issues that it can cause.

@tomchristie
Copy link
Member

tomchristie commented Nov 28, 2014

I think it is worth exploring possible solutions to mitigate the issues that it can cause.

Very well explained and yes I think some forward thinking on this is worthwhile.

Not able to do this comment justice right now, but there are some future horizon questions there. Eg do we consider DRF as middleware which could solve the issue, do we look at bringing some if this back into Django core etc.

Those sorts of things are likely far off, but def worth opening the conversation on.

Thanks!

@jpadilla
Copy link
Member

jpadilla commented Nov 29, 2014

I've had this reported before multiple times on django-rest-framework-jwt. Last one with a "workaround" was this one.

@tomchristie
Copy link
Member

tomchristie commented Dec 11, 2014

On further review I don't see any problem with the pull request as it stands. Anyone else?

@tomchristie
Copy link
Member

tomchristie commented Dec 11, 2014

Probably we need a test case to demonstrate the change in behavior tho.

@tomchristie tomchristie added this to the 3.0.2 Release milestone Dec 11, 2014
@tomchristie
Copy link
Member

tomchristie commented Dec 11, 2014

We might want to consider something similar for .data and .files.

@tomchristie tomchristie self-assigned this Dec 11, 2014
@martinmaillard
Copy link
Contributor Author

martinmaillard commented Dec 11, 2014

I added a test. Let me know if this seems ok.

@jpadilla
Copy link
Member

jpadilla commented Dec 11, 2014

Yeah this looks to me as is.

def test_logged_in_user_is_set_on_wrapped_request(self):
login(self.request, self.user)
self.assertEqual(self.wrapped_request.user, self.user)

Copy link
Member

@tomchristie tomchristie Dec 12, 2014

Choose a reason for hiding this comment

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

I was actually hoping for a test demonstrating the change this has against middleware.
The test as it is here demonstrates the implementation, but doesn't make the motivation clear.

Copy link
Member

@jpadilla jpadilla Dec 12, 2014

Choose a reason for hiding this comment

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

Right, perhaps write a simple Middleware that you can unittest process_response() and demonstrate being able to access the expected user there?

@martinmaillard
Copy link
Contributor Author

martinmaillard commented Dec 16, 2014

I wrote a test using a simple Middleware. I'm not so sure about the way I did it though. I'm going to need your guidance.

  • Where should I put the test? I put it in a separate module for now, but it should probably still be in test_request, right?
  • The assertions are not in the test method, but in the middleware. This is probably not a good idea but I could not come up with another solution.

@kevin-brown
Copy link
Member

kevin-brown commented Dec 16, 2014

Where should I put the test? I put it in a separate module for now, but it should probably still be in test_request, right?

Sounds right.

The assertions are not in the test method, but in the middleware. This is probably not a good idea but I could not come up with another solution.

I'm not personally a huge fan of this, but I can't see an easy way to test this other than pushing the data onto the response and testing the response.

@maryokhin
Copy link
Contributor

maryokhin commented Dec 16, 2014

I haven't tested this, so it's just a guess. But have you tried going through the middleware stack manually?

middleware = MyMiddleware()
factory = RequestFactory()
request = factory.get('/', HTTP_AUTHORIZATION=auth)
response = middleware.process_request(request)
middleware.process_response(request, response)
self.assertEquals(request.user, user)
self.assertTrue(request.user.is_authenticated())

@jpadilla
Copy link
Member

jpadilla commented Dec 16, 2014

@maryokhin yeah that sounds about the right approach.

@martinmaillard
Copy link
Contributor Author

martinmaillard commented Dec 17, 2014

The goal of this test is to document the fact that the middleware can access the authenticated user in process_response. Your approach would be perfect if we were testing the middleware class itself, but it's not testing anything in this case.

My first instinct was to push the user onto the response and test the response in the test case, as suggested by @kevin-brown, but I think it just makes it harder to understand.

@jpadilla
Copy link
Member

jpadilla commented Dec 17, 2014

I think either ways it currently works. @tomchristie you might want to take another look at this and perhaps just merge as is. Thoughts?

tomchristie added a commit that referenced this issue Dec 17, 2014
@tomchristie tomchristie merged commit 7fbf5b0 into encode:master Dec 17, 2014
1 check passed
@tomchristie
Copy link
Member

tomchristie commented Dec 17, 2014

Go go go.
Thanks for your hard work @martinmaillard!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants