mezzio / mezzio-cors

CORS component for Mezzio and other PSR-15 middleware runners.
https://docs.mezzio.dev/mezzio-cors/
BSD 3-Clause "New" or "Revised" License
17 stars 14 forks source link

Missing setter method for property allowedMethods in AbstractConfiguration #55

Closed bitkorn closed 1 year ago

bitkorn commented 1 year ago

...that is all: the function setAllowedMethods() is missing in AbstractConfiguration.

Ocramius commented 1 year ago

In here, it's done in https://github.com/mezzio/mezzio-cors/blob/37d8146281391eb6fc08d381f3e1e79b15328956/src/Configuration/RouteConfiguration.php#L77-L85

Id' say that AbstractConfiguration should probably be dropped, and only the interface should survive :thinking:

bitkorn commented 1 year ago

ah, ok.

Because class ProjectConfiguration extends AbstractConfiguration mezzio-cors can not work with project config.

bitkorn commented 1 year ago

I have write my own ProjectConfiguration extends AbstractConfiguration with ProjectConfigurationFactory, put it in my ConfigProvider and it works :)

<?php

declare(strict_types=1);

namespace App\Configuration;

use Mezzio\Cors\Configuration\ConfigurationInterface;
use Mezzio\Cors\Configuration\Exception\InvalidConfigurationException;
use Mezzio\Cors\Exception\BadMethodCallException;
use Webmozart\Assert\Assert;

use function array_unique;
use function array_values;
use function call_user_func;
use function in_array;
use function is_callable;
use function lcfirst;
use function sprintf;
use function str_replace;
use function ucfirst;
use function ucwords;

abstract class AbstractConfiguration implements ConfigurationInterface
{
    /**
     * @psalm-var list<string>
     */
    protected $allowedOrigins = [];

    /**
     * @psalm-var list<string>
     */
    protected $allowedMethods = [];

    /**
     * @psalm-var list<string>
     */
    protected $allowedHeaders = [];

    /** @var string */
    protected $allowedMaxAge = ConfigurationInterface::PREFLIGHT_CACHE_DISABLED;

    /** @var bool */
    protected $credentialsAllowed = false;

    /**
     * @psalm-var list<string>
     */
    protected $exposedHeaders = [];

    /**
     * @psalm-param array<string,mixed> $parameters
     */
    public function __construct(array $parameters)
    {
        try {
            $this->exchangeArray($parameters);
        } catch (BadMethodCallException $exception) {
            throw InvalidConfigurationException::create($exception->getMessage());
        }
    }

    /**
     * @param array<string,mixed> $data
     */
    public function exchangeArray(array $data): self
    {
        $instance = clone $this;

        /** @psalm-suppress MixedAssignment */
        foreach ($data as $property => $value) {
            $property = lcfirst(str_replace('_', '', ucwords($property, '_')));
            $setter   = sprintf('set%s', ucfirst($property));
            $callable = [$this, $setter];
            if (! is_callable($callable)) {
                throw BadMethodCallException::fromMissingSetterMethod($property, $setter);
            }

            call_user_func($callable, $value);
        }

        return $instance;
    }

    /**
     * @psalm-param list<string> $origins
     */
    public function setAllowedOrigins(array $origins): void
    {
        Assert::allString($origins);

        $origins = array_values(array_unique($origins));

        if (in_array(ConfigurationInterface::ANY_ORIGIN, $origins, true)) {
            $origins = [ConfigurationInterface::ANY_ORIGIN];
        }

        $this->allowedOrigins = $origins;
    }

    public function allowedOrigins(): array
    {
        return $this->allowedOrigins;
    }

    public function setAllowedMethods(array $methods): void
    {
        Assert::allString($methods);
        $methods = array_values(array_unique($methods));
        $this->allowedMethods = $methods;
    }

    public function allowedMethods(): array
    {
        return $this->allowedMethods;
    }

    /**
     * @psalm-param list<string> $headers
     */
    public function setAllowedHeaders(array $headers): void
    {
        Assert::allString($headers);
        $this->allowedHeaders = array_values(array_unique($headers));
    }

    public function allowedHeaders(): array
    {
        return $this->allowedHeaders;
    }

    public function setAllowedMaxAge(string $age): void
    {
        if ($age) {
            Assert::numeric($age);
        }

        $this->allowedMaxAge = $age;
    }

    public function allowedMaxAge(): string
    {
        return $this->allowedMaxAge;
    }

    /**
     * @psalm-param list<string> $headers
     */
    public function setExposedHeaders(array $headers): void
    {
        Assert::allString($headers);
        $this->exposedHeaders = array_values(array_unique($headers));
    }

    public function exposedHeaders(): array
    {
        return $this->exposedHeaders;
    }

    public function credentialsAllowed(): bool
    {
        return $this->credentialsAllowed;
    }

    public function setCredentialsAllowed(bool $credentialsAllowed): void
    {
        $this->credentialsAllowed = $credentialsAllowed;
    }

    public function allowedCredentials(): bool
    {
        return $this->credentialsAllowed;
    }

    public function setAllowedCredentials(bool $credentialsAllowed): void
    {
        $this->credentialsAllowed = $credentialsAllowed;
    }
}
boesing commented 1 year ago

There is no reason to have request methods configured on project level. This module detects automatically which request methods are supported by the requested endpoint (unless there is a route configuration in-place).

If you want to have request method specific configurations, you have to configure it per-route and that is why it is not registered to the project configuration.

Could you probably explain the use-case why you need this setting on the project level?

bitkorn commented 1 year ago

I want to allow the same methods in many requests. For this I need the ProjectConfiguration. In the other requests, the RouteConfiguration overwrites the ProjectConfiguration.

froschdesign commented 1 year ago

@bitkorn

I want to allow the same methods in many requests.

And these are already defined in your routing configuration and mezzio-cors will check your routes:

It uses the mezzio-router to match the incoming URI. It starts with the HTTP request method which is provided by the Request via the Access-Control-Request-Method header and checks all request methods until it matches a route. If that route states to be explicit, the response is created immediately.

If the route is not explicit, all request methods are checked to provide a list of possible request methods to the client.

https://docs.mezzio.dev/mezzio-cors/v1/intro/#features

Or did I miss something?

bitkorn commented 1 year ago

I define the routes in the module's ConfigProvider as an Array.

The client threw "Missing Allowed Method". That's why I wanted a ProjectConfiguration with "allowed_methods". But, now I can't recreate the behavior of the client.

boesing commented 1 year ago

by default, the cors module allows any request method by default (by any I mean any which is allowed by the route configuration, i.e. via allowed_methods within routes). If you do not limit the request method by the route configuration, no changes needed as the cors module automatically reads allowed_methods from your route.

Configuring methods on a project level makes no sense at all since some endpoints are usually POST and some are GET and others are DELETE or maybe PATCH/PUT. But thats not the rule for all endpoints and thus I've never added this functionality to the project config on purpose.

Maybe you experienced a bug which was fixed in v1.0.2 but since this issue is just 2 days old and v1.0.2 was released back in 2020, I don't think thats the case. However, I'd love to see the configuration to understand what you try to achieve. IMHO, its not necessary to configure project wide allowed_methods.