Secured Links 2.0

[JanTvrdik]

This is a WIP of experimental redesign of Nextras Secured Links.

The new implementation (SecuredRouter) moved the logic from controls / presenters to router layer.

Benefits

• Does not require using trait in possibly multiple places.
• Can solve the problem pointed out by @dg, that user not author of the component should decide what should be secured.
• Can solve protecting actions @secured annotation does not work with actions #1, this is more difficult to implement in current design.

Problems

• Routers are AFAIK not originally designed (cc @dg) to return 403, returning NULL will result in 404 and may have security implications if user structures the routers badly (another router may match).
• I can find the target action* / handle* method (and therefore its annotation) in presenters but there is no way to find target handle* method inside components.
  • One way to solve this is to move from using @secured annotations to configuration passed to the router.
  • Another interesting idea is to not solve it. SecuredRouterFlagBased handles request signing and signature verification with request flags but not solve forcing flag existence and ensuring that request is generated with the flag.

Using configuration instead of @secured annotation.

The configuration may look sth like this.

return $this->securedRouterFactory->create($innerRouter, [
    'Admin:Product:delete!' => TRUE, // protect all params for handleDelete
    'Admin:Product:like!' => ['foo', 'bar'], // protect only foo and bar params for handleLike
]);

Or we may move it to neon configuration.

[enumag]

Routers are AFAIK not originally designed (cc @dg) to return 403

Can't you just throw a ForbiddenRequestException?

... but there is no way to find target handle* method inside components.

Actually there is. I needed to read @param annotations of handle methods in router to implement entities as parameters. The "downside" (personally I consider it a feature) is that it only works for components created by createComponent* method and all such methods need to have @return annotation (exception is thrown otherwise). You can see my implementation here. It's not PHP7-ready though.

[VaclavSir]

Ad error 403) IMHO router should match and return an application request to a special presenter, that throws the error. Something like this:

class BadRequestPresenter implements IPresenter
{

    /**
     * @inheritdoc
     */
    function run(Request $request)
    {
        throw new BadRequestException($request->getParameter('message'), $request->getParameter('code'));
    }

}

[JanTvrdik]

Can't you just throw a ForbiddenRequestException?

Yes I can, the question is whether it is IRouter contract violation or not.

it only works for components created by createComponent* method and all such methods need to have @return annotation

Good point, hopefully it will good enough. I'll try to implement it today. Possibly as CompilerExtension to avoid any runtime analysis.

router should match and return an application request to a special presenter

Yes, that is a nice trick I didn't thought about.

[JanTvrdik]

I feel that the implementation is now mostly finished, I just need to write more tests.

[enumag]

@JanTvrdik It look good. Except that I'd like to use those generators for future version of EntityLoader as well.

[hrach]

Probably we should not remove the old traits. Let's add a warning to theirs methods.

[JanTvrdik]

@hrach I'm still undecided on that but lean towards removing them. This is such a major rewrite that we will have to release it as a new major version (2.0). If you use both traits and new router in one project, it will break entirely. Therefore I don't see much benefit in keeping them in 2.0.

[hrach]

Yeah, that seems ok, but let me ask:

What if detection of some @secured annotations would fail (you have mentioned some cases, and I'm not sure everything is everytime doable and solved for now). It would be giant fail if user would update to newer version, remove the traits and some secured things would stop working because the reflection wouldn't be able traverse the tree down.

[JanTvrdik]

The new system relies on convention of method names action*, handle* and createComponent*. If the application does not use this convention, the system will fail. Additionally it requires createComponent* to have either return type or a @return annotation. Otherwise, it will fail.

Additionally to scanning classes and looking for @secured annotation, it provides a more reliable (but less convenient?) method. You can specify what request needs to be signed in config.neon as well.

nextras.securedLinks:
    destinations:
        Admin\ProductsPresenter: 
            delete!: true # protect all params for handleDelete
            kill: [foobar]  # protect all params except for param foobar for actionKill

It would be giant fail if user would update to newer version (…) and some secured things would stop working

Yes, that is a problem I don't know how to solve well, except for documenting it.

[JanTvrdik]

Yes, that is a problem I don't know how to solve well, except for documenting it.

Oh, I just figure out, how we can solve this. I'm amazing.

[hrach]

Additionally it requires createComponent* to have either return type or a @return annotation. Otherwise, it will fail.

I have ton of code which doesnt specify return type (neither phpdoc nor php 7 return type hint), beacuse it's totally useless. So my code will have to rely on the scanning & name convetion - is it right?

[JanTvrdik]

@hrach No, you're screwed. The name conventions and return type for createComponent is currently a requirement for the static analysis to work. I can add option to emit warning when createComponent without return type is found (currently it is just silently ignored). I can modify the current control trait to throw an exception if signal leading to secured handler is received but the request was not signed.

I wonder how difficult would to implement a simple code-flow analysis which would reliably deduce the return type (even when neither @return annotation nor return typehint is provided) in most cases.

[hrach]

In such case the exception is a must have. This could cause serius security issue.

[JanTvrdik]

I pushed a better version which throws exception if things go wrong and which can detect method return type even when neither return annotation or return type hint is presenter (requires nikic/php-parser)

[JanTvrdik]

Note: Nextras Secured Links 1.x used to sign only parameters in handle* method signature without default value. The new implementation signs the entire request (presenter + all parameters - ignored parameters).