[Security] Json login exception

[Gabbarowski]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	yes (but I am not sure that it is on the right place :( )
Tickets	No Ticket
License	MIT
Doc PR	?

Hello everyone,

to find my error in my last project, it took me a few hours, but it was such a simple and stupid mistake of mine. The mean thing was that I didn't get an error message. Had I had an error message, it would have been a matter of minutes.

@weaverryan gave me a hint via symfonycast to watch Symfony\Component\Security\Http\Authenticator\JsonLoginAuthenticator and there it was so easy to figure out that I was just trying with application/text and not json. :D

Ryan suggested making a request for a class update, and so I implemented a new method that checks if the user has any content that looks like they are trying to log in, the error message is displayed if they use the wrong request method.

For me it is a first pull request. I hope I didn't do too much wrong :-)

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[carsonbot]

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

[weaverryan]

I love this in theory, however, in practice, I think this breaks backwards-compatibility. For example, suppose someone is currently using json_login AND another authenticator and that both are active on the same URL. The json_login is used when the content-type is application/json and the other authenticator is used otherwise (or maybe for some different content type). This is a legal thing to do, and, unfortunately, this PR would break that.

Would having something in the documentation have helped you? I'm trying to think of another way that we could help users debug, assuming that others agree that this PR won't be able to be accepted.

Cheers!

[wouterj]

In other authenticators, we use the logger to expose this information:

symfony/src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php

Line 67 in 013857a

$this->logger?->debug('Skipping pre-authenticated authenticator as a BadCredentialsException is thrown.', ['exception' => $e, 'authenticator' => static::class]);

We can maybe have a nice log message saying something like Skipping JSON authenticator as the request body is not JSON (got: "%s").?

I'm aware we have to make this information more visible for login actions btw (ref #36668)

[Gabbarowski]

Thank you guys for the nice and very good feedback.

@weaverryan you are totally right, I don't expect this situation, that two authenticators work with the same URL, damn.

@wouterj Yes, I think a logging message is better than nothing. Whereby, I must confess that I not often work with log files. And in my mistake from last week, I don't check the logfiles. But maybe it is because I am not professional ;-)

What is next?
I have to close the request? Or shall I try to make the logger message like the other authenticator work? It doesn't sound complicated, but maybe I'm missing something again? If yes in the same request?
Or would like someone others could try this?

Sorry for my stupid questions.

[weaverryan]

Not stupid questions - you're new here! And you've done a fine start.

You can update THIS PR to revert these changes and add the logger. And you don't need to worry about rebasing or squashing: just add new commits :).

Or would like someone others could try this?

This is definitely for you - you had the great idea to start to see how we could improve things. And, in the open source world, often if YOU don't do it, nobody will. So thanks for showing up!

[Gabbarowski]

Thanks. I will try my best. 😊

[Gabbarowski]

Wow, it was more difficult as I thought in the beginning. :-D
Respect guys that you did all this thing everyday and such faster then me.

I have two further questions:

1. What I have to do with message "modified the milestones: 6.2, 6.3"?
2. Why does the custom authentication remove the body content from request? So it isn't possible to create two login method in one path. So JsonLogin only work with a different path as the custom authenticator. For this I added the message "... May you forget to add content or you installed a custom authenticator on same path ... "