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

API Make token regeneration optional during autologin session renewal #11299

Conversation

Cheddam
Copy link
Member

@Cheddam Cheddam commented Jul 4, 2024

Description

Renewing the token/hash during an active session can trigger a logout in the event of request failures or simultaneous requests. This PR makes the regeneration logic optional, though still enabled by default, and ensures a new cookie is only transmitted to the client if the token is regenerated.

This also marks the renew method as deprecated, to be removed entirely in 6.0. There's an extension point in this method that the Login Session module relies on to correctly augment session data, so this will need to be moved.

NOTE: There's a $this->write() call in RememberLoginHash::renew() that follows the extension point, which I've intentionally left intact regardless of the new configuration setting. The likelihood that a project/module is relying on this write is extremely low, but not zero.

Manual testing steps

  1. Set the new config to disable token regeneration:
    SilverStripe\Security\RememberLoginHash:
      replace_token_during_session_renewal: false
    
  2. Log in with the 'Remember me' checkbox ticked
  3. Test that destroying the PHPSESSID / SECSESSID cookie and refreshing consistently results in recreation, and the user remains logged in, but that the alc_enc cookie does not change
     

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Member

Prefer-lowest CI failure is unrelated to this PR.

src/Security/RememberLoginHash.php Show resolved Hide resolved
src/Security/RememberLoginHash.php Outdated Show resolved Hide resolved
src/Security/RememberLoginHash.php Outdated Show resolved Hide resolved
src/Security/RememberLoginHash.php Outdated Show resolved Hide resolved
tests/php/Security/RememberLoginHashTest.php Outdated Show resolved Hide resolved
src/Security/RememberLoginHash.php Outdated Show resolved Hide resolved
@Cheddam Cheddam force-pushed the pulls/5/session-token-renewal-configuration branch from 30de8ec to 9136112 Compare July 15, 2024 23:51
Resolves silverstripe#11281. Renewing the token/hash during an active session
can trigger a logout in the event of request failures or simultaneous
requests.

This also marks the renew method as deprecated, to be removed
entirely in 6.0.
@Cheddam Cheddam force-pushed the pulls/5/session-token-renewal-configuration branch from 9136112 to 724e813 Compare July 16, 2024 00:08
$this->Hash = $hash;
}

$this->extend('onAfterRenewToken', $replaceToken);
Copy link
Member Author

Choose a reason for hiding this comment

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

@GuySartorelli Just considering how we'll handle this extension point once we drop the method; My current thought is that we shift the call-site up to CookieAuthenticationHandler::authenticateRequest(), but still have it be an extension of RememberLoginHash (possibly with a tweaked name to reflect that the token itself is not being renewed.)

There's precedent in the authenticateRequest() method for calling an extension point externally on an object instance (see the $member->extend('memberAutoLoggedIn'); call) but not sure if this falls within our general best practices?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the idea with deprecating and eventually removing the renew() method that there will be no renewal necessary? If we don't renew the token, we don't need to have a onAfterRenewToken extension hook, right?

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit d14ec28 into silverstripe:5 Jul 22, 2024
15 checks passed
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