Tynamo
  1. Tynamo
  2. TYNAMO-102

Specify id for RequestExceptionHandler advice for preventing unintentional override

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: security-0.4.0
    • Fix Version/s: security-0.4.1
    • Component/s: security
    • Labels:
      None
    • Number of attachments :
      0

      Description

      The advice for RequestExceptionHandler is not given an id.
      Providing advice for same service in another module could cause naming conflicts and unintentionally override an existing advice.
      Tapestry-security should specify id for the advice.

      --ref from mail correspondence-
      So I changed from decorator to advice - much like adviseRequestExceptionHandler as in securitymodule.
      public static void adviseRequestExceptionHandler(
      MethodAdviceReceiver receiver,
      final Logger logger,
      final @Local MovellasExceptionHandler handler,
      @Symbol(SymbolConstants.PRODUCTION_MODE) boolean productionMode) {

      if(!productionMode) return;

      final String methodName = "handleRequestException";
      Method handleMethod;

      try

      { Class<?> serviceInterface = receiver.getInterface(); handleMethod = serviceInterface.getMethod(methodName, Throwable.class); }

      catch (Exception e)

      { throw new RuntimeException("Can't find method " + "RequestExceptionHandler." + methodName + ". Changed API?", e); }

      MethodAdvice advice = new MethodAdvice(){
      @Override
      public void advise(Invocation invocation) {
      Throwable exception = (Throwable) invocation.getParameter(0);

      /**

      • Let's not try to handle this as it's the responsibility of tapestry-security
        */
        if(exception instanceof UnauthenticatedException) { invocation.proceed(); return; }

      try

      { handler.handle(exception); }

      catch (Exception e)

      { logger.error("Error handling exception", e); invocation.proceed(); }

      }
      };
      receiver.adviseMethod(handleMethod, advice);
      }

      Now by provoking an UnauthenticatedException the advice will call proceed(due to the check for exception type). However the advice in security module is not called before invoking method on the RequestExceptionHandler.

      Then I tried specifying ordering -
      @Order("after:*")
      public static void adviseRequestExceptionHandler(...}

      Then the advice in securitymodule is called first. Sounds like problem solved.

      However - if I now trigger an exception that is not a ShiroException the problem is reversed and my advice is not called.
      It seems that now there is only one advice for the service - whichever is first. Perhaps its assigned same id or replaced - I don't know...

      Then I tried specifying an @Advice annotation
      @Advise(serviceInterface=RequestExceptionHandler.class, id="MyRequestExceptionAdvice")
      public static void adviseRequestExceptionHandler(...)

      Then it seems to be working properly in cases when there is an ShiroException AND other exceptions.

      BTW - how is it possible to specify fine grained ordering? Seems like its either before:* or after:* - what will this guarantee me if other advice's are doing the same..?
      Is this what the @Advice is for also - so that I could specify @Order("before:MyRequestExceptionAdvice") (if I had defined a second advice).?
      If this is the case then perhaps the SecurityModule should provide id so my check for exception instanceof would not be needed - and I could just specify @Order("after:ShiroRequestExceptionAdvice")

        Activity

        Kalle Korhonen made changes -
        Field Original Value New Value
        Assignee Kalle Korhonen [ kaosko ]
        Kalle Korhonen made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s security-0.4.1 [ 17404 ]
        Resolution Fixed [ 1 ]
        Alejandro Scandroli made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Kalle Korhonen
            Reporter:
            Magnus Kvalheim
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: