Skip to content

Conversation

@llupa
Copy link
Contributor

@llupa llupa commented Feb 27, 2024

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

@llupa llupa requested a review from chalasr as a code owner February 27, 2024 14:54
@carsonbot carsonbot added this to the 5.4 milestone Feb 27, 2024
@llupa llupa changed the title [Security] Update functional tests to better reflect end-user scenarios [SecurityBundle][Tests] Update functional tests to better reflect end-user scenarios Feb 27, 2024
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looking good (small suggestion to make it super clear what the intent is)

For anyone who doesn't understand the context: we need to make these tests more correct before we can validate usernames/passwords must be non-blank in 7.x (#53851)

@carsonbot carsonbot changed the title [SecurityBundle][Tests] Update functional tests to better reflect end-user scenarios [Security] [Tests] Update functional tests to better reflect end-user scenarios Feb 27, 2024
@OskarStark OskarStark changed the title [Security] [Tests] Update functional tests to better reflect end-user scenarios [Security][Tests] Update functional tests to better reflect end-user scenarios Feb 29, 2024
@chalasr
Copy link
Member

chalasr commented Mar 1, 2024

Thank you @llupa.

@chalasr chalasr merged commit 1f386a3 into symfony:5.4 Mar 1, 2024
@llupa llupa deleted the test-update branch June 10, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants