Merge lp:~kelvin.li/canonical-identity-provider/error_normalization_tokens_discharge into lp:canonical-identity-provider/release

[Kelvin Li]

update the snap related 2 endpoints to use the normalization error format.

update the snap related 2 endpoints to use the normalization error format.

[Facundo Batista (facundo)]

Please include a general format assertion at test case level as I did in SCA, so we're sure ALL error responses are in the new format.

Also, added some notes and stuff to fix in the inline comments.

Thanks!

review: Needs Fixing

[Kelvin Li (kelvin.li)]

Thanks for the review, after putting the general format assertion in the test cases,
I fixed the related new issues in test cases. also applied your comments into the code.

[Facundo Batista (facundo)]

Just for completeness, commenting here what we discussed in IRC: the "code" in the error_list items must not be the current ones, but some specific codes

[Kelvin Li (kelvin.li)]

updated the standard error code in the response and related api doc.

[Facundo Batista (facundo)]

As discussed in IRC: please include only the errors that SSO is using, and make the relationship between each error and its code something of the ERRORS structure itself.

Thanks!

[Kelvin Li (kelvin.li)]

1. Clean the standard error codes to keep the ones SSO is using;
2. Update the API doc;
3. Move the error code into the ERRORS dict to create the relation mapping to the old code;

[Facundo Batista (facundo)]

A couple of inline questions, thanks!

[Kelvin Li (kelvin.li)]

Hi, Please see my replies in below inline.

[Facundo Batista (facundo)]

Looks fine, thanks!

review: Approve

=== modified file 'docs/api.txt'
--- docs/api.txt	2015-10-22 17:07:12 +0000
+++ docs/api.txt	2016-09-02 11:35:34 +0000
@@ -97,3 +97,119 @@
   and a reason why they failed.
 
 See the relevant documentation for errors specific to each api.
+
+
+New format Errors
+=================
+
+The *Snap Packages API* uses conventional HTTP response codes to indicate
+success or failure of an API request.
+
+In general, codes in the 2xx range indicate success, codes in the 4xx range
+indicate an error that resulted from the provided information (e.g. a
+required parameter was missing) and codes in the 5xx range indicate an
+error with our servers.
+
+Here is detailed the format for API responses that end in error.
+
+This applies to all the 4xx responses, but also to some 5xx ones (if
+possible, the client should be prepared to handle 5xx responses with no
+informational body). Note that this structure format does not apply to 2xx and
+3xx responses, as those are note errors.
+
+.. important::
+
+    Not all API endpoints are migrated yet to this new error format
+
+
+Format
+------
+
+An error response body will contain the following field:
+
+- ``error_list``: a list of one or several items (never empty), each item
+  described by...
+
+  - ``message``: a text in English describing the error that happened,
+    ready to show to the user.
+
+  - ``code``: a short (but representative) string indicating concisely the
+    error; it's aimed for clients to take specific actions and react to the
+    problem. See below for the list of existing codes.
+
+
+Additionally and for backwards compatibility reasons, some other fields may
+be present as well, but are considered deprecated and will be removed in
+the near future.
+
+No status or success indication is returned inside the response body, the
+client should react properly to the received HTTP return code according to
+its `well stablished semantics
+<http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html>`_.
+
+
+Codes
+-----
+
+These are the codes used in the response and their meanings:
+
+- ``account-not-ready``: the account is deactivated, suspended or account email
+  is not valildated.
+
+- ``bad-request``: there is a problem in the structure of the request.
+
+- ``internal-server-error``: some unexpected problem server side; this will be
+  the code in all 5xx cases.
+
+- ``invalid-credentials``: the credentials for the authentication are not valid
+  (e.g.: the username, password are not correct, the 2 factor is needed, etc).
+
+- ``invalid-data``: the data is incorrect or corrupt.
+
+- ``permission-required``: the macaroon authorization is missing in
+  the received request or not enough for it to be fulfilled.
+
+- ``required``: the field in the request can't be empty or null.
+
+- ``resource-not-found``: one or more fields are included to specify a
+  resource, but it is not found in the Store.
+
+- ``resource-not-ready``: the request actions on a resource that is not
+  ready yet for that purpose; normally something else would need to be done
+  first on the resource before this request can be repeated.
+
+- ``user-not-ready``: the user is not ready to issue the received request;
+  normally some actions would need to be done in the user account before
+  repeating the request.
+
+
+Examples
+--------
+
+A simple error:
+
+.. sourcecode:: json
+
+   {
+     "error_list": [{
+        "message": "The field 'expiration' must be an integer",
+        "code": "invalid-field"
+     }]
+   }
+A multiple error:
+
+.. sourcecode:: json
+
+   {
+     "error_list": [{
+        "message": "The 'foo' field is required",
+        "code": "missing-field"
+     }, {
+        "message": "The 'bar' field is required",
+        "code": "missing-field"
+     }, {
+        "message": "The 'baz' field must not be empty",
+        "code": "invalid-field"
+     }]
+   }
+

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-08-11 22:06:36 +0000
+++ src/api/v20/handlers.py	2016-09-02 11:35:34 +0000
@@ -94,13 +94,14 @@
     statsd.timing('view.{module}.{name}.{method}.{suffix}'.format(**data), ms)
 
 
