Preventing Infinite Redirects in Access Control Library

Question

I am working on an Open Source Role Based Access Control Library for PHP called PHP-Bouncer. PHP-Bouncer allows the user to define a list of roles, which pages each role provides access to, and each role may also define a list of pages which override other pages (so going to the overridden page will redirect you to the overriding page). Here is an example of how this would work (From the Access Managed Example in the documentation):

$bouncer = new Bouncer();
// Add a role     Name,      Array of pages role provides
    $bouncer->addRole("Public", array("index.php", "about.php", "fail.php"));
// Add a role          Name,              Array of pages role provides
    $bouncer->addRole("Registered User", array("myaccount.php", "editaccount.php", "viewusers.php"));
// Add a role          Name,   Array of pages role provides       List of pages that are overridden by other pages
    $bouncer->addRole("Admin", array("stats.php", "manageusers.php"), array("viewusers.php" => "manageusers.php"));

// Here we add some users. The user class here extends the BouncerUser class, so it can still do whatever you
// would normally create a user class to do..
    $publicUser         = new User();
    $registeredUser     = new User();
    $adminUser          = new User();
    $registeredAndAdmin = new User();

    $publicUser->addRole("Public");

    $registeredUser->addRole("Public"); // We add the public group to all users since they need it to see index.php
    $registeredUser->addRole("Registered User");

    $adminUser->addRole("Public"); // We add the public group to all users since they need it to see index.php
    $adminUser->addRole("Admin");

    $registeredAndAdmin->addRole("Public"); // We add the public group to all users since they need it to see index.php
    $registeredAndAdmin->addRole("Registered User");
    $registeredAndAdmin->addRole("Admin");

    $bouncer->manageAccess($publicUser->getRoles(), substr($_SERVER["PHP_SELF"], 1), "fail.php");

Here's the problem I am having: In the manageAccess function you see above, everything works well as long as the roles are defined sanely, and all users have access to fail.php (or fail.php does not implement the $bouncer object). As soon as someone creates a role which has an inherent conflict (such as overriding a page with itself) or fails to give all users access to the failure page, the manageAccess function results in an infinite loop. Since this is bad, I would like to fix it. I am not sure however, what would be the best approach for allowing a few redirects (it is feasible that redirecting up to two or three times could be desired behaviour) while preventing an infinite loop. Here is the manageAccess function:

/**
 * @param array  $roleList
 * @param string $url
 * @param string $failPage
 */
public function manageAccess($roleList, $url, $failPage = "index.php"){
    $granted = false;
    foreach($roleList as $role){
        if(array_key_exists($role, $this->roles)){
            $obj = $this->roles[$role];
            /** @var $obj BouncerRole */
            $response = $obj->verifyAccess($url);
            if($response->getIsOverridden()){ // If access to the page is overridden forward the user to the overriding page
                $loc            = ($obj->getOverridingPage($url) !== false) ? $obj->getOverridingPage($url) : $failPage;
                $locationString = "Location: ".$loc;
                header($locationString);
                // I broke something in the last commit, perhaps this comment will help?
            }
            if($response->getIsAccessible()){ // If this particular role contains access to the page set granted to true
                $granted = true; // We don't return yet in case another role overrides.
            }
        }
    }
    // If we are here, we know that the page has not been overridden
    // so let's check to see if access has been granted by any of our roles.
    // If not, the user doesn't have access so we'll forward them on to the failure page.
    if(!$granted){
        $locationString = "Location: ".$failPage."?url=".urlencode($url)."&roles=".urlencode(serialize($roleList));
        header($locationString);
    }
}

Any Suggestions?

Answer 1

Since your desired functionality is to allow "a few redirects", the best way I can think of approaching this is creating either a $_SESSION (or $_GET I suppose would work) parameter that you increment every time you issue a redirect. I should point out that from a user standpoint this would be pretty annoying to deal with, but it is your website.

Let's assume we named the $_SESSION parameter num_redirects:

1. Before we redirect, check to see if $_SESSION['num_redirects'] is set. If it is not, set it to 0.
2. Get the current value of $_SESSION['num_redirects'], and see if it is above the redirect threshold.
3. If it is above the threshold, do not redirect, simply die();
4. If it is not above the threshold, increment $_SESSION['num_redirects'], store it back, and redirect.

So, that code would look like this (I've also improved your URL query string with a call to http_build_query():

Somewhere, we need to define the threshold:

define( 'MAX_NUM_REDIRECTS', 3);

Then, we can do:

// Assuming session_start(); has already occurred
if(!$granted) {
    if( !isset( $_SESSION['num_redirects'])) {
        $_SESSION['num_redirects'] = 0;
    }

    // Check if too many redirects have occurred
    if( $_SESSION['num_redirects'] >= MAX_NUM_REDIRECTS) {
        die( "Severe Error: Misconfigured roles - Maximum number of redirects reached\n");
    }

    // If we get here, we can redirect the user, just add 1 to the redirect count
    $_SESSION['num_redirects'] += 1;

    $query_string = http_build_query( array(
        'url' => $url,
        'roles' => serialize($roleList)
    ));
    $locationString = "Location: " . $failPage . '?' . $query_string;
    header($locationString);
    exit(); // Probably also want to kill the script here
}

That's it! Note that you'll have to add the same logic for the first call to header(). If you're going to repeat this functionality elsewhere, it might be worthwhile to encapsulate redirection within a helper function:

function redirect( $url, array $params = array()) {
    if( !isset( $_SESSION['num_redirects'])) {
        $_SESSION['num_redirects'] = 0;
    }

    // Check if too many redirects have occurred
    if( $_SESSION['num_redirects'] >= MAX_NUM_REDIRECTS) {
        die( "Severe Error: Maximum number of redirects reached\n");
    }

    // If we get here, we can redirect the user, just add 1 to the redirect count
    $_SESSION['num_redirects'] += 1;

    $query_string = http_build_query( $params);
    $locationString = "Location: " . $url . ( !empty( $query_string) ? ('?' . $query_string) : '');
    header($locationString);
    exit(); // Probably also want to kill the script here
}

Then, you can call it like this:

if(!$granted){
    redirect( $failPage, array(
        'url' => $url,
        'roles' => serialize($roleList)
    ));
}

This way you can encapsulate all of the logic necessary to redirecting within that one function.