Skip to content

Should Symfony translate security exception messages? #33168

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.

Sign up for GitHub

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

Jump to bottom
Closed
javiereguiluz opened this issue Aug 14, 2019 · 9 comments · Fixed by #38037
Closed

Should Symfony translate security exception messages? #33168

javiereguiluz opened this issue Aug 14, 2019 · 9 comments · Fixed by #38037
Labels
Security Translation

Comments

@javiereguiluz
Copy link
Member

Description
Symfony includes the translation of Security exception messages ... but they are not used automatically.

For example, if you build a custom Guard-based login mechanism ... UsernamePasswordJsonAuthenticationListener can return return new JsonResponse(['error' => $failed->getMessageKey()], 401); and you get Bad credentials. message in English, regardless of the current application locale.

Is this correct? Look at PHPdoc:

   /**
     * Message key to be used by the translation component.
     *
     * @return string
     */
    public function getMessageKey()

So, this is meant to be translated. Should we do it?

In the login form docs we translate this manually:

   {% if error %}
        <div class="alert alert-danger">{{ error.messageKey|trans(error.messageData, 'security') }}</div>
    {% endif %}

Should this message come translated instead?
@javiereguiluz
Copy link
Member Author

Some people said on Slack that the current behavior is correct. The returned value is the translation key because "you can translate it" ... not because "it's translated by Symfony automatically".

And we use real messages instead of message keys to make messages readable when you don't have the Translation component installed.

@linaori
Copy link
Contributor

linaori commented Aug 15, 2019

Could make this behavior configurable in the authenticator, as there can be valid cases where you don't want to translate it yet.

@wouterj
Copy link
Member

wouterj commented Aug 26, 2020 •
edited
Loading

I just looked at the translation integration of the Form component and I don't think there is anything to do here. The form component integrates with translation only in their default Twig templates.

The Security component is "translation ready", we ship translations (src/Symfony/Component/Security/Core/Resources/translations/) and the exceptions have getMessageKey()/getMessageData() methods.

Automatically translating them would mean we have to catch them somewhere, translate them (if translator is enabled) and then rethrow it. That would also require an option to opt-out/in of this feature. I don't think it's worth it. We might benefit from better documenting this though

@xabbuh
Copy link
Member

xabbuh commented Aug 26, 2020

I agree with Wouter.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 26, 2020

UsernamePasswordJsonAuthenticationListener not translating the security exception looks like a bug to me btw :)

@wouterj
Copy link
Member

wouterj commented Aug 26, 2020 •
edited
Loading

UsernamePasswordJsonAuthenticationListener not translating the security exception looks like a bug to me btw :)

I think this is an interesting idea and quite simple to implement. We can add a setTranslator() method to the json authenticator and translate the error if it's set:

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

Lines 117 to 121 in 6d521d4

public function onAuthenticationFailure(Request $request, AuthenticationException $exception): ?Response
{
if (null === $this->failureHandler) {
return new JsonResponse(['error' => $exception->getMessageKey()], JsonResponse::HTTP_UNAUTHORIZED);
}

Just checked, this is the only authenticator/failure handler that explicitly creates an error response.

@malteschlueter malteschlueter mentioned this issue Sep 2, 2020
Merged
@malteschlueter
Copy link
Contributor

Is it a bug or new feature? At the moment i changed it in master.

@ro0NL
Copy link
Contributor

ro0NL commented Sep 2, 2020

technically a new feature, behavior-wise a bug. Let's be safe and target master :)

@fabpot fabpot closed this as completed Sep 3, 2020
fabpot added a commit that referenced this issue Sep 3, 2020
@fabpot
feature #38037 Translate failure messages of json authentication (Mal…
b094d43 
…te Schlüter)

This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

Translate failure messages of json authentication

| 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.

Commits
-------

7684663 Translate failure messages of json authentication
@malteschlueter malteschlueter mentioned this issue Sep 3, 2020
Merged
@malteschlueter
Copy link
Contributor

I know this issue is already merged and closed but i added some tests now. See #38044.

fabpot added a commit that referenced this issue Sep 3, 2020
@fabpot
minor #38044 Add tests for translated error messages of json authenti…
4c96577 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Security Translation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Translate failure messages of json authentication
7 participants
@fabpot @javiereguiluz @wouterj @ro0NL @linaori @xabbuh @malteschlueter