Closed GuySartorelli closed 2 years ago
Re the hybridsessions AC
Checked manually in browser and by running the tests with hybrid sessions installed
So, what was validated here? Did you confirm that session data was removed the cookies? Hybrid sessions is a strange one, it's easiest to test by commenting out this line to ensure the database is used. Maybe try that, ensure there's session data in the HybridSessionDataObject table, then run this task in the PR
If it's not deleted, something along the lines of a class_exists() check to see if CookieStore/DatabaseStore exist, then singleton(DatabaseStore::class)->gc()
would work, though it's messy.
There should be something within the session-manager to "logout" a member that cascades down to the hybridsession stores to remove data
There should be something within the session-manager to "logout" a member that cascades down to the hybridsession stores to remove data
This is probably what that onBeforeRemoveLoginSession
hook is for (used in LoginSessionController::remove()
and discussed briefly in the description of this PR) so it looks like I will need to call that hook from this task after all.
I think instead of this module doing stuff to hybridsession, instead I will add a PR in hybridsession to use that extension hook if session manager is installed.
EDIT: No, we don't need to do this - see my comment below.
I've just realised that hybridsession doesn't play into it at all. Hybridsession stores session data, but it doesn't touch authentication at all. That's handled by framework if session-manager isn't installed, and by session-manager if it is installed.
Imagine the scenario where a person has logged in and is accessing the CMS, but then their session gets revoked by the task in this PR. What happens when they refresh or navigate to a new page is this:
AuthenticationMiddleware
in framework processed, which results in authenticateRequest
to be called on classes that implement AuthenticationHandler
SessionAuthenticationHandler
sees that, based on the information stored for the current session, a user appears to be logged in.Member
from the session data set as the 'current' user via Security::setCurrentUser()
.LoginSessionMiddleware
is processed. This gets the Member
via Security::getCurrentUser()
(if there was no member it would just return out at this point)LoginSessionMiddleware
checks if there is a LoginSession
object in the database that corresponds to the current session's ID. Because there isn't one, it logs the user out (which also revokes that session).HybridSessionDataObject
s.In other words, we don't need to do anything special here to handle hybridsessions.
I'm still unsure whether we need to call the onBeforeRemoveLoginSession
hook in the new task - I'm not sure what it's for, to be honest.
On the one hand calling it would potentially be a massive hit on performance for any sites with lots of sessions running, since they'd all need to be fetched and instantiated to call the hook.
On the other hand, is there a potential that some custom code relies on that hook to invalidate some weird custom session handling nonsense? I'm not sure.
There's a PHPCS linting thing to fix in travis
Fixed the phpcs issue and added a blurb to the readme. Also added one about users revoking their own sessions, which didn't seem to be documented there.
Note that the
onBeforeRemoveLoginSession
extension hook (seeLoginSessionController::remove()
) is not called. The thinking is that this task is effectively for emergency situations anyway to prevent an identified actively logged in potential attacker from making additional changes - the aim is to outright remove all current sessions. This is also consistent with theGarbageCollectionTask
.Acceptance criteria (from parent issue, for the sake of transparency)
SilverStripe\Security\Tests\RememberLoginHashTest::testClear()
in framework - for the purposes of this PR, testing that they're removed is sufficient.Parent issue: