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

Allow "remote" logout for other accounts' sessions #1051

Closed
maltemuth opened this Issue Aug 11, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@maltemuth
Copy link
Contributor

maltemuth commented Aug 11, 2017

Description

Currently, there is no clean way to remove sessions that belong to another account. Deactivating an account (e.g. by setting a past expirationDate) will leave the sessions intact.

A way around this is to mimic the behavior of the AuthenticationProviderManager when creating sessions:

$sessionManager->destroySessionsByTag( 'Neos-Flow-Security-Account-' . md5($account->getAccountIdentifier()) );

which will effectively "log out" the given $account.

Wanted Behavior

Some API that does not involve calculating / guessing session tags, perhaps $sessionManager->destroySessionsByAccount($account).

Affected Versions

Flow 4.1.5, maybe earlier.

@albe

This comment has been minimized.

Copy link
Member

albe commented Aug 11, 2017

Thanks for creating the issue! Definitely worth having

@maltemuth maltemuth changed the title Allow "remote" logout for other accounts sessions Allow "remote" logout for other accounts' sessions Aug 11, 2017

@maltemuth

This comment has been minimized.

Copy link
Contributor

maltemuth commented Mar 26, 2018

After reading the code some more, I found the following:

The mentioned line relies on the fact that sessions for a given $account are tagged with 'Neos-Flow-Security-Account-' . md5($account->getAccountIdentifier()); this appears in

  1. The AuthenticationProviderManager when creating a session
  2. The AccountRepository when removing an account (also logging out all sessions for that account)
  3. Perhaps other places I haven't found

Right now, there's no "single source of truth" for the fact that sessions are being tagged that way, and adding this feature will most definitely add a third place, so I think it would be wise to remove this implicit knowledge now and also add a service method that returns the expected session tag for a given account.

I'm really unsure about the right class to add these methods to:
a) The AuthenticationProviderManager does not have Account anywhere in its Interface yet (neither does the AuthenticationManagerInterface, but both methods would have Account in their signature.
b) The SessionManager does not know about accounts at all.

It seems to me that the only place in the codebase that knows about Sessions and Accounts is the AuthenticationProviderManager, whose responsibility is the authentication of tokens, which does not seem to match this feature (log out accounts remotely).

So I'm looking for feedback: Where should these methods go? a or b? Or even a new class? What would that be called?

@kitsunet

This comment has been minimized.

Copy link
Member

kitsunet commented Mar 26, 2018

It rightfully belongs to neither Security nor Session but should be in a new joined package/place. If anything then security probably knows more and I am not sure if security even works remotely without knowledge of Sessions, which I doubt. So it could be in security.

@maltemuth

This comment has been minimized.

Copy link
Contributor

maltemuth commented Mar 26, 2018

not sure if security even works remotely without knowledge of Sessions

It doesn't (as far as I can see). The only thing needed to "cleanly" introduce the concept of a remote (non-current) session is, I think, the formalization of the session-account relation.

Maybe something like

class AccountSessionList {
  public function __construct (Account $account) {

  }

  public function destroyAll() {

  }

  public function getSessionTag() {

  }
}
@albe

This comment has been minimized.

Copy link
Member

albe commented Mar 27, 2018

Well, the API to destroy a session should belong to the SessionManager ofc. It just shouldn't know about Accounts.

Optimally, Authentication should work without knowing about sessions, but only about some "TokenStorage" - currently this is the Security Context. The context is also the place that connects Accounts and Sessions. It merges Tokens from the AuthenticationManager and the current session. It also maps Accounts to Roles and provides the currently authenticated Account. So I would put the logic for tagging and destroying sessions (by current or arbitrary Account) there.

@bwaidelich bwaidelich added this to To do in Neos 4.2 & Flow 5.2 Release Board via automation Nov 15, 2018

@bwaidelich bwaidelich moved this from To do to In progress in Neos 4.2 & Flow 5.2 Release Board Nov 15, 2018

@bwaidelich bwaidelich added the T: PHP label Nov 15, 2018

@albe albe moved this from In progress to Needs review in Neos 4.2 & Flow 5.2 Release Board Nov 16, 2018

@albe albe removed this from Needs review in Neos 4.2 & Flow 5.2 Release Board Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment