odan / session

A middleware oriented session handler for PHP and Slim 4+
https://odan.github.io/session/
MIT License
56 stars 11 forks source link

Is it possible to destroy the session and add a new flash message in the same action? #24

Closed samuelgfeller closed 2 years ago

samuelgfeller commented 2 years ago

When the user deletes his account, I want to log him out, redirect to home page and inform of the success via flash message. This is what I've tried that doesn't work:

$this->session->destroy();
$this->session->start();
$this->session->getFlash()->add('success', 'Successfully deleted account. You are now logged out.');
$this->session->save();

$responseBody['redirectUrl'] = $this->responder->urlFor('home-page');

// Less important but I put it for context
return $this->responder->respondWithJson($response, $responseBody);

And then I load the page after the DELETE Ajax call but I don't think it's very relevant as the flash messages work when I remove $this->session->destroy();

let responseBody = JSON.parse(xHttp.responseText);
if (typeof responseBody.redirectUrl !== 'undefined'){
    window.location.href = responseBody.redirectUrl;
}

It seems I have something to learn about sessions.

odan commented 2 years ago
$this->session->destroy();
$this->session->start();

The start method should use only within the SessionMiddleware, but not directly. There is maybe one exception, for example when you implement a login, then you need to call them manually (for security reasons to create a new session id).

But generally, let the session start using the middleware:

use Odan\Session\Middleware\SessionMiddleware;

$app->add(SessionMiddleware::class);

https://odan.github.io/session/v5/#session-middleware

samuelgfeller commented 2 years ago

$app->add(SessionMiddleware::class); is the second middleware I add in middleware.php (after addBodyParsingMiddleware()). The flash messages work, I have no session issues, I believe.

Apart from this case, where I want to log the user out and inform him via flash message of the success. Maybe we can take the LogoutAction.php, this will be the best example of what I am trying to achieve.

final class LogoutAction
{
    /**
     * LogoutAction constructor
     * 
     * @param SessionInterface $session
     * @param Responder $responder
     */
    public function __construct(
        private SessionInterface $session,
        private Responder $responder,
    ) {
    }

    public function __invoke(ServerRequest $request, Response $response): Response
    {
        // Logout user
        $this->session->destroy();

        // Add flash message to inform user of the success which does NOT work like this - how do I make this line work?
        $this->session->getFlash()->add('success', 'Logged out successfully.');

        return $this->responder->redirectToRouteName($response, 'login-page');
    }
}

Adding ->add(SessionMiddleware::class) to the route does not change anything.

Now my question is, how do I tell the user via flash message that he is successfully logged out, is it even possible?

Edit

Okay, I've done some more trying and figured that the combination of $this->session->start(); and $this->session->regenerateId(); does archive what I wanted:

    public function __invoke(ServerRequest $request, Response $response): Response
    {
        // Logout user
        $this->session->destroy();
        // Re start session to bind flash message
        $this->session->start();
        $this->session->regenerateId();
        // Add flash message to inform user of the success and it WORKS
        $this->session->getFlash()->add('success', 'Logged out successfully.');

        return $this->responder->redirectToRouteName($response, 'login-page');
    }

But you said that the start() method should not be used directly... Is that a worthy exception, or is there a better workaround?

I'll close this issue, so it isn't in the "open issues" tab, but if what I did is not proper I absolutely want to know how to do better

odan commented 2 years ago

But you said that the start() method should not be used directly... Is that a worthy exception, or is there a better workaround?

Yes, I mentioned it here. In practice, the new session id should be generated after a successful login.

There is maybe one exception, for example when you implement a login, then you need to call them manually (for security reasons to create a new session id).

See here: Renew the Session ID After Any Privilege Level Change https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change

samuelgfeller commented 2 years ago

Not sure if we understand each other well. I'd love to improve my writing to be more clear, hints are welcomed. Or maybe I'm just bad at understanding things. In either case, I have a bit of difficulty to understand how "In practice, the new session id should be generated after a successful login." is an answer to "Is that a worthy exception, or is there a better workaround?".

Anyway

Yes, I mentioned it here. In practice, the new session id should be generated after a successful login.

Yes, I am regenerating it after a successful login.

My question is about the solution I presented in my edit (logout), as it also regenerates the session ID after a logout. And to regenerate session id I have to start it first otherwise there is an exception.

So, is it justified to use start() and regenerateId() it at this place with the reason being that I need it to display flash messages, or is what I've done bad practice and a workaround has to be found?

See here: Renew the Session ID After Any Privilege Level Change

In the docs, the following is written:

The session ID must be renewed or regenerated by the web application after any privilege level change within the associated user session.

The most common scenario where the session ID regeneration is mandatory is during the authentication process, as the privilege level of the user changes from the unauthenticated (or anonymous) state to the authenticated state.

Common scenarios to consider include; password changes, permission changes, or switching from a regular user role to an administrator role within the web application.

Does that mean that a change from the authenticated state to anonymous (logout) is also a privilege change? If that's the case, I accidentally did exactly the right thing by regenerating it, but couldn't without the use of the start() method. Is this one of the "exceptions" you talked about where I can use the method start() inside the logout action?

Just wanted to add, thank you so much for the time you're taking to reply to me. It is of great use and I appreciate it a lot :)