Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

UX with cookie-based refresh tokens #287

Closed
bpolaszek opened this issue Nov 18, 2021 · 8 comments
Closed

UX with cookie-based refresh tokens #287

bpolaszek opened this issue Nov 18, 2021 · 8 comments

Comments

@bpolaszek
Copy link

bpolaszek commented Nov 18, 2021

Hello there! 馃憢

I've been testing the "cookie" option of this bundle, lately.
This is awesome and it works great! 馃帀 It avoids having to store the token on the client side and expose it to creepy people.

However, I notice 2 usability issues with this:

  1. When the user logs out from the SPA, client just destroys the access token and the user gets redirected to the login page. Fine. problem is, since client has not access to the refreshToken anymore, it can no longer destroy it. This means that next call to the refresh token route will log the user in, even if they wanted to logout, because the cookie is still here 馃槗

  2. Since the refreshToken is no longer the client's business, 2 different users can't be logged in within the same browser. I usually have an "admin" tab and a "customer" tab. Since both share the same cookie (same API), as soon as I sign in as a customer, I get kicked from the admin tab once this one asks for a new token.

Regarding 1: how about the following flow:

# Refresh current token
POST /api/token/refresh
Cookie: refreshToken=myCurrentRefreshToken

HTTP/2 200 OK
Set-Cookie: refreshToken=someOtherRefreshToken, expires=in one week

# Revoke that token
DELETE /api/token/refresh
Cookie: refreshToken=someOtherRefreshToken

HTTP/2 200 OK
Set-Cookie: refreshToken=, expires=right now

Regarding 2... I have no real idea on how to address this. Thoughts?

@Jayfrown
Copy link
Contributor

Hey,

Regarding 1: how about the following flow

Yeah, this is similar to what I do in my applications. I use a custom route POST /api/token/invalidate which invalidates the refresh_token and clears the cookie in the response. Exposing a similar route by default sounds good, I might look into that later.

Here's the code (adapted from something I found in the comments of another issue which I cannot find anymore):

src/Controller/InvalidateRefreshTokenController.php
<?php

namespace App\Controller;

use Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManagerInterface;
use Gesdinet\JWTRefreshTokenBundle\Request\Extractor\ExtractorInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\JsonResponse;

class InvalidateRefreshTokenController extends AbstractController
{
    /**
     * @var RefreshTokenManagerInterface
     */
    private $refreshTokenManager;

    /**
     * @var ExtractorInterface
     */
    private $refreshTokenExtractor;

    public function __construct(
        RefreshTokenManagerInterface $refreshTokenManager,
        ExtractorInterface $refreshTokenExtractor
    ) {
        $this->refreshTokenManager = $refreshTokenManager;
        $this->refreshTokenExtractor = $refreshTokenExtractor;
    }

    public function invalidate(Request $request): JsonResponse
    {
        // TODO: get tokenParameter from service configuration
        $tokenParameter = 'refresh_token';
        $tokenString = $this->refreshTokenExtractor->getRefreshToken($request, $tokenParameter);
        if (!$tokenString) {
            return new JsonResponse(
                [
                    'code' => 422,
                    'message' => 'No refresh_token found.',
                ],
                JsonResponse::HTTP_UNPROCESSABLE_ENTITY
            );
        }

        $refreshToken = $this->refreshTokenManager->get($tokenString);
        if (!$refreshToken) {
            return new JsonResponse(
                [
                    'code' => 422,
                    'message' => 'Invalid refresh_token supplied.',
                ],
                JsonResponse::HTTP_UNPROCESSABLE_ENTITY
            );
        }

        $this->refreshTokenManager->delete($refreshToken);
        $response = new JsonResponse(
            [
                'code' => 200,
                'message' => 'The refresh_token has been invalidated.',
            ],
            JsonResponse::HTTP_OK
        );

        // TODO: check if cookies are enabled in service configuration
        $response->headers->clearCookie($tokenParameter);
        return $response;
    }
}

2 different users can't be logged in within the same browser

This is actually the case with any (traditional) cookie-based authentication. I also personally expect this kind of behavior, as I tend to use multiple tabs and expect to be the same user everywhere, or still be logged in on a new tab. (I use private browsing for a second session)

@bpolaszek
Copy link
Author

bpolaszek commented Nov 22, 2021

Hi @Jayfrown - yeah, that's pretty similar to what I did, but I was wondering if this feature should not be part of this bundle, otherwise user has no way to properly "log out".

Regarding my 2nd point, I came up with a crappy but very efficient solution to expose my API on 2 different domains, so that employee cookies and user cookies remain isolated. 馃憤

