JWTRefreshTokenBundle icon indicating copy to clipboard operation
JWTRefreshTokenBundle copied to clipboard

Testing before 1.0.0 release

Open markitosgv opened this issue 2 years ago • 23 comments

Hi!

In a few time we are going to release a v1.0 stable version, now we are testing v1.0.0-beta2 . Could you please give me some feedback to try to fix some issues before releasing it? Thanks!!

markitosgv avatar Jul 12 '21 19:07 markitosgv

I'm on v1.0.0-beta2, and I'm getting some deprecation notices when running symfony commands like bin/console list or bin/console doctrine:schema:validate:

2021-07-15T22:34:14+00:00 [info] User Deprecated: Since gesdinet/jwt-refresh-token-bundle 1.0: The "Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManager" class is deprecated, implement "Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManagerInterface" directly.

2021-07-15T22:29:24+00:00 [info] User Deprecated: The "Gesdinet\JWTRefreshTokenBundle\Model\AbstractRefreshToken::setValid()" method will require a new "\DateTimeInterface|null $refreshToken" argument in the next major version of its interface "Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenInterface", not defining it is deprecated.

2021-07-15T22:29:24+00:00 [info] User Deprecated: The "Gesdinet\JWTRefreshTokenBundle\Model\AbstractRefreshToken::setUsername()" method will require a new "string|null $refreshToken" argument in the next major version of its interface "Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenInterface", not defining it is deprecated.
  • The first one seems to be because \Gesdinet\JWTRefreshTokenBundle\Doctrine\RefreshTokenManager uses the deprecated Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManager.
  • The latter two are caused by what look like invalid phpdocs for setValid() and setUsername() methods in \Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenInterface.

I'm on a mostly-default setup from docs. Am I doing something wrong?

pmikolajek avatar Jul 15 '21 22:07 pmikolajek

The doc blocks should be fixed up after https://github.com/markitosgv/JWTRefreshTokenBundle/pull/259

I didn’t think about it before, but breaking the inheritance on the Doctrine manager would be OK here (technically it’s a minor B/C break because it would no longer extend a class meaning an instanceof check for the specific class no longer would pass, but it would still implement the interface so it’d be OK).

mbabker avatar Jul 16 '21 12:07 mbabker

@pmikolajek it must be fixed in beta3 release

markitosgv avatar Jul 19 '21 18:07 markitosgv

I can confirm the notices are gone in beta3, thank you kindly :slightly_smiling_face:

pmikolajek avatar Jul 19 '21 22:07 pmikolajek

Hi, I've got some issues I can't figure out using version 1.0.0-beta3 and dev-master on Symfony 5.3:

Following the docs for 5.3+, I configured as follows:

security.yaml:
security:
    enable_authenticator_manager: true

    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        app_ldap:
            ldap:
                service: Symfony\Component\Ldap\Ldap
                base_dn: '%env(resolve:LDAP_BASE_DN)%'
                search_dn: '%env(resolve:LDAP_RO_DN)%'
                search_password: '%env(resolve:LDAP_RO_PASSWORD)%'
                default_roles: ROLE_USER
                uid_key: uid

    encoders:
        Symfony\Component\Ldap\Security\LdapUser:
            algorithm: auto

    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        api_token_refresh:
            pattern:  ^/api/token/refresh
            stateless: true
            refresh_jwt: ~
        api_token_auth:
            pattern: ^/api/token/auth
            stateless: true
            provider: app_ldap
            json_login_ldap:
                service: Symfony\Component\Ldap\Ldap
                dn_string: 'uid={username},%env(resolve:LDAP_BASE_DN)%'
                check_path:               /api/token/auth
                success_handler:          lexik_jwt_authentication.handler.authentication_success
                failure_handler:          lexik_jwt_authentication.handler.authentication_failure
                require_previous_session: false
        api_docs:
            pattern: ^/api/docs
            stateless: true
        api:
            pattern: ^/api
            stateless: true
            provider: app_ldap
            guard:
                authenticators:
                    - lexik_jwt_authentication.jwt_token_authenticator
        main:
            stateless: true
            lazy: true
            logout: ~

            # activate different ways to authenticate
            # https://symfony.com/doc/current/security.html#firewalls-authentication

            # https://symfony.com/doc/current/security/impersonating_user.html
            switch_user: true

    # This creates a simple hierarchy of roles so that one role can imply others
    role_hierarchy:
        ROLE_ADMIN:       ROLE_USER
        ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH]

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        - { path: ^/api/docs,  roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/api/token, roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/api,       roles: ROLE_USER }

