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

Translate failure messages of json authentication #38037

Merged
merged 1 commit into from Sep 3, 2020
Merged

Translate failure messages of json authentication #38037

merged 1 commit into from Sep 3, 2020

Conversation

malteschlueter
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Resolves #33168
License MIT
Doc PR -

Until now the failure messages of the json authentication were not translated. I'm not sure if it's a bug or a new feature. The changes shouldn't be a BC.

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.

Thanks for working on this one. It's definitely a feature (so targetting master is a good idea).

I don't think we have to add this to the UsernamePasswordJsonAuthenticationListener (those are part of a system that will be removed in Symfony 6). If you can, I think it's also a good idea to add a test for this new feature.

@xabbuh xabbuh added the Security label Sep 2, 2020
@malteschlueter
Copy link
Contributor Author

Thanks for working on this one. It's definitely a feature (so targetting master is a good idea).

My pleasure.

I don't think we have to add this to the UsernamePasswordJsonAuthenticationListener (those are part of a system that will be removed in Symfony 6). If you can, I think it's also a good idea to add a test for this new feature.

I will add a test. Takes some time^^

@malteschlueter
Copy link
Contributor Author

During writing the test i saw some other error messages like "Invalid username." (JsonLoginAuthenticator.php#L167).
Am i blind or is there no translation for it?

What do you think about the other error messages. Should they also translated see "Invalid JSON." or "The key "username" must be provided."

@ro0NL
Copy link
Contributor

ro0NL commented Sep 2, 2020

i figure we can re-use a bunch of translations from the validator domain.

@ro0NL
Copy link
Contributor

ro0NL commented Sep 2, 2020

no wait, i dont think http exceptions need to be translated at all. Usually they are caught by an ExceptionListener exposing the http status text in prod (while preserving the original message in dev).

In this case im fine with only translating the responses generated here 👍

@wouterj
Copy link
Member

wouterj commented Sep 2, 2020

Please note that the $message constructor is a non-user safe string and the messageKey is almost always hardcoded in the exception class. And this message key does exists:

<trans-unit id="4">
<source>Invalid credentials.</source>
<target>Invalid credentials.</target>
</trans-unit>

@xabbuh
Copy link
Member

xabbuh commented Sep 3, 2020

@malteschlueter looks like the PR already needs a rebase 🙈

@xabbuh xabbuh added this to the next milestone Sep 3, 2020
@fabpot
Copy link
Member

fabpot commented Sep 3, 2020

Thank you @malteschlueter.

@fabpot fabpot merged commit b094d43 into symfony:master Sep 3, 2020
@malteschlueter malteschlueter deleted the issue/33168-translate-exceptions branch September 3, 2020 08:05
@malteschlueter
Copy link
Contributor Author

malteschlueter commented Sep 3, 2020

Thanks for merging @fabpot. It was a little bit too fast^^
I missed to add some tests. I've done it here #38044 now. :)

fabpot added a commit that referenced this pull request Sep 3, 2020
…cation (Malte Schlüter)

This PR was merged into the 5.2-dev branch.

Discussion
----------

Add tests for translated error messages of json authentication

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #33168
| License       | MIT
| Doc PR        | -

In PR #38037 i added the translator to the json authenticator but there are some tests missing. I added some now.

Commits
-------

b50fc19 Add tests for translated error messages of json authentication
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
chalasr added a commit to lexik/LexikJWTAuthenticationBundle that referenced this pull request Jan 27, 2022
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Translate message errors

Hi `@chalasr`,

The exception messages are not translated even if the locale was set.
I largely ~copied~ inspired my code from this PR: symfony/symfony#38037 to translate the message.

Let me know if any changes need to be done.

Commits
-------

7c7bf70 Translate message errors
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.

Should Symfony translate security exception messages?
7 participants