Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

\Mezzio\Authentication\OAuth2\ConfigTrait::getPrivateKey() - does not accept array configuration for private key #47

Closed
10n opened this issue Apr 6, 2022 · 10 comments · Fixed by #48
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@10n
Copy link
Contributor

10n commented Apr 6, 2022

BC Break Report

Return type declaration for \Mezzio\Authentication\OAuth2\ConfigTrait::getPrivateKey():string is not allowing array configurations anymore. This removes the possibility of configuring a private key with passphrase.

Code reference: https://github.com/mezzio/mezzio-authentication-oauth2/blob/2.4.0/src/ConfigTrait.php#L14

Q A
Version 2.4.0

Summary

Return type declaration for \Mezzio\Authentication\OAuth2\ConfigTrait::getPrivateKey() is not allowing array configurations anymore.
Code reference: https://github.com/mezzio/mezzio-authentication-oauth2/blob/2.4.0/src/ConfigTrait.php#L14

This trait is used in \Mezzio\Authentication\OAuth2\AuthorizationServerFactory:34
$privateKey = $this->getCryptKey($this->getPrivateKey($container), 'authentication.private_key');

So, even though \Mezzio\Authentication\OAuth2\CryptKeyTrait::getCryptKey() can accept as argument string|array $keyConfig, because of the type hinting protected function getPrivateKey(ContainerInterface $container): string, array cannot be loaded anymore from container config.

Previous behavior

\Mezzio\Authentication\OAuth2\ConfigTrait::getPrivateKey() did NOT force string as return type.
Was allowing also array to be used in container config.

Note: The array could contain the following keys key_or_path (mandatory), pass_phrase (optional) , key_permissions_check (optional)
https://github.com/mezzio/mezzio-authentication-oauth2/blob/2.4.0/src/CryptKeyTrait.php#L23

Current behavior

\Mezzio\Authentication\OAuth2\ConfigTrait::getPrivateKey(): string`

How to reproduce

\Mezzio\Authentication\OAuth2\CryptKeyTrait::getCryptKey - has explicit string and array in PHPDoc

Commit reference that introduced restriction:
e34d9e8

@10n 10n added the BC Break label Apr 6, 2022
@Ocramius
Copy link
Member

Ocramius commented Apr 6, 2022

Can we please get a test case, or code reference?

I see that CryptKeyTrait is designed to support array|string $keyConfig:

/**
* @param array|string $keyConfig
*/
protected function getCryptKey($keyConfig, string $configPath): CryptKey

@10n
Copy link
Contributor Author

10n commented Apr 7, 2022

I have updated (and fixed) the BC report.

@Ocramius
Copy link
Member

Ocramius commented Apr 7, 2022

/cc @arueckauer - we could expand the return type on the factory

@10n
Copy link
Contributor Author

10n commented Apr 7, 2022

I've created a pull request, but I've just realised that is started from 2.5.x

@arueckauer
Copy link
Contributor

@10n Thanks for the report and PR #48. Let me know, if you need any assistance with this.

@10n
Copy link
Contributor Author

10n commented Apr 7, 2022

@10n Thanks for the report and PR #48. Let me know, if you need any assistance with this.

I will need your help on 2 things:

  • lets fixed the CI (I did not add Signed-off-by )
  • I want to add / expand the tests in order to check arra configuration

@arueckauer
Copy link
Contributor

You can sign off last commit with git commit --amend --signoff.

For a test case it is required to set up the config array with an array value for the private key. Something like the following should work (I have not tested it) in ConfigTraitTest.

    public function testGetPrivateKeyReturnsArray()
    {
        $trait = new class {
            use ConfigTrait;

            /**
             * @return array|string
             */
            public function proxy(string $name, ContainerInterface $container)
            {
                return $this->$name($container);
            }
        };
        $config    = [
            'authentication' => [
                'private_key'          => ['xxx', 'yyy'],
            ],
        ];
        $container = $this->prophesize(ContainerInterface::class);
        $container
            ->get('config')
            ->willReturn($config);

        $result = $trait->proxy('getPrivateKey', $container->reveal());
        $this->assertEquals($config['authentication']['private_key'], $result);
    }

@10n
Copy link
Contributor Author

10n commented Apr 7, 2022

I've pushed changes to the ConfigTraitTest, I hope I've done it right.

Thanks for the help.

@10n
Copy link
Contributor Author

10n commented Apr 7, 2022

Do I need to fix the SignOff issue on all of the commits ?

@Ocramius
Copy link
Member

Ocramius commented Apr 7, 2022

Handled in #48

@Ocramius Ocramius closed this as completed Apr 7, 2022
@Ocramius Ocramius added Bug Something isn't working and removed BC Break labels Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
3 participants