Sessions should only be visible by owning user

[brynwhyman]

Overview

A user should be the only one who can view and manage existing sessions.

Currently with this module installed, a user who has access to view other member profiles (like the default Administrator role) has the ability to view and manage their sessions. This shouldn't be the case.

Acceptance Criteria

• If you do not have access to view another user's member form then the session manager component should not show
• By default, Admin roles do not have access to view the session manager component for another member
• Session data can only be viewed and managed by the user that it relates to
• Validate that all access control measures on the session manager components are as expected, i.e. CanView and CanDelete are doing the right thing
• Confirm there is a sensible way for Developers to alter view and management permissions on the session manager component, allowing privileged users to manage sessions if that's a requirement of theirs.
• The ability for Developers to alter the behaviour (and allow other users to access member session data) should be documented so the feature is discoverable
• Confirm that there is no permission that can be assigned to a user to give them access to view another member session details

Pull request

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

[emteknetnz]

This code in LoginSession::handlePermission() should probably be changed from

        // Members can manage their own sessions
        if ($this->ID == $member->ID) {
            return true;
        }

        // Access to SecurityAdmin implies session management permissions
        return Permission::checkMember($member, 'CMS_ACCESS_SecurityAdmin');

to

        // Members can manage their own sessions
        if ($this->MemberID === $member->ID) {
            return true;
        }
        
        return false;

[emteknetnz]

Probably worth updating following code in LoginSessionController::removeLoginSession():

            $id = $request->param('ID');
            $loginSession = LoginSession::get()->byID($id);

to

            $id = $request->param('ID');
            $memberID = Security::getCurrentUser()->ID;
            $loginSession = LoginSession::get()->filter(['ID' => $id, 'MemberID' => $memberID])->first();

[maxime-rainville]

The current permission model is poorly implemented. MemberExtension has a bunch of logic to determine if the current user can edit and/or view the current session, but it completely bypasses LoginSession::canView/LoginSession::canEdit methods.

MemberExtension is also a PermissionProvider for SESSION_MANAGER_ADMINISTER_SESSIONS. If we want a permission provider, it should be on LoginSession because that's the object we are managing.

MemberExtension also tries to set the SessionManagerField to read-only if you are only allowed to view the current login session. However, we don't have a read-only view for this field.

LoginSession::handlePermission looks pretty good and already has permission extensibility built-in like @bergice mentioned in backlog grooming.

Here's what I want to do:

• Update MemberExtension
  • Remove all the PermissionProvider logic
  • Use LoginSession::canView to determine if the field should be displayed or not
  • Set SessionManagerField to readonly if the user doesn't have canDelete permission and spin off an issue to implement a read-only view for this field.
• Update LoginSession to
  • remove the permission check on CMS_ACCESS_SecurityAdmin
  • Set CanEdit to always return false ... the idea is that you should never be able to directly update a LoginSession object.
  • Set CanCreate to always return false for the same reason
  • Review CanView and CanDelete to be based on the owner of the LoginSession
• Update the doc with an example of how you could implement your own permission layer if you wanted to.

[emteknetnz]

Looks good

how you could implement your own permission layer if you wanted to.

How do you see this working?

[maxime-rainville]

You would stick an extension on LoginSession an hook into the canView/canDelete extension points.

Maybe you use a pre-existing permission to decide if the user can delete the login session. Maybe you just check if the current user can edit the member. Maybe you implement your own permission provider. Ultimately, that's a decision the developer has to make for themselves.

We've got a follow up issue to explore this in further detail #57 (comment)

[brynwhyman]

The ability for Developers to alter the behaviour (and allow other users to access member session data) should be documented so the feature is discoverable

I've made a suggestion in #63 that we should recommend that if the behaviour is altered, the privileged user should get both CanView and CanDelete actions (given there's no read-only view).

[emteknetnz]

Linked PR has been merged

Noted on the pull request there's an issue with non-admin users revoking sessions. We've agreed to split that of as a separate issue. #67