Skip to content
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

[Security] Custom Authenticator: What does "validate no parameter is empty" mean? #19812

Open
ThomasLandauer opened this issue Apr 21, 2024 · 2 comments
Labels

Comments

@ThomasLandauer
Copy link
Contributor

@wouterj You were the last one who edited the code block at https://symfony.com/doc/current/security/custom_authenticator.html#passport-badges in this commit: 01cb2b0

What does "validate no parameter is empty" mean there?

  1. Why should I do that? If there's no password given, it's treated as wrong password, so that looks OK for me.
  2. How should I do that? If the password is indeed '', I still need to return a Passport. So (except from throwing an exception), there's nothing much I could do about it (especially can't generate a form error message), right?
  3. Why is the comment shown in a code sample about CSRF?

    For instance, if you want to add CSRF to your custom authenticator, you would initialize the passport like this:

=> So I'd say either just delete it, or give a more complete idea of what to do:

if ('' === $password) {
    // ... ?
}
@wouterj
Copy link
Member

wouterj commented Apr 22, 2024

Hi! I agree with (3), this seems an irrelevant to mention in this particular example. Let's remove the text and leave just // ....

As for your other 2 questions: I would say it's good to validate user input related to authentication explicitly if it confirms the rules of your application, rather than relying on "implementation details" of the Symfony framework.
This is also an approach we use in the build-in Symfony authenticators (we recently added it for form login: symfony/symfony#53851).

Throwing an authentication exception is the best solution, this will give the validation the same handling as other authentication error messages like "bad credentials" (i.e. call the onAuthenticationFailure() method of the current authenticator, which then has some way to return the error to the client).

@ThomasLandauer
Copy link
Contributor Author

Sorry, I wasn't aware that an exception in this context isn't really thrown, but caught by the next method. So raising an exception is indeed the way to go!

So I think this code snippet should be expanded to something like:

if ('' === $username || '' === $password) {
    throw new WhateverException();
}

But where to put it?
This page is missing a full example of a form login. This is due to the fact that there cannot be a full example for every possible setup. But: When introducing the sub-headings (as I suggested in #19813 (comment) ), the new section under authenticate() would be the perfect place!

So in case you're already losing overview of what I have in mind ;-)
Should I create one big PR for everything on this page, or try to do it in smaller steps?

javiereguiluz added a commit that referenced this issue Jun 17, 2024
This PR was merged into the 6.4 branch.

Discussion
----------

[Security] Remove an unneeded comment

It fixes one of the issues reported in #19812.

Commits
-------

eccdbfd [Security] Remove an unneeded comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants