silexphp / Silex

[DEPRECATED -- Use Symfony instead] The PHP micro-framework based on the Symfony Components
https://silex.symfony.com
MIT License
3.58k stars 718 forks source link

[SecurityServiceProvider] security.last_error starts the session if no error is found in the request #1127

Closed MeuhMeuh closed 9 years ago

MeuhMeuh commented 9 years ago

Hello,

I'm currently working with Silex and especially with the SecurityServiceProvider. I just found out that when retrieving the last error known by the security provider, it first checks for an error in the request, and if no error has been found, the provider then checks in the session, this way :

$app['security.last_error'] = $app->protect(function (Request $request) {
            if ($request->attributes->has(SecurityContextInterface::AUTHENTICATION_ERROR)) {
                return $request->attributes->get(SecurityContextInterface::AUTHENTICATION_ERROR)->getMessage();
            }

            $session = $request->getSession();
            // Comment added : here is the call implicitly starting the session
            if ($session && $session->has(SecurityContextInterface::AUTHENTICATION_ERROR)) {
                $error = $session->get(SecurityContextInterface::AUTHENTICATION_ERROR)->getMessage();
                $session->remove(SecurityContextInterface::AUTHENTICATION_ERROR);

                return $error;
            }
        });

It seems like a logical behavior but it is not when we can consider that a context when a user that hasn't been logged yet (for example) shouldn't handle a session.

For example, when I'm on my login form page, and before logging in, I don't want any session to be created, but I still want to be able to check for errors in the request. I know that I can still check for it "manually" simply by retrieving it from the request as the closure is doing, but to me, checking for an error related to the security shouldn't implicitly start a session because it's not the closure's role and expected behavior.

What do you think about this ?

Thanks !

stof commented 9 years ago

technically, the session is not started by $request->getSession() (this is a simple getter) but by $session->has()

MeuhMeuh commented 9 years ago

Oops, my mistake. I corrected it in the original question.

To me, It may actually be a "deeper" problem, because $session->has() shouldn't start the session either.

fabpot commented 9 years ago

I'm closing this issue as we have already the same on symfony/symfony with a lengthly discussion there. See symfony/symfony#6388 for instance.