Shiro
  1. Shiro
  2. SHIRO-243

when method is unauthorized, please include method info in stack trace

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.0
    • Component/s: None
    • Labels:
      None

      Description

      We are using Shiro's annotation-based method authorization support, to enforce security checks on remotely invoked services. The problem is that when we get an AuthorizationException, it doesn't include any information about which particular method failed. Looks like it would be really easy to include this in AuthorizingAnnotationMethodInterceptor.assertAuthorized() as follows:

      public void assertAuthorized(MethodInvocation method) throws AuthorizationException {
      try

      { ((AuthorizingAnnotationHandler)getHandler()).assertAuthorized(getAnnotation(mi)); }

      catch(AuthorizationException ae)

      { throw new AuthorizationException("method not authorized: " + method.getMethod(), ae); }

      }

        Activity

        Hide
        Jim Newsham added a comment -

        P.S. I've already implemented a similar approach in the method interceptors for our own custom annotations, but I would have to override Shiro's method interceptors to get the same benefit there; seems more sensible for this to be in the parent class – AuthorizingAnnotationMethodInterceptor.

        Show
        Jim Newsham added a comment - P.S. I've already implemented a similar approach in the method interceptors for our own custom annotations, but I would have to override Shiro's method interceptors to get the same benefit there; seems more sensible for this to be in the parent class – AuthorizingAnnotationMethodInterceptor.
        Hide
        Les Hazlewood added a comment -

        Good idea Jim - thanks for the issue.

        Show
        Les Hazlewood added a comment - Good idea Jim - thanks for the issue.
        Hide
        Kalle Korhonen added a comment -

        Re-evaluating the suggested solution, wrapping breaks unit tests in aspectj. I was thinking exactly that it's not necessarily a good practice to hide the actual root cause but didn't run tests elsewhere than core itself. Reverting for now, it's possible I'll just fix the unit tests.

        Show
        Kalle Korhonen added a comment - Re-evaluating the suggested solution, wrapping breaks unit tests in aspectj. I was thinking exactly that it's not necessarily a good practice to hide the actual root cause but didn't run tests elsewhere than core itself. Reverting for now, it's possible I'll just fix the unit tests.
        Hide
        Jared Bunting added a comment -

        There are subclasses of AuthorizationException. Wrapping it will also break any attempts to catch certain subclasses (UnauthenticatedException for example).

        Show
        Jared Bunting added a comment - There are subclasses of AuthorizationException. Wrapping it will also break any attempts to catch certain subclasses (UnauthenticatedException for example).
        Hide
        Kalle Korhonen added a comment -

        Right, that's one of main reasons wrapping probably isn't worth the price.

        Show
        Kalle Korhonen added a comment - Right, that's one of main reasons wrapping probably isn't worth the price.
        Hide
        Kalle Korhonen added a comment -

        Might still be useful to make it visible in the exception hierarchy rather than just write to a log. Perhaps make an attempt to initCause() with AuthorizationException and a message set.

        Show
        Kalle Korhonen added a comment - Might still be useful to make it visible in the exception hierarchy rather than just write to a log. Perhaps make an attempt to initCause() with AuthorizationException and a message set.
        Hide
        Kalle Korhonen added a comment -

        Initialize the cause of the thrown Exception if not previously set. May not read that well if stacktrace is printed out (UnauthenticatedException caused by AuthorizationException) but semantically can be seen as correct (method authorization caused permission check).

        Show
        Kalle Korhonen added a comment - Initialize the cause of the thrown Exception if not previously set. May not read that well if stacktrace is printed out (UnauthenticatedException caused by AuthorizationException) but semantically can be seen as correct (method authorization caused permission check).
        Hide
        Les Hazlewood added a comment -

        Closing with the 1.2.0 release.

        Show
        Les Hazlewood added a comment - Closing with the 1.2.0 release.

          People

          • Assignee:
            Kalle Korhonen
            Reporter:
            Jim Newsham
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development