-def valid_email(email):
+def valid_email(email, new_style=False):
     if email is None:
-        return errors.INVALID_DATA(email=[FIELD_REQUIRED])
+        return errors.INVALID_DATA(email=[FIELD_REQUIRED], new_style=new_style)
+
     try:
         validate_email(email)
     except ValidationError as e:
-        result = errors.INVALID_DATA(email=e.messages)
+        result = errors.INVALID_DATA(email=e.messages, new_style=new_style)
     else:
         result = email
     return result
@@ -110,8 +111,8 @@
     return openid
 
 
-def get_email_from_request_data(request):
-    return valid_email(request.data.get('email'))
+def get_email_from_request_data(request, new_style):
+    return valid_email(request.data.get('email'), new_style)
 
 
 def get_token_from_args(request, token):
@@ -373,7 +374,7 @@
 
     @require_mime('json')
     @throttle(extra_callback=get_email_from_request_data)
-    def create(self, request):
+    def create(self, request, new_style=False):
         data = request.data
         try:
             email = data['email']
@@ -444,9 +445,10 @@
 
     @require_mime('json')
     @throttle(extra_callback=get_email_from_request_data)
-    def create(self, request):
+    def create(self, request, new_style=True):
+
         if settings.CRYPTO_SSO_PRIVKEY is None:
-            return errors.RESOURCE_NOT_FOUND()
+            return errors.RESOURCE_NOT_FOUND(new_style=new_style)
 
         data = request.data
         try:
@@ -455,7 +457,7 @@
         except KeyError:
             expected = {'email', 'password'}
             missing = dict((k, [FIELD_REQUIRED]) for k in expected - set(data))
-            return errors.INVALID_DATA(**missing)
+            return errors.INVALID_DATA(new_style=new_style, **missing)
 
         if 'macaroon' in data:
             simple_macaroon = True
@@ -473,24 +475,24 @@
         else:
             # let's flag the new one only, not the deprecated ones
             missing = {'caveat_id': [FIELD_REQUIRED]}
-            return errors.INVALID_DATA(**missing)
+            return errors.INVALID_DATA(new_style=new_style, **missing)
 
         account = None
         response = None
         try:
             account = authenticate_user(email, password, request=request)
         except AccountSuspended:
-            response = errors.ACCOUNT_SUSPENDED()
+            response = errors.ACCOUNT_SUSPENDED(new_style=new_style)
         except AccountDeactivated:
-            response = errors.ACCOUNT_DEACTIVATED()
+            response = errors.ACCOUNT_DEACTIVATED(new_style=new_style)
         except EmailInvalidated:
-            response = errors.EMAIL_INVALIDATED()
+            response = errors.EMAIL_INVALIDATED(new_style=new_style)
         except PasswordPolicyError as e:
             root_url = request.build_absolute_uri('/')
             response = errors.PASSWORD_POLICY_ERROR(
-                reason=unicode(e), location=root_url)
+                reason=unicode(e), location=root_url, new_style=new_style)
         except AuthenticationError:
-            response = errors.INVALID_CREDENTIALS()
+            response = errors.INVALID_CREDENTIALS(new_style=new_style)
 
         if account is not None:
             otp = data.get('otp')
@@ -499,9 +501,9 @@
                     twofactor.authenticate(account, otp)
                 except AuthenticationError:
                     account = None
-                    response = errors.TWOFACTOR_FAILURE()
+                    response = errors.TWOFACTOR_FAILURE(new_style=new_style)
             elif account.twofactor_required:
