On the User Sessions Page (/user/{uid}/sessions), a "Delete All Sessions" option would be convenient.

A "Warning: This will log you out of your current session!" message would need to be acknowledged before executing the bulk delete.

Alternatively, a "Delete All Other Sessions" option could clear out all sessions but the current/active one, bypassing the issue above.

CommentFileSizeAuthor
#9 3272786-9.patch18.85 KBlarisse
#3 3272786-3.patch7.68 KBlarisse

Comments

cmarcera created an issue. See original summary.

philipnorton42’s picture

An excellent idea.

I think the "Delete other sessions" might be the most useful. Otherwise the button would be more of an annoyance.

I'll see what I can do :)

larisse’s picture

Status: Active » Needs review
StatusFileSize
new7.68 KB

Hi! I created this patch with the comment #2 suggestion...
I think will need some adjusts in the delete page, but I will put the patch here to know if this is the way...
Feedback are welcomed :)

philipnorton42’s picture

Hi larisse,

Firstly, thank you so much for taking the time to contribute. I really appreciate your patch!

That being said, I think you've missed the brief slightly. You code does delete the sessions, but it deletes the sessions for all users on the site, essentially logging everyone out of the site at the same time.

What was required was a way to allow a user to delete all sessions that belong to them (that aren't their current session). On other words, the path of the form would be "/user/{user}/sessions/{sid}/delete-all" and not "/admin/config/people/session_inspector/delete".

The code you have written does have some good ideas. I think a service to delete sessions is a good idea, instead of adding deletion code into the session inspector service (like I have done). Would you mind if continue that work and implement suggestion #2?

philipnorton42’s picture

Sorry, I'm talking nonsense. The code does work, but as it loops through every user on the site it's not very performant.

larisse’s picture

Status: Needs review » Needs work

Oh, sorry! Yes, I missed the brief slightly.
I can continue work on it, of course! =)
But now I'm a little lost... Can I remove the wrong thing that I've done, once that is not very performant? Because I not able to think another way to do this, if you want this feature too.

philipnorton42’s picture

Hi larisse,

No worries! What you've written is a great start :)

I've been thinking about this and I think there might be a couple of ways to cut this.

Firstly, the destroySession() method should move from the session_inspector service should move into the deletion service.
Then, it's just a case of doing either:

1) Figure out the sessions that need deleting first, and then send that to the deleteAllSessions() method, like this:

  public function deleteAllSessions(array $session_ids): void {
    foreach ($session_ids as $session_id) {
      $this->destroySession($session_id);
    }
  }

2) Just pass the user to the deleteAllSessions() method, and the method figures out everything else. The isCurrentSession() method is currently part of the controller so it would need to be refactored into the session_inspector service. The code would look something like this.

  public function deleteAllSessions(User $user): void {
    $sessions = $this->sessionInspector->getSessions($user);
    foreach ($sessions as $session) {
      if ($this->sessionInspector->isCurrentSession($session['sid']) === FALSE) {
        $this->destroySession($session['sid']);
      }
    }
  }

3) Something else?...

Open to suggestions. I wrote the original code to do the job it needed to do. Now that we are introducing additional functionality it makes sense to refactor it a little.

Also, the session delete form triggers an event so that other modules can react to the session deletion. It doesn't make sense to trigger that from the form when we are doing this. It would therefore need to be moved to the either the deleteAllSessions() or destroySession() methods.

Let me know what you think.

larisse’s picture

Oh, ok, ok! Yeah, the second suggestion make more sense to mee.
I'll work on this and create another patch. For while, I don't have any other suggestion, I'll find while I code, I guess...
Thank you for support!.

larisse’s picture

Status: Needs work » Needs review
StatusFileSize
new18.85 KB

Hi @philipnorton42. Here's a new patch. I think now I was able to implement the correct feature request.

I implemented the suggest 2 in comment #7. I just needed made some adjust in the code. To put the new link on the table, I don't have sure that I made the better way, so feel free to adjust this or anything that you think will be necessary. :)

philipnorton42’s picture

Thank you so much @larisse.

I just tested your code and it works! I really appreciate you taking the time to do this. You've done almost all of the things I was planning to do here, including moving the isCurrentSession() method into the session_inspector service. Great work!

There are a few coding style issues being reported by phpcs, but I'm happy to sort those out from here if that's ok with you? I'll make sure you get the commit credit for this.

larisse’s picture

Hi @philipnorton42!

Sorry for the phpcs problems! Feel free to fix what was necessary.
Was very cool and I learned to much doing this new feature. I was happy to be able to contribute. :)

  • philipnorton42 committed ecb17aa on 1.0.x
    Issue #3272786: Refactoring services to improve class names. Moved the...

  • philipnorton42 committed 2b76f9f on 1.0.x
    Issue #3272786: Added feature to show the delete other sessions button...
  • philipnorton42 committed a824bb6 on 1.0.x
    Issue #3272786: Correcting some minor code errors and spelling....
philipnorton42’s picture

Status: Needs review » Fixed
philipnorton42’s picture

Status: Fixed » Closed (fixed)

This fix has been merged into release 1.0.1 (https://www.drupal.org/project/session_inspector/releases/1.0.1).

Thanks again cmarcera and larisse!