odan / session

A middleware oriented session handler for PHP and Slim 4+
https://odan.github.io/session/
MIT License
58 stars 11 forks source link

Provide a getStorage function for the PHPSession #26

Closed murilohpucci closed 2 years ago

murilohpucci commented 2 years ago

Hey! Great project!

I've been using it with Slim V4 and I was wondering to use this library with the slimphp/Slim-Csrf. I know both libraries are not supposed to work together, but it would be really nice to have function to get the storage from the session. I know there's #6 but right now the links provided by @odan are not available anymore.

Any way, I'm trying to do something like:

// dependencies.php
 'csrf' => function (ContainerInterface $c) {
      $responseFactory = $c->get(ResponseFactoryInterface::class);
      /** @var SessionInterface $session */
      $session = $c->get(SessionInterface::class);
      $storage = $session->getStorage();
      if (!$session->isStarted()) {
          $session->start();
      }
      return new Guard($responseFactory, 'csrf', $storage);
  },

This way we can pass the storage to the csrf library and make all the things work.

odan commented 2 years ago

Hi!

I see some issues with this approach. The DI container is only responsible to create the objects. The session itself should not be started within the DI container, because the session start is an HTTP context specific concern and belongs to a middleware. Second issue, is that the Guard class is not very middleware friendly or build with that in mind. Third issues are that the Guard class uses the hardly coded session_* functions, which makes it harder to write tests for it.

As a workaround, you could try to create the Csrf object without the storage and pass that storage afterwards within a custom CsrfMiddleware via the constructor.

DI container

use Psr\Container\ContainerInterface;
use Psr\Http\Message\ResponseFactoryInterface;
use Slim\Csrf\Guard;
// ...

return [
    // ...
    Csrf::class => function (ContainerInterface $container) {
        $responseFactory = $container->get(ResponseFactoryInterface::class);
        return new Guard($responseFactory);
    },
]

CsrfMiddleware

<?php

namespace App\Middleware;

use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Slim\Csrf\Guard;

final class CsrfMiddleware implements MiddlewareInterface
{
    private ResponseFactoryInterface $responseFactory;
    private Csrf $csrf;

    public function __construct(ResponseFactoryInterface $responseFactory, Csrf $csrf)
    {
        $this->responseFactory = $responseFactory;
        $this->csrf = $csrf;
    }

    public function process(
        ServerRequestInterface $request,
        RequestHandlerInterface $handler
    ): ResponseInterface {
        // Change storage
        $this->csrf->__invoke($this->responseFactory, 'csrf', $_SESSION);

        return $handler->handle($request);
    }
}
murilohpucci commented 2 years ago

Thanks for the answer!

The session itself should not be started within the DI container

Good catch! And thanks for providing this workaround, it's working pretty well. I think we can close both issue and the PR what do you think?

odan commented 2 years ago

I'm happy to hear that. Yes, we can close both.