FIX Revoke a single session

[emteknetnz]

Issue #46

Related
silverstripe/silverstripe-framework#9917

[maxime-rainville]

Logging out from one session invalidates my "Remember" me token in other browsers. Feel like this should be addressed by this PR

https://youtu.be/GoxWA0EoCtA

[emteknetnz]

@maxime-rainville Have updated so that RememberLoginHash's for unrevoked sessions are untouched

[maxime-rainville]

Following silverstripe/silverstripe-framework#9794, we decided that logout_across_devices would default to false when you install session manager.

My thinking is this:

• The vanilla install should keep the current default behaviour even if we don't think it's the best one in most cases. You need to set $logout_across_devices to false if you want the better log out behaviour.
• Session manager should implicitly set $logout_across_devices to false. If you logout, only the "Remember Me" token for the current session will be invalidated.
• Irrespective of what value is set for $logout_across_devices, when you use session manager to terminate a specific session, any "remember me" token associated to that session must be invalidated.

This seems like the sensible place to implement this change.

[maxime-rainville]

Tested this locally and it works great.

The only thing that's missing is adding a bit of YML config so logout_across_devices defaults to false

[maxime-rainville]

It's still trying to test 4.7, 4.6 and 4.5 releases. That's preventing the builds from being 🟢

[emteknetnz]

I've tried changing this to use a different provision

[emteknetnz]

New provision worked, travis green

[emteknetnz]

The only thing that's missing is adding a bit of YML config so logout_across_devices defaults to false

Do we do that here? It's not an AC on the issue.

This pull-request works with logout_across_devices true or false - the decision to change the default of logout_across_devices to false made total sense as an outcome of the investigation, and it's what this pull-request does though on a temporary basis.

I'm just wondering if we need to reevaluate whether we even need to change the default value? Either way, should probably happen on a seperate PR since it's not an AC here?

[maxime-rainville]

It was a pretty clear conclusion from silverstripe/silverstripe-framework#9794 (comment)

Yes, we should have defined an AC for it. But if no one is arguing that this shouldn't happen is there much point spinning up a separate issue to manage the addition of 3 lines of YML?

[emteknetnz]

OK rereading the previous investigation it makes a bit more sense. Partially to help clarify my own understanding, I've spun up a new issue and brought into sprint (needs to happen) #65 - could you check that it's correct? PR for it should probably only take a few minutes.

[maxime-rainville]

#65 looks good. Just make sure your YML section is named so people can use the #after statement to override it

[emteknetnz]

Great ta will do. This PR should be good to merge now. I'll sort the YML PR now.

[maxime-rainville]

I don't understand the point of splitting this up in a separate issue. But otherwise it looks good.