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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Nov 13, 2018

This PR is a work in progress to solve the issue about OAuth2 client authentication reported in zendframework/zend-expressive-authentication-oauth2#55. The idea is to use a general IdentityInterface as follows:

namespace Zend\Expressive\Authentication;

interface IdentityInterface
{
    /**
     * Get the unique identity
     */
    public function getIdentity() : string;
}

And a specific UserInterface that extends the IdentityInterface, as follows:

namespace Zend\Expressive\Authentication;

interface UserInterface extends IdentityInterface
{
    /**
     * Get all user roles
     *
     * @return Iterable
     */
    public function getRoles() : iterable;

    /**
     * Get a detail $name if present, $default otherwise
     */
    public function getDetail(string $name, $default = null);

    /**
     * Get all the details, if any
     */
    public function getDetails() : array;
}

Regarding the AuthenticationMiddleware, this PR generates a UserInterface PSR-7 attribute if authenticate($request) returns an instance of UserInterface. Otherwise, it will returns a IdentityInterface attribute.

These changes should prevent BC breaks for existing implementations using zend-expressive-authentication and offers a new solution to zend-expressive-authentication-oauth2 for implementing a ClientInterface (extending IdentityInterface).

@weierophinney
Copy link
Member

@ezimuel Is there still work to do on this patch?

@marc-mabe
Copy link
Member

sorry for the super late response - LGTM 👍

@ezimuel
Copy link
Contributor Author

ezimuel commented Mar 21, 2019

@ezimuel Is there still work to do on this patch?

@weierophinney this PR is ready, of course we need to implement a new ClientInterface for zend-expressive-authentication-oauth2 to solve zendframework/zend-expressive-authentication-oauth2#55

@ezimuel ezimuel changed the title WIP: A new IdentityInterface for authentication, without BC breaks A new IdentityInterface for authentication, without BC breaks Mar 21, 2019
Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@ezimuel please see my comment, I think this PR contains BC break.

* or null if not authenticated
*/
public function authenticate(ServerRequestInterface $request) : ?UserInterface;
public function authenticate(ServerRequestInterface $request) : ?IdentityInterface;
Copy link
Member

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

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

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

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

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

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

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

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

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

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).

@ezimuel ezimuel changed the title A new IdentityInterface for authentication, without BC breaks A new IdentityInterface for authentication Mar 21, 2019
@weierophinney
Copy link
Member

I've got this and the related one for oauth2 on my todo list - not sure if I'll have time to evaluate them this week, or if it will be early next. I'll drop a note with what route I go (new minor or new major) when I do.

@weierophinney
Copy link
Member

This repository has been closed and moved to mezzio/mezzio-authentication; a new issue has been opened at mezzio/mezzio-authentication#2.

@weierophinney
Copy link
Member

This repository has been moved to mezzio/mezzio-authentication. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone mezzio/mezzio-authentication to another directory.
  • Copy the files from the second bullet point to the clone of mezzio/mezzio-authentication.
  • In your clone of mezzio/mezzio-authentication, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants