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

A new IdentityInterface for authentication #39

Open
ezimuel wants to merge 2 commits into
base: master
Choose a base branch
from
Open

A new IdentityInterface for authentication #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c85f760
Updated CHANGELOG for 1.0.1
ezimuel Sep 28, 2018
2c725d3
Added IdentityInterface to manage user and client identity scenarios
ezimuel Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,29 @@

All notable changes to this project will be documented in this file, in reverse chronological order by release.

## 1.0.1 - TBD
## 1.0.2 - TBD

### Added

- Nothing.

### Changed

- Nothing.

### Deprecated

- Nothing.

### Removed

- Nothing.

### Fixed

- Nothing.

## 1.0.1 - 2018-09-28

### Added

Expand Down
4 changes: 2 additions & 2 deletions src/AuthenticationInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
interface AuthenticationInterface
{
/**
* Authenticate the PSR-7 request and return a valid user
* Authenticate the PSR-7 request and return a valid identity
* or null if not authenticated
*/
public function authenticate(ServerRequestInterface $request) : ?UserInterface;
public function authenticate(ServerRequestInterface $request) : ?IdentityInterface;
Copy link
Member

@michalbundyra michalbundyra Mar 21, 2019

Choose a reason for hiding this comment

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

This is BC Break, imho.

Anyone who has implement that interface need update code to change the RTH in implementation of that method.

Copy link
Contributor Author

@ezimuel ezimuel Mar 21, 2019

Choose a reason for hiding this comment

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

Thechnically speaking yes, but you need to change only the return type of AuthentictionInterface::authenticate() to IdentityInterface. Your existing code that returns a UserInterface object will remain as is.

This code shows that is possible:

declare(strict_types=1);

interface IdentityInterface {
    public function getIdentity(): string;
}

interface UserInterface extends IdentityInterface {
    public function getRoles() : iterable;
}

interface ClientInterface extends IdentityInterface {
    public function getScopes() : iterable;
}

interface AuthenticationInterface
{
    public function authenticate(string $param): ?IdentityInterface;
}

class Auth implements AuthenticationInterface
{
    public function authenticate(string $param): ?IdentityInterface {
        return new class implements UserInterface {
            public function getIdentity(): string {
                return 'identity';
            }
            public function getRoles() : iterable {
                return [];
            }
        };
    }
}

$a = new Auth(); // no error here

I think this can be an acceptable (small) BC breaks and we can document it in the release note.

Copy link
Member

@michalbundyra michalbundyra Mar 21, 2019

Choose a reason for hiding this comment

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

@ezimuel

Technically speaking yes, but you need to change only the return type

It is not my decision, but in my opinion we shouldn't release it in minor release. If someone was using it as it was it's going to stop working after running composer update.
For me the question is simply: do we really follow semver or not?

Copy link
Member

@froschdesign froschdesign Mar 21, 2019

Choose a reason for hiding this comment

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

@ezimuel

I think this can be an acceptable (small) BC breaks

"small" or "big" doesn't matter.

@webimpress

For me the question is simply: do we really follow semver or not?

We follow the semantic versioning!

Copy link
Member

@froschdesign froschdesign Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ezimuel

…we can document it in the release note.

The goal should be: It must work without a user having to read the release notes or documentation. (mini and minor version!)

Copy link
Contributor Author

@ezimuel ezimuel Mar 21, 2019

Choose a reason for hiding this comment

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

@froschdesign thanks for explain me how it works the versioning!!!

Copy link
Contributor Author

@ezimuel ezimuel Mar 21, 2019

Choose a reason for hiding this comment

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

@weierophinney it's up to you, for me we can release it in a minor/major, no matter. I was just trying to improve the existing code with minimum (one line change code) impact (btw, only if a user has implemented a specific interface). @webimpress and @froschdesign we don't leave in a perfect world.

Copy link
Contributor Author

@ezimuel ezimuel Mar 21, 2019

Choose a reason for hiding this comment

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

@froschdesign and @webimpress I removed the "without BC break" in the title and added BC break label.

Copy link
Member

@froschdesign froschdesign Mar 21, 2019

Choose a reason for hiding this comment

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

@ezimuel

thanks for explain me how it works the versioning!!!

I'm sorry, this is and was not my intention. I have only seen the outcry from users which run an update via Composer and did not read the release notes.

Sorry for the misunderstanding!

Copy link
Contributor Author

@ezimuel ezimuel Mar 21, 2019

Choose a reason for hiding this comment

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

@froschdesign it's ok. I would have to wait 10 minutes before answering (busy day). I see your point here and I actually agree, this is a BC break (even if it's a really small one).