-                response = errors.TWOFACTOR_REQUIRED()
+                response = errors.TWOFACTOR_REQUIRED(new_style=new_style)
 
         if account is None:
             # track failed login attempt
@@ -525,9 +527,10 @@
                         account, root_macaroon_raw)
                     all_discharges.append((mac_id, discharge.serialize()))
         except ValidationError:
-            return errors.INVALID_DATA()
+            return errors.INVALID_DATA(new_style=new_style)
+
         except AuthenticationError:
-            return errors.INVALID_CREDENTIALS()
+            return errors.INVALID_CREDENTIALS(new_style=new_style)
 
         response = rc.ALL_OK
         if simple_macaroon:
@@ -546,24 +549,25 @@
 
     @require_mime('json')
     def create(self, request):
+
         if settings.CRYPTO_SSO_PRIVKEY is None:
-            return errors.RESOURCE_NOT_FOUND()
+            return errors.RESOURCE_NOT_FOUND(new_style=True)
 
         data = request.data
         try:
             discharge_macaroon_raw = data['discharge_macaroon']
         except KeyError:
             missing = {'discharge_macaroon': [FIELD_REQUIRED]}
-            return errors.INVALID_DATA(**missing)
+            return errors.INVALID_DATA(new_style=True, **missing)
 
         try:
             new_discharge = auth.refresh_discharge(discharge_macaroon_raw)
         except ValidationError:
-            return errors.INVALID_DATA()
+            return errors.INVALID_DATA(new_style=True)
         except AccountDeactivated:
-            return errors.ACCOUNT_DEACTIVATED()
+            return errors.ACCOUNT_DEACTIVATED(new_style=True)
         except AuthenticationError:
-            return errors.INVALID_CREDENTIALS()
+            return errors.INVALID_CREDENTIALS(new_style=True)
 
         response = rc.ALL_OK
         response.content = dict(discharge_macaroon=new_discharge.serialize())

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2016-08-11 22:06:36 +0000
+++ src/api/v20/tests/test_handlers.py	2016-09-02 11:35:34 +0000
@@ -25,6 +25,7 @@
 from timeline import Timeline
 
 from api.v20 import handlers, whitelist
+from api.v20.utils import install_error_format_checker
 from identityprovider.auth import build_discharge_macaroon
 from identityprovider.login import PasswordPolicyError
 from identityprovider.models import (
@@ -2506,6 +2507,10 @@
 
     url = reverse('api-macaroon-discharge')
 
+    def setUp(self):
+        super(MacaroonDischargeHandlerTestCase, self).setUp()
+        install_error_format_checker(self)
+
     def do_post(self, data=None, expected_status_code=200,
                 content_type='application/json', **kwargs):
         if data is None:
@@ -2564,7 +2569,13 @@
         self.assertEqual(json_body, {
             'code': 'INVALID_DATA',
             'extra': {'email': [handlers.FIELD_REQUIRED]},
-            'message': 'Invalid request data'},
+            'message': 'Invalid request data',
+            'error_list': [{
+                'code': 'invalid-data',
+                'extra': {'email': [handlers.FIELD_REQUIRED]},
+                'message': 'Invalid request data'
+                }],
+            },
         )
         self.assertEqual(self.login_failed_calls, [])
 
@@ -2791,6 +2802,7 @@
             unbound_discharge)
 
         self.data = {'discharge_macaroon': unbound_discharge.serialize()}
+        install_error_format_checker(self)
 
     def do_post(self, data=None, expected_status_code=200,
                 content_type='application/json', **kwargs):
@@ -2822,12 +2834,17 @@
 
     def test_required_parameters_discharge(self):
         json_body = self.do_post(expected_status_code=400, data={})
-
-        self.assertEqual(json_body, {
+        expected = {
             'code': 'INVALID_DATA',
             'extra': {'discharge_macaroon': [handlers.FIELD_REQUIRED]},
-            'message': 'Invalid request data'},
-        )
+            'message': 'Invalid request data',
+            'error_list': [{
+                'message': 'Invalid request data',
+                'code': 'invalid-data',
+                'extra': {'discharge_macaroon': ['Field required']}
+            }]
+        }
+        self.assertEqual(json_body, expected)
         self.assertEqual(self.login_failed_calls, [])
 
     def test_account_missing(self):

