[DX][Testing] Added a loginUser() method to test protected resources #32850
Conversation
javiereguiluz
added
FrameworkBundle
DX
| $this->assertNotNull($client->getCookieJar()->get('MOCKSESSID')); | ||
| } | ||
|
|
||
| public function getUsers() |
There was a problem hiding this comment.
| public function getUsers() | |
| public function getUsers(): \Generator |
OskarStark
left a comment
OskarStark
left a comment
There was a problem hiding this comment.
Nice addition Javier 👍
| { | ||
| public function loginUser(UserInterface $user, string $firewallContext = 'main'): self | ||
| { | ||
| $token = new UsernamePasswordToken($user, null, $firewallContext, $user->getRoles()); |
There was a problem hiding this comment.
Can we use a token specifically designed for testing here (perhaps an object as simple as possible)? The UsernamePasswordToken is specific to certain implementation. This also means that if you don't define any roles for your user, which is a very legit case, it would never consider you as authenticated, as this is a requirement for this particular token, but not Guard for example.
I've traced back the usages of this token in conjunction with "being authenticated", as it's injected into the session, and it should accept any token, regardless of authenticator
There was a problem hiding this comment.
Thanks for the review! Sadly, my knowledge of the Symfony Security parts is not as good as yours ... so I don't understand your comment. I'm afraid that I'll need the full copy+pasteable code to update this pull request. Thanks!
There was a problem hiding this comment.
The issue is this line:
Meaning that whenever you want to authenticate, you are forced to create a user that has a minimum of 1 role, which isn't enforced in any other tokens. One option would be to use the PostAuthenticationGuardToken, the other would be to extends the AbstractToken and implement only what's not there yet.
There's also a ConcreteToken in security/Core/Tests/Authentication/Token/AbstractTokenTest.php, this is basically what I mean with a test specific token. Perhaps it's an idea to extract this token and make it a stub for multiple tests?
|
Hi @javiereguiluz! Thanks for starting this PR, I love how you implemented this feature with a very minimal changeset. As I would like to have this in a next Symfony release, I've continued your work in #35997 . |
|
Closing in favor of #35997, which is much better! Thanks Wouter! |
…cted resources (javiereguiluz, wouterj) This PR was merged into the 5.1-dev branch. Discussion ---------- [DX][Testing] Added a loginUser() method to test protected resources | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #26839 | License | MIT | Doc PR | tbd This finishes #32850 original description: > I know this won't work for 100% of our users ... but the goal is to make life easier to *most* of them. Thanks! A custom `ConcreteToken` test-object is created as suggested by @linaori, to not bind this token to any specific implementation (as other implementations aren't fully compatible with eachother). Commits ------- 2980a68 Added special test token and implemented 'real' functional tests f516829 [DX][Testing] Added a loginUser() method to test protected resources
…cted resources (javiereguiluz, wouterj) This PR was merged into the 5.1-dev branch. Discussion ---------- [DX][Testing] Added a loginUser() method to test protected resources | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #26839 | License | MIT | Doc PR | tbd This finishes symfony/symfony#32850 original description: > I know this won't work for 100% of our users ... but the goal is to make life easier to *most* of them. Thanks! A custom `ConcreteToken` test-object is created as suggested by @linaori, to not bind this token to any specific implementation (as other implementations aren't fully compatible with eachother). Commits ------- 2980a680d4 Added special test token and implemented 'real' functional tests f516829d99 [DX][Testing] Added a loginUser() method to test protected resources
… (artyuum) This PR was submitted for the 5.3 branch but it was squashed and merged into the 5.4 branch instead. Discussion ---------- [Security] Added a note regarding the loginUser() method As `@javiereguiluz` [stated](symfony/symfony#32850), this method won't work for 100% of Symfony users. This is my case. I'm testing some protected API routes and the firewall is using a custom authenticator that relies on a custom header called "x-api-key". After spending hours trying to understand what I did wrong in my tests, I decided to dig into the code that defines the `loginUser()` method and noticed that [it can only work with session-based authentication](https://github.com/symfony/symfony/blob/18ab810a8d6d4c17497303df17e931261d542fce/src/Symfony/Bundle/FrameworkBundle/KernelBrowser.php#L139). This little note could have saved me some time, so I believe it could help future users as well who are in the same case as me. <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/releases for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `5.x` for features of unreleased versions). --> Commits ------- 4d29c99 [Security] Added a note regarding the loginUser() method
7 participants
I know this won't work for 100% of our users ... but the goal is to make life easier to most of them. Thanks!