/**
* Generate the unauthorized response
Expand Down
9 changes: 6 additions & 3 deletions src/AuthenticationMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ public function __construct(AuthenticationInterface $auth)
*/
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface
{
$user = $this->auth->authenticate($request);
if (null !== $user) {
return $handler->handle($request->withAttribute(UserInterface::class, $user));
$identity = $this->auth->authenticate($request);
if (null !== $identity) {
if ($identity instanceof UserInterface) {
return $handler->handle($request->withAttribute(UserInterface::class, $identity));
}
return $handler->handle($request->withAttribute(IdentityInterface::class, $identity));
}
return $this->auth->unauthorizedResponse($request);
}
Expand Down
18 changes: 18 additions & 0 deletions src/IdentityInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php
/**
* @see https://github.com/zendframework/zend-expressive-authentication for the canonical source repository
* @copyright Copyright (c) 2017-2018 Zend Technologies USA Inc. (https://www.zend.com)
* @license https://github.com/zendframework/zend-expressive-authentication/blob/master/LICENSE.md New BSD License
*/

declare(strict_types=1);

namespace Zend\Expressive\Authentication;

interface IdentityInterface
{
/**
* Get the unique identity
*/
public function getIdentity() : string;
}
7 changes: 1 addition & 6 deletions src/UserInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,8 @@

namespace Zend\Expressive\Authentication;

interface UserInterface
interface UserInterface extends IdentityInterface
{
/**
* Get the unique user identity (id, username, email address or ...)
*/
public function getIdentity() : string;

/**
* Get all user roles
*
Expand Down
27 changes: 24 additions & 3 deletions test/AuthenticationMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Psr\Http\Server\RequestHandlerInterface;
use Zend\Expressive\Authentication\AuthenticationInterface;
use Zend\Expressive\Authentication\AuthenticationMiddleware;
use Zend\Expressive\Authentication\IdentityInterface;
use Zend\Expressive\Authentication\UserInterface;

class AuthenticationMiddlewareTest extends TestCase
Expand All @@ -27,7 +28,8 @@ public function setUp()
{
$this->authentication = $this->prophesize(AuthenticationInterface::class);
$this->request = $this->prophesize(ServerRequestInterface::class);
$this->authenticatedUser = $this->prophesize(UserInterface::class);
$this->user = $this->prophesize(UserInterface::class);
$this->identity = $this->prophesize(IdentityInterface::class);
$this->handler = $this->prophesize(RequestHandlerInterface::class);
}

Expand Down Expand Up @@ -59,10 +61,29 @@ public function testProcessWithAuthenticatedUser()
{
$response = $this->prophesize(ResponseInterface::class);

$this->request->withAttribute(UserInterface::class, $this->authenticatedUser->reveal())
$this->request->withAttribute(UserInterface::class, $this->user->reveal())
->willReturn($this->request->reveal());
$this->authentication->authenticate($this->request->reveal())
->willReturn($this->authenticatedUser->reveal());
->willReturn($this->user->reveal());
$this->handler->handle($this->request->reveal())
->willReturn($response->reveal());

$middleware = new AuthenticationMiddleware($this->authentication->reveal());
$result = $middleware->process($this->request->reveal(), $this->handler->reveal());

$this->assertInstanceOf(ResponseInterface::class, $result);
$this->assertEquals($response->reveal(), $result);
$this->handler->handle($this->request->reveal())->shouldBeCalled();
}

public function testProcessWithAuthenticatedIdentity()
{
$response = $this->prophesize(ResponseInterface::class);

$this->request->withAttribute(IdentityInterface::class, $this->identity->reveal())
->willReturn($this->request->reveal());
$this->authentication->authenticate($this->request->reveal())
->willReturn($this->identity->reveal());
$this->handler->handle($this->request->reveal())
->willReturn($response->reveal());

Expand Down