8000 minor #60550 [Security] do not construct Vote instances inside vote()… · symfony/symfony@cbab5a8 · GitHub
[go: up one dir, main page]
Skip to content

Commit cbab5a8

Browse files
nicolas-grekasnicolas-grekas
committed
minor #60550 [Security] do not construct Vote instances inside vote() (xabbuh)
This PR was merged into the 7.3 branch. Discussion ---------- [Security] do not construct Vote instances inside vote() | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT The so constructed objects will never be seen from the outside. Thus, adding reasons to them doesn't have an effect. Commits ------- 5aaa978 do not construct Vote instances inside vote()
2 parents 69018fe + 5aaa978 commit cbab5a8

File tree

9 files changed

+55
-34
lines changed

9 files changed

+55
-34
lines changed

‎UPGRADE-7.3.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,7 @@ Security
179179
```php
180180
protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token, ?Vote $vote = null): bool
181181
{
182-
$vote ??= new Vote();
183-
184-
$vote->reasons[] = 'A brief explanation of why access is granted or denied, as appropriate.';
182+
$vote?->addReason('A brief explanation of why access is granted or denied, as appropriate.');
185183
}
186184
```
187185

‎src/Symfony/Component/Security/Core/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;

‎src/Symfony/Component/Security/Core/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;

‎src/Symfony/Component/Security/Core/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;

‎src/Symfony/Component/Security/Core/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;

‎src/Symfony/Component/Security/Core/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
}

‎src/Symfony/Component/Security/Core/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
/**

‎src/Symfony/Component/Security/Core/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'], VoterInt 10000 erface::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()

‎src/Symfony/Component/Security/Http/Tests/EventListener/IsGrantedAttributeListenerTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ protected function supports(string $attribute, mixed $subject): bool
232232

233233
protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token, ?Vote $vote = null): bool
234234
{
235-
$vote->reasons[] = 'Because I can 😈.';
235+
$vote?->addReason('Because I can 😈.');
236236

237237
return false;
238238
}

0 commit comments

Comments
 (0)
0