Code review comment for lp:~ev/canonical-identity-provider/error-messages

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Ev and I discussed this on IRC.

I'm -1 to this approach because it has the potential to leak information that we may not want end users seeing. Programmers raising these exceptions have a somewhat-reasonable expectation that the exception messages won't get seen by end users, but will instead end up in sentry. For that reason, exception messages often contain as much information as possible in order to aid debugging. The risk of exposing private information in this fashion is high.

The actual problem in ev's case was that:

"I was handing it a bound discharge rather than the original discharge for refreshing and in that case it was [raising] AuthenticationError"

As discussed on IRC, a better solution here is to find the code that handles that specific case and raise a different exception (probably derived from AuthenticationError), so we can catch it in the API handler and give the user a more descriptive error message.

The API handler would now look something like:

except AuthenticationBoundMacaroonError:
    return errors.INVALID_CREDENTIALS("You gave us a bound discharge macaroon rather than the original discharge macaroon, you dolt!")

...which neatly solves both problems: give users more specific error message, AND avoid leaking private information.

« Back to merge proposal