Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security][Guard] check if session exist before using it#19218
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJun 29, 2016
Can you please add a test covering the fix? |
pasdeloup commentedJun 29, 2016
Ok, I will add it. |
pasdeloup commentedJun 29, 2016
Ok tests added. |
pasdeloup commentedJun 29, 2016
AppVeyor is not passing :( |
pasdeloup commentedJun 29, 2016
And now AppVeyor is passing, strange... |
| /** | ||
| * @author Jean Pasdeloup <jpasdeloup@sedona.fr> | ||
| */ | ||
| class MockFormLoginAuthenticator extends AbstractFormLoginAuthenticator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This class should be moved at the bottom of the test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, moved
| } | ||
| if (!$targetPath) { | ||
| if (!isset($targetPath) || !$targetPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would prefer to explicitly initialize$targetPath tonullabove and make a check onnull here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, done.
I also improved the tests.
| public function testAuthenticateWithoutSession() | ||
| { | ||
| $authenticator = new MockFormLoginAuthenticator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not use a PHPUnit mock instead?
fabpot commentedJun 30, 2016
Thank you@pasdeloup. |
…pasdeloup)This PR was squashed before being merged into the 2.8 branch (closes#19218).Discussion----------[Security][Guard] check if session exist before using it| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18958| License | MIT| Doc PR | -As stated by@Shekhovtsovy when the Guard component is used without the Symfony full stack (for instance in Laravel), $request->getSession() may be null.An additionnal PR will be needed for 3.1 but it may be better to check this one before.Commits-------a3f7510 [Security][Guard] check if session exist before using it
As stated by@Shekhovtsovy when the Guard component is used without the Symfony full stack (for instance in Laravel), $request->getSession() may be null.
An additionnal PR will be needed for 3.1 but it may be better to check this one before.