laminas / laminas-authentication

provides an API for authentication and includes concrete authentication adapters for common use case scenarios
https://docs.laminas.dev/laminas-authentication/
BSD 3-Clause "New" or "Revised" License
24 stars 16 forks source link

[ZF3] new Authentication #8

Open weierophinney opened 4 years ago

weierophinney commented 4 years ago

Hi everyone,

Authentication in ZF2 has always been a bit brocken and hard to use. One of its main flow is notably its reliance on the global Request object.

Dasprid has done some work on this in the past (https://github.com/Bacon/BaconAuthentication/blob/master/README.md), it would be awesome if someone could take over this work, port it to Diactoros...

Ping @danizord, interested in taking this one? :)


Originally posted by @bakura10 at https://github.com/zendframework/zend-authentication/issues/5

weierophinney commented 4 years ago

Authentication in ZF2 has always been a bit brocken and hard to use.

I need a LOT more description than this. In what ways is it broken? In what ways is it hard to use? What use cases are difficult or impossible to implement currently? How would you like to use it?

One of its main flow is notably its reliance on the global Request object.

I struggle to understand this assertion. The AuthenticationService does not use either the request or response object, nor does it access any superglobals. You pass it a storage adapter at instantiation — either a session-based one or a null adapter, currently — and later pass an authentication adapter, and it proxies to that to perform authentication and deliver an authentication result.

The HTTP adapter does use a request and response object, but these need to be passed to it during instantiation; it is not using global instances. It would be trivial to update it to PSR-7.

(Note: the factories used by Apigility to create an AuthenticationService instance with an HTTP adapter, however, would need to be updated to pass the request and response instances at invocation, instead of pulling them from the service manager. However, those factories are part of Apigility, and not present in zend-authentication, which makes that argument out-of-scope!)

So, please: can you elaborate, @bakura10 ?


Originally posted by @weierophinney at https://github.com/zendframework/zend-authentication/issues/5#issuecomment-125228543

weierophinney commented 4 years ago

Hi @weierophinney ,

I remember that I had a hard time writing authentication in ZfrOAuth2 due to the architecture of the component. The main problem is that the authentication adapter is not given nor the request nor response: https://github.com/zendframework/zend-authentication/blob/master/src/AuthenticationServiceInterface.php#L22

On contrary, see the @DASPRiD 's refactor: https://github.com/Bacon/BaconAuthentication/blob/master/src/BaconAuthentication/AuthenticationServiceInterface.php#L27

This has one main issue is that I had to inject a Request in my Authentication Service to being able to properly do my work. Not only this favors having the request as a service, but it will also have some problems with the PSR-7 interfaces, as injecting a request in factory no longer guarantees me that the request/response I receive when the auth service is called is actually the current ones.

I really like the idea of being able to create my validator adapter without having to care to inject the Request, and just receive them when the validation must occurs. Really make things clearer.

Having said that you provide a possible fix to the issue, but it would be nice if we could fix this architecture flaw to make it easier to consume.


Originally posted by @bakura10 at https://github.com/zendframework/zend-authentication/issues/5#issuecomment-125336329

weierophinney commented 4 years ago

@bakura10 @weierophinney

I don't know much about the PSR-7 specifications and the involved changes regarding them but from my point of view, passing an HTTP message to the authentication service would be a mistake.

Authentication service should stay loosely coupled. Think that some could inject an authentication adapter which doesn't have any interest for an HTTP message. This could be the case of any command line tool interface providing an authentication layer. Strange you say? Yes it is but.... People always like to do strange things.

Anyway, It is always possible to inject your HTTP message into your authentication adapter using a factory or by injecting an event from which you can retrieve your HTTP message.

Furthermore, allow to set the authentication adapter after instantiation as it is done right now is a great thing. This covers use case scenarios where several adapters are used and set at runtime. Another way would be to implement a chainable interface for adapters.

