RFC: Separate authorization and token endpoints #42

tux-rampage · 2018-05-25T07:55:13Z

According to the OAuth2 RFC 6749 the authorization and token endpoint provide two different concerns and should be separated.

Details

As of section 3.1, the authorization endpoint is designed
to Initiate an interactive authorization flow, where the user (resource owner) may be asked to authenticate against the authorization server as well as for consent to provide access to the requesting party. This endpoint will either redirect to the requesting party with an access code, the access token, or an error or should display an error page to the resource owner when the requesting party is invalid as defined in section 3.1.2.4. This endpoint must allow GET requests and may allow POST.

In contrast section 3.2, the token endpoint is designed for automated access token provision without user interaction. Usually this endpoint is used on the server side of the application to prevent access token exposure to the user agent.

With the current single middleware implementation consumers will face a bunch of issues:

If the consumer wants to accept POST for the authorization endpoint (i.e. to post the consent decision of the resource owner), this middleware cannot be used as is.
The consumer has no chance to intercept between validateAuthorizationRequest() and finishAuthorizationRequest() to provide the desired UserEntityInterface implementation or to set the approval flag.

Therefore this PR suggests to separate this MiddleWare (Which actually is a Request Handler) into 3 parts (Consider these classes are in namespace Zend\Expressive\Authentication\OAuth2):

TokenHandler - Provides the implementation of the OAuth2 Token endpoint.
AuthorizatonMiddleware - Prepares and validates the OAuth2 authrization request and passes it with the request object to the next handler(s)
AuthorizationHandler Uses the League\OAuth2\Server\RequestTypes\AuthorizationRequest from the request and completes the response (proper redirecting to the requesting party)

It is up to the consumer to provide the User entity and approval information to the authorization request instance via a middleware. This gives consumers the flexibility to handle this step in any way they need to fulfill their business needs.

Example:

<?php

use Zend\Expressive\Authentication\OAuth2;

$app->route('/oauth2/authorize', [
    OAuth2\AuthorizatonMiddleware,
    Application\MyOAuthAuthorizationMiddleware::class, // <- this ensures authentication, populates the user and handles conscent
    OAuth2\AuthorizationHandler
], ['GET', 'POST']);
$app->route('/oauth2/token', OAuth2\TokenHandler::class, ['POST']);

Are you creating a new feature?
- Why is the new feature needed? What purpose does it serve?
- How will users use the new feature?
- Base your feature on the develop (:warning: missing) branch, and submit against that branch.
- Add only one feature per pull request; split multiple features over multiple pull requests
- Add tests for the new feature.
- Add documentation for the new feature.
- Add a CHANGELOG.md entry for the new feature.
Is this related to quality assurance?

tux-rampage

I annotated my intentions in the changeset

tux-rampage · 2018-05-25T08:04:35Z

src/AuthFlowFactoryTrait.php

+ * Provides helper methods for authorization flow related factories
+ * @internal
+ */
+trait AuthFlowFactoryTrait


Even though this is discouraged by some, traits offer a solution to re-use code without building an inheritance hell. Plus, there is no official decision that forbids them.

This code is used in 3 factories and I don't see any benefit in duplicating this code three times which needs maintanence in three files if something needs to be changed in this code segment.

I don't know why git magically thinks this was moved from the OAuth2MittlewareFactory, it was not.

I don't think we need this Trait. A simple $container->get(AuthorizationServer::class) is enough in each factory class. All the checks present in AuthFlowFactoryTrait are performed by the service manager by default.

Agree. Getting rid of the additional checks will simplify the codebase and increase maintainability.

I just ported it from the existing OAuth2MiddlewareFactory, because I didn't know if there was a specific intend for the specialized exception.

src/AuthorizationHandler.php

src/AuthorizationMiddleware.php

tux-rampage · 2018-05-25T08:12:59Z

src/AuthorizationMiddleware.php

+            // authenticated user and the approval
+            $authRequest->setAuthorizationApproved(false);
+
+            return $handler->handle($request->withAttribute(AuthorizationRequest::class, $authRequest));


Pass the the auth request via the server request object to the next handler to complete it.
(ensure authentication, giving consent, etc ...). This gives consumers the freedom to act according to their business needs

src/AuthorizationMiddleware.php

tux-rampage · 2018-05-25T08:16:37Z

src/AuthorizationMiddlewareFactory.php

+use Psr\Http\Message\ResponseInterface;
+use function sprintf;
+
+class AuthorizationMiddlewareFactory


Changed the name fom OAuth2MiddlewareFactory according to the middleware and deduplicated code.

tux-rampage · 2018-05-25T08:19:40Z

src/TokenEndpointHandler.php

+
+        if (strtoupper($request->getMethod()) !== 'POST') {
+            return $response->withStatus(501); // Method not implemented
+        }


As for the Authorization endpoint, do we need this here, or should this be delegated to the
route() call? Anyways it should be consistent in both cases.

I think we can remove this, since you have splitted the authorization endpoint from the token endpoint.

src/OAuth2Middleware.php

ezimuel · 2018-05-30T12:01:49Z

@tux-rampage thanks for this PR, it's a great improvement from an architectural point of view.
I just reviewed it.
We need to fix and complete the uni tests. Let me know if you can complete the PR, otherwise I can work on it. Thanks again for your great work!

ezimuel

Please review my feedbacks, thanks!

tux-rampage · 2018-05-30T14:03:36Z

@ezimuel Thanks for the feedback. I'll review and update the PR accordingly the next days.

