Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

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

Merged

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Oct 1, 2018

This PR adds more data to the authenticated user stored in the PSR-7 UserInterface attribute. I added all the information provided here by thephpleague/oauth2-server.

This PR uses the zend-expressive-authentication ^1.0, providing the a User Factory instead of the previous User trait implementation.

I also fixed the documentation and added the authenticated user section to report this new feature.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good; mostly docs changes. I'll do a final edit prior to merging, but some of these need to be addressed first.

CHANGELOG.md Outdated

### Added

- [#41](https://github.com/zendframework/zend-expressive-authentication-oauth2/pull/41) Allows
existing PDO service to be used. This will allow us to reuse existing pdo
services instead of opening up a second connection for oauth.
- [#42](https://github.com/zendframework/zend-expressive-authentication-oauth2/pull/42) Adds `TokenEndpointHandler`,
`AuthorizationMiddleware` and `AuthorizationHandler` in the `Zend\Expressive\Authentication\OAuth2` namespace
to [implement an authorization server](docs/book/authorization-server.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link this to the actual page on docs.zendframework.com. Also, that page is now under a /v1/ segment...

# Authenticated user

Once the user is authenticated, `zend-expressive-authentication-oauth2` stores
the user's credential in a PSR-7 attribute under the name `Zend\Expressive\Authentication\UserInterface`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not storing the user's credential, though, but rather the username and any authorization details, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's true. It stores the following information:

[
    'oauth_user_id' => /* user's identifier (string) */,
    'oauth_client_id' => /* the client id (string) */,
    'oauth_access_token_id' => /* the access token id (string) */,
    'oauth_scopes' => /* the scopes (mixed, usually an array) */
]

I'll rephrase the sentence.

```

You can retrieve all these values using `getDetails()` or `getDetail($name)`
functions of `UserInterface`. Here is reported an examples:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase:

You may retrieve these values en masse using the getDetails() method or individually using the getDetail($name) method of the user instance. As an example:

`UserInterface`. To customize the user object you need to change the service
with name `Zend\Expressive\Authentication\UserInterface` pointing to your
implementation. For instance, you can point to a `CustomUserFactory` class
that returns a `CustomUser` object (that implements `UserInterface`) as follows:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above is confusion. May I recommend the following?

If you wish to provide a custom Zend\Expressive\Authentication\UserInterface implementation, you will need to provide:

  • a custom implementation of the the interface
  • a factory capable of generating instances of that interface
  • a DI factory for generating the previous factory
  • configuration wiring the UserInterface service to your factory.

The factory noted in the second step should be a callable with the following signature:

function (
    string $identity,
    array $roles = [],
    array $details = []
) : Zend\Expressive\Authentication\UserInterface

As an example of the factory in the third point, you will create a standard DI factory to return it. It could, for instance, compose a database adapter to pull information and create your custom user implementation:

class CustomUserFactory
{
    public function __invoke(Psr\Container\ContainerInterface $container) : callable
    {
        $db = $container->get(Zend\Db\Adapter\AdapterInterface::class);
        return function (string $identity, array $roles = [], array $details = []) use ($db) : Zend\Expressive\Authentication\UserInterface {
            // get some data from $db
            // return a new instance
            return new MyCustomUserType(/* ... */);
        });
    }
}

You will then need to wire this factory to the UserInterface service, per the following example:

and then you would have the example you detail below for configuration.

for your application.

Since there are authorization flows that require user interaction,
your application is expected to provide the middleware to handle this.

## Add the token endpoint

This is the most simple part, since this library provides
`Zend\Expressive\Authentication\OAuth2\TokenHandler` to deal with it.
This is the most simple part, since this library provides
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid words like "simple" and "easy" in the documentation. When a user doesn't find the instructions simple or easy, they then feel they are not smart enough to use the code.

The above can be rephrased to something like:

Adding the token endpoint involves routing to the provided Zend\Expressive\Authentication\OAuth2\TokenHandler.

@@ -68,46 +68,46 @@ use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Psr\Http\Message\ResponseInterface;
use League\OAuth2\Server\RequestTypes\AuthorizationRequest;
use Zend\Expressive\Authentication\UserInterface;

class OAuthAuthorizationMiddleware implements MiddlewareInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is there any reason we should NOT provide this middleware by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This middleware is related to the business logic of the application and can vary from case to case. Maybe, we should think to provide an example for a common scenario.

{
// Assume a middleware handled the authentication check and
// populates the user object which also implements the
// OAuth2 UserEntityInterface
$user = $request->getAttribute('authenticated_user');

$user = $request->getAttribute(UserInterface::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document this change in the CHANGELOG.md as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually a documentation error introduced by #42. The correct attribute for user was UserInterface::class and not authenticated_user.

@@ -2,15 +2,16 @@ docs_dir: docs/book
site_dir: docs/html
pages:
- index.md
- Introduction: intro.md
- Usage: usage.md
- "Authorization server": authorization-server.md
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note: because these pages are already published, we need to provide them in the repository as redirects. You can see how this is done in the zend-expressive repo; it involves having pages with a JS redirect, and then having _-prefixed pages in the mkdocs.yml file that point to them (so they don't end up in the TOC).

$clientId = $result->getAttribute('oauth_client_id', null);
if (isset($userId) || isset($clientId)) {
return ($this->userFactory)(
$userId ?? $clientId ?? '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since one or the other of these has to be set (see previous conditional), this can be just $userId ?? $clientId.

@ezimuel
Copy link
Contributor Author

ezimuel commented Oct 2, 2018

@weierophinney I updated the PR including all your feedbacks. Thanks for the review!

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezimuel I've pushed documentation edits to your branch; it's ready to merge and tag whenever you're available.

@ezimuel ezimuel merged commit 15f2f88 into zendframework:master Oct 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants