Page MenuHomePhabricator

Don't require users be logged in to access the Logout controller, so users with no Spaces can log out

Authored by epriestley on Jun 13 2019, 11:59 PM.



Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support.

Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out.

For users who are already logged out, this just redirects home with no effect.

Test Plan
  • As a user with access to no Spaces, logged out. (Before: error; after: worked).
  • As a logged-out user, logged out (was redirected).
  • As a normal user, logged out (normal logout).

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jun 13 2019, 11:59 PM
epriestley requested review of this revision.Jun 14 2019, 12:00 AM
amckinley accepted this revision.Jun 17 2019, 8:22 PM
amckinley added inline comments.


This revision is now accepted and ready to land.Jun 17 2019, 8:22 PM
epriestley added inline comments.Jun 17 2019, 8:31 PM

Haha, in retrospect this is fairly confusing -- let me take another shot at it.

epriestley updated this revision to Diff 49096.Jun 17 2019, 8:37 PM

Explain the behavior more clearly -- there are three classes of users:

  1. Logged in, normal users.
  2. Logged in users with no access to Spaces.
  3. Logged out users.

If we require login, users in bucket (1) can log out but users in bucket (2) get a "No Access To Spaces" error.

If we don't require login, users in buckets (1) and (2) can log out, since the "No Access To Spaces" error only cares about login-required pages.

Users in bucket (3) also get to log out, which is silly/pointless (they are already logged out), but not harmful.

epriestley updated this revision to Diff 49097.Jun 17 2019, 8:39 PM
  • Rebase to pick up the unit test fix.