@Jayfrown
Copy link
Contributor

Jayfrown commented Feb 4, 2022

I have tried several times now to take a look at including this behavior by default, as I agree that currently it is a bit awkward that one has to ship their own controller in order to invalidate the refresh_token and unset the corresponding cookie. Moreover, this requirement is undocumented.

However, I find myself too inexperienced working with Symfony's authenticator system, and cannot quite figure out where to begin.

@mbabker would you mind sharing your thoughts/opinions on the matter and giving me some direction?

@mbabker
Copy link
Contributor

mbabker commented Feb 4, 2022

My initial thought is that this bundle will need an event listener for the logout event to be able to destroy the cookie.

@mbabker
Copy link
Contributor

mbabker commented Feb 4, 2022

Or, the symfony/security-http component already has a CookieClearingLogoutListener class. So, maybe this config will work? (I'm going off of a bin/console config:dump-reference security output since the option isn't actually covered in the Symfony docs)

security:
    firewalls:
        api:
            logout:
                delete_cookies:
                    <cookie_name>:
                        path: ~
                        domain: ~
                        secure: false
                        samesite: ~

@Jayfrown
Copy link
Contributor

Jayfrown commented Feb 4, 2022

Great, thanks! I'll experiment with the CookieClearingLogoutListener and if that doesn't work out I was thinking something like this based on what you said about the EventListener:

# in security.yaml
security:
    firewalls:
        api:
            logout:
                path: api_token_invalidate
# in routes.yaml
api_token_invalidate:
    path: /api/token/invalidate
    methods: ['POST']

I believe then POSTing to /api/token/invalidate should trigger a logout event, and we can add an EventListener like you mentioned.

@Jayfrown
Copy link
Contributor

Jayfrown commented Feb 5, 2022

So, using the CookieClearingLogoutListener does work with the following configuration:

# in gesdinet_jwt_refresh_token.yaml
gesdinet_jwt_refresh_token:
    ttl: 7200
    single_use: true
    cookie:
        enabled: true
        path: /api/token/
        same_site: none
# in security.yaml
security:
    firewalls:
        api:
            logout:
                path: api_token_invalidate
                delete_cookies:
                    refresh_token:
                        path: /api/token/
                        samesite: none
# in routes.yaml
api_token_invalidate:
    path: /api/token/invalidate

However, there are some things to consider:

  • The cookie is removed, but of course the refresh_token itself is not invalidated.
  • The cookie configuration needs to match exactly for it to work correctly, which means if you're using non-default values you'd need to configure them in two places - once in gesdinet_jwt_refresh_token.yaml to set the cookie, and once in security.yaml to unset the cookie.

So I think it would make more sense to include an EventListener for logout events within the bundle. This would make configuration easier, and the refresh_token could be invalidated as well.

I will start working on a PR for this soon.

Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Feb 5, 2022
This commit introcudes a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If there is no `refresh_token`, an error is returned and the cookie is
not unset. The same happens if the supplied `refresh_token` is invalid.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Feb 5, 2022
This commit introcudes a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If there is no `refresh_token`, an error is returned and the cookie is
not unset. The same happens if the supplied `refresh_token` is invalid.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Feb 5, 2022
This commit introcudes a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If there is no `refresh_token`, an error is returned but the cookie is
still unset. The same happens if the supplied `refresh_token` is invalid.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
@Jayfrown
Copy link
Contributor

Jayfrown commented Feb 5, 2022

PR: #302

Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Feb 5, 2022
This commit introduces a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If there is no `refresh_token`, an error is returned but the cookie is
still unset. The same happens if the supplied `refresh_token` is invalid.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Mar 23, 2022
This commit introduces a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If there is no `refresh_token`, an error is returned but the cookie is
still unset. The same happens if the supplied `refresh_token` is invalid.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Mar 23, 2022
This commit introduces a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If no refresh token is supplied, an error is returned and the cookie
remains untouched. If the supplied refresh token is (already) invalid,
the cookie is unset.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Jayfrown added a commit to Jayfrown/JWTRefreshTokenBundle that referenced this issue Mar 23, 2022
This commit introduces a `LogoutEventListener` which invalidates the
given `refresh_token` and unsets the cookie, if enabled.

If no refresh token is supplied, an error is returned and the cookie
remains untouched. If the supplied refresh token is (already) invalid,
the cookie is unset.

Because the `LogoutEventListener` always sets a response, it would
inhibit normal logout behavior and therefore should only run on a
specifically configured firewall.

Therefore a new configuration option is introduced, called
`logout_firewall`, which contains the name of the firewall that
triggers the logout event we want to hook into (default: `api`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants