Oauth2-server: Change AuthCodeGrant to make Templating optional

[Bobselp]

Hey,

I tried to switch to the new V5-RC1. The client credentials grant works fine, however I have an issue with the authentication code grant.

In my opinion, AuthCodeGrant->respondToAuthorizationRequest() tries to do too much in a single function, and even stuff that in my opinion doesn't belong in the oauth-server code: Logging the user in via a web-based form. Every page that has users already has a login form, maybe extra secured or whatnot. The current flow forces you to create yet another login form, or to make your current login form & template work with the way required by the oauth-server code.

I would propose that if no templateRenderer is provided to AuthCodeGrant::_construct, respondToAuthorizationRequest() returns at around line 90 of AuthCodeGrant.php. It is then up to the developer to make the login & app validation functions and assemble a UserEntity. Lines 193-226 (after the validation part in the server-code) get moved to an own function that can be called with the UserEntity to generate the URL to redirect to after the (custom) login stuff validated the user.
This kind of resembles the flow of V4, where you could call checkAuthorizeParams(), do the validation, and then call newAuthorizeRequest() with the user ID to generate the redirect URI with the code.

This is only a rough proposal, there certainly is a better way to solve this. I hope you understand the problem I have with the code that tries to re-do what every website already has! :)

Other than that, congratulations to V5, besides this it really looks awesome so far!

[alexbilbie]

Hi,

Thanks for getting in touch. I'm away until the 8th April, but will get
back to you as soon as I'm home.

Please could you flesh out this issue as much as you can so I can think
about it whilst I'm travelling and can better answer you when I'm back?

Thanks,

Alex
On Sun, 27 Mar 2016 at 21:37, Bobselp [email protected] wrote:

Hey,

I tried to switch to the new V5-RC1. The client credentials grant works
fine, however I have an issue with the authentication code grant.

In my opinion, AuthCodeGrant->respondToAuthorizationRequest() tries to do
too much in a single function, and even stuff that in my opinion don't
belong in the oauth-server code: Logging the user in via a web-based form.
Every page that has users already has a login form, maybe integrated on the
page. The current flow forces you to create yet another login form, or to
make your current login form & template work with the way required by the
oauth-server code.

I would propose that if no templateRenderer is provided to
AuthCodeGrant::_construct, respondToAuthorizationRequest() returns at
around line 90 of AuthCodeGrant.php. It is then up to the developer to
make the login & app validation functions and assemble a UserEntity.
Lines 193-226 (after the validation part in the server-code) get moved to
an own function that can be called with the UserEntity to generate the URL
to redirect to after the (custom) login stuff validated the user.
This kind of resembles the flow of V4, where you could call
checkAuthorizeParams(), do the validation, and then call
newAuthorizeRequest() with the user ID to generate the redirect URI with
the redirect URI.

This is only a rough proposal, there certainly is a better way to solve
this. I hope you understand the problem I have with the code that tries to
re-do what every website already has! :)

Other than that, congratulations to V5, besides this it really looks
awesome so far!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/thephpleague/oauth2-server/issues/499

[warlock39]

I have the same issue. I want to render oauth dialog by single page application, instead of render template on server side.

[frederikbosch]

This can be solved together with PR #513, if we change the signature of newHtmlResponse inside. I am willing to help, if my help is being appreciated.

[frederikbosch]

Then we could also remove AbstractAuthorizeGrant and by doing also reduce inheritance. I feel a grant has to too many layers: it implements, extends and uses (traits). This makes things too complex.

Though we must not move too much responsibility into the response factory. If it becomes too fat, we can inject delegated response handlers in the factory.

[warlock39]

@frederikbosch actually, i don't know what kind of problem your PR solves. It slighlty increases readability, incapsulates some details, adds some flexibility, i like it. But:

• if i need construct JWT with custom data, i can't
• if i don't need template rendering, it doesn't solves it
• if i don't want to use JWT, it is hard to replace with another implementation
• according to oauth spec, refresh token is optional, but it is hard to disable

