Merge pull request #2166 from ckan/2165-check-access-log

[#2165] Only one log message now on check_access.

ckan/controllers/api.py

@@ -209,15 +209,15 @@ def action(self, logic_function, ver=None):
                                     'message': _('Access denied')}
             return_dict['success'] = False
 
-            if e.extra_msg:
-                return_dict['error']['message'] += ': %s' % e.extra_msg
+            if unicode(e):
+                return_dict['error']['message'] += u': %s' % e
 
             return self._finish(403, return_dict, content_type='json')
         except NotFound, e:
             return_dict['error'] = {'__type': 'Not Found Error',
                                     'message': _('Not found')}
-            if e.extra_msg:
-                return_dict['error']['message'] += ': %s' % e.extra_msg
+            if unicode(e):
+                return_dict['error']['message'] += u': %s' % e
             return_dict['success'] = False
             return self._finish(404, return_dict, content_type='json')
         except ValidationError, e:
@@ -289,11 +289,9 @@ def list(self, ver=None, register=None, subregister=None, id=None):
         try:
             return self._finish_ok(action(context, {'id': id}))
         except NotFound, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_found(extra_msg)
+            return self._finish_not_found(unicode(e))
         except NotAuthorized, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_authz(extra_msg)
+            return self._finish_not_authz(unicode(e))
 
     def show(self, ver=None, register=None, subregister=None,
              id=None, id2=None):
@@ -321,11 +319,9 @@ def show(self, ver=None, register=None, subregister=None,
         try:
             return self._finish_ok(action(context, data_dict))
         except NotFound, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_found(extra_msg)
+            return self._finish_not_found(unicode(e))
         except NotAuthorized, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_authz(extra_msg)
+            return self._finish_not_authz(unicode(e))
 
     def _represent_package(self, package):
         return package.as_dict(ref_package_by=self.ref_package_by,
@@ -371,11 +367,9 @@ def create(self, ver=None, register=None, subregister=None,
             return self._finish_ok(response_data,
                                    resource_location=location)
         except NotAuthorized, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_authz(extra_msg)
+            return self._finish_not_authz(unicode(e))
         except NotFound, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_found(extra_msg)
+            return self._finish_not_found(unicode(e))
         except ValidationError, e:
             # CS: nasty_string ignore
             log.error('Validation error: %r' % str(e.error_dict))
@@ -428,11 +422,9 @@ def update(self, ver=None, register=None, subregister=None,
             response_data = action(context, data_dict)
             return self._finish_ok(response_data)
         except NotAuthorized, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_authz(extra_msg)
+            return self._finish_not_authz(unicode(e))
         except NotFound, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_found(extra_msg)
+            return self._finish_not_found(unicode(e))
         except ValidationError, e:
             # CS: nasty_string ignore
             log.error('Validation error: %r' % str(e.error_dict))
@@ -477,11 +469,9 @@ def delete(self, ver=None, register=None, subregister=None,
             response_data = action(context, data_dict)
             return self._finish_ok(response_data)
         except NotAuthorized, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_authz(extra_msg)
+            return self._finish_not_authz(unicode(e))
         except NotFound, e:
-            extra_msg = e.extra_msg
-            return self._finish_not_found(extra_msg)
+            return self._finish_not_found(unicode(e))
         except ValidationError, e:
             # CS: nasty_string ignore
             log.error('Validation error: %r' % str(e.error_dict))

ckan/controllers/group.py

@@ -817,11 +817,11 @@ def follow(self, id):
             h.flash_success(_("You are now following {0}").format(
                 group_dict['title']))
         except ValidationError as e:
-            error_message = (e.extra_msg or e.message or e.error_summary
+            error_message = (e.message or e.error_summary
                              or e.error_dict)
             h.flash_error(error_message)
         except NotAuthorized as e:
-            h.flash_error(e.extra_msg)
+            h.flash_error(e.message)
         h.redirect_to(controller='group', action='read', id=id)
 
     def unfollow(self, id):
@@ -836,11 +836,11 @@ def unfollow(self, id):
             h.flash_success(_("You are no longer following {0}").format(
                 group_dict['title']))
         except ValidationError as e:
-            error_message = (e.extra_msg or e.message or e.error_summary
+            error_message = (e.message or e.error_summary
                              or e.error_dict)
             h.flash_error(error_message)
         except (NotFound, NotAuthorized) as e:
-            error_message = e.extra_msg or e.message
+            error_message = e.message
             h.flash_error(error_message)
         h.redirect_to(controller='group', action='read', id=id)
 

ckan/controllers/package.py

@@ -1213,11 +1213,11 @@ def follow(self, id):
             h.flash_success(_("You are now following {0}").format(
                 package_dict['title']))
         except ValidationError as e:
-            error_message = (e.extra_msg or e.message or e.error_summary
+            error_message = (e.message or e.error_summary
                     or e.error_dict)
             h.flash_error(error_message)
         except NotAuthorized as e:
-            h.flash_error(e.extra_msg)
+            h.flash_error(e.message)
         h.redirect_to(controller='package', action='read', id=id)
 
     def unfollow(self, id):
@@ -1232,11 +1232,11 @@ def unfollow(self, id):
             h.flash_success(_("You are no longer following {0}").format(
                 package_dict['title']))
         except ValidationError as e:
-            error_message = (e.extra_msg or e.message or e.error_summary
+            error_message = (e.message or e.error_summary
                     or e.error_dict)
             h.flash_error(error_message)
         except (NotFound, NotAuthorized) as e:
-            error_message = e.extra_msg or e.message
+            error_message = e.message
             h.flash_error(error_message)
         h.redirect_to(controller='package', action='read', id=id)
 

ckan/controllers/user.py

@@ -671,11 +671,11 @@ def follow(self, id):
             h.flash_success(_("You are now following {0}").format(
                 user_dict['display_name']))
         except ValidationError as e:
-            error_message = (e.extra_msg or e.message or e.error_summary
+            error_message = (e.message or e.error_summary
                              or e.error_dict)
             h.flash_error(error_message)
         except NotAuthorized as e:
-            h.flash_error(e.extra_msg)
+            h.flash_error(e.message)
         h.redirect_to(controller='user', action='read', id=id)
 
     def unfollow(self, id):
@@ -691,10 +691,10 @@ def unfollow(self, id):
             h.flash_success(_("You are no longer following {0}").format(
                 user_dict['display_name']))
         except (NotFound, NotAuthorized) as e:
-            error_message = e.extra_msg or e.message
+            error_message = e.message
             h.flash_error(error_message)
         except ValidationError as e:
-            error_message = (e.error_summary or e.message or e.extra_msg
+            error_message = (e.error_summary or e.message
                              or e.error_dict)
             h.flash_error(error_message)
         h.redirect_to(controller='user', action='read', id=id)

ckan/logic/__init__.py

@@ -34,14 +34,7 @@ def __setattr__(self, name, value):
 
 
 class ActionError(Exception):
-    def __init__(self, extra_msg=None):
-        self.extra_msg = extra_msg
-
-    def __str__(self):
-        err_msgs = (super(ActionError, self).__str__(),
-                    self.extra_msg)
-        return ' - '.join([str(err_msg) for err_msg in err_msgs if err_msg])
-
+    pass
 
 class NotFound(ActionError):
     '''Exception raised by logic functions when a given object is not found.
@@ -84,7 +77,7 @@ def __init__(self, error_dict, error_summary=None, extra_msg=None):
             error_dict['tags'] = tag_errors
         self.error_dict = error_dict
         self._error_summary = error_summary
-        self.extra_msg = extra_msg
+        super(ValidationError, self).__init__(extra_msg)
 
     @property
     def error_summary(self):
@@ -280,25 +273,31 @@ def check_access(action, context, data_dict=None):
         context['__auth_audit'].pop()
 
     user = context.get('user')
-    log.debug('check access - user %r, action %s' % (user, action))
 
-    if not 'auth_user_obj' in context:
-        context['auth_user_obj'] = None
-
-    if not context.get('ignore_auth'):
-        if not context.get('__auth_user_obj_checked'):
-            if context.get('user') and not context.get('auth_user_obj'):
-                context['auth_user_obj'] = model.User.by_name(context['user'])
-            context['__auth_user_obj_checked'] = True
-
-    context = _prepopulate_context(context)
-
-    logic_authorization = new_authz.is_authorized(action, context, data_dict)
-    if not logic_authorization['success']:
-        msg = logic_authorization.get('msg', '')
-        raise NotAuthorized(msg)
-
-    log.debug('Access OK.')
+    try:
+        if not 'auth_user_obj' in context:
+            context['auth_user_obj'] = None
+
+        if not context.get('ignore_auth'):
+            if not context.get('__auth_user_obj_checked'):
+                if context.get('user') and not context.get('auth_user_obj'):
+                    context['auth_user_obj'] = \
+                        model.User.by_name(context['user'])
+                context['__auth_user_obj_checked'] = True
+
+        context = _prepopulate_context(context)
+
+        logic_authorization = new_authz.is_authorized(action, context,
+                                                      data_dict)
+        if not logic_authorization['success']:
+            msg = logic_authorization.get('msg', '')
+            raise NotAuthorized(msg)
+    except NotAuthorized, e:
+        log.debug(u'check access NotAuthorized - %s user=%s "%s"',
+                  action, user, unicode(e))
+        raise
+
+    log.debug('check access OK - %s user=%s', action, user)
     return True
 
 

ckan/new_tests/controllers/test_api.py

@@ -0,0 +1,21 @@
+'''
+NB Don't test logic functions here. This is just for the mechanics of the API
+controller itself.
+'''
+import json
+
+import ckan.new_tests.helpers as helpers
+
+
+class TestApiController(helpers.FunctionalTestBase):
+
+    def test_unicode_in_error_message_works_ok(self):
+        # Use tag_delete to echo back some unicode
+        app = self._get_test_app()
+        org_url = '/api/action/tag_delete'
+        data_dict = {'id': u'Delta symbol: \u0394'}  # unicode gets rec'd ok
+        postparams = '%s=1' % json.dumps(data_dict)
+        response = app.post(url=org_url, params=postparams, status=404)
+        # The unicode is backslash encoded (because that is the default when
+        # you do str(exception) )
+        assert 'Delta symbol: \\u0394' in response.body

ckan/new_tests/logic/action/test_delete.py

@@ -29,3 +29,14 @@ def test_resource_delete(self):
         # It is still there but with state=deleted
         res_obj = model.Resource.get(resource['id'])
         assert_equals(res_obj.state, 'deleted')
+
+    def test_tag_delete_with_unicode_returns_unicode_error(self):
+        # There is not a lot of call for it, but in theory there could be
+        # unicode in the ActionError error message, so ensure that comes
+        # through in NotFound as unicode.
+        try:
+            helpers.call_action('tag_delete', id=u'Delta symbol: \u0394')
+        except logic.NotFound, e:
+            assert u'Delta symbol: \u0394' in unicode(e)
+        else:
+            assert 0, 'Should have raised NotFound'