Add support for the "authorization_code" grant type

[ajgarlag]

Fix #2

[codecov-io]

Codecov Report

Merging #18 into master will decrease coverage by 1.28%.
The diff coverage is 81.76%.

@@             Coverage Diff              @@
##             master      #18      +/-   ##
============================================
- Coverage     90.81%   89.52%   -1.29%     
- Complexity      280      341      +61     
============================================
  Files            43       52       +9     
  Lines           980     1146     +166     
============================================
+ Hits            890     1026     +136     
- Misses           90      120      +30

Impacted Files	Coverage Δ	Complexity Δ
Manager/InMemory/AuthorizationCodeManager.php	100% <100%> (ø)	3 <3> (?)
League/Repository/UserRepository.php	100% <100%> (ø)	3 <1> (ø)	⬇️
Event/AuthorizationRequestResolveEventFactory.php	100% <100%> (ø)	2 <2> (?)
DependencyInjection/TrikoderOAuth2Extension.php	86.48% <100%> (+1.33%)	25 <0> (+1)	⬆️
OAuth2Grants.php	100% <100%> (ø)	1 <0> (ø)	⬇️
Converter/UserConverter.php	100% <100%> (ø)	2 <2> (?)
Manager/Doctrine/AuthorizationCodeManager.php	100% <100%> (ø)	4 <4> (?)
DependencyInjection/Configuration.php	100% <100%> (ø)	9 <0> (ø)	⬇️
Controller/AuthorizationController.php	100% <100%> (ø)	4 <4> (?)
Event/AuthorizationRequestResolveEvent.php	46.8% <46.8%> (ø)	22 <22> (?)
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ad80d0...87475b4. Read the comment docs.

[ajgarlag]

@spideyfusion Could you review this PR?What do you think about the new event to resolve the authorization decision?

Thanks in advance!

[spideyfusion]

Great work thus far @ajgarlag! I definitely like the idea of having an event available where the developer has the chance to take complete control of the authorization process.

Could you give me a code example on how would you implement a simple decision page and an event listener which would appropriately approve or deny an authorization request?

[ajgarlag]

I've written a simple proof of concept (not ready for production) which uses the UriSigner class from the HttpKernel component.

https://gist.github.com/ajgarlag/1f84d29ee0e1a92c8878f44a902338cd

[MichaelKubovic]

Your implementation of authorization controller expects user to be already authenticated. The component demo says that you should first call validateAuthorizationRequest() and just then attempt authentication.

https://oauth2.thephpleague.com/authorization-server/auth-code-grant/

This allows SSO implementations where client (relying party in oidc terms) sends guests to authorization endpoint. The authorization endpoint validates request (authenticate against client). Then it should either re-use existing session or let user authenticate (could be redirect to route with form_login or any other configured firewall). The authentication on it's own is not in scope of OAuth, but we should be able to configure it.

Once logged in, user should be taken back to authorization endpoint to optionally authorize the client (this is where the event kicks in). Finally, user is redirected back with the authorization code and the flow continues on the RP side....

We could fix this easily with following changes:

• configure option to pickup previous session (check IS_AUTHENTICATED_REMEMBERED or stricter IS_AUTHENTICATED_FULLY)
• optionally configure login route
• store validated request in application session, redirect if login url configured, else throw unauthorized exception
• restore oauth authorization, either directly by appending callback query param to login url or by providing authentication listener that one has to use to pickup the flow and continue.

[ajgarlag]

Good catch, but I prefer a different way to fix it.

That check with the AuthorizationChecker is there from the first commit, before the AuthorizationRequestResolveEvent was introduced.

Now, I would move the responsibility of checking the user authentication and the responsibility of setting the UserEntity into the event, to two different event listeners. The first one will set the attribute required to process the AuthorizationRequest. I would put it to IS_AUTHENTICATED_REMEMBERED by default, but it could be overridden by configuration.

This will allow to prepend an EventListener from an OIDC implementation that will manage the prompt parameter.

What do you think?

[MichaelKubovic]

Sounds good to me. As long as we are able to plug-in any authentication provided by Symfony Security, I'd be happy :)

[ajgarlag]

