Skip to content
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.

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

Detect lack of authorization checks #13

Open
DavidMikeSimon opened this issue Apr 9, 2014 · 3 comments
Open

Detect lack of authorization checks #13

DavidMikeSimon opened this issue Apr 9, 2014 · 3 comments

Comments

@DavidMikeSimon
Copy link
Contributor

Using a pattern like in http://symfony.com/doc/current/cookbook/event_dispatcher/before_after_filters.html we should allow app to mark controllers which should authorize all actions, and then check to make sure that an authorization check was made. There should also be a way for actions to explicitly declare that they do not require authorization.

@evillemez
Copy link
Contributor

@DavidMikeSimon You can use the "defaults" param to specify extra request attributes on a per-route basis. Something like this:

some_route_name:
    path: /hello/{name}
    defaults:
        _controller: AcmeHelloBundle:Hello:index
        _kalinka:
            guard: app-controller-guard
            action: someKalinkaActionName

The listener would then just do something like:

<?php

public function onKernelController($event)
{
    $req = $event->getRequest();

    if (!$check = $req->attributes->get('_kalinka', false)) {
        return;
    }

    $auth = $this->container->get('kalinka.authorizor');
    if (!$auth->can($check['guard'], $check['action'], $req)) {
        throw new HttpException(403, "Computer says no. :(");
    }

    //say this req was authorized if we really need to check for it elsewhere
    $req->attributes->set('_kalinka_authorized', true);
}

There shouldn't need to be any step for checking if a call was made. If there really does need to be, we could just add arbitrary app-level attributes on the request in the pre-controller stage in the listener, and check for them later:

<?php
//...elsewhere
if (!$request->attributes->has('_kalinka_authorized')) {
    //throw new ...
}

@DavidMikeSimon
Copy link
Contributor Author

@evillemez We are going to have to define those checks for actions outside of the methods at some point anyways, if we want to allow OPTIONS checking. Probably it will be necessary to have some kind of separate method that does the check, since the number of different things we might need to do to decide if an action is authorized is too great to be expressed by just config parameters.

Anyways, the reason I want the external check is not to actually implement auth logic, but as a double check on us, that we didn't forget it on an action. Having the auth stuff be annotated or implemented outside the actual actions solves that problem nicely anyways, so that's another benefit.

@DavidMikeSimon
Copy link
Contributor Author

After discussion, Evan and I think we probably shouldn't do this in the above way...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants