Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEP Add session manager #48

Conversation

emteknetnz
Copy link
Member

Issue #47

@emteknetnz emteknetnz mentioned this pull request Aug 3, 2021
21 tasks
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the readme and phpunit.xml.dist?

@emteknetnz emteknetnz force-pushed the pulls/4/add-session-manager branch from 8f7230f to 80fe651 Compare August 4, 2021 21:47
@maxime-rainville
Copy link
Contributor

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

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

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 10, 2021

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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

@emteknetnz emteknetnz force-pushed the pulls/4/add-session-manager branch from 80fe651 to 8d0f13f Compare August 18, 2021 02:28
@emteknetnz emteknetnz force-pushed the pulls/4/add-session-manager branch from 8d0f13f to 03c6888 Compare August 19, 2021 02:33
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@maxime-rainville maxime-rainville merged commit 6fe092a into silverstripe:4 Aug 19, 2021
@maxime-rainville maxime-rainville deleted the pulls/4/add-session-manager branch August 19, 2021 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants