BUG Only allow the owner of a LoginSession to view/delete it

lang/en.yml

@@ -7,13 +7,12 @@ en:
   SilverStripe\SessionManager\Extensions\MemberExtension:
     DEVICES: 'Authenticated devices'
     LEARN_MORE: 'Learn more'
-    SESSION_MANAGER_PERMISSION_DESCRIPTION: 'Ability to view and purge active login sessions for other members. Requires the "Access to ''Security'' section" permission.'
-    SESSION_MANAGER_PERMISSION_LABEL: 'View/purge login sessions for other members'
   SilverStripe\SessionManager\Forms\GridFieldRevokeLoginSessionAction:
     DeletePermissionsFailure: 'No delete permissions'
   SilverStripe\SessionManager\Job\GarbageCollectionJob:
     TITLE: 'Session manager garbage collection'
   SilverStripe\SessionManager\Model\LoginSession:
+    BROWSER_ON_OS: '{browser} on {os}.'
     PLURALNAME: 'Login Sessions'
     PLURALS:
       one: 'A Login Session'

src/Extension/MemberExtension.php

@@ -2,22 +2,26 @@
 
 namespace SilverStripe\SessionManager\Extensions;
 
-use SilverStripe\Control\Session;
-use SilverStripe\Core\Extension;
 use SilverStripe\Forms\FieldList;
-use SilverStripe\Forms\GridField\GridField;
-use SilverStripe\Forms\GridField\GridFieldConfig_Base;
-use SilverStripe\ORM\FieldType\DBDatetime;
-use SilverStripe\Security\Permission;
-use SilverStripe\Security\PermissionProvider;
-use SilverStripe\Security\Security;
+use SilverStripe\ORM\DataExtension;
+use SilverStripe\Security\Member;
 use SilverStripe\SessionManager\FormField\SessionManagerField;
-use SilverStripe\SessionManager\Forms\GridFieldRevokeLoginSessionAction;
 use SilverStripe\SessionManager\Model\LoginSession;
 