Some of statements may relate to original implementation, yes.

[frederikbosch]

@warlock39 Wonderful feedback: constructive and critic. While some points were not part of the PR to solve, I will take 'em into account during update. Majority is valid and will be taken care of. Thanks again. Hope @alexbilbie will consider to merge.

[alexbilbie]

As soon as I'm home this weekend I will review all PRs and open issues :)
On Tue, 5 Apr 2016 at 05:48, Frederik Bosch [email protected]
wrote:

@warlock39 https://github.com/warlock39 Wonderful feedback:
constructive and critic. While some points were not part of the PR to
solve, I will take 'em into account during update. Majority is valid and
will be taken care of. Thanks again. Hope @alexbilbie
https://github.com/alexbilbie will consider to merge.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/thephpleague/oauth2-server/issues/499#issuecomment-205489719

[Bobselp]

Hey @frederikbosch. I looked at your PR #513, and I don't think it solves the issue I have. If I understand it correctly, your proposed way with optional/external templating is to create my own ResponseFactory class, which has custom code for newHtmlLoginResponse() and the other functions.
However, the whole user authentication & app approving still happens in AuthCodeGrant->respondToAuthorizationRequest(). I want this part completly seperated (on my main website!) from the OAuth Server Library. As explained, ideally I would call a method to check that the OAuth request itself is valid. For your PR, this would be from the start of respondToAuthorizationRequest() up until around line 104. Then, I want to do all user validation & app approving by myself, on my own website. After the user is validated, I want to call another method (with a UserEntity I assembled on my own) to finalize the OAuth Request and get the URI I should redirect the user to (line 171 up until the return).

For the V5 code, I suggested in the first post here that you could "toggle" to the alternative ("validate the user on your own") behaviour by checking if a $templateRenderer has been provided to AuthCodeGrant->__construct or not. For your PR, this looks a bit more complicated, since ResponseFactory seems to be necessary, even if you don't want to do _any_ HTML output by the API library.

[frederikbosch]

@Bobselp Thanks for the feedback. I get your points. Basically, you want this library to act as middleware in cases like AuthCodeGrant. You do not only want to inject a different renderer but you want to create a response yourself, right?

While I do think my PR gives a much better separation of concerns, I do agree that your request is not honoured by it. Let me think how to improve it as such that it solves your case too.

[Bobselp]

Basically, you want this library to act as middleware in cases like AuthCodeGrant. You do not only want to inject a different renderer but you want to create a response yourself, right?

Yes, exactly this. This is a better description then my "stuff that in my opinion doesn't belong in the oauth-server code". I get that the current way is convenient for some users, however it excludes other people that don't have their website set up the way the oauth-server code requires it, like me and @warlock39. So making it optional would be really nice.

[alexbilbie]

I agree; AuthCodeGrant->respondToAuthorizationRequest() does way too much and the whole templating thing adds additional bloat.

I've got a work in progress solution that I've come up with in another branch; will post a link here when I'm done

[alexbilbie]

I've added some new methods to the auth server and grants.

Please check out this example of the API for the auth code (I've not yet updated the implicit grant) - https://github.com/thephpleague/oauth2-server/blob/V5-authorization-request-flow/examples/public/auth_code.php#L61-L86

In my opinion this solves a number of problems:

1. Removes the requirement for the library to implement any templating for user login and authorization
2. If the user is already signed in, has already authorised the client or you want to auto-authorise the client then this new flow makes all of that possible
3. The library is dramatically simplified which I am all for
4. This implementation also solves #514

What do you think @warlock39 @Bobselp @frederikbosch ?

[Bobselp]

Hey @alexbilbie, the new flow looks very good in my opinion, and is exactly what I wanted. Much simpler and gives the developer more options on how to handle user sign in & app auth. Thank you for the fast change!

[alexbilbie]

I've decided to merge that branch in; I really like and prefer the new implementation.

I'm closing this issue now. Am going to cut an RC2 tag shortly

[frederikbosch]

Nice work @alexbilbie