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.

Sign up for GitHub

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

Jump to bottom

Fix #5016 - Views mutate user object when using force_request #5066

Closed
wants to merge 1 commit into from
Closed

Fix #5016 - Views mutate user object when using force_request #5066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions rest_framework/test.py
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
# to make it harder for the user to import the wrong thing without realizing.
from __future__ import unicode_literals

import copy
import io

from django.conf import settings
Expand All @@ -22,7 +23,7 @@


def force_authenticate(request, user=None, token=None):
request._force_auth_user = user
request._force_auth_user = copy.deepcopy(user)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little concerned about the use of deepcopy here. It's pretty much magic, and there's no guarantee that it might not fail in some cases (user can be a custom model, or have been modified by middleware, and might have almost anything stored in state on it)

I'm not sure what the best action is here. Perhaps we should catch exceptions and fall back to simply assigning user, tho that seems problematic too.

Perhaps I'm being overly cautious about the possibility of deepcopy failing here?

Copy link
Member

@rpkilby rpkilby May 3, 2017 •
edited

Choose a reason for hiding this comment

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

I've ran into some weird issues recently with deepcopy in regards to the request object. Although it'd be unusual, it's not inconceivable that the request would be somehow accessible from the user via an authenticator or middleware, which deepcopy would pick up. I'm also hesitant about deepcopy here.

Although, if this is happening before any middleware or authenticator machinery, then it's probably fine?

request._force_auth_token = token


Expand Down Expand Up @@ -268,7 +269,7 @@ def force_authenticate(self, user=None, token=None):
Forcibly authenticates outgoing requests with the given
user and/or token.
"""
self.handler._force_user = user
self.handler._force_user = copy.deepcopy(user)
self.handler._force_token = token
if user is None:
self.logout() # Also clear any possible session info if required
Expand Down