=== modified file 'src/api/v20/utils.py'
--- src/api/v20/utils.py	2016-08-04 12:03:24 +0000
+++ src/api/v20/utils.py	2016-09-02 11:35:34 +0000
@@ -17,12 +17,13 @@
             # response from inside the API are always expected to have a JSON
             # content
             msg = '%s: %s' % (exception.__class__.__name__, unicode(exception))
-            response = api_error(status=500, code='SERVER_ERROR', message=msg)
+            response = api_error(status=500, code='SERVER_ERROR', message=msg,
+                                 error_code=ErrorCode.INTERNAL_ERROR)
 
         return response
 
 
-def api_error(status, code, message, extra=None):
+def api_error(status, code, message, error_code, extra=None):
     if extra is None:
         extra = {}
 
@@ -39,58 +40,128 @@
         'message': unicode(message),
         'extra': extra_data,
     }
+
+    # check if we're going to use the new style error format
+    new_style = extra_data.pop('new_style', False)
+    if new_style:
+        error_list_data = {
+            'code': error_code,
+            'message': data['message'],
+        }
+        if data['extra']:
+            error_list_data['extra'] = data['extra']
+        data['error_list'] = [error_list_data]
+
     response = HttpResponse(content=json.dumps(data), status=status,
                             content_type='application/json; charset=utf-8')
     response['Vary'] = 'Accept'
     return response
 
 
+class ErrorCode:
+    ACCOUNT_NOT_READY = 'account-not-ready'
+    BAD_REQUEST = 'bad-request'
+    INTERNAL_ERROR = 'internal-server-error'
+    INVALID_DATA = 'invalid-data'
+    INVALID_CREDENTIALS = 'invalid-credentials'
+    INVALID_REQUEST = 'invalid-request'
+    PERMISSION_REQUIRED = 'permission-required'
+    REQUIRED = 'field-required'
+    RESOURCE_NOT_FOUND = 'resource-not-found'
+    RESOURCE_NOT_READY = 'resource-not-ready'
+
+    def __init__(self):
+        self._codes = {getattr(self, code) for code in dir(self)
+                       if code[0] != "_" and code != 'is_valid'}
+
+    def is_valid(self, code):
+        return code in self._codes
+
+
 ERRORS = dict(
-    INVALID_DATA=(400, _("Invalid request data")),
+    INVALID_DATA=(400, _("Invalid request data"), ErrorCode.INVALID_DATA),
 
     CAPTCHA_REQUIRED=(
-        401, _('A captcha challenge is required to complete the request.')),
-    INVALID_CREDENTIALS=(401, _('Provided email/password is not correct.')),
-    TWOFACTOR_REQUIRED=(401, _("2-factor authentication required.")),
+        401, _('A captcha challenge is required to complete the request.'),
+        ErrorCode.REQUIRED),
+
+    INVALID_CREDENTIALS=(
+        401, _('Provided email/password is not correct.'),
+        ErrorCode.INVALID_CREDENTIALS),
+
+    TWOFACTOR_REQUIRED=(
+        401, _("2-factor authentication required."),
+        ErrorCode.REQUIRED),
 
     ACCOUNT_SUSPENDED=(
         403, _('Your account has been suspended. Please contact login '
-               'support to re-enable it')),
+               'support to re-enable it'),
+        ErrorCode.ACCOUNT_NOT_READY),
+
     ACCOUNT_DEACTIVATED=(
         403, _('Your account has been deactivated. To reactivate it, '
-               'please reset your password')),
+               'please reset your password'),
+        ErrorCode.ACCOUNT_NOT_READY),
+
     EMAIL_INVALIDATED=(
         403, _("This email address has been invalidated. Please contact "
-               "login support.")),
-    CAN_NOT_RESET_PASSWORD=(403, _('Can not reset password. Please contact '
-                                   'login support')),
-    CAPTCHA_FAILURE=(403, _('Failed response to captcha challenge.')),
-    TOO_MANY_TOKENS=(403, _('Too many non-consumed tokens exist. Further '
-                            'token creation is not allowed until existing '
-                            'tokens are consumed.')),
+               "login support."),
+        ErrorCode.ACCOUNT_NOT_READY),
+
+    CAN_NOT_RESET_PASSWORD=(
+        403, _('Can not reset password. Please contact login support'),
+        ErrorCode.INVALID_REQUEST),
+
+    CAPTCHA_FAILURE=(
+        403, _('Failed response to captcha challenge.'),
+        ErrorCode.RESOURCE_NOT_FOUND),
+
+    TOO_MANY_TOKENS=(
+        403, _('Too many non-consumed tokens exist. Further '
+               'token creation is not allowed until existing '
+               'tokens are consumed.'),
+        ErrorCode.RESOURCE_NOT_READY),
+
     TWOFACTOR_FAILURE=(
-        403, _("The provided 2-factor key is not recognised.")),
-    PASSWORD_POLICY_ERROR = (403, _("Your password doesn't meet "
-                                    "security constraints.")),
-    FORBIDDEN=(403, _("Insufficient permissions.")),
-
-    RESOURCE_NOT_FOUND=(404, _("The resource requested was not found.")),
-
-    ALREADY_REGISTERED=(409, _("The email address is already registered")),
-
-    TOO_MANY_REQUESTS=(429, _('Too many requests. Please try again later.')),
-
-    CAPTCHA_ERROR=(502, _('Unable to get a valid response from '
-                          'reCaptcha service')),
+        403, _("The provided 2-factor key is not recognised."),
+        ErrorCode.INVALID_CREDENTIALS),
+
+    PASSWORD_POLICY_ERROR = (
+        403, _("Your password doesn't meet security constraints."),
+        ErrorCode.INVALID_CREDENTIALS),
+
+    FORBIDDEN=(
+        403, _("Insufficient permissions."),
+        ErrorCode.PERMISSION_REQUIRED),
+
+    RESOURCE_NOT_FOUND=(
+        404, _("The resource requested was not found."),
+        ErrorCode.RESOURCE_NOT_FOUND),
+
+    ALREADY_REGISTERED=(
+        409, _("The email address is already registered"),
+        ErrorCode.RESOURCE_NOT_READY),
+
+    TOO_MANY_REQUESTS=(
+        429, _('Too many requests. Please try again later.'),
+        ErrorCode.BAD_REQUEST),
+
+    CAPTCHA_ERROR=(
+        502, _('Unable to get a valid response from reCaptcha service'),
+        ErrorCode.INTERNAL_ERROR),
+
+    SERVER_ERROR=(
+        500, _('internal server error'),
+        ErrorCode.INTERNAL_ERROR)
 )
 
 
 class Errors(object):
     def wrap(self, code):
