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

After handler is always called even if the security check failed #503

Closed cma closed 11 years ago

cma commented 12 years ago

When a security check fails (user enters wrong password, e.g.), no before handlers are called. However, after handlers are always called in this case. Isn't this inconsistent?

A simple example is below:

<?php
$loader = require __DIR__ . '/../vendor/autoload.php'; // use composer autoloader

use Silex\Application;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

$app = new Application();

$app->before(function () {
  syslog(LOG_INFO, 'before handler is called');
});

$app->after(function () {
  syslog(LOG_INFO, 'after handler is called');
});

// set up a simple http basic auth firewall
$app->register(new \Silex\Provider\SecurityServiceProvider);
$app['security.firewalls'] = array(
  'home' => array(
    'pattern' => '.*',
    'http' => true,
    'users' => array( 'user1' => array('ROLE_USER', '112431243')),
  ),
);

$app->get('/', function() {
  syslog(LOG_INFO, "Can never reach here without a valid password.");
});

$app->run();

// Only after handler is called when http basic auth fails. Example:
/* curl -i http://api-dev/  ===> 
HTTP/1.1 401 A Token was not found in the SecurityContext.
Server: nginx/1.2.3
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.4.6-2~lucid+1
cache-control: no-cache
date: Thu, 04 Oct 2012 04:56:22 GMT
www-authenticate: Basic realm="Secured"

in syslog:
Oct  4 06:41:25 lucid64 ool api-dev[17561]: after handler is called
*/
fabpot commented 12 years ago

Yes, this is an issue but one that is not easy to fix. The before event is called too late. It should be called as early as possible, but right now, it is called almost as late as possible (if we only look at the built-in events). Moving the event earlier breaks some expectation. So, we need to investigate further what is the right way to fix this and what are the BC break consequences.

For future reference, here is the default built-in listener priorities: http://symfony.com/doc/master/reference/dic_tags.html#core-event-listener-reference

cma commented 12 years ago

Thanks for the quick response and the link. I'll dig into that.

I'm new to both Silex and Symfony (around 2 months). So my original understanding was that the security firewall sits in front of application (http://symfony.com/doc/2.0/_images/security_ryan_no_role_admin_access.png). As a result, I assume that before hander, controller, and after handler are the 'application' part. So none of the handlers should be called if security check fails.

However, you expect both before handler and after handler should be called in this case, e.g., for every request.

To me, both interpretations make sense. As long as they are consistent, it'll be better.

fabpot commented 11 years ago

Code and docs have been updated to be more consistent and flexible (PR #525).