markitosgv / JWTRefreshTokenBundle

Implements a Refresh Token system over Json Web Tokens in Symfony
MIT License
663 stars 159 forks source link

Testing before 1.0.0 release #255

Open markitosgv opened 3 years ago

markitosgv commented 3 years ago

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!!

pmikolajek commented 3 years ago

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.

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

mbabker commented 3 years ago

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).

markitosgv commented 3 years ago

@pmikolajek it must be fixed in beta3 release

pmikolajek commented 3 years ago

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

Jayfrown commented 3 years ago

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: ```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>)

mbabker commented 3 years ago

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).

Jayfrown commented 3 years ago

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

mbabker commented 3 years ago

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).

Jayfrown commented 3 years ago

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)

mbabker commented 3 years ago

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.

michaoj commented 3 years ago

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
maxhelias commented 3 years ago

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

webignition commented 3 years ago

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.

sukhoy94 commented 2 years ago

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
Jayfrown commented 2 years ago

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: ```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: ```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 ```
PSF1 commented 2 years ago

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.

mbabker commented 2 years ago

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.

webignition commented 2 years ago

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 commented 2 years ago

PR is at #299

Jayfrown commented 2 years ago

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 ```yaml 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=;' {"token":""} ```
With controller definition ```yaml 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=;' {"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

darthf1 commented 2 years ago

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 ```yaml 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=;' {"token":""} ```
With controller definition ```yaml 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=;' {"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.

mbabker commented 2 years ago

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 commented 2 years ago

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