Page MenuHomePhabricator

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

Authored by epriestley on Jun 13 2019, 11:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 12:41 PM
Unknown Object (File)
Sat, Mar 23, 12:41 PM
Unknown Object (File)
Sat, Mar 23, 12:41 PM
Unknown Object (File)
Sat, Mar 23, 12:41 PM
Unknown Object (File)
Sat, Mar 23, 12:41 PM
Unknown Object (File)
Thu, Mar 21, 1:10 AM
Unknown Object (File)
Mon, Mar 18, 1:03 AM
Unknown Object (File)
Sun, Mar 17, 9:06 PM
Subscribers
None

Details

Summary

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

Repository
rP Phabricator
Branch
logout1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22971
Build 31527: Run Core Tests
Build 31526: arc lint + arc unit

Unit TestsFailed

TimeTest
1 msMetaMTAEmailTransactionCommandTestCase::testGetAllTypes
EXCEPTION (Exception): msort() was passed a method ("getRevisionActionOrderVector") which returns "PhutilSortVector" objects. Use "msortv()", not "msort()", to sort a list which produces vectors. #0 /core/data/drydock/workingcopy-86/repo/phabricator/src/applications/differential/command/DifferentialActionEmailCommand.php(59): msort(Array, 'getRevisionActi...') #1 [internal function]: DifferentialActionEmailCommand->getCommandObjects()
1 msAlmanacNamesTestCase::testServiceOrDeviceNames
30 assertions passed.
0 msAlmanacServiceTypeTestCase::testGetAllServiceTypes
1 assertion passed.
0 msAphrontHTTPSinkTestCase::testHTTPHeaderNames
2 assertions passed.
0 msAphrontHTTPSinkTestCase::testHTTPSinkBasics
3 assertions passed.
View Full Test Results (1 Failed · 369 Passed)

Event Timeline

amckinley added inline comments.
src/applications/auth/controller/PhabricatorLogoutController.php
7–26

wat

This revision is now accepted and ready to land.Jun 17 2019, 8:22 PM
src/applications/auth/controller/PhabricatorLogoutController.php
7–26

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

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.

  • Rebase to pick up the unit test fix.