Fixes Session timeout error #2896 and #2897

[sancsin]

Fixes #2896 and #2897 by providing clear feedback to the user. The fix does so by using a middleware that creates a cookie to compare the elapsed time since a user last logged-in and the session lifetime configuration setting. If elapsed time exceeds the session lifetime, a SessionExpiredException is thrown. This error is also caught in the front-end through axios interceptor to inform the user using an alert that the session has expired. Once the user clicks on "OK", the page reloads. Reloading page provides expected behaviour. For example, if the user clicks on an album that is public, the page will reload and the now guest user, will still be able to browse the public album. However, if the user clicks on an album that is not public, the page will reload and provide clear exception feedback to the user that they are not authorized to view the album/page since their session expired.

Another scenario that this fix provides better user experience for is when the user is on a navigated page that is provided through the left menu. Once the session expires and the now guest user tries to access any of the pages that they should not have access to, they are clearly provided with the session expired alert. Furthermore, the page reloads instead of showing errors that don't make sense for the user, such as the ones described in #2897.

[ildyria]

what error are you catching here ?

[ildyria]

okay I figured it out...

[sancsin]

It is the following error that I'm catching. Which happens after session has timed out. If we don't catch it then we get an uncaught exception in the browser console.

[ildyria]

Yep I saw that. I made quite a few changes to your implementation, can you check if that is working satisfactory for you ? :)

[sancsin]

Looks quite more like an elegant solution! :)

[ildyria]

LGTM, waiting for double check from @sancsin

[sancsin]

The Session Expired dialog appears even though user is not logged-in and tries to open a publicly available album. Is that the desired behaviour?

[sancsin]

At the moment, the user seems to be redirected to http://address/gallery/gallery instead of http://address/gallery/ when clicking on Go to the Gallery. Furthemore, the dialog does not close when clicking on Go to the Gallery button.

[ildyria]

The Session Expired dialog appears even though user is not logged-in and tries to open a publicly available album. Is that the desired behaviour?

definitely not.

[sancsin]

Seeing the below errors when navigating to "Settings"

[ildyria]

That is so weird, because I do not have that same behaviour here. O.o

[ildyria]

Wait... Are you on http or https ?
Though it shouldn't matter. This is tied to the xsrf token.

[sancsin]

Wait... Are you on http or https ? Though it shouldn't matter. This is tied to the xsrf token.

I'm on http

[ildyria]

So I see what you mean with the dialog popping up if you e.g. open an album while you spent too much time on the page...

[ildyria]

So I figured it out.

The CSRF check is as follows:

    public function handle($request, Closure $next)
    {
        if (
            $this->isReading($request) ||
            $this->runningUnitTests() ||
            $this->inExceptArray($request) ||
            $this->tokensMatch($request)
        ) {
            return tap($next($request), function ($response) use ($request) {
                if ($this->shouldAddXsrfTokenCookie()) {
                    $this->addCookieToResponse($request, $response);
                }
            });
        }

        throw new TokenMismatchException('CSRF token mismatch.');
    }

The TokenMismatchException was responsible for the 419 error.

The function isReading() is the important one here. As opposed to version 4 and 5 where all requests were done in POST, the v6 makes use also of other verbs such as DELETE, PATCH, PUT and more importantly GET.

Following the code path, we see the two following important functions:

    protected function isReading($request)
    {
        return in_array($request->method(), ['HEAD', 'GET', 'OPTIONS']);
    }

and this (which previously was always used).

    protected function tokensMatch($request)
    {
        $token = $this->getTokenFromRequest($request);

        return is_string($request->session()->token()) &&
               is_string($token) &&
               hash_equals($request->session()->token(), $token);
    }

Conclusion: GET, HEAD, and OPTION request do not require the XSRF token.

But if your session times out, and try to access a protected folder you will get a 401.
We took care of the exception which was triggering the white screen.

Note that your original middleware idea was leading to the same result as the behaviour you observed: triggering a error and refresh request but with the use of an extra cookie.

Now the question: Should we have the XSRF token made mandatory for the GET requests of v6 or not?
If yes, then the current implementation checking the token absence or not is the way to go with the client side trigger of the modal.

If not, then this requires a more difficult handling of 401 errors.

[ildyria]

@sancsin see latest commit, in the behavior change. :)

[sancsin]

So I figured it out.

The CSRF check is as follows:

    public function handle($request, Closure $next)
    {
        if (
            $this->isReading($request) ||
            $this->runningUnitTests() ||
            $this->inExceptArray($request) ||
            $this->tokensMatch($request)
        ) {
            return tap($next($request), function ($response) use ($request) {
                if ($this->shouldAddXsrfTokenCookie()) {
                    $this->addCookieToResponse($request, $response);
                }
            });
        }

        throw new TokenMismatchException('CSRF token mismatch.');
    }

The TokenMismatchException was responsible for the 419 error.

The function isReading() is the important one here. As opposed to version 4 and 5 where all requests were done in POST, the v6 makes use also of other verbs such as DELETE, PATCH, PUT and more importantly GET.

Following the code path, we see the two following important functions:

    protected function isReading($request)
    {
        return in_array($request->method(), ['HEAD', 'GET', 'OPTIONS']);
    }

and this (which previously was always used).

    protected function tokensMatch($request)
    {
        $token = $this->getTokenFromRequest($request);

        return is_string($request->session()->token()) &&
               is_string($token) &&
               hash_equals($request->session()->token(), $token);
    }

Conclusion: GET, HEAD, and OPTION request do not require the XSRF token.

But if your session times out, and try to access a protected folder you will get a 401. We took care of the exception which was triggering the white screen.

Note that your original middleware idea was leading to the same result as the behaviour you observed: triggering a error and refresh request but with the use of an extra cookie.

Now the question: Should we have the XSRF token made mandatory for the GET requests of v6 or not? If yes, then the current implementation checking the token absence or not is the way to go with the client side trigger of the modal.

If not, then this requires a more difficult handling of 401 errors.

Thanks for the detailed explanation. Really appreciate it. Now I understand a little better. I think the behaviour now is quite good even though the session timeout dialog is shown when the user is logged-out and tries to access public album. I don't know if you noticed though, with the cookie approach this issue was not there.

[sancsin]

@sancsin see latest commit, in the behavior change. :)

I made a new commit as a suggestion to handle the cases where the SessionExpiredReload dialog was not closing when user clicks on Go to gallery button.