Finally, I have decided to remove any authorization check from the authorization endpoint. It's not needed at all. It is up to you to decide how to protect the authorization endpoint.

Anyway, I have added a recommendation to the README to protect it, so that anonymous users cannot access to the authorization endpoint and cannot approve authorization requests.

[MichaelKubovic]

I believe this is not entirely correct. Anonymous users are welcome to authorization endpoint. See https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowSteps
It's just after you validated authorization request that you authenticate user. What I miss is redirect to login page if TokenStorage does not provide authenticated token.

[ajgarlag]

Yes, I know that, but remember that we are implementing here the authorization_code grant for an oAuth 2.0 server, not for an OIDC provider.

In an OIDC provider implementation, the security configuration should allow anonymous access to authorization endpoint, and an EventListener will have to manage the prompt parameter, and redirect to the login if needed; but in this PR we does not need anything like that.

Anyway, the access to authorization endpoint it's not limited by default. If you know what are you doing, you can omit the recommendation and allow anonymous users access.

[MichaelKubovic]

👍 @spideyfusion what do you think, ready to merge?

[ajgarlag]

@spideyfusion The PR is ready to merge for me.

[MichaelKubovic]

I've been working with the code in this PR and I dislike how the AuthorizationRequestResolve event in AuthorizationController is tightly coupled to the listener that reacts to it. I've tried to make it a bit more flexible, so I've moved the responsibility to create response or approve authorization request to the listener.

Furthermore I've encapsulated the authorization approval bit so that we can employ any other strategy.

I've created a PR against @ajgarlag 's feature branch so you could review it separately
https://github.com/ajgarlag/oauth2-bundle/pull/1/files

[ajgarlag]

@MichaelKubovic Thanks for your PR, I'll review it after all requested changes by @alenpokos are addressed.

[ajgarlag]

@alenpokos I've introduced some changes in the the AuthorizationRequestResolveEvent workflow.

The authorizationResolution is a boolean value, false by default. Instead of a resolutionUri the event can have a ResponseInterface.

The controller checks if the event has a ResponseInterface. If it has one, the controller should return that response. If the event has no response, the controller should resolve the authorization request with the authorizationResolution value.

When AuthorizationRequestResolveEvent::resolveAuthorization is called, the event discards the ResponseInterface if it exists.

[ajgarlag]

@alenpokos @spideyfusion Is there any pending change?

[MichaelKubovic]

ping, what's the state of this issue?

[froozeify]

Any news on this pr ?

[spideyfusion]

We'll take a look at resolving this PR by the end of the week. Sorry for the lack of communication, we've been extremely busy this past month.

[ajgarlag]

Next up, we'll focus on writing documentation in a separate PR and then we'll tag a new major release.

@spideyfusion before tagging a new major release, I'd like to include #21. I'll rebase it once #18 gets merged.

[Nyholm]

"or set the uri"

You mean or add a RedirectResponse to the event?

Question: If I redirect the user to a form where they accept or deny the claim. How do I get back?
Could you add some more documentation about this?

(Im currently testing this PR)
Thank you for you work!

[ajgarlag]

Yes, I mean "or set the response, (usually a redirect response), that will be returned to the user to resolve the authorization process"

Regarding your question, you can have a listener which can extract the user decision from the request. See #18 (comment)

All the discussion points have been addressed.

[Nyholm]

I did a quick review. But I have tried the implementation and I did not find any issues.

[spideyfusion]

Great work @ajgarlag! Next up, let's integrate #21 into the master branch.

[DonCallisto]

