Fixed dependency issue when SecurityBundle is not installed

[reypm]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#1265
License	MIT

[alanpoulain]

You need to modify the code inside the stages too to handle the case when $resourceAccessChecker is null.

[reypm]

You need to modify the code inside the stages too to handle the case when $resourceAccessChecker is null.

What do you mean? Not following you on the word stages 🤔

[alanpoulain]

The services you are modifying are "stages" (SecurityStage, SecurityPostDenormalizeStage).

[reypm]

The services you are modifying are "stages" (SecurityStage, SecurityPostDenormalizeStage).

You mean add an extra check for null check at src/GraphQl/Resolver/Stage/SecurityStage.php and in the other stage using $resourceAccessChecker??

    if (null === $isGranted || null === $this->resourceAccessChecker || $this->resourceAccessChecker->isGranted($resourceClass, (string) $isGranted, $context['extra_variables'])) {
        return;
    }

[alanpoulain]

Even before this condition but that's the idea. And make the variable in the constructor nullable.
Don't forget to add unit tests too!

[teohhanhui]

@reypm

    if (null === $isGranted || null === $this->resourceAccessChecker || $this->resourceAccessChecker->isGranted($resourceClass, (string) $isGranted, $context['extra_variables'])) {
        return;
    }

This is wrong. We must not silently allow access if the user has specified some condition (i.e. $isGranted is not null) but $this->resourceAccessChecker is null. We should fail in this case.

[alanpoulain]

Oh yes you're right @teohhanhui

[reypm]

We should fail in this case.

@teohhanhui and by "fail" you mean throw the Error as in last line on that __invoke function? if I am following you right you want this?

/**  @var ResolveInfo $info */
$info = $context['info'];

$isGranted = $resourceMetadata->getGraphqlAttribute($operationName, 'security', null, true);
if (null === $this->resourceAccessChecker && $isGranted) {
    throw Error::createLocatedError($resourceMetadata->getGraphqlAttribute($operationName, 'security_message', 'Access Denied.'), $info->fieldNodes, $info->path);
}

if (null === $isGranted || $this->resourceAccessChecker->isGranted($resourceClass, (string) $isGranted, $context['extra_variables'])) {
    return;
}

throw Error::createLocatedError($resourceMetadata->getGraphqlAttribute($operationName, 'security_message', 'Access Denied.'), $info->fieldNodes, $info->path);

@alanpoulain yes, I will add the test once the desired solution is approved by you both :)

Sorry for not being following you guys, first time "fixing" an api-platform issue and understanding it's internal.

[teohhanhui]

@teohhanhui and by fail you mean throw the Error as in last line on that __invoke function? I am following you right you want this?

I think we should do something like:

throw new \LogicException('Cannot check security expression when SecurityBundle is not installed. Try running "composer require symfony/security-bundle".');

as this is a configuration issue.

Or do you always want to avoid throwing exceptions at all costs, @alanpoulain? (Why?)

[alanpoulain]

We can throw a LogicException, it will be converted in an Error when #3063 will be merged (and it's not an issue in the meantime if it's not the case).

[reypm]

@teohhanhui / @alanpoulain better now? Did I miss something else?

[dunglas]

Should be null, not ignore isn't it? Or calling the constructor will still fail.

[reypm]

Wonder why test are passing with this configuration? 🤔

[dunglas]

Because the security component is installed on Travis.

[teohhanhui]

No, they behave the same way. The only difference is when inside a method call.

https://symfony.com/doc/current/service_container/optional_dependencies.html#ignoring-missing-dependencies

[teohhanhui]

I suggest for consistency's sake, we should not use on-invalid="null" except in the specific cases where we need that behaviour (i.e. a method call with null value).

[alanpoulain]

I've rebased your PR for 2.5 and made some little changes. Thank you @reypm!