Skip to content

Commit

Permalink
minor #54086 [Security][Tests] Update functional tests to better refl…
Browse files Browse the repository at this point in the history
…ect end-user scenarios (llupa)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Security][Tests] Update functional tests to better reflect end-user scenarios

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | N/A
| License       | MIT

Pinging `@wouterj`

This PR is related to #53851 's Context.

> A person going through Symfony docs for the first time wanted to create their own LoginFormType as a next step in their learning Symfony journey and noticed that you can submit empty username/password with form login.
>
> They wanted to disallow this and tried to add validation. To validate a login form is not so straight forward as it either needs to be done with a custom authenticator (complex validation) or user provider if the data checks are simple.

Following comments:

#53851 (comment)
> Given the broken high-deps build, I wonder if this shouldn't even be done with a deprecation notice before making it throw in 8.0?

#53851 (comment)
> These are 3 tests submitting an empty login form to trigger a CSRF token error. This new condition now takes precedence, meaning it returns the wrong error.
I don't think that is something we have to worry about (in both situations, login errors), it rather reveals a bad test in our codebase. I can't think of a use-case that would result in success and will become a failure after this merge.

#53851 (comment)
> I think we need consensus on whether we find this a hard BC break that deserves a smooth upgrade path, but the test need to be fixed whatever the conclusion

Commits
-------

4155f66 [Security][Tests] Update functional tests to better reflect end-user scenarios
  • Loading branch information
chalasr committed Mar 1, 2024
2 parents bb7c711 + 4155f66 commit 1f386a3
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 2 deletions.
Expand Up @@ -126,13 +126,13 @@ public function testCustomFailureHandler()

$client->request('POST', '/firewall1/login', [
'_username' => 'jane@example.org',
'_password' => '',
'_password' => 'wrong',
]);
$this->assertResponseRedirects('http://localhost/firewall1/login');

$client->request('POST', '/firewall1/dummy_login', [
'_username' => 'jane@example.org',
'_password' => '',
'_password' => 'wrong',
]);
$this->assertResponseRedirects('http://localhost/firewall1/dummy_login');
}
Expand Down
Expand Up @@ -68,6 +68,8 @@ public function testFormLoginWithInvalidCsrfToken($options)
});

$form = $client->request('GET', '/login')->selectButton('login')->form();
$form['user_login[username]'] = 'johannes';
$form['user_login[password]'] = 'test';
$form['user_login[_token]'] = '';
$client->submit($form);

Expand Down

0 comments on commit 1f386a3

Please sign in to comment.