Skip to content

Commit ea9789f

Browse files
xabbuhxabbuh
committed
do not construct Vote instances inside vote()
The so constructed objects will never be seen from the outside. Thus, adding reasons to them doesn't have an effect.
1 parent 2488ef4 commit ea9789f

File tree

7 files changed

+53
-30
lines changed

7 files changed

+53
-30
lines changed

‎Authorization/Voter/AuthenticatedVoter.php

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,10 @@ public function __construct(
4545
*/
4646
public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int
4747
{
48-
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
49-
$vote ??= new Vote();
48+
$vote = 3 < \func_num_args() ? func_get_arg(3) : null;
5049

5150
if ($attributes === [self::PUBLIC_ACCESS]) {
52-
$vote->reasons[] = 'Access is public.';
51+
$vote?->addReason('Access is public.');
5352

5453
return VoterInterface::ACCESS_GRANTED;
5554
}
@@ -73,40 +72,40 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
7372
if ((self::IS_AUTHENTICATED_FULLY === $attribute || self::IS_AUTHENTICATED_REMEMBERED === $attribute)
7473
&& $this->authenticationTrustResolver->isFullFledged($token)
7574
) {
76-
$vote->reasons[] = 'The user is fully authenticated.';
75+
$vote?->addReason('The user is fully authenticated.');
7776

7877
return VoterInterface::ACCESS_GRANTED;
7978
}
8079

8180
if (self::IS_AUTHENTICATED_REMEMBERED === $attribute
8281
&& $this->authenticationTrustResolver->isRememberMe($token)
8382
) {
84-
$vote->reasons[] = 'The user is remembered.';
83+
$vote?->addReason('The user is remembered.');
8584

8685
return VoterInterface::ACCESS_GRANTED;
8786
}
8887

8988
if (self::IS_AUTHENTICATED === $attribute && $this->authenticationTrustResolver->isAuthenticated($token)) {
90-
$vote->reasons[] = 'The user is authenticated.';
89+
$vote?->addReason('The user is authenticated.');
9190

9291
return VoterInterface::ACCESS_GRANTED;
9392
}
9493

9594
if (self::IS_REMEMBERED === $attribute && $this->authenticationTrustResolver->isRememberMe($token)) {
96-
$vote->reasons[] = 'The user is remembered.';
95+
$vote?->addReason('The user is remembered.');
9796

9897
return VoterInterface::ACCESS_GRANTED;
9998
}
10099

101100
if (self::IS_IMPERSONATOR === $attribute && $token instanceof SwitchUserToken) {
102-
$vote->reasons[] = 'The user is impersonating another user.';
101+
$vote?->addReason('The user is impersonating another user.');
103102

104103
return VoterInterface::ACCESS_GRANTED;
105104
}
106105
}
107106

108107
if (VoterInterface::ACCESS_DENIED === $result) {
109-
$vote->reasons[] = 'The user is not appropriately authenticated.';
108+
$vote?->addReason('The user is not appropriately authenticated.');
110109
}
111110

112111
return $result;

‎Authorization/Voter/ClosureVoter.php

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ public function supportsType(string $subjectType): bool
4242

4343
public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int
4444
{
45-
$vote ??= new Vote();
4645
$context = new IsGrantedContext($token, $token->getUser(), $this->authorizationChecker);
4746
$failingClosures = [];
4847
$result = VoterInterface::ACCESS_ABSTAIN;
@@ -54,7 +53,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes, ?
5453
$name = (new \ReflectionFunction($attribute))->name;
5554
$result = VoterInterface::ACCESS_DENIED;
5655
if ($attribute($context, $subject)) {
57-
$vote->reasons[] = \sprintf('Closure %s returned true.', $name);
56+
$vote?->addReason(\sprintf('Closure %s returned true.', $name));
5857

5958
return VoterInterface::ACCESS_GRANTED;
6059
}
@@ -63,7 +62,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes, ?
6362
}
6463

6564
if ($failingClosures) {
66-
$vote->reasons[] = \sprintf('Closure%s %s returned false.', 1 < \count($failingClosures) ? 's' : '', implode(', ', $failingClosures));
65+
$vote?->addReason(\sprintf('Closure%s %s returned false.', 1 < \count($failingClosures) ? 's' : '', implode(', ', $failingClosures)));
6766
}
6867

6968
return $result;

‎Authorization/Voter/ExpressionVoter.php

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,7 @@ public function supportsType(string $subjectType): bool
4949
*/
5050
public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int
5151
{
52-
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
53-
$vote ??= new Vote();
52+
$vote = 3 < \func_num_args() ? func_get_arg(3) : null;
5453
$result = VoterInterface::ACCESS_ABSTAIN;
5554
$variables = null;
5655
$failingExpressions = [];
@@ -64,7 +63,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
6463
$result = VoterInterface::ACCESS_DENIED;
6564

6665
if ($this->expressionLanguage->evaluate($attribute, $variables)) {
67-
$vote->reasons[] = \sprintf('Expression (%s) is true.', $attribute);
66+
$vote?->addReason(\sprintf('Expression (%s) is true.', $attribute));
6867

6968
return VoterInterface::ACCESS_GRANTED;
7069
}
@@ -73,7 +72,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
7372
}
7473

7574
if ($failingExpressions) {
76-
$vote->reasons[] = \sprintf('Expression (%s) is false.', implode(') || (', $failingExpressions));
75+
$vote?->addReason(\sprintf('Expression (%s) is false.', implode(') || (', $failingExpressions)));
7776
}
7877

7978
return $result;

‎Authorization/Voter/RoleVoter.php

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,7 @@ public function __construct(
3030
*/
3131
public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int
3232
{
33-
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
34-
$vote ??= new Vote();
33+
$vote = 3 < \func_num_args() ? func_get_arg(3) : null;
3534
$result = VoterInterface::ACCESS_ABSTAIN;
3635
$roles = $this->extractRoles($token);
3736
$missingRoles = [];
@@ -44,7 +43,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
4443
$result = VoterInterface::ACCESS_DENIED;
4544

4645
if (\in_array($attribute, $roles, true)) {
47-
$vote->reasons[] = \sprintf('The user has %s.', $attribute);
46+
$vote?->addReason(\sprintf('The user has %s.', $attribute));
4847

4948
return VoterInterface::ACCESS_GRANTED;
5049
}
@@ -53,7 +52,7 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
5352
}
5453

5554
if (VoterInterface::ACCESS_DENIED === $result) {
56-
$vote->reasons[] = \sprintf('The user doesn\'t have%s %s.', 1 < \count($missingRoles) ? ' any of' : '', implode(', ', $missingRoles));
55+
$vote?->addReason(\sprintf('The user doesn\'t have%s %s.', 1 < \count($missingRoles) ? ' any of' : '', implode(', ', $missingRoles)));
5756
}
5857

5958
return $result;

‎Authorization/Voter/TraceableVoter.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,9 @@ public function __construct(
3232

3333
public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int
3434
{
35-
$result = $this->voter->vote($token, $subject, $attributes, $vote ??= new Vote());
35+
$result = $this->voter->vote($token, $subject, $attributes, $vote);
3636

37-
$this->eventDispatcher->dispatch(new VoteEvent($this->voter, $subject, $attributes, $result, $vote->reasons), 'debug.security.authorization.vote');
37+
$this->eventDispatcher->dispatch(new VoteEvent($this->voter, $subject, $attributes, $result, $vote->reasons ?? []), 'debug.security.authorization.vote');
3838

3939
return $result;
4040
}

‎Authorization/Voter/Voter.php

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,9 @@ abstract class Voter implements VoterInterface, CacheableVoterInterface
2929
*/
3030
public function vote(TokenInterface $token, mixed $subject, array $attributes/* , ?Vote $vote = null */): int
3131
{
32-
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
33-
$vote ??= new Vote();
32+
$vote = 3 < \func_num_args() ? func_get_arg(3) : null;
3433
// abstain vote by default in case none of the attributes are supported
35-
$vote->result = self::ACCESS_ABSTAIN;
34+
$voteResult = self::ACCESS_ABSTAIN;
3635

3736
foreach ($attributes as $attribute) {
3837
try {
@@ -48,15 +47,27 @@ public function vote(TokenInterface $token, mixed $subject, array $attributes/*
4847
}
4948

5049
// as soon as at least one attribute is supported, default is to deny access
51-
$vote->result = self::ACCESS_DENIED;
50+
$voteResult = self::ACCESS_DENIED;
51+
52+
if (null !== $vote) {
53+
$vote->result = $voteResult;
54+
}
5255

5356
if ($this->voteOnAttribute($attribute, $subject, $token, $vote)) {
5457
// grant access as soon as at least one attribute returns a positive response
55-
return $vote->result = self::ACCESS_GRANTED;
58+
if (null !== $vote) {
59+
$vote->result = self::ACCESS_GRANTED;
60+
}
61+
62+
return self::ACCESS_GRANTED;
5663
}
5764
}
5865

59-
return $vote->result;
66+
if (null !== $vote) {
67+
$vote->result = $voteResult;
68+
}
69+
70+
return $voteResult;
6071
}
6172

6273
/**

‎Tests/Authorization/Voter/VoterTest.php

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,35 +33,51 @@ public static function getTests(): array
3333

3434
return [
3535
[$voter, ['EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute and class are supported and attribute grants access'],
36+
[$voter, ['EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute and class are supported and attribute grants access', new Vote()],
3637
[$voter, ['CREATE'], VoterInterface::ACCESS_DENIED, new \stdClass(), 'ACCESS_DENIED if attribute and class are supported and attribute does not grant access'],
38+
[$voter, ['CREATE'], VoterInterface::ACCESS_DENIED, new \stdClass(), 'ACCESS_DENIED if attribute and class are supported and attribute does not grant access', new Vote()],
3739

3840
[$voter, ['DELETE', 'EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if one attribute is supported and grants access'],
41+
[$voter, ['DELETE', 'EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if one attribute is supported and grants access', new Vote()],
3942
[$voter, ['DELETE', 'CREATE'], VoterInterface::ACCESS_DENIED, new \stdClass(), 'ACCESS_DENIED if one attribute is supported and denies access'],
43+
[$voter, ['DELETE', 'CREATE'], VoterInterface::ACCESS_DENIED, new \stdClass(), 'ACCESS_DENIED if one attribute is supported and denies access', new Vote()],
4044

4145
[$voter, ['CREATE', 'EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if one attribute grants access'],
46+
[$voter, ['CREATE', 'EDIT'], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if one attribute grants access', new Vote()],
4247

4348
[$voter, ['DELETE'], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if no attribute is supported'],
49+
[$voter, ['DELETE'], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if no attribute is supported', new Vote()],
4450

4551
[$voter, ['EDIT'], VoterInterface::ACCESS_ABSTAIN, new class {}, 'ACCESS_ABSTAIN if class is not supported'],
52+
[$voter, ['EDIT'], VoterInterface::ACCESS_ABSTAIN, new class {}, 'ACCESS_ABSTAIN if class is not supported', new Vote()],
4653

4754
[$voter, ['EDIT'], VoterInterface::ACCESS_ABSTAIN, null, 'ACCESS_ABSTAIN if object is null'],
55+
[$voter, ['EDIT'], VoterInterface::ACCESS_ABSTAIN, null, 'ACCESS_ABSTAIN if object is null', new Vote()],
4856

4957
[$voter, [], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if no attributes were provided'],
58+
[$voter, [], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if no attributes were provided', new Vote()],
5059

5160
[$voter, [new StringableAttribute()], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute and class are supported and attribute grants access'],
61+
[$voter, [new StringableAttribute()], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute and class are supported and attribute grants access', new Vote()],
5262

5363
[$voter, [new \stdClass()], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if attributes were not strings'],
64+
[$voter, [new \stdClass()], VoterInterface::ACCESS_ABSTAIN, new \stdClass(), 'ACCESS_ABSTAIN if attributes were not strings', new Vote()],
5465

5566
[$integerVoter, [42], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute is an integer'],
67+
[$integerVoter, [42], VoterInterface::ACCESS_GRANTED, new \stdClass(), 'ACCESS_GRANTED if attribute is an integer', new Vote()],
5668
];
5769
}
5870

5971
/**
6072
* @dataProvider getTests
6173
*/
62-
public function testVote(VoterInterface $voter, array $attributes, $expectedVote, $object, $message)
74+
public function testVote(VoterInterface $voter, array $attributes, $expectedVote, $object, $message, ?Vote $vote = null)
6375
{
64-
$this->assertEquals($expectedVote, $voter->vote($this->token, $object, $attributes), $message);
76+
$this->assertSame($expectedVote, $voter->vote($this->token, $object, $attributes, $vote), $message);
77+
78+
if (null !== $vote) {
79+
self::assertSame($expectedVote, $vote->result);
80+
}
6581
}
6682

6783
public function testVoteWithTypeError()

0 commit comments

Comments
 (0)