knpuniversity / oauth2-client-bundle

Easily talk to an OAuth2 server for social functionality in Symfony
https://symfonycasts.com
MIT License
786 stars 146 forks source link

Bundle Configuration badly merges clients #74

Closed alterphp closed 6 years ago

alterphp commented 6 years ago

I have the following configuration in the main config file (config/packages/knpu_oauth2_client.yaml) :

knpu_oauth2_client:
    clients:
        gsuite:
            # must be "google" - it activates that type!
            type: google
            # add and configure client_id and client_secret in parameters.yaml
            client_id: "%gsuite_client_id%"
            client_secret: "%gsuite_client_secret%"
            # a route name you'll create
            redirect_route: admin_connect_gsuite_check
            redirect_params: {}

And, in another config file (config/packages/another_vendor.yaml) :

knpu_oauth2_client:
    clients:
        custom_connect:
            type: generic
            provider_class: Vendor\ConnectBundle\Security\Authentication\Provider\CustomConnectProvider
            ...

After configuration being processed, the clients are badly indexed :

array:1 [▼
    "clients" => array:2 [▼
        "kiplin_connect" => array:6 [▶]
        0 => array:6 [▶]
    ]
]

This is due to the way prototype arrays are merged in the Symfony Config component (from PrototpyedArrayNode class) :

    protected function mergeValues($leftSide, $rightSide)
    {
        if (false === $rightSide) {
            // if this is still false after the last config has been merged the
            // finalization pass will take care of removing this key entirely
            return false;
        }

        if (false === $leftSide || !$this->performDeepMerging) {
            return $rightSide;
        }

        foreach ($rightSide as $k => $v) {
            // prototype, and key is irrelevant, so simply append the element
            if (null === $this->keyAttribute) {
                $leftSide[] = $v;
                continue;
            }

            // no conflict
            if (!array_key_exists($k, $leftSide)) {
                if (!$this->allowNewKeys) {
                    $ex = new InvalidConfigurationException(sprintf(
                        'You are not allowed to define new elements for path "%s". '.
                        'Please define all elements for this path in one config file.',
                        $this->getPath()
                    ));
                    $ex->setPath($this->getPath());

                    throw $ex;
                }

                $leftSide[$k] = $v;
                continue;
            }

            $prototype = $this->getPrototypeForChild($k);
            $leftSide[$k] = $prototype->merge($leftSide[$k], $v);
        }

        return $leftSide;
    }

See the first line in the foreach statement :

// prototype, and key is irrelevant, so simply append the element

And so the right site is appended to the left side with no key preservation...

bocharsky-bw commented 6 years ago

Just to be clear, it's failed only when you have a few config files of oauth2-client-bundle? If you move all the configuration to one config file e.g. config/packages/knpu_oauth2_client.yaml it works, right?

alterphp commented 6 years ago

Exactly.

I use a third party bundle that "pushes" a client to the knpu_oauth2_client config entry. But it does not work with an existing client defined on main level (the client key becomes numeric).

weaverryan commented 6 years ago

Good bug report :). See #75

weaverryan commented 6 years ago

Fixed and tagged: https://github.com/knpuniversity/oauth2-client-bundle/releases/tag/1.16.1

If you have any issue @alterphp, please let us know!

Cheers!

alterphp commented 6 years ago

It works !

Thanka a lot for the fast fix !