Skip to content

Fixed dependency issue when SecurityBundle is not installed #3149

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 7, 2019
Merged

Fixed dependency issue when SecurityBundle is not installed #3149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54563cc
Fixed dependency issue when SecurityBundle is not installed
reypm Oct 4, 2019
c61cbb9
Fix tests
alanpoulain Oct 7, 2019
dad4c52
Move condition in SecurityPostDenormalizeStage
alanpoulain Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Bridge/Symfony/Bundle/Resources/config/graphql.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@

<service id="api_platform.graphql.resolver.stage.security" class="ApiPlatform\Core\GraphQl\Resolver\Stage\SecurityStage" public="false">
<argument type="service" id="api_platform.metadata.resource.metadata_factory" />
<argument type="service" id="api_platform.security.resource_access_checker" />
<argument type="service" id="api_platform.security.resource_access_checker" on-invalid="ignore" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@reypm reypm Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder why test are passing with this configuration? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the security component is installed on Travis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

</service>

<service id="api_platform.graphql.resolver.stage.security_post_denormalize" class="ApiPlatform\Core\GraphQl\Resolver\Stage\SecurityPostDenormalizeStage" public="false">
<argument type="service" id="api_platform.metadata.resource.metadata_factory" />
<argument type="service" id="api_platform.security.resource_access_checker" />
<argument type="service" id="api_platform.security.resource_access_checker" on-invalid="ignore" />
</service>

<service id="api_platform.graphql.resolver.stage.serialize" class="ApiPlatform\Core\GraphQl\Resolver\Stage\SerializeStage" public="false">
Expand Down
7 changes: 6 additions & 1 deletion src/GraphQl/Resolver/Stage/SecurityPostDenormalizeStage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ final class SecurityPostDenormalizeStage implements SecurityPostDenormalizeStage
private $resourceMetadataFactory;
private $resourceAccessChecker;

public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, ResourceAccessCheckerInterface $resourceAccessChecker)
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, ?ResourceAccessCheckerInterface $resourceAccessChecker)
{
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->resourceAccessChecker = $resourceAccessChecker;
Expand All @@ -44,6 +44,7 @@ public function __invoke(string $resourceClass, string $operationName, array $co
$resourceMetadata = $this->resourceMetadataFactory->create($resourceClass);

$isGranted = $resourceMetadata->getGraphqlAttribute($operationName, 'security_post_denormalize', null, true);

if (null === $isGranted) {
// Backward compatibility
$isGranted = $resourceMetadata->getGraphqlAttribute($operationName, 'access_control', null, true);
Expand All @@ -52,6 +53,10 @@ public function __invoke(string $resourceClass, string $operationName, array $co
}
}

if (null !== $isGranted && null === $this->resourceAccessChecker) {
throw new \LogicException('Cannot check security expression when SecurityBundle is not installed. Try running "composer require symfony/security-bundle".');
}

if (null === $isGranted || $this->resourceAccessChecker->isGranted($resourceClass, (string) $isGranted, $context['extra_variables'])) {
return;
}
Expand Down
7 changes: 6 additions & 1 deletion src/GraphQl/Resolver/Stage/SecurityStage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ final class SecurityStage implements SecurityStageInterface
private $resourceMetadataFactory;
private $resourceAccessChecker;

public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, ResourceAccessCheckerInterface $resourceAccessChecker)
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, ?ResourceAccessCheckerInterface $resourceAccessChecker)
{
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->resourceAccessChecker = $resourceAccessChecker;
Expand All @@ -44,6 +44,11 @@ public function __invoke(string $resourceClass, string $operationName, array $co
$resourceMetadata = $this->resourceMetadataFactory->create($resourceClass);

$isGranted = $resourceMetadata->getGraphqlAttribute($operationName, 'security', null, true);

if (null !== $isGranted && null === $this->resourceAccessChecker) {
throw new \LogicException('Cannot check security expression when SecurityBundle is not installed. Try running "composer require symfony/security-bundle".');
}

if (null === $isGranted || $this->resourceAccessChecker->isGranted($resourceClass, (string) $isGranted, $context['extra_variables'])) {
return;
}
Expand Down
30 changes: 30 additions & 0 deletions tests/GraphQl/Resolver/Stage/SecurityPostDenormalizeStageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,34 @@ public function testNotGranted(): void
'extra_variables' => $extraVariables,
]);
}

public function testNoSecurityBundleInstalled(): void
{
$this->securityPostDenormalizeStage = new SecurityPostDenormalizeStage($this->resourceMetadataFactoryProphecy->reveal(), null);

$operationName = 'item_query';
$resourceClass = 'myResource';
$isGranted = 'not_granted';
$resourceMetadata = (new ResourceMetadata())->withGraphql([
$operationName => ['security_post_denormalize' => $isGranted],
]);
$this->resourceMetadataFactoryProphecy->create($resourceClass)->willReturn($resourceMetadata);

$this->expectException(\LogicException::class);

($this->securityPostDenormalizeStage)($resourceClass, 'item_query', []);
}

public function testNoSecurityBundleInstalledNoExpression(): void
{
$this->securityPostDenormalizeStage = new SecurityPostDenormalizeStage($this->resourceMetadataFactoryProphecy->reveal(), null);

$resourceClass = 'myResource';
$resourceMetadata = new ResourceMetadata();
$this->resourceMetadataFactoryProphecy->create($resourceClass)->willReturn($resourceMetadata);

$this->resourceAccessCheckerProphecy->isGranted(Argument::any())->shouldNotBeCalled();

($this->securityPostDenormalizeStage)($resourceClass, 'item_query', []);
}
}
30 changes: 30 additions & 0 deletions tests/GraphQl/Resolver/Stage/SecurityStageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,34 @@ public function testNotGranted(): void
'extra_variables' => $extraVariables,
]);
}

public function testNoSecurityBundleInstalled(): void
{
$this->securityStage = new SecurityStage($this->resourceMetadataFactoryProphecy->reveal(), null);

$operationName = 'item_query';
$resourceClass = 'myResource';
$isGranted = 'not_granted';
$resourceMetadata = (new ResourceMetadata())->withGraphql([
$operationName => ['security' => $isGranted],
]);
$this->resourceMetadataFactoryProphecy->create($resourceClass)->willReturn($resourceMetadata);

$this->expectException(\LogicException::class);

($this->securityStage)($resourceClass, 'item_query', []);
}

public function testNoSecurityBundleInstalledNoExpression(): void
{
$this->securityStage = new SecurityStage($this->resourceMetadataFactoryProphecy->reveal(), null);

$resourceClass = 'myResource';
$resourceMetadata = new ResourceMetadata();
$this->resourceMetadataFactoryProphecy->create($resourceClass)->willReturn($resourceMetadata);

$this->resourceAccessCheckerProphecy->isGranted(Argument::any())->shouldNotBeCalled();

($this->securityStage)($resourceClass, 'item_query', []);
}
}