Skip to content

[Security] Using IsGranted in combination with MapRequestPayload passes incorrect subject to voter #50964

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

Closed
DjordyKoert opened this issue Jul 13, 2023 · 6 comments

Comments

@DjordyKoert
Copy link
Contributor

DjordyKoert commented Jul 13, 2023

Symfony version(s) affected

6.3.0

Description

Using the IsGranted attribute in combination with the MapRequestPayload results in the wrong subject being passed into the voters.

What I expected was that my voter would receive a DTO class (ExampleDTO) or atleast the class-string when the supports method gets called by symfony for the $subject argument, instead what I got was $subject of type MapRequestPayload which is the attribute of the controller argument.

How to reproduce

Controller route

...
    #[Route(
        '/api/v1/example',
        name: 'exampleMethod',
        methods: ['POST']
    )]
    #[IsGranted(
        'DTO',
        subject: 'exampleDTO',
    )]
    public function exampleMethod(#[MapRequestPayload] ExampleDTO $exampleDTO): Response {
...

DTO

#[DTO('example:permission')]
class ExampleDTO
{
    public function __construct(
        private int $id,
    ) {
    }

    public function getId(): int
    {
        return $this->id;
    }
}

Customer DTO Attribute

#[Attribute(Attribute::TARGET_CLASS)]
final readonly class DTO
{
    /**
     * @param string[] | string $permissions
     */
    public function __construct(
        private array | string $permissions
    ) {
    }

    /**
     * @return string[] | string
     */
    public function getPermissions(): array | string
    {
        return $this->permissions;
    }
}

Custom DTO voter

final class DTOVoter extends Voter
{
    /**
     * @param class-string|object $subject
     */
    protected function supports(string $attribute, mixed $subject): bool
    {
        // TODO: Check if attribute is of type DTO
        $class = new ReflectionClass($subject);

        /**
         * @var DTO[] $attributes
         */
        $attributes = $class->getAttributes(DTO::class, ReflectionAttribute::IS_INSTANCEOF);

        dump($subject);

        if ($attributes === []) {
            return false;
        }

       return true;
    }
...

Possible Solution

if ($arguments[$subjectRef] instanceof MapRequestPayload) {
    return $arguments[$subjectRef]->metadata->getType();
}

return $arguments[$subjectRef];

Additional Context

No response

@HypeMC
Copy link
Member

HypeMC commented Jul 13, 2023

This is actually expected behavior, see #50120

@DjordyKoert
Copy link
Contributor Author

DjordyKoert commented Jul 14, 2023

It isn't really expected behavior though. It has been accepted as a 'fix this later'. #50125 (review)

Let's merge this PR and solve late the case of accessing the parsed subject from IsGranted?

In our case we do not necessarily need the deserialized payload though, working with a simply class-string of said deserialized payload would also works for us. For this reason is I have described a possible solution which would return the type (class-string) whenever it is an instance of MapRequestPayload. So we can still do some validation based on defined Metadata (attributes).

@derrabus
Copy link
Member

It isn't really expected behavior though.

Yes, it is. This is not a bug.

@derrabus derrabus added Feature and removed Bug labels Jul 14, 2023
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Could I get a reply or should I close this?

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants