FOSUserBundle icon indicating copy to clipboard operation
FOSUserBundle copied to clipboard

Overriding behavior when confirmation token not found

Open maryo opened this issue 10 years ago • 4 comments

Looks like there is no easy/clean way for overriding behavior when confirmation token is not found. I'd like to display a flash message instead of displaying a generic 404 page.

Currently it throws generic NotFoundHttpException exception. https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/Controller/RegistrationController.php#L107

There is no event for it and the exception is catchable only by the exception class + message (that's smell ofc). So probably the only viable way to override this is to route this url to custom action or override the controller (i prefer no to do this).

Or is there a better way? What do you think? Should the bundle allow an easier way?

Few solutions came up my mind:

  • dispatch an event and allow to set a custom response
  • create a special exception class
  • just add an exception code

maryo avatar Oct 08 '15 12:10 maryo

Hmm.. I'm sorry. Actually the 404 is a better behavior than the flash message. So just translating it should be enough.

There are two messages:

  • The user with confirmation token "%s" does not exist in case of registration
  • The user with "confirmation token" does not exist for value "%s" in case of resetting

They look almost the same but they are displayed in different scenarios. I think it would be better to be able to better distinguish the scenario. But replacing the default message would be a sort of BC break. So I'm closing this.

EDIT: Or maybe it should be kept open. I'd personally prefer two exception classes with no message change.

EDIT2: Actually it is event not simple to translate the message since it changes based on the token token "%s" so it's far from ideal. Even the exception code would help and it wouldn't cause any BC break. I guess nobody depends on exception code 0.

maryo avatar Oct 08 '15 13:10 maryo

Displaying the exception message directly to the end user is a bad idea though. Exception messages in Symfony are not meant to be safe for display to users, but to provide enough info for developers. This means 2 things:

  • there is no BC on the exception message itself (we can improve it at any time, which would break your translations if you use it as key)
  • the exception message can leak information which should not be given to the end-user

stof avatar Oct 08 '15 15:10 stof

Yes. I am aware of this.

{% set translatedMessage = exception.message|trans({}, 'exceptions') %}
{{ message == translatedMessage ? status_text|trans({}, 'exceptions') : translatedMessage  }}

Even it's far from a good solution it mostly solves it (except you cannot translate to same text, maybe using someting like "true" :D - and it depends on the message which should not)

Ofc I could write custom exception mapper. Something simmilar to what's present in FOSRestBundle but not sure I like it :).

BUT... Since the exception message is created using sprintf token "%s" it's not translatable at all :(. Only using regexp but that's also dirty...

maryo avatar Oct 08 '15 15:10 maryo

BTW OFC I'd made a PR if we aggreed it should be solved with an acceptable solution.

first solution: using new exception class(es)

  • either NonExistentTokenException/NonExistentUserTokenException/TokenNotFoundException/UserTokenNotFoundException with 2 codes like CODE_CONFIRMATION, CODE_RESETTING
  • using two new exception classes like NonExistentConfirmationToken and NonExistentResettingToken

But I'm not sure where to put them, there is no custom exception in the whole bundle.

second solution: using events but I don't know which event would fit the concept, the initialize event is dispatched AFTER fetching the user by the token

third solution: adding an exception codes

I doubt overriding/replacing the whole controller action just because of a message is acceptable DX. Another workarround is probably by listening on exception event based on controller event and action, also not so cool (but probably the best workarround).

maryo avatar Oct 08 '15 16:10 maryo