silverstripe / recipe-cms

Silverstripe recipe for fully featured page and asset content editing
BSD 3-Clause "New" or "Revised" License
20 stars 14 forks source link

DEP Add session manager #48

Closed emteknetnz closed 3 years ago

emteknetnz commented 3 years ago

Issue https://github.com/silverstripe/recipe-cms/issues/47

maxime-rainville commented 3 years ago

The build is still failing and it doesn't look like a pre-existing failure :cry:

https://app.travis-ci.com/github/silverstripe/recipe-cms/branches

emteknetnz commented 3 years ago

So the issue here is that unit/functional tests that use Security::setCurrentUser(); doesn't quite work due to LoginSession's not being created, as this relies on SilverStripe\SessionManager\Security\LogInAuthenticationHandler::logIn() being called

This following POC code will get CMSProfileControllerTest::testMemberEditsOwnProfile (a random failing functional test) to pass

This code isn't great :-/ The bit with AssetControlExtension (which is on DataObject) is particularly bad due to it failing because (I think) of Member:actAs()

config.yml (session-manager)

---
Name: session-manager-security
After: '#coresecurity'
---
SilverStripe\Security\Security:
  extensions:
    - 'SilverStripe\SessionManager\Extensions\SecurityExtension'

Security.php (framework)

    public static function setCurrentUser($currentUser = null)
    {
        self::$currentUser = $currentUser;
        static::singleton()->extend('updateSetCurrentUser', self::$currentUser);

SecurityExtension.php (session-manager)

class SecurityExtension extends Extension
{
    public function updateSetCurrentUser(?Member $member)
    {
        if (is_null($member)) {
            $handler = Injector::inst()->get(LogOutAuthenticationHandler::class);
            $handler->logOut();
            return;
        }
        $handler = Injector::inst()->get(LogInAuthenticationHandler::class);
        $handler->logIn($member);
    }
}

AssetControlExtension.php (assets)

    public function onBeforeWrite()
    {
        if ($this->owner instanceof LoginSession) {
            return; // prevent infinite recursion due to the Member::actAs() being called
        }
maxime-rainville commented 3 years ago

If setting Security::setCurrentUser() blows stuff up then that's a major bug. This method can potentially be used for a lot of other things aside from unit tests ... so it's not enough just to patch the affected tests.

emteknetnz commented 3 years ago

I've added a bunch of PR's on the parent issue so that Login sessions are now created/destroyed when calling Security::setCurrentUser()

emteknetnz commented 3 years ago

Update: much better solution, change failing unit tests from using $this->session()->set('loggedInAs', $member->ID); to $this->logInAs($member); which means that LoginSession authenticator LogIn() method is called