-class MemberExtension extends Extension implements PermissionProvider
+/**
+ * Augment `Member` to allow relationship to the LoginSession DataObject
+ * @method LoginSession[]|DataList LoginSessions()
+ * @method Member getOwner()
+ */
+class MemberExtension extends DataExtension
 {
-    public const SESSION_MANAGER_ADMINISTER_SESSIONS = 'SESSION_MANAGER_ADMINISTER_SESSIONS';
+    /**
+     * URL to the user help abot managing session
+     * @var string
+     * @config
+     */
+    private static $session_login_help_url =
+        'https://userhelp.silverstripe.org/en/4/optional_features/managing_devices';
 
     /**
      * @var array
@@ -31,95 +35,31 @@ class MemberExtension extends Extension implements PermissionProvider
      */
     public function updateCMSFields(FieldList $fields)
     {
-        $fields->removeByName(['LoginSessions', 'SessionManagerField']);
+        $fields->removeByName('LoginSessions');
 
-        if (!$this->owner->exists() || !$this->currentUserCanViewSessions()) {
-            return $fields;
+        $member = $this->getOwner();
+        if (!$member->exists()) {
+            // We're creating a new member
+            return;
         }
 
+        $session = $member->LoginSessions()->First();
+        if (!$session || !$session->canView()) {
+            // We're assuming that the first session permission are representative of the other sessions
+            return;
+        }
+
+        $helpUrl = $member::config()->get('session_login_help_url');
+
         $fields->addFieldToTab(
             'Root.Main',
-            $sessionManagerField = SessionManagerField::create(
-                'SessionManagerField',
+            SessionManagerField::create(
+                'LoginSessions',
                 _t(__CLASS__ . '.DEVICES', 'Authenticated devices'),
                 $this->owner->ID,
-                _t(__CLASS__ . '.LEARN_MORE', 'Learn more'),
-                'http://userhelp.silverstripe.org/TODO'
-            )
-        );
-
-        if (!$this->currentUserCanEditSessions()) {
-            $sessionManagerField->setReadonly(true);
-        }
-
-        return $fields;
-    }
-
-    /**
-     * @return int
-     */
-    private function getSessionLifetime(): int
-    {
-        if ($lifetime = Session::config()->get('timeout')) {
-            return $lifetime;
-        }
-
-        return LoginSession::config()->get('default_session_lifetime');
-    }
-
-    /**
-     * Determines whether the logged in user has sufficient permission to see the SessionManager config for this Member.
-     *
-     * @return bool
-     */
-    public function currentUserCanViewSessions(): bool
-    {
-        return (Permission::check(self::SESSION_MANAGER_ADMINISTER_SESSIONS)
-            || $this->currentUserCanEditSessions());
-    }
-
-    /**
-     * Determines whether the logged in user has sufficient permission
-     * to modify the SessionManager config for this Member.
-     * Note that this is different from being able to _reset_ the config (which administrators can do).
-     *
-     * @return bool
-     */
-    public function currentUserCanEditSessions(): bool
-    {
-        return (Security::getCurrentUser() && Security::getCurrentUser()->ID === $this->owner->ID);
-    }
-
-    /**
-     * Provides the SessionManager view/reset permission for selection in the permission list in the CMS.
-     *
-     * @return array
-     */
-    public function providePermissions(): array
-    {
-        $label = _t(
-            __CLASS__ . '.SESSION_MANAGER_PERMISSION_LABEL',
-            'View/purge login sessions for other members'
+                $helpUrl ? _t(__CLASS__ . '.LEARN_MORE', 'Learn more') : '',
+                $helpUrl ?: ''
+            )->setReadonly(!$session->canDelete())
         );
-
-        $category = _t(
-            'SilverStripe\\Security\\Permission.PERMISSIONS_CATEGORY',
-            'Roles and access permissions'
-        );
-
-        $description = _t(
-            __CLASS__ . '.SESSION_MANAGER_PERMISSION_DESCRIPTION',
-            'Ability to view and purge active login sessions for other members.'
-            . ' Requires the "Access to \'Security\' section" permission.'
-        );
-
-        return [
-            self::SESSION_MANAGER_ADMINISTER_SESSIONS => [
-                'name' => $label,
-                'category' => $category,
-                'help' => $description,
-                'sort' => 200,
-            ],
-        ];
     }
 }

src/FormField/SessionManagerField.php

@@ -3,9 +3,9 @@
 namespace SilverStripe\SessionManager\FormField;
 
 use SilverStripe\Forms\FormField;
-use SilverStripe\SessionManager\Control\LoginSessionController;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Security\Member;
+use SilverStripe\SessionManager\Control\LoginSessionController;
 use SilverStripe\SessionManager\Model\LoginSession;
 use SilverStripe\View\ViewableData;
 
@@ -123,6 +123,9 @@ protected function getLoginSessions(Member $member)
         $loginSessions = [];
         /** @var LoginSession $loginSession */
         foreach (LoginSession::getCurrentSessions($member) as $loginSession) {
+            if (!$loginSession->canView()) {
+                continue;
+            }
             $loginSessions[] = [
                 'ID' => $loginSession->ID,
                 'IPAddress' => $loginSession->IPAddress,

src/Model/LoginSession.php

@@ -6,30 +6,29 @@
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\Session;
 use SilverStripe\Core\Injector\Injector;
-use SilverStripe\Core\Config\Configurable;
+use SilverStripe\ORM\DataList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\Security\Member;
-use SilverStripe\Security\Permission;
 use SilverStripe\Security\RememberLoginHash;
 use SilverStripe\Security\Security;
 use SilverStripe\SessionManager\Security\LogInAuthenticationHandler;
 use UAParser\Parser;
 
 /**
- * Class LoginSession
- * @package SilverStripe\SessionManager\Model
+ * Tracks a login session for a specific user on a specific device.
+ *
+ * Deleting the LoginSession object for a device will terminate that session for the user.
  *
  * @property DBDatetime $LastAccessed
  * @property string $IPAddress
  * @property string $UserAgent
  * @property bool $Persistent
- * @method Member Member
+ * @property integer $MemberID
+ * @method Member Member()
  */
 class LoginSession extends DataObject
 {
-    use Configurable;
-
     /**
      * @var array
      */
@@ -114,14 +113,8 @@ public function canCreate($member = null, $context = [])
             return $extended;
         }
 
-        // Must be logged in to act on login sessions
-        if (!$member) {
-            return false;
-        }
-
-        // Any user with access to SecurityAdmin can create a session
-        // @todo Does this even make sense? When would you non-programatically create a session?
-        return Permission::checkMember($member, 'CMS_ACCESS_SecurityAdmin');
+        // Only the system is allowed to create new LoginSession.
+        return false;
     }
 
     /**
@@ -139,7 +132,18 @@ public function canView($member = null)
      */
     public function canEdit($member = null)
     {
-        return $this->handlePermission(__FUNCTION__, $member);
+        if (!$member) {
+            $member = Security::getCurrentUser();
+        }
+
+        // Allow extensions to overrule permissions
+        $extended = $this->extendedCan(__FUNCTION__, $member);
+        if ($extended !== null) {
+            return $extended;
+        }
+
+        // By default, no one can edit a LoginSession because it's not an object users can directly interact with.
+        return false;
     }
 
     /**
@@ -156,7 +160,7 @@ public function canDelete($member = null)
      * @param Member $member
      * @return bool
      */
-    public function handlePermission(string $funcName, $member): bool
+    private function handlePermission(string $funcName, $member): bool
     {
         if (!$member) {
             $member = Security::getCurrentUser();
@@ -174,12 +178,12 @@ public function handlePermission(string $funcName, $member): bool
         }
 
         // Members can manage their own sessions
-        if ($this->ID == $member->ID) {
+        if ($this->MemberID === $member->ID) {
             return true;
         }
 
-        // Access to SecurityAdmin implies session management permissions
-        return Permission::checkMember($member, 'CMS_ACCESS_SecurityAdmin');
+        // By default, members can not see other members' sessions
+        return false;
     }
 
     /**
@@ -229,15 +233,18 @@ public function getFriendlyUserAgent(): string
         $parser = Parser::create();
         $result = $parser->parse($this->UserAgent);
 
-        return sprintf('%s on %s', $result->ua->family, $result->os->toString());
+        return _t(
+            __CLASS__ . '.BROWSER_ON_OS',
+            "{browser} on {os}.",
+            ['browser' => $result->ua->family, 'os' => $result->os->toString()]
+        );
     }
 
     /**
      * @param Member|null $member
      * @param HTTPRequest|null $request
-     * @return LoginSession|null
      */
-    public static function getCurrentLoginSession(Member $member = null, HTTPRequest $request = null)
+    public static function getCurrentLoginSession(Member $member = null, HTTPRequest $request = null): ?self
     {
         if (!$member) {
             $member = Security::getCurrentUser();
@@ -263,9 +270,8 @@ public static function getCurrentLoginSession(Member $member = null, HTTPRequest
     /**
      * @param Member|null $member
      * @param HTTPRequest|null $request
-     * @return static|null
      */
-    public function isCurrent(Member $member = null, HTTPRequest $request = null)
+    public function isCurrent(Member $member = null, HTTPRequest $request = null): bool
     {
         $currentLoginSession = static::getCurrentLoginSession($member, $request);
         if (!$currentLoginSession) {
@@ -277,7 +283,7 @@ public function isCurrent(Member $member = null, HTTPRequest $request = null)
 
     /**
      * @param Member $member
-     * @return mixed
+     * @return DataList|LoginSession[]
      */
     public static function getCurrentSessions(Member $member)
     {
@@ -293,7 +299,7 @@ public static function getCurrentSessions(Member $member)
     /**
      * @return int
      */
-    public static function getSessionLifetime()
+    public static function getSessionLifetime(): int
     {
         if ($lifetime = Session::config()->get('timeout')) {
             return $lifetime;

tests/php/Extension/ForcePermission.php

@@ -0,0 +1,46 @@
+<?php
+
+namespace SilverStripe\SessionManager\Tests\Extension;
+
+use SilverStripe\ORM\DataExtension;
+use SilverStripe\Dev\TestOnly;
+
+/**
+ * This extension is meant to be applied to LoginSession so we can force permission to scenarios that won't happen
+ * natively with a default install of the session manager module.
+ */
+class ForcePermission extends DataExtension implements TestOnly
+{
+    private static $canView = null;
+    private static $canDelete = null;
+
+    public static function forceCanView($val)
+    {
+        self::$canView = $val;
+    }
+
+    public static function forceCanDelete($val)
+    {
+        self::$canDelete = $val;
+    }
+
+    public static function reset()
+    {
+        self::$canView = null;
+        self::$canDelete = null;
+    }
+
+    public function canView($member)
+    {
+        if (self::$canView !== null) {
+            return self::$canView;
+        }
+    }
+
+    public function canDelete($member)
+    {
+        if (self::$canDelete !== null) {
+            return self::$canDelete;
+        }
+    }
+}