The documentation clearly states that the routes.yaml configuration is for Symfony 5.2-, so I had initially omitted it, but then I get No route found for "GET https://<hostname>/api/token/refresh".

If I add the following to routes.yaml:

api_token_refresh:
    path: /api/token/refresh
    controller: gesdinet.jwtrefreshtoken::refresh

I get the following DI error:

Too few arguments to function Gesdinet\JWTRefreshTokenBundle\Security\Authenticator\RefreshTokenAuthenticator::__construct(), 2 passed in <project_path>/var/cache/dev/ContainerAv4TZ3Z/getGesdinet_Jwtrefreshtoken_AuthenticatorService.php on line 27 and exactly 3 expected

Line 27 in that generated file looks like this:

return new \Gesdinet\JWTRefreshTokenBundle\Security\Authenticator\RefreshTokenAuthenticator(($container->privates['security.user_checker'] ?? ($container->privates['security.user_checker'] = new \Symfony\Component\Security\Core\User\InMemoryUserChecker())), 'refresh_token');

Am I missing something?

(edit: wrap in <details></details>)

Jayfrown avatar Jul 23 '21 13:07 Jayfrown

Try omitting the controller from the route definition, so use this:

api_token_refresh:
    path: /api/token/refresh

(I don't remember if it's 100% necessary to have a route configured in the router for the security URLs, but that might only apply specifically for an authenticator which has a check_path configuration which this authenticator doesn't have).

As for the DI error, yeah, that's a bug (my bad). https://github.com/markitosgv/JWTRefreshTokenBundle/blob/master/Resources/config/services.php#L97 is missing the gesdinet.jwtrefreshtoken.request.extractor.chain reference, so a PR is needed to fix that (I can get to it at some point over the weekend if nobody else does first).

mbabker avatar Jul 23 '21 14:07 mbabker

Thanks for the suggestion @mbabker, but without the controller in the route definition I get the following error:

Unable to find the controller for path "/api/token/refresh". The route is wrongly configured.

I can confirm that adding new Reference('gesdinet.jwtrefreshtoken.request.extractor.chain'), in vendor/gesdinet/jwt-refresh-token-bundle/Resources/config/services.php after line 101 fixes the DI error, and things seem to work (if I include the controller in my route definition)

So I made #267 - let met know if it needs more than just that change

Jayfrown avatar Jul 24 '21 14:07 Jayfrown

Thanks for the suggestion @mbabker, but without the controller in the route definition I get the following error:

I just noticed your access_control section doesn’t have an entry for the refresh endpoint. I honestly don’t remember in what order the Symfony internals run between handling authentication, routing the request, and doing firewall checks, but maybe that’s causing an issue?

I did double check things against one of my client apps yesterday, that’s how our route is configured so I’m not quite sure why the missing controller is an issue (basically if the authenticator is kicking in right it handles returning a response and a controller should never be triggered).

mbabker avatar Jul 24 '21 14:07 mbabker

The second entry in access_control is ^/api/token - I figured it would match both /api/token/auth and /api/token/refresh, which should both be accessible anonymously. Splitting it to one entry for ^/api/token/auth and ^/api/token/refresh does not seem to make a difference either way.

The route works with #267 applied and explicitly setting the controller in routes.yaml, and returns the appropriate responses (MissingTokenException, TokenNotFoundException, and in valid cases, returns a new token and refresh token)

Jayfrown avatar Jul 24 '21 21:07 Jayfrown

https://github.com/mbabker/refresh-token-tester is the local app I've been using to validate a lot of the changes I make here in a full Symfony app. I don't have the issue with Symfony trying to execute a controller when hitting the refresh token route, and I threw a functional test in there triggering both login and refresh just to make sure it's right.

mbabker avatar Jul 25 '21 15:07 mbabker

I confirm I also had to add the controller part in the routes.yaml file to get the endpoint responding.

Another small point in the new Security/Authenticator/RefreshTokenAuthenticator.php there is the trigger deprecation copied from Service/RefreshToken.php

trigger_deprecation('gesdinet/jwt-refresh-token-bundle', '1.0', 'The "%s" class is deprecated, use the `refresh_jwt` authenticator instead.', RefreshTokenAuthenticator::class);

/**
 * @deprecated use the `refresh_jwt` authenticator instead
 */
class RefreshTokenAuthenticator extends AbstractGuardAuthenticator

michaoj avatar Aug 30 '21 09:08 michaoj

I tested the "v1.0.0-beta4" on 2 of my projects and everything works fine 👍

maxhelias avatar Sep 16 '21 13:09 maxhelias

Just installed v1.0.0-beta4 for a new project and all went (eventually) smoothly. Most of the non-smoothness was due to me not having used this bundle before and so I was investigating how things all work.

I did at one point run into an error of the type Unable to find the controller for path "/api/token/refresh". The route is wrongly configured.. I wasn't sure what I was doing wrong to know what to do right.

I resolved my self-inflicted configuration mishap by:

  1. Copying the routing, firewall and access control config from https://github.com/mbabker/refresh-token-tester
  2. Copying the functional test from https://github.com/mbabker/refresh-token-tester
  3. Verify that the configuration and functional test from https://github.com/mbabker/refresh-token-tester do indeed work within the context of my app (they indeed do)
  4. Duplicate the functional test
  5. Slowly swap in my own routes to the duplicated test (first login, see partial failure, than login and refresh and see full passing). This allowed me to carefully mutate the working config supplied by https://github.com/mbabker/refresh-token-tester into what I need for my app

No idea really what config mishap I made to begin with but everything works just fine now.

webignition avatar Sep 30 '21 10:09 webignition

Just installed v1.0.0-beta4 for a new project and all went (eventually) smoothly. Most of the non-smoothness was due to me not having used this bundle before and so I was investigating how things all work.

I did at one point run into an error of the type Unable to find the controller for path "/api/token/refresh". The route is wrongly configured.. I wasn't sure what I was doing wrong to know what to do right.

I resolved my self-inflicted configuration mishap by:

  1. Copying the routing, firewall and access control config from https://github.com/mbabker/refresh-token-tester
  2. Copying the functional test from https://github.com/mbabker/refresh-token-tester
  3. Verify that the configuration and functional test from https://github.com/mbabker/refresh-token-tester do indeed work within the context of my app (they indeed do)
  4. Duplicate the functional test
  5. Slowly swap in my own routes to the duplicated test (first login, see partial failure, than login and refresh and see full passing). This allowed me to carefully mutate the working config supplied by https://github.com/mbabker/refresh-token-tester into what I need for my app

No idea really what config mishap I made to begin with but everything works just fine now.

I had similar issue, in my case it helped to move in security.yaml

enable_authenticator_manager: true

under my api firewalls. So the structure something like this:

firewalls:
    dev: ...
    api: ....
    enable_authenticator_manager: true

sukhoy94 avatar Nov 16 '21 15:11 sukhoy94

Tried reproducing what @sukhoy94 suggested, only to find that I no longer need to explicitly configure the controller in the route.yaml.

My configuration now looks as follows:

security.yaml:
security:
    # https://symfony.com/doc/current/security/authenticator_manager.html
    enable_authenticator_manager: true

    # https://symfony.com/doc/current/security.html#c-hashing-passwords
    password_hashers:
        Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface: 'auto'
        Symfony\Component\Ldap\Security\LdapUser: 'auto'
        App\Entity\Employee: 'auto'

    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        chain_provider:
            chain:
                providers: [users_in_db, app_ldap]
        users_in_db:
            entity:
                class: App\Entity\Employee
                property: username
        app_ldap:
            ldap:
                service: Symfony\Component\Ldap\Ldap
                base_dn: '%env(resolve:LDAP_BASE_DN)%'
                search_dn: '%env(resolve:LDAP_RO_DN)%'
                search_password: '%env(resolve:LDAP_RO_PASSWORD)%'
                default_roles: ROLE_USER
                uid_key: uid

    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        api_token_refresh:
            pattern: ^/api/token/refresh
            stateless: true
            refresh_jwt:
                provider: chain_provider
        api_token_auth:
            pattern: ^/api/token/auth
            stateless: true
            provider: chain_provider
            json_login_ldap:
                service: Symfony\Component\Ldap\Ldap
                dn_string: 'uid={username},%env(resolve:LDAP_BASE_DN)%'
                check_path: /api/token/auth
                success_handler: lexik_jwt_authentication.handler.authentication_success
                failure_handler: lexik_jwt_authentication.handler.authentication_failure
                require_previous_session: false
        api_docs:
            pattern: ^/api/docs
            stateless: true
        api:
            pattern: ^/api
            stateless: true
            provider: chain_provider
            jwt: ~
        main:
            stateless: true
            lazy: true
            logout: ~

            # activate different ways to authenticate
            # https://symfony.com/doc/current/security.html#firewalls-authentication

            # https://symfony.com/doc/current/security/impersonating_user.html
            switch_user: false

    # This creates a simple hierarchy of roles so that one role can imply others
    role_hierarchy:
        ROLE_ADMIN:       ROLE_USER
        ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH]

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        - { path: ^/api/docs,  roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/api/token, roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/api,       roles: IS_AUTHENTICATED_FULLY }
routes.yaml:
index:
    path: /
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction
    defaults:
        path: /api/docs
        permanent: false

