-
Notifications
You must be signed in to change notification settings - Fork 20
Added oauth2 data provided by thephpleague/oauth2-server to user object #50
Added oauth2 data provided by thephpleague/oauth2-server to user object #50
Conversation
There was a problem hiding this 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). |
There was a problem hiding this comment.
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...
docs/book/v1/authenticated-user.md
Outdated
# 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`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
docs/book/v1/authenticated-user.md
Outdated
``` | ||
|
||
You can retrieve all these values using `getDetails()` or `getDetail($name)` | ||
functions of `UserInterface`. Here is reported an examples: |
There was a problem hiding this comment.
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 thegetDetail($name)
method of the user instance. As an example:
docs/book/v1/authenticated-user.md
Outdated
`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: |
There was a problem hiding this comment.
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\UserInterfaceAs 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.
docs/book/v1/authorization-server.md
Outdated
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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).
src/OAuth2Adapter.php
Outdated
$clientId = $result->getAttribute('oauth_client_id', null); | ||
if (isset($userId) || isset($clientId)) { | ||
return ($this->userFactory)( | ||
$userId ?? $clientId ?? '', |
There was a problem hiding this comment.
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
.
@weierophinney I updated the PR including all your feedbacks. Thanks for the review! |
There was a problem hiding this 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.
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.