Remove the challenge method from the AuthenticationMethodInterface

[klsoft-web]

Proposed new feature or change

Remove the challenge method from the AuthenticationMethodInterface. These methods violate the Interface segregation principle and lead to.
Code duplication:
https://github.com/yiisoft/user/blob/master/src/Method/ApiAuth.php#L22
https://github.com/yiisoft/user/blob/master/src/Method/WebAuth.php#L28

Redundunt code:
https://github.com/yiisoft/user/blob/master/src/Method/ApiAuth.php#L31
https://github.com/yiisoft/auth/blob/master/src/Method/HttpCookie.php#L38
https://github.com/yiisoft/auth/blob/master/src/Method/HttpHeader.php#L46
https://github.com/yiisoft/auth/blob/master/src/Method/QueryParameter.php#L35

Proposing refactoring:

//Yiisoft\Auth\AuthenticationMethodInterface.php
interface AuthenticationMethodInterface
{
    public function authenticate(ServerRequestInterface $request): ?IdentityInterface;
}

//Yiisoft\Auth\Middleware\Authentication.php;
class Authentication implements MiddlewareInterface
{
    // ...

    public function __construct(
        private AuthenticationMethodInterface $authenticationMethod,
        private RequestHandlerInterface $authenticationFailureHandler
    ) {
    }

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        $identity = $this->authenticationMethod->authenticate($request);
        $request = $request->withAttribute(self::class, $identity);

        if ($identity === null && !$this->isOptional($request)) {
            return $this->authenticationFailureHandler->handle($request);
        }

        return $handler->handle($request);
    }

    // ...
}

//Yiisoft\Auth\Handler\HttpBasicFailureHandler.php;
final class HttpBasicFailureHandler implements RequestHandlerInterface
{
    private string $realm = 'api';

    public function __construct(private AuthenticationFailureHandler $authenticationFailureHandler)
    {
    }

    public function handle(ServerRequestInterface $request): ResponseInterface
    {
        $response = $this->authenticationFailureHandler->handle($request);
        return $response->withHeader(Header::WWW_AUTHENTICATE, "Basic realm=\"{$this->realm}\"");;
    }

    public function withRealm(string $realm): self
    {
        $new = clone $this;
        $new->realm = $realm;
        return $new;
    }
}

//Yiisoft\Auth\Middleware\HttpBasicAuthentication.php;
final class HttpBasicAuthentication extends Authentication
{
    public function __construct(
        HttpBasic $httpBasic,
        HttpBasicFailureHandler $httpBasicFailureHandler
    ) {
        parent::__construct($httpBasic, $httpBasicFailureHandler);
    }
}

[samdark]

Valid. Could be done in a minor version without a backwards compatibility break like this:

 interface AuthenticatorInterface
  {
      public function authenticate(ServerRequestInterface $request): ?IdentityInterface;
  }

  interface ChallengeInterface
  {
      public function challenge(ResponseInterface $response): ResponseInterface;
  }

  /**
   * @deprecated Use AuthenticatorInterface and optionally ChallengeInterface.
   */
  interface AuthenticationMethodInterface extends AuthenticatorInterface, ChallengeInterface
  {
  }

Then in the next versions we can deprecate/remove AuthenticationMethodInterface.

[klsoft-web]

It's a good idea to introduce the AuthenticatorInterface for backwards compatibility.
But what about the authentication middleware? Is this valid?

//Yiisoft\Auth\Middleware\AuthenticationMiddleware.php;
class AuthenticationMiddleware implements MiddlewareInterface
{
    // ...

    public function __construct(
        private AuthenticationMethodInterface $authenticationMethod,
        private RequestHandlerInterface $authenticationFailureHandler
    ) {
    }

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        $identity = $this->authenticationMethod->authenticate($request);
        $request = $request->withAttribute(self::class, $identity);

        if ($identity === null && !$this->isOptional($request)) {
            return $this->authenticationFailureHandler->handle($request);
        }

        return $handler->handle($request);
    }

    // ...
}

/**
 * @deprecated Use the AuthenticationMiddleware instead 
 */
final class Authentication implements MiddlewareInterface
{
}

[samdark]

Not like that.

HttpBasic (sending WWW-Authenticate), HttpBearer, Composite, and yiisoft/user WebAuth (redirecting to login) need challenge().

So something like:

public function __construct(
    private AuthenticatorInterface $authenticator,
    private RequestHandlerInterface $authenticationFailureHandler,
) {}

public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
    $identity = $this->authenticator->authenticate($request);
    $request = $request->withAttribute(self::class, $identity);

    if ($identity === null && !$this->isOptional($request)) {
        $response = $this->authenticationFailureHandler->handle($request);

        return $this->authenticator instanceof ChallengeInterface
            ? $this->authenticator->challenge($response)
            : $response;
    }

    return $handler->handle($request);
}

[klsoft-web]

So, based on all the previous comments, the final refactoring should look like this. Any other comments?

 interface AuthenticatorInterface
 {
      public function authenticate(ServerRequestInterface $request): ?IdentityInterface;
 }

interface ChallengeInterface
{
      public function challenge(ResponseInterface $response): ResponseInterface;
}

  /**
   * @deprecated Use AuthenticatorInterface and optionally ChallengeInterface.
   */
interface AuthenticationMethodInterface extends AuthenticatorInterface, ChallengeInterface
{
}
  
class Authentication implements MiddlewareInterface
{
    // ...

    public function __construct(
        private AuthenticatorInterface $authenticator,
        private RequestHandlerInterface $authenticationFailureHandler,
    ) {}

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        $identity = $this->authenticator->authenticate($request);
        $request = $request->withAttribute(self::class, $identity);

        if ($identity === null && !$this->isOptional($request)) {
            $response = $this->authenticationFailureHandler->handle($request);

            return $this->authenticator instanceof ChallengeInterface
                ? $this->authenticator->challenge($response)
                : $response;
        }

        return $handler->handle($request);
    }

    // ...
}

[samdark]

Nope. Looks good. Do you want to handle it?

[klsoft-web]

Nope. Looks good. Do you want to handle it?

Yes, I'll try to handle it.

[klsoft-web]

I think this issue should be resolved by someone who is more involved in the YII3 release cycle.

[samdark]

@klsoft-web please check #115

[klsoft-web]

@klsoft-web please check #115

It looks pretty good!