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

Allow setting advanced cookie name by dedicated configuration attribute #1180

Closed
wants to merge 2 commits into from

Conversation

webda2l
Copy link

@webda2l webda2l commented Dec 5, 2023

Allow configurations like:

lexik_jwt_authentication:
    ...
    token_extractors:
        split_cookie:
            enabled: true
            cookies:
                - jwt_hp
                - jwt_s
    set_cookies:
        jwt_hp:
            customName: '%env(JWT_COOKIE_PREFIX)%jwt_hp'
            ...
        jwt_s:
            customName: '%env(JWT_COOKIE_PREFIX)%jwt_s'
            ...

With Gesdinet, it's already possible with:

gesdinet_jwt_refresh_token:
    token_parameter_name: '%env(JWT_COOKIE_PREFIX)%refresh_token'
    ...

@maxhelias
Copy link
Contributor

Hummm.. It's already possible, the attributes key is the name. ex : jwt_hp & jwt_s

@webda2l
Copy link
Author

webda2l commented Dec 6, 2023

lexik_jwt_authentication:
    ...
    token_extractors:
        split_cookie:
            enabled: true
            cookies:
                - '%env(JWT_COOKIE_PREFIX)%jwt_hp'
                - '%env(JWT_COOKIE_PREFIX)%jwt_s'
    set_cookies:
        '%env(JWT_COOKIE_PREFIX)%jwt_hp':
            lifetime: 2592000
           ...
        '%env(JWT_COOKIE_PREFIX)%jwt_s':
            lifetime: 2592000
           ...

works but IMO, when the cookie naming require a more advanced syntax like in my case, it's less conventional and clean than using a dedicated attribute.
My proposition is optional for ease some advanced cases only.

And the current and basic attribute key for basic naming remains useful for service naming https://github.com/lexik/LexikJWTAuthenticationBundle/blob/2.x/DependencyInjection/LexikJWTAuthenticationExtension.php#L125 and in the token_extractors.[cookie|split_cookie] part.

@webda2l webda2l changed the title Allow setting cookie name by configuration Allow setting advanced cookie name by dedicated configuration attribute Dec 6, 2023
@maxhelias
Copy link
Contributor

maxhelias commented Dec 6, 2023

For advanced syntax, I'd recommend using the parameters instead.

In your case : you can setting a customName and attributes key which have 2 different behaviors. The token extractors doesn't use the customName but the attributes key and the cookie name isn't the attributes key, it's a mix and imo these changes bring more complexity to the bundle.

@chalasr
Copy link
Collaborator

chalasr commented Dec 22, 2023

I agree with @maxhelias this looks rather confusing. Thanks for the PR, I hope to see you again for another.

@chalasr chalasr closed this Dec 27, 2023
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.

3 participants