BUG Refactor LoginSessionController to use sensible REST request and …

…to allow all users to invalidate their session

_config/routes.yml

@@ -4,4 +4,4 @@ After: '#coreroutes'
 ---
 SilverStripe\Control\Director:
   rules:
-    'loginsession//remove/$ID': SilverStripe\SessionManager\Controllers\LoginSessionController
+    'loginsession//$ID': SilverStripe\SessionManager\Controllers\LoginSessionController

client/src/components/LoginSession/LoginSessionContainer.js

@@ -34,27 +34,32 @@ function LoginSessionContainer(props) {
       id: props.ID,
       SecurityID: Config.get('SecurityID')
     })
-      .then(response => {
-        const failure = !response.success;
-        setFailed(failure);
-
-        if (failure) {
-          props.displayToastFailure(response.message);
-        } else {
-          props.displayToastSuccess(response.message);
+    .then(response => {
+      props.displayToastSuccess(response.message);
+    })
+    .catch(err => {
+      setFailed(true);
+      return err.response.text().then(json => {
+        // Try to parse the error response
+        const response = JSON.parse(json);
+        if (typeof response !== 'object' || typeof response.message !== 'string') {
+          return Promise.reject('No readable error message');
         }
-      })
-      .catch(() => {
-        setFailed(true);
-        props.displayToastFailure(i18n._t(
-          'SessionManager.COULD_NOT_LOGOUT',
-          'Could not log out of session. Try again later.'
-        ));
-      })
-      .finally(() => {
-        setComplete(true);
-        setSubmitting(false);
+        props.displayToastFailure(response.message);
+        return Promise.resolve();
       });
+    })
+    .catch(() => {
+      // Catch all error handler
+      props.displayToastFailure(i18n._t(
+        'SessionManager.COULD_NOT_LOGOUT',
+        'Could not log out of session. Try again later.'
+      ));
+    })
+    .finally(() => {
+      setComplete(true);
+      setSubmitting(false);
+    });
   }
 
   const { ID, ...loginSessionProps } = props;

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

@@ -74,11 +74,7 @@ describe('LoginSessionContainer', () => {
       submitting: true
     });
 
-    httpResolve({
-      error: false,
-      success: true,
-      message: 'amazing success'
-    });
+    httpResolve({ message: 'amazing success' });
     await logoutRequest;
 
     // Test final state
@@ -118,10 +114,9 @@ describe('LoginSessionContainer', () => {
       submitting: true
     });
 
-    httpResolve({
-      error: true,
-      success: false,
-      message: 'horrible failure'
+    httpReject({
+      response: {
+        text: () => Promise.resolve(JSON.stringify({ message: 'horrible failure' })) }
     });
     await logoutRequest;
 
@@ -163,7 +158,9 @@ describe('LoginSessionContainer', () => {
     });
 
     // Cause an HTTP Failure
-    httpReject();
+    httpReject({
+      response: { text: () => Promise.resolve('Horrible HTTP Failure') }
+    });
     await logoutRequest;
 
     // Test error state

lang/en.yml

@@ -1,8 +1,9 @@
 en:
   SilverStripe\SessionManager\Controllers\LoginSessionController:
-    REMOVE_FAILURE: 'Something went wrong.'
-    REMOVE_PERMISSION: 'You do not have permission to delete this record.'
+    INVALID_REQUEST: 'Invalid request'
     REMOVE_SUCCESS: 'Successfully removed session.'
+    SESSION_COULD_NOT_BE_FOUND_OR_NO_LONGER_ACTIVE: 'This session could not be found or is no longer active.'
+    YOUR_SESSION_HAS_EXPIRED: 'Your session has expired'
   SilverStripe\SessionManager\Extensions\MemberExtension:
     LEARN_MORE: 'Learn more'
     LOGIN_SESSIONS: 'Login sessions'

src/Controllers/LoginSessionController.php

@@ -6,17 +6,22 @@
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware;
+use SilverStripe\Security\Security;
 use SilverStripe\Security\SecurityToken;
 use SilverStripe\SessionManager\Models\LoginSession;
 
+/**
+ * Handles session revocation AJAX request
+ */
 class LoginSessionController extends Controller
 {
-    private static $url_segment = 'loginsession';
-
     private static $url_handlers = [
-        'DELETE remove/$ID' => 'remove',
+        'DELETE $ID' => 'remove',
     ];
 
+    private static $url_segment = 'loginsession';
+
     private static $allowed_actions = [
         'remove',
     ];
@@ -29,50 +34,52 @@ class LoginSessionController extends Controller
      */
     public function remove(HTTPRequest $request): HTTPResponse
     {
-        return $this->removeLoginSession($request);
-    }
-
-    private function removeLoginSession(HTTPRequest $request): HTTPResponse
-    {
-        $failureMessage = _t(__CLASS__ . '.REMOVE_FAILURE', 'Something went wrong.');
-        try {
-            // Ensure CSRF protection
-            if (!SecurityToken::inst()->checkRequest($request)) {
-                return $this->jsonResponse([
-                    'success' => false,
-                    'message' => $failureMessage
-                ]);
-            }
+        if (empty(Security::getCurrentUser())) {
+            return $this->jsonResponse(
+                [
+                    'message' => _t(
+                        __CLASS__ . '.YOUR_SESSION_HAS_EXPIRED',
+                        'Your session has expired'
+                    )
+                ],
+                401
+            );
+        }
 
-            $id = $request->param('ID');
-            $loginSession = LoginSession::get()->byID($id);
-            if (!$loginSession) {
-                return $this->jsonResponse([
-                    'success' => false,
-                    'message' => $failureMessage
-                ]);
-            }
+        // Validate CSRF token and ID parameter
+        $id = $request->param('ID');
+        if (!SecurityToken::inst()->checkRequest($request) || !is_numeric($id)) {
+            return $this->jsonResponse(
+                [
+                    'message' => _t(
+                        __CLASS__ . '.INVALID_REQUEST',
+                        'Invalid request'
+                    )
+                ],
+                400
+            );
+        }
 
-            if (!$loginSession->canDelete()) {
-                $message = _t(__CLASS__ . '.REMOVE_PERMISSION', 'You do not have permission to delete this record.');
-                return $this->jsonResponse([
-                    'success' => false,
-                    'message' => $message
-                ]);
-            }
-        } catch (Exception $e) {
-            return $this->jsonResponse([
-                'success' => false,
-                'message' => $failureMessage
-            ]);
+        $loginSession = LoginSession::get()->byID($id);
+        if (!$loginSession || !$loginSession->canDelete()) {
+            // We're not making a difference between a non-existent session and session you can not revoke
+            // to prevent an adversary from scanning LoginSession IDs
+            return $this->jsonResponse(
+                [
+                    'message' => _t(
+                        __CLASS__ . '.SESSION_COULD_NOT_BE_FOUND_OR_NO_LONGER_ACTIVE',
+                        'This session could not be found or is no longer active.'
+                    )
+                ],
+                404
+            );
         }
 
         $this->extend('onBeforeRemoveLoginSession', $loginSession);
 
         $loginSession->delete();
 
         return $this->jsonResponse([
-            'success' => true,
             'message' => _t(__CLASS__ . '.REMOVE_SUCCESS', 'Successfully removed session.')
         ]);
     }
@@ -86,6 +93,7 @@ private function removeLoginSession(HTTPRequest $request): HTTPResponse
      */
     private function jsonResponse(array $response, int $code = 200): HTTPResponse
     {
+        HTTPCacheControlMiddleware::singleton()->disableCache();
         return HTTPResponse::create(json_encode($response))
             ->addHeader('Content-Type', 'application/json')
             ->setStatusCode($code);

src/FormFields/SessionManagerField.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\SessionManager\FormFields;
 
+use SilverStripe\Control\Director;
 use SilverStripe\Forms\FormField;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Security\Member;
@@ -118,7 +119,7 @@ public function getSchemaDataDefaults()
      */
     protected function getLoginSessions(Member $member)
     {
-        $logOutEndpoint = LoginSessionController::singleton()->Link('remove');
+        $logOutEndpoint = LoginSessionController::singleton()->Link();
 
         $loginSessions = [];
         /** @var LoginSession $loginSession */

tests/behat/features/revoke.feature

@@ -6,16 +6,13 @@ Feature: See other devices and revoke their access
   So that I can revoke their access
 
   Background:
-    Given I am logged in with "ADMIN" permissions
+    Given a "group" "AUTHOR group" has permissions "Access to 'Pages' section"
+    And I am logged in with "AUTHOR" permissions
     # Create a mock login session
     And There is a login session for a second device
 
   Scenario: I can see other devices and revoke their access
-    When I go to "/admin/security"
-    # Click the ADMIN user
-    And I click on the ".col-FirstName" element
-    # Ensure XHR loaded from endpoint
-    And I wait until I see the ".login-session  .text-success" element
+    When I go to "/admin/myprofile"
     # Assert text for the two login sessions
     Then I should see the text "Current" in the ".login-session  .text-success" element
     Then I should see the text "Log out" in the ".login-session__logout" element

tests/behat/src/FixtureContext.php

@@ -56,7 +56,6 @@ private function createLoginSession(): LoginSession
     private function getMember(): Member
     {
         /** @var Member $member */
-        $member = Member::get()->find('FirstName', 'ADMIN');
-        return $member;
+        return Member::get()->find('FirstName', 'AUTHOR');
     }
 }