Merge lp:~ev/canonical-identity-provider/error-messages into lp:canonical-identity-provider/release

[Evan]

Hand error messages up to the response

Pretty straightforward. We were writing helpful little error messages and then throwing them away.

[Thomi Richards (thomir-deactivatedaccount)]

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.

[Natalia Bidart (nataliabidart)]

I agree with the outcome of the discussion, and wanted to add that tests were missing, so please consider adding the proper suite for the new approach :-)

Unmerged revisions

1495. By Evan on 2016-07-06

Actually pass the exception message up to the response.

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-06-08 00:55:13 +0000
+++ src/api/v20/handlers.py	2016-07-06 00:53:40 +0000
@@ -565,12 +565,12 @@
 
         try:
             new_discharge = method(*params)
-        except ValidationError:
-            return errors.INVALID_DATA()
+        except ValidationError as e:
+            return errors.INVALID_DATA(message=e.message)
         except AccountDeactivated:
             return errors.ACCOUNT_DEACTIVATED()
-        except AuthenticationError:
-            return errors.INVALID_CREDENTIALS()
+        except AuthenticationError as e:
+            return errors.INVALID_CREDENTIALS(message=e.message)
 
         response = rc.ALL_OK
         response.content = dict(discharge_macaroon=new_discharge.serialize())