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

Client.logout() also cancels any existing force_authenticate. #2218

Closed
gaffney opened this issue Dec 6, 2014 · 5 comments · Fixed by #2259
Closed

Client.logout() also cancels any existing force_authenticate. #2218

gaffney opened this issue Dec 6, 2014 · 5 comments · Fixed by #2259

Comments

@gaffney
Copy link
Contributor

gaffney commented Dec 6, 2014

Hello

Currently logout() does not technically "log out" a user in a test if force_authenticate is enabled.

I think it would make more sense if the forced authentication was cancelled by default since I think it is more common that "logout" called from within a test would subsequently be testing unauthenticated behavior. (In our case, all tests use force_authenticate by default).

Current implementation:

def logout(self):
  self._credentials = {}
  return super(APIClient, self).logout()

Proposed implementation:

def logout(self, cancel_force_authenticate=True):
  self._credentials = {}

  if cancel_force_authenticate:
    self.handler._force_user  = None
    self.handler._force_token = None

  return super(APIClient, self).logout()

P.S. I would have made a PR for this but you might not agree with the change so I am just making an issue to suggest it.

Regards,

-- Ryan

@tomchristie
Copy link
Member

tomchristie commented Dec 6, 2014

Could you also include an example of how the behaviour changes? That'd be super helpful for review.

@gaffney
Copy link
Contributor Author

gaffney commented Dec 7, 2014

For instance, we have a base test class that looks something like this:

class BaseTestCase(TestCase):

  def setUp(self):
    logging.disable(logging.CRITICAL)
    # ...
    self.user   = self.create_user()
    self.client = APIClient(enforce_csrf_checks=True)
    self.client.force_authenticate(self.user)
    # ...
    self.client.login(username=user.username, password=self.password)

  # ...

And a test that tests redirect in the home view:

class HomeViewTest(BaseTestCase):

  # ...

  def test_redirect_to_next(self):
    self.client.logout()

    response = self.client.get('/?next=settings/notifications')
    self.assertEqual(response.status_code, 302)
    self.assertEqual(response.url,
      'http://testserver' + settings.APP_URL + 'splash/#/?next=settings/notifications')

With rest_framework as-is, this test will fail because the .get call succeeds (200 OK) even though we have logged the user out. The change proposed forces the logout to work by default.

I could be wrong but I would guess it is much more common to authenticate by default in a test, which is why I suggested the above. Also I am aware that I can alternatively call self.client.force_authenticate() instead of logout, however, I feel it makes sense for logout to have the behavior described above so that it's expected behavior takes precedence over force_authenticate... at least by default.

@tomchristie
Copy link
Member

tomchristie commented Dec 8, 2014

I don't want to see the cancel_force_authenticate=True flag - just have the behavior always.
Otherwise you're welcome to tackle a pull request for it - no 100% promises here, but it does look reasonable to me.
Keep the unit tests as minimal as possible, so that it's easy to review the behavior change.

@tomchristie tomchristie changed the title Questionable behavior with logout in APIClient when force_authenticate is enabled Client.logout() should also cancel any previous .force_authenticate() Dec 8, 2014
@jpadilla
Copy link
Member

jpadilla commented Dec 8, 2014

I second the removal of cancel_force_authenticate let's just make it the default behavior which sounds sane.

@tomchristie
Copy link
Member

tomchristie commented Dec 8, 2014

Thanks, always helpful to have a second review :)

@tomchristie tomchristie modified the milestone: 3.0.2 Release Dec 11, 2014
@tomchristie tomchristie changed the title Client.logout() should also cancel any previous .force_authenticate() Client.logout() also cancels any existing force_authenticate. Dec 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants