Page MenuHomePhabricator

Return a status from all AuthManager methods
Open, Needs TriagePublic

Description

AuthManager::changeAuthenticationData is not allowed to fail and returns nothing. Unexpected failure can be handled with an exception but unexpected partial failure (e.g. the data change was successful but a notification email should have been sent and that failed) can't, so it would be better to return a status object so that partial failure can be communicated to the user. Even when there was no failure, sometimes a provider might want to send information to the user, which could be done with a status warning.

That second use case also exists for the other flows which return an AuthenticationResponse. A status would probably make less sense there but the ability to return an array of messages would be nice.

Event Timeline

Tgr created this task.Apr 11 2016, 11:35 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 11 2016, 11:35 AM
Anomie added a subscriber: Anomie.May 23 2016, 6:53 PM

OTOH, AuthManager::changeAuthenticationData failing at all is scary since it can mean that the password was updated in one authentication provider but not another.

Tgr added a comment.May 23 2016, 11:52 PM

OTOH, AuthManager::changeAuthenticationData failing at all is scary since it can mean that the password was updated in one authentication provider but not another.

Yes, but all the more reason to warn the user that something went wrong. In many cases you would do that by throwing an exception (eg. on DB errors) so a return status is not really needed, but for an email notification failure that seems extreme.