yii2 icon indicating copy to clipboard operation
yii2 copied to clipboard

CompositeAuth requires auth methods to be ActionFilters

Open SamMousa opened this issue 3 years ago • 3 comments

What steps will reproduce the problem?

Prior to #19418 it was allowed to create an auth implementation that implemented AuthInterface.

What is the expected result?

Minor changes shouldn't break backwards compatibility. Based on the docblock from CompositeAuth.php:

    /**
     * @var array the supported authentication methods. This property should take a list of supported
     * authentication methods, each represented by an authentication class or configuration.
     *
     * If this property is empty, no authentication will be performed.
     *
     * Note that an auth method class must implement the [[\yii\filters\auth\AuthInterface]] interface.
     */
    public $authMethods = [];

I'd expect that the only requirement is to implement the mentioned interface.

What do you get instead?

#19418 blindly calls a function isActive which is not in defined in AuthInterface, without checking whether the concrete authentication method implemention actually extends ActionFilter.

Proposed solution

Fix it so that it checks whether the auth method extends ActionFilter before blindly calling a function on it:

if ($auth instanceof \yii\base\ActionFilter) && isset($this->owner->action) && $auth->isActive($this->owner->action)) {
                $identity = $auth->authenticate($user, $request, $response);
                if ($identity !== null) {
                    return $identity;
                }
            }

This will fix the issue without breaking stuff. It'll even leave the "bugfix" intact. Note that the original bugfix still leaves room for changed behavior; for example when an auth method is used inside CompositeAuth, no events are fired since the filter's beforeFilter and afterFilter methods are never called.

Additional info

Q A
Yii version 2.0.46
PHP version N/A
Operating system N/A

SamMousa avatar Sep 15 '22 14:09 SamMousa

if (
    $this->owner instanceof Controller
    && (
       !isset($this->owner->action)
       || (
           $auth instanceof \yii\base\ActionFilter
           && !$auth->isActive($this->owner->action)
       )
    )
) {
    continue;
}

WinterSilence avatar Sep 15 '22 15:09 WinterSilence

That's not the code that's in master AFAIK? Or are you proposing an alternative fix?

SamMousa avatar Sep 15 '22 15:09 SamMousa

@SamMousa master + your fix

WinterSilence avatar Sep 15 '22 16:09 WinterSilence

could you maybe release this as a fix? 2.0.46.1? we could not switch to 2.0.46 because of this and https://github.com/yiisoft/yii2/issues/19534

papppeter avatar Nov 11 '22 15:11 papppeter