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

Add set_cookies option to store JWT in secure cookies #753

Merged
merged 1 commit into from
May 28, 2020

Conversation

chalasr
Copy link
Collaborator

@chalasr chalasr commented May 27, 2020

Fixes #646

  • More tests
  • Documentation

@@ -75,6 +75,19 @@ public function getConfigTreeBuilder()
->info('If null, the user ID claim will have the same name as the one defined by the option "user_identity_field"')
->end()
->append($this->getTokenExtractorsNode())
->arrayNode('set_cookie')
Copy link

Choose a reason for hiding this comment

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

Could it be possible to set several cookies, or to configure the name per firewall? For instance Mercure requires to set a cookie containing a JWT names mercureAuthorization, but the app or other subsystems could need another token stored in another cookie.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Node renamed to set_cookies which now takes an arbitrary number of cookies indexed by name, accepting the cookie lifetime, path, domain and samesite attributes as options.
Adding a firewall option in another PR is possible, in case setting the cookie path is not enough. Thanks!

@chalasr chalasr changed the title Add set_cookie option to store JWT in secure cookies Add set_cookies option to store JWT in secure cookies May 28, 2020
@chalasr chalasr closed this in 886315f May 28, 2020
@chalasr chalasr merged commit 886315f into lexik:master May 28, 2020
@chalasr chalasr deleted the secure-jwt-cookie branch May 28, 2020 23:07
Comment on lines +104 to +109
->setDefinition($id = "lexik_jwt_authentication.cookie_provider.$name", new ChildDefinition('lexik_jwt_authentication.cookie_provider'))
->replaceArgument(0, $name)
->replaceArgument(1, $attributes['lifetime'] ?: ($config['token_ttl'] ?: 0))
->replaceArgument(2, $attributes['samesite'])
->replaceArgument(3, $attributes['path'])
->replaceArgument(4, $attributes['domain']);

Choose a reason for hiding this comment

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

Why you dont allow to set the secure above the config?
So you can set the value for secure default = true
and the Developer can change it for development

chalasr added a commit that referenced this pull request Aug 7, 2020
…" or not (Mael-91)

This PR was merged into the 2.x-dev branch.

Discussion
----------

Added the possibility to choose if the cookie is "secure" or not

I have added the possibility to choose if the cookie is "secure" or not. This allows when we work in localhost or on a development server that is not in HTTPS that the cookie is created.

I encountered this problem today and I saw that it was not available which is a pity.

See [753](#753 (comment))

Have a nice evening!

Commits
-------

764fa03 Added the possibility to choose if the cookie is "secure" or not
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a Set-Cookie header if 'cookie' is enabled?
3 participants