The only thing with which I disagreed in the current implementation is the Resolver interface for HTTP adapters, which from my point of view, should be moved on, and be a bit more generic.

For instance, in my project, I want uses several adapters (Form adapter, HTTP adapter, OAuth adapter...). From my point of view, an adapter should not be aware of data store (it should not know how to resolve credentials). Instead, it should always use a resolver that is responsible to resolve credentials by looking up the client's identity from a data store (flat files, RDBMS, LDAP ...).

To give you a concret idea, let's talk about your Doctrine ObjectRepository adapter. From my point of view, this adapter should be a configurable resolver which could be used by many adapters.

Something like:

namespace Zend\Authentication\Resolver;

/**
 * Interface CredentialsResolverInterface
 */
interface CredentialsResolverInterface
{
    /**
     * Resolve authentication credentials by looking up client's identity in
     * a data store.
     *
     * @param string $identity
     * @param string $credential
     * @return mixed Resolved credentials, FALSE otherwise
     */
    public function resolve($identity, $credential);
}
namespace Zend\Authentication\Resolver;

use Doctrine\Common\Persistence\ObjectManager;

/**
 * Class ObjectRepositoryCredentialsResolver
 */
class ObjectRepositoryCredentialsResolver implements CredentialsResolverInterface
{
    /**
     * @var ObjectManager
     */
    protected $objectManager;

    /**
     * @var string Identity class
     */
    protected $identityClass;

    /**
     * @var string Identity property
     */
    protected $identityProperty;

    /**
     * @var string Credential property
     */
    protected $credentialProperty;

    /**
     * Constructor
     *
     * @param ObjectManager $objectManager
     * @param string $identityClass
     * @param string $identityProperty
     * @param string $credentialProperty
     */
    public function __constructor(
        ObjectManager $objectManager,
        $identityClass,
        $identityProperty,
        $credentialProperty
    ) {
        $this->objectManager = $objectManager;
        $this->identityClass = (string)$identityClass;
        $this->identityProperty = (string)$identityProperty;
        $this->credentialProperty = (string)$credentialProperty;
    }

    /**
     * Resolve authentication credentials using an object repository
     *
     * @param string $identity
     * @param string $credential
     * @return array containing Identity object and credential, FALSE otherwise
     */
    public function resolve($identity, $credential)
    {
        if (empty($identity)) {
            throw new \InvalidArgumentException('Identity is required');
        }

        if (empty($credential)) {
            throw new \InvalidArgumentException('Credential is required');
        }

        $identityRepository = $this->objectManager->getRepository($this->identityClass);
        $identity = $identityRepository->findOneBy([$this->identityProperty => $identity]);

        if (!$identity) {
            return false;
        }

        $credentialProperty = $this->credentialProperty;
        $getter = 'get' . ucfirst($credentialProperty);

        if (!is_callable([$identity, $getter])) {
            throw new \UnexpectedValueException(sprintf(
                'Property (%s) in (%s) is not accessible. You should implement %s::%s()',
                $credentialProperty,
                get_class($identity),
                get_class($identity),
                $getter
            ));
        }

        return ['identity' => $identity, 'credential' => $identity->$getter()];
    }
}

Thus here, many of my authentication adapters can use that resolver.

HTTP basic/digest, DbTable, ObjectRepository.... resolvers should be better to have that a concret adapter which is aware of the data store.

Furthermore with this approach, we could have a resolver chain, allowing an adapter to resolve credential from many places, including remote data store:

namespace Zend\Authentication\Resolver;

/**
 * Interface CredentialsResolverChainInterface
 */
interface CredentialsResolverChainInterface extends CredentialsResolverInterface
{
    /**
     * Add a credentials resolver in the chain
     *
     * @param CredentialsResolverInterface $resolver
     * @return self
     */
    public function addResolver(CredentialsResolverInterface $resolver);
}

I hope you get the idea.


Originally posted by @nuxwin at https://github.com/zendframework/zend-authentication/issues/5#issuecomment-164681743