-        status, message = ERRORS[code]
+        status, message, error_code = ERRORS[code]
 
         def wrapper(**extra):
-            return api_error(status, code, message, extra)
+            return api_error(status, code, message, error_code, extra)
         wrapper.__name__ = code
         return wrapper
 
@@ -100,6 +171,7 @@
         else:
             raise AttributeError
 
+
 errors = Errors()
 
 
@@ -172,3 +244,54 @@
             token_secret=token.secret,
         ))
     return result
+
+
+def assert_response_error_format(resp):
+    """Check that the error format is ok."""
+    if resp.status_code < 400:
+        # not checked
+        return
+
+    if resp.status_code >= 500 and resp['content-type'] == 'text/plain':
+        # simple crashing
+        return
+
+    # we have now a 4xx response or a 5xx with interesting body, verify it!
+    assert resp['content-type'] == 'application/json; charset=utf-8', (
+        "bad CT: " + repr(resp['content-type']))
+    content = json.loads(resp.content)
+
+    error_list = content['error_list']
+    assert error_list, "Empty error list"
+
+    ec = ErrorCode()
+    for item in error_list:
+        code = item['code']
+        assert ec.is_valid(code), "Invalid code: %r" % (code,)
+
+        message = item['message']
+        assert isinstance(message, basestring), "message must be a string"
+
+        if 'extra' in item:
+            assert isinstance(item['extra'], dict), (
+                "extra info must be a dict")
+
+
+def install_error_format_checker(testcase):
+    # put the ApiResource import here to avoid the following error
+    # AttributeError: 'module' object has no attribute 'test_handlers'
+    from api.v20.auth import ApiResource
+    """Install an error format checker in the TestCase instance."""
+    orig_method = ApiResource.__call__
+
+    def mitm(*a, **k):
+        """Send resource/handler response for proper format assertion."""
+        resp = orig_method(*a, **k)
+        try:
+            assert_response_error_format(resp)
+        except Exception as err:
+            testcase.fail("[Response Format Error] %s(%s)" % (
+                err.__class__.__name__, err))
+        return resp
+
+    testcase.patch(ApiResource, '__call__', new=mitm)