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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a request extractor API to replace the static method for extracting the refresh token from the request #256

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 13, 2021

Fixes #95

RequestRefreshToken::getRefreshToken() is a static method which makes it pretty difficult to customize the behavior for extracting the refresh token from the request data (as essentially you need to replace all services calling it with a custom implementation of those services then add a custom handler for pulling data from the request). To get around this limitation, a new request extractor API (inspired by the similar API in LexikJWTAuthenticationBundle) is introduced with this PR and the current static method removed.

The default implementation that is aliased to the new interface is a chain extractor that will iterate over all extractors until finding a token. Similar to the current behavior, null is returned from the extractor API if a token can't be found.

Extractors can be added in applications by implementing Gesdinet\JWTRefreshTokenBundle\Request\Extractor\ExtractorInterface (which is configured to support autowiring and automatically tagging the class with the gesdinet_jwt_refresh_token.request_extractor container tag). The tag also supports priorities, so applications can prioritize the extractors as desired.

services:
    gesdinet.jwtrefreshtoken.request.extractor.request_body:
        class: Gesdinet\JWTRefreshTokenBundle\Request\Extractor\RequestBodyExtractor
        tags:
            - { name: gesdinet_jwt_refresh_token.request_extractor, priority: 25 }

@markitosgv
Copy link
Owner

markitosgv commented Jul 13, 2021

@mbabker What do you think about remove instead of marking as deprecated because we're going to release it in a BC major update (v1.0) , maybe we need to introduce a CHANGELOG.md

And it would be interesting to add a section in readme telling how to use extractor

@mbabker
Copy link
Contributor Author

mbabker commented Jul 13, 2021

Changelog file started, removed the old class, and updated the README.

@markitosgv markitosgv merged commit cb744d0 into markitosgv:master Jul 15, 2021
@mbabker mbabker deleted the token-extractor branch July 15, 2021 12:34

$services->set('gesdinet.jwtrefreshtoken.request.extractor.request_body')
->class(RequestBodyExtractor::class)
->public()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to have the extractors in public, the chain is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you’re probably right. That was most likely just excessive copy/paste.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a detail, don't listen me 😄
It's a great evolution in any case, good job !

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

Successfully merging this pull request may close these issues.

Header based refreshToken
3 participants