wshafer · 2018-05-30T19:24:12Z

Thanks for tackling this issue. This was something I was also attempting to look at myself. I'm glad you beat me to it. ;)

One thing that I was having an issue with was the serialization and storage of the outh2 auth request object. Perhaps I didn't see it, but how are you saving the request to be approved on approval requests, or how were you seeing this being addressed going forward?

tux-rampage · 2018-06-01T11:51:21Z

@wshafer, I had an interesting chat with @xtreamwayz in slack about the issue.

I'd not recommend to serialize the OAuth2 Request Object. That would require you to have knowledge of iternals of Leaque's OAuth2 library. To me that looks like a not so good idea.

One option would be to keep the HTTP-Request parameters and Redirect to the authorization endpoint with these parameters when ready. In the OpenID provider that I am currently implementing, I use Angular as Frontend, so it's a one-pager and I can just render the UI view (parameterized) when user interaction is required. Additionally I have a middleware converting HTTP 301/302 redirects to JSON 200 responses and the redirect info in the response body so Angular will handle it.

tux-rampage · 2018-06-01T17:21:58Z

I'll add some examples to the docs and drop a comment when this PR is ready for another review (there are still some tests missing).

tux-rampage · 2018-07-10T23:13:19Z

@ezimuel I've completed the PR, added docs and the CHANGELOG entries (feel free to rephrase - I'll apreciate any improvements).

Sorry that this took a bit longer and thanks for your patience.

The drop of coverage is caused by the @coversNothing that I added to the integration test (see http://phpunit.readthedocs.io/en/7.1/annotations.html#coversnothing and http://phpunit.readthedocs.io/en/7.1/code-coverage-analysis.html#specifying-covered-methods why). Before this the classes in Entities were reported to be test covered, which is actually not the case.

ezimuel · 2018-07-11T16:05:37Z

@tux-rampage thanks for this great work! I'll review it asap.

tux-rampage · 2018-09-02T08:12:58Z

@ezimuel Had an ytime to review this, yet? Let me know if I can support you.

ezimuel · 2018-09-21T13:17:27Z

@tux-rampage thanks for the hard work!

Separate authorization and token endpoints

ec97704

tux-rampage commented May 25, 2018

View reviewed changes

tux-rampage changed the title ~~RFC: Separate authorization and token endpoints (WIP)~~ RFC: Separate authorization and token endpoints May 29, 2018

ezimuel self-requested a review May 30, 2018 11:42

ezimuel suggested changes May 30, 2018

View reviewed changes

tux-rampage added 5 commits June 1, 2018 10:58

Update test cases

4178d45

Remove trait

f71faf4

Remove HTTP method check

bac7c6a

Add missing service config

2a74fa5

Fix coding style

755b8aa

tux-rampage added 7 commits July 9, 2018 21:22

Add auth handler test (wip)

4547811

Add missing authorization handler tests

9be2919

Add token endpoint handler tests

28497b2

fix coding style

b9f3775

Update docs

885d462

Remove unused imports

e315586

Add changelog entries

cee014a

ezimuel approved these changes Sep 19, 2018

View reviewed changes

ezimuel merged commit cee014a into zendframework:master Sep 21, 2018

ezimuel mentioned this pull request Oct 2, 2018

Added oauth2 data provided by thephpleague/oauth2-server to user object #50

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Separate authorization and token endpoints #42

RFC: Separate authorization and token endpoints #42

tux-rampage commented May 25, 2018 •

edited

Loading

tux-rampage left a comment

tux-rampage May 25, 2018

ezimuel May 30, 2018

tux-rampage May 30, 2018

tux-rampage May 25, 2018

tux-rampage May 25, 2018

tux-rampage May 25, 2018

ezimuel May 30, 2018

ezimuel commented May 30, 2018

ezimuel left a comment

tux-rampage commented May 30, 2018

wshafer commented May 30, 2018

tux-rampage commented Jun 1, 2018 •

edited

Loading

tux-rampage commented Jun 1, 2018

tux-rampage commented Jul 10, 2018

ezimuel commented Jul 11, 2018

tux-rampage commented Sep 2, 2018

ezimuel commented Sep 21, 2018

RFC: Separate authorization and token endpoints #42

RFC: Separate authorization and token endpoints #42

Conversation

tux-rampage commented May 25, 2018 • edited Loading

Details

Example:

tux-rampage left a comment

Choose a reason for hiding this comment

tux-rampage May 25, 2018

Choose a reason for hiding this comment

ezimuel May 30, 2018

Choose a reason for hiding this comment

tux-rampage May 30, 2018

Choose a reason for hiding this comment

tux-rampage May 25, 2018

Choose a reason for hiding this comment

tux-rampage May 25, 2018

Choose a reason for hiding this comment

tux-rampage May 25, 2018

Choose a reason for hiding this comment

ezimuel May 30, 2018

Choose a reason for hiding this comment

ezimuel commented May 30, 2018

ezimuel left a comment

Choose a reason for hiding this comment

tux-rampage commented May 30, 2018

wshafer commented May 30, 2018

tux-rampage commented Jun 1, 2018 • edited Loading

tux-rampage commented Jun 1, 2018

tux-rampage commented Jul 10, 2018

ezimuel commented Jul 11, 2018

tux-rampage commented Sep 2, 2018

ezimuel commented Sep 21, 2018

tux-rampage commented May 25, 2018 •

edited

Loading

tux-rampage commented Jun 1, 2018 •

edited

Loading