Skip to content

[HttpFoundation] Add Psr6SessionHandler #35643

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[HttpFoundation] Add Psr6SessionHandler #35643

wants to merge 2 commits into from

Conversation

atailouloute
Copy link
Contributor

@atailouloute atailouloute commented Feb 7, 2020 •
edited by nicolas-grekas
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets https://github.com/orgs/symfony/projects/1#card-30499487
License MIT
Doc PR TODO

Add a PSR6 based session handler
By using this handler we can benefit from the cache SodiumMarshaller implemented in #35019, to encrypt/decrypt session data

Here is an example of how this can be used:

# services.yaml
services:
    # ...
    Symfony\Component\HttpFoundation\Session\Storage\Handler\Psr6SessionHandler:
        arguments:
            $cache: '@cache.app'
# packages/framework.yaml
framework:
    # ...
    session:
        handler_id: Symfony\Component\HttpFoundation\Session\Storage\Handler\Psr6SessionHandler

Task list

  • Add unit test
  • Add doc

/cc @nicolas-grekas

@atailouloute atailouloute force-pushed the http-foundation-add-psr6-session-handler branch from 9f2c3c5 to 2d7df8a Compare February 7, 2020 22:01
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 7, 2020
@nicolas-grekas
Copy link
Member

Thanks for working on this. Please compare your implementation to #23321. We'd need to ensure all failures of the cache adapter are properly turned to exceptions.

@atailouloute
Copy link
Contributor Author

atailouloute commented Feb 8, 2020 •
edited
Loading

I compared both implementations, and technically they are nearly the same

I also noticed that the Psr6 handler is rejected twice as some cache adapter are by-design unreliable for session handling (lack of locking ...)

As the main benefit is to be able to encrypt session data, wdyt about experimenting also a SodiumSessionHandler?

@atailouloute atailouloute force-pushed the http-foundation-add-psr6-session-handler branch from 2d7df8a to b36629f Compare February 8, 2020 21:42
@derrabus
Copy link
Member

I think that remark here is still valid: #19193 (comment)

A cache can rightfully loose items. A session handler can't.

@atailouloute atailouloute force-pushed the http-foundation-add-psr6-session-handler branch 3 times, most recently from ec32114 to 65a2a47 Compare February 20, 2020 13:45
nicolas-grekas
nicolas-grekas reviewed Feb 20, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I think that remark here is still valid: #19193 (comment)

A cache can rightfully loose items. A session handler can't.

yep, that's still a valid argument. We can decide to ignore it, but we'd need a reason.
Before trying to do so, would it make sense to use another approach?
The linked card says:

Add SodiumSessionHandler to encrypt session data XOR add a PSR-6 handler

I'm now thinking about a 3rd way: adding a MarshallingSessionHandler that transforms the session data string before passing it to the decorated session handler:

new MarshallingSessionHandler($someAbstractSessionHandler, $someMarshallerInterfaceInstance)

That'd allow reusing the code of the marshallers in the Cache component, which is the real motivation for this card.

WDYT?

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/Psr6SessionHandler.php
if ($item->isHit()) {
return $item->get();
}
$item = $this->cache->getItem($this->prefix.$sessionId);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of fetching the item twice, the class should accept a 2nd pool as argument, which would act as a factory for empty items. When this 2nd pool is missing, we would default to the NullAdapter from the Cache component - or throw a LogicException when it's missing.

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/Psr6SessionHandler.php
*/
public function gc($lifetime)
{
// not required here because cache will auto expire the records anyhow.
Copy link
Member

Choose a reason for hiding this comment

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

actually, we should use PrunableInterface when possible for this purpose

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/Psr6SessionHandler.php
*/
public function updateTimestamp($sessionId, $data)
{
$cacheItem = $this->cache->getItem($this->prefix.$sessionId);
Copy link
Member

Choose a reason for hiding this comment

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

this one is quite unfortunate - we don't have a way to update a TTL without fetching the whole value...

@atailouloute atailouloute force-pushed the http-foundation-add-psr6-session-handler branch from 65a2a47 to 73e15e0 Compare February 20, 2020 15:57
@atailouloute atailouloute force-pushed the http-foundation-add-psr6-session-handler branch from 73e15e0 to 1b4db2f Compare February 20, 2020 15:58
@atailouloute atailouloute mentioned this pull request Feb 20, 2020
Merged
1 task
@atailouloute
Copy link
Contributor Author

Closed in favor of #35804

fabpot added a commit that referenced this pull request Feb 25, 2020
@fabpot
feature #35804 [HttpFoundation] Added MarshallingSessionHandler (atai…
28a249f 
…louloute)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[HttpFoundation] Added MarshallingSessionHandler

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | TODO

Added `MarshallingSessionHandler`, a decorator for session handlers which uses the cache marshaller in order to encrypt session data.

(This is an alternative solution to #35643)

To use it, we can simply decorate the session marshaller, after that all session data will be encrypted

```yaml
Symfony\Component\Cache\Marshaller\SodiumMarshaller:
    decorates: 'session.marshaller'
    arguments:
        - ['%env(file:resolve:SODIUM_DECRYPTION_FILE)%']
        - '@symfony\Component\Cache\Marshaller\SodiumMarshaller.inner'
```

TODO:
- [x] unit tests

Commits
-------

155d980 [HttpFoundation][Cache] Added MarshallingSessionHandler
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Bug Feature HttpFoundation Status: Needs Review
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
5 participants
@atailouloute @nicolas-grekas @derrabus @carsonbot @Nyholm