General questions about the library and its future

[sukihub]

Hello, I've been looking at this project lately. It's definitely a good start for me, but to be able to use it, I would need some changes done. So I guess what I'm asking is whether you are planing to develop and support this package in the future? I'll do the work either way, would you accept pull requests, or should I just create my own fork?

Specifically, I'm looking to improve following:

• security; if I'm not mistaken (and I might be), getting user id (owner id) from authorization request (here: https://github.com/uafrica/oauth-server/blob/master/src/Controller/OAuthController.php#L123) is a huge security problem; an attacker can basically authorize the client for any user (or all of them)
• there are hardcoded login routes https://github.com/uafrica/oauth-server/blob/master/src/Controller/OAuthController.php#L123 and Auth component offers nice ways how to get rid of it
• hardcoded model name https://github.com/uafrica/oauth-server/blob/master/src/Controller/OAuthController.php#L82 which can also be overriden by attacker to any random string (I dont see any risk in that now, but still)
• CakePHP 3.3 is now PSR-7 compatible, league/oauth2-server 5 also is, maybe we can use that

Thanks for the response ;)

[dakota]

Hi @sukihub,

Yes, we are planning on developing and supporting this package into the future, and will happily accept pull requests.

security; if I'm not mistaken (and I might be), getting user id (owner id) from authorization reques

I don't think this is a problem, the controller action is only hit when a user clicks on the "approve" or "deny" buttons on the oauth authentication page. As long as your application has the security and csrf components loaded, you should never see an issue here.

there are hardcoded login routes

Yes, those should probably be changed to use the configuration from the Auth component.

hardcoded model name

Easily changed by using your own authorize.ctp template to override the default one.

CakePHP 3.3 is now PSR-7 compatible, league/oauth2-server 5 also is, maybe we can use that

Actually, 3.3 only has a PSR-7 compatible middleware, we'll need to wait for 3.4 to be able to update to league/oauth2-server 5. I would also like to change this plugin to use middleware like the new cakephp [https://github.com/cakephp/authentication](authentication library)

[sukihub]

I don't think this is a problem, the controller action is only hit when a user clicks on the "approve" or "deny" buttons on the oauth authentication page.

Yes, exactly. If I'm attacker, I'd let authentication page show up (using my credentials) and before hitting "approve" just insert hidden input owner_id with id of any user. Plugin then approves the app not for me, but for given user id. Or am I missing something here?

Actually, 3.3 only has a PSR-7 compatible middleware, we'll need to wait for 3.4

Well, it's going to be here soon :) Cool, I like the idea.

[dakota]

Yes, exactly. If I'm attacker, I'd let authentication page show up (using my credentials) and before hitting "approve" just insert hidden input owner_id with id of any user.

The security component will immediately black hole the request if a hidden field has been modified. The reason this exists is for cases the the oauth is done for something other than a standard user. In our case, the oauth token exists for the entire account (Which has multiple users)

[Mirelath]

By relying on the behaviour of the security plugin, does this mean that having the security component on full protection is a requirement for this plugin?

[sukihub]

@Mirelath I guess yes.

@dakota I see. I currently have only very limited knowledge of OAuth, however it seems that the behavior you describe is specific to your application. I like the plugin, but maybe this doesn't need to be in it :) I'll share some ideas in a couple of days ;)

[sukihub]

Hello again :)

The security component will immediately black hole the request if a hidden field has been modified.

Yes. But consider this. We are an eshop and we want to allow 3rd party apps to access electronic content that our customers bought (ebooks, audiobooks). And I'm just terrified of having an option which would enable attacker to access content of other customers. I know that security component should prevent it, but what if there's a bug? Way to work around it? I would prefer not to have that option there at all, and I think most people would also.

So, here's my idea. It allows app to override ownerId and ownerModel using OAuthServer.beforeAuthorize event: https://github.com/sukihub/oauth-server/compare/feature/login-redirect...sukihub:feature/credentials?expand=1

The way to use it in your app would be:

EventManager::instance()->on('OAuthServer.beforeAuthorize', function () {
    return [
        'ownerId' => $this->request->data('owner_id') ?: $this->request->query('owner_id'),
        'ownerModel' => $this->request->data('owner_model') ?: $this->request->query('owner_model')
    ];
});

[dakota]

@sukihub That is a very good idea!