Skip to content

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

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 1, 2024
Merged

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