api_token_auth:
    path: /api/token/auth
    methods: ['POST']

api_token_refresh:
    path: /api/token/refresh
    methods: ['POST']

api_token_invalidate:
    path: /api/token/invalidate
    methods: ['POST']
    controller: App\Controller\InvalidateRefreshTokenController::invalidate

Jayfrown avatar Nov 16 '21 16:11 Jayfrown

Hi,

I haved problems configuring this bundle, release 1.0.0, if I define the firewall how it's documented I get the next error:

Not configuring explicitly the provider for the "refresh_jwt" authenticator on "api_token_refresh" firewall is ambiguous as there is more than one registered provider.

I found a solution with the security.yaml published by @Jayfrown before this comment. The problem was with refresh_jwt key.

PSF1 avatar Jan 20 '22 10:01 PSF1

IIRC, if your application has multiple user providers, you always have to explicitly set the “provider” key when either on the firewall or the authenticator; that’s a Symfony convention and not something unique to this bundle. All of the docs here are primed toward single authenticator apps which don’t require you to set that.

mbabker avatar Jan 20 '22 12:01 mbabker

Yes, setting provider key on either the firewall or the authenticator works. Just tried this out with an app that has four user providers.

Specifically, any of the following are valid if you have more than one user provider (some more sensible than others):

firewalls:
    firewall_one:
        refresh_jwt: ~
        provider: firewall_one_user_provider

    firewall_two:
        refresh_jwt:
            provider: firewall_two_user_provider

    firewall_three:
        refresh_jwt:
            provider: firewall_three_user_provider
        provider: firewall_one_user_provider

Clearly the latter example, despite being functionally valid, is really best avoided.

Since this has caused a tiny bit of confusion for at least one person, I'll create a tiny-as-possible PR to update the readme.

webignition avatar Jan 20 '22 14:01 webignition

PR is at #299

webignition avatar Jan 20 '22 15:01 webignition

I looked into the issue I was experiencing again, and found that by omitting the controller in the route.yaml, the route 404s if there is no refresh_token cookie set.

By adding the controller definition to the routes.yaml, the route works as expected even when there is no refresh_token cookie.

Without controller definition
api_token_refresh:
    path: /api/token/refresh
    methods: ['POST']
❯ curl -sk -o /dev/null --show-error --fail -X POST 'https://localhost/api/token/refresh'
curl: (22) The requested URL returned error: 404
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=invalid;'

{"code":401,"message":"JWT Refresh Token Not Found"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=<valid refresh token>;'
{"token":"<valid jwt token>"}
With controller definition
api_token_refresh:
    path: /api/token/refresh
    methods: ['POST']
    controller: gesdinet.jwtrefreshtoken::refresh
❯ curl -sk -X POST 'https://localhost/api/token/refresh'
{"code":401,"message":"Missing JWT Refresh Token"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=invalid;'

{"code":401,"message":"JWT Refresh Token Not Found"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=<valid refresh token>;'
{"token":"<valid jwt token>"}

It appears something in the new authenticator set-up does not trigger when the refresh_token cookie is not set. I have not tested this behavior in a scenario where cookies are not used, so it might be an issue I introduced here.

Thoughts? @mbabker @markitosgv

Jayfrown avatar Feb 04 '22 18:02 Jayfrown

I looked into the issue I was experiencing again, and found that by omitting the controller in the route.yaml, the route 404s if there is no refresh_token cookie set.

By adding the controller definition to the routes.yaml, the route works as expected even when there is no refresh_token cookie.

Without controller definition
api_token_refresh:
    path: /api/token/refresh
    methods: ['POST']
❯ curl -sk -o /dev/null --show-error --fail -X POST 'https://localhost/api/token/refresh'
curl: (22) The requested URL returned error: 404
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=invalid;'

{"code":401,"message":"JWT Refresh Token Not Found"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=<valid refresh token>;'
{"token":"<valid jwt token>"}
With controller definition
api_token_refresh:
    path: /api/token/refresh
    methods: ['POST']
    controller: gesdinet.jwtrefreshtoken::refresh
❯ curl -sk -X POST 'https://localhost/api/token/refresh'
{"code":401,"message":"Missing JWT Refresh Token"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=invalid;'

{"code":401,"message":"JWT Refresh Token Not Found"}
❯ curl -sk -X POST 'https://localhost/api/token/refresh' -H 'Cookie: refresh_token=<valid refresh token>;'
{"token":"<valid jwt token>"}

It appears something in the new authenticator set-up does not trigger when the refresh_token cookie is not set. I have not tested this behavior in a scenario where cookies are not used, so it might be an issue I introduced here.

Thoughts? @mbabker @markitosgv

See also my findings in https://github.com/markitosgv/JWTRefreshTokenBundle/issues/294#issuecomment-1030054498.

darthf1 avatar Feb 04 '22 19:02 darthf1

If I had to guess, the 404 is because the authenticator only reports as supporting the request if it can extract a token from the request and since the authenticator won't handle the request it falls through to needing a controller to handle the request. What probably needs to happen is we bring in the check_path config key (similar to other authenticators) then have the supports method just match the URL (and deprecate having the supports method match based on whether the token is in the request). Which, I was planning on sending up a PR at some point for that anyway because the refresh token authenticator shouldn't need to be on its own firewall and with that config the authenticator should both work properly AND the security config in general be much simpler (as in everything API should be able to be on one firewall):

security:
    firewalls:
        api:
            pattern: ^/api
            stateless: true
            jwt: ~
            json_login:
                success_handler: lexik_jwt_authentication.handler.authentication_success
                failure_handler: lexik_jwt_authentication.handler.authentication_failure
                check_path: api_login
            refresh_jwt:
                check_path: api_token_refresh

(Still gotta think through the B/C path for that though, but I've got a rough idea I just need to put into code)

mbabker avatar Feb 04 '22 19:02 mbabker

See https://github.com/markitosgv/JWTRefreshTokenBundle/pull/303 for the authenticator config update.

mbabker avatar Feb 06 '22 20:02 mbabker