From what I see here, this bundle still misses the part of Authentication (that's not the scope of OAuth2.0 I know). I mean, even with the snippet of code provided above, you are implicitly saying that the Authorization Provider/Endpoint/Server (call it whatever you want) also is the reponsible for Authenticating the user, for displaying a consent dialog to him and for having some resources to provide.
That's quite troublesome from my POV as AuthenticationProvider could be even a different service, without any knowledge of the users as its main reason to exist is to provide token and verify that tose tokens are valid (whatever "valid" means here).
So basically, just to make an example, I can't provide a microservice responsible only for tokens and a different resource endpoint.
Am I right?

ps.: I've only tried to implement Authorization Code Grant so didn't saw the others.

[MichaelKubovic]

It's completely up to you how you implement the authentication flow for the user. The authorization flow implemented in this bundle relies on the user being set in TokenStorage. That's where AuthorizationRequestUserResolvingListener looks for it. It's up to you to populate token storage with authenticated Token. You could have your custom security firewall that delegates some other service.

[DonCallisto]

Well, so this means that also AuthorizationProvider has authenticated user (as long as them should be into TokenStorage. Moreover the Listener is something "syncronous", what if (and that's my case) I need the GET ..../authorization endpoint to be redirected to LoginProvider (call it this way, I don't have any better name as is not part of the protocol) and then to the ConsentProvider (ok, it could be the same of LoginProvider but just to give it different names).

We should be able to receive from ConsentProvider any kind of User id (not something that forces a user fetch from the DB!) in a JWT token or something like this. Of course that's not part of the protocol so any form of provider output should be accepted but the constraint of having a User here in this bundle means that AuthorizationProvider has access to user db (or should implement a service that can return some information used to "create" a User "on the fly" and authenticate it inside the AuthorizationProvider itself).

Looking at the controller for example https://github.com/trikoder/oauth2-bundle/blob/v2.x/Controller/AuthorizationController.php#L60-L65
we're not be able to implement any of this kind of login/consent providers that requires additional call to external "AuthenticationProviders".

It makes sense what I'm stating here?

[MichaelKubovic]

What I've done is, that I accept anonymous users on /authorization endpoint. I registered another event listener, that runs before the AuthorizationRequestUserResolvingListener and redirects appropriately. https://github.com/MichaelKubovic/oauth2-bundle/blob/develop/EventListener/AuthorizationRequestAuthenticationResolvingListener.php

You can take user to 3rd party service if you wish.

[DonCallisto]

@MichaelKubovic well, so how do you "restore" the user from the redirect? You need to have access to same db and you need to return with some information from the 3rd party (like an id).
And what about user consent? Is provided by the 3rd party after the login? And how do you return from the 3rd party login to the "original" /authorization call if is handled by the 3rd party?

[MichaelKubovic]

I do authentication within the same Symfony instance. But this bundle does not force you. For the consent flow, I have yet another listener hooked to the same authorization event.

It's not the responsibility of this bundle, neither the responsibility of oauth 2 to deal with details of authentication.

[DonCallisto]

It's not the responsibility of this bundle, neither the responsibility of oauth 2 to deal with details of authentication.

I know this but as long as you need a logged user inside this bundle I suppose that you're assuming that everything is tied togheter: AuthorizationRequest needs a UserEntityInterface in order to work, it should rely on some other thing like an identifier or something.

The fact that you're doint authentication inside the same symfony instance maybe don't let you grasp the problem or probably I'm explaining it in the wrong way.

[DonCallisto]

Take a look at this especially when it says

As you already know by now, ORY Hydra does not come with any type of user management (login, registration, ...).

That's the key aspect as User concept is not an OAuth concept.

[MichaelKubovic]

I see what confuses you. The interface that this bundle relies on, the Symfony\Component\Security\Core\User\UserInterface, is often used to implement user in the context of Symfony Security and it's related concepts. It's just convenient. It's a Symfony bundle after all. It does not imply authentication by security component. It implies instance of user must be provided by you.

Think of UserEntityInterface in your symfony service as a resolved identity. The object does not have to come from the database. It does not have to be used to really authenticate user. It just must exist in token storage for this bundle to work.

If you have authentication implemented elsewhere, you should know how to communicate with it. Do you federate to OIDC provider? Do you share cookie? Do you use some means of one-time access token? Up to you.

That's the key aspect as User concept is not a OAuth concept.

Disagree, the User is mentioned in spec many times. See https://tools.ietf.org/html/rfc6749#section-4.1. Spec says that the user should be authenticated through user agent.

[DonCallisto]

I know the key concepts of symfony, I just only mistaken the interface that's not from Symfony but from league bundle (the symfony one has also the getPassword method, meaning that you need some kind of authentication on it or at least that you need a password somehow).
Nevermind, I get your point.