Directory ApacheDS
  1. Directory ApacheDS
  2. DIRSERVER-1829

bug in initializing authenticators for AuthenticatorInterceptor

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-M11
    • Fix Version/s: 2.0.0-M13
    • Component/s: None
    • Labels:
      None

      Description

      Le 4/16/13 8:05 PM, Wu, James C. a écrit :
      > Hi,
      >
      > I am looking at this two method definition in the AuthenticationInterceptor.java class. This two method should both set to the authenticators for the interceptor to the input given in the parameter given both are a setter. However, the logic of method body of these two methods are quite different. The first method just set the member variable (authenticators) to the input parameters, while the second method will register the authenticator and strangely enough, it does not set the member variable (authenticators) to the input parameter.
      >
      > /**
      > * @param authenticators authenticators to be used by this AuthenticationInterceptor
      > */
      > public void setAuthenticators( Set<Authenticator> authenticators )
      > {
      > if ( authenticators == null )
      >

      { > this.authenticators.clear(); > }

      > else
      >

      { > this.authenticators = authenticators; > }

      > }
      >
      >
      > /**
      > * @param authenticators authenticators to be used by this AuthenticationInterceptor
      > */
      > public void setAuthenticators( Authenticator[] authenticators )
      > {
      > if ( authenticators == null )
      >

      { > throw new IllegalArgumentException( "The given authenticators set is null" ); > }

      >
      > this.authenticators.clear();
      > this.authenticatorsMapByType.clear();
      >
      > for ( Authenticator authenticator : authenticators )
      > {
      > try
      >

      { > register( authenticator, directoryService ); > }

      > catch ( LdapException le )
      > {
      > LOG.error( "Cannot register authenticator {}", authenticator );
      > }
      > }
      > }
      >
      >
      > I traced the execution of the start of the apacheds service. During the startup of the apacheds service, three authenticators will be created, and the setAuthenticators( Authenticator[] authenticators ) method will be called. But due to the fact that, the member variable authenticators is not set, when the execution gets to init method of the authenticatorinterceptor, three new default authenticator will be created because the authenticators is null and default authenticators are created. Now we have six authenticators. Three of them (Simple, Strong, Anonymous) created by the setDefaultAuthenticators are in the authenticators list, the other three (Simple, Strong, Anonymous) created by createInterceptors in the ServiceBuilder class are in the authenticatorsMapByType map.
      >
      > I am thinking this is a bug. First of all, these two methods should behave in the same way. Second, we should not have duplicated authenticators, plus the three authenticators created by the createInterceptors method in the ServiceBuilder class is not properly initialized. Their directoryService field is null.

      This is clearly a wrong initialization. My understanding is that we have stacked layers on top of layers during years, but never cleaned up the class.

      I also modified this code 8 weeks ago, I don't remember why I change it this way (cf http://svn.apache.org/viewvc/directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/AuthenticationInterceptor.java?r1=1424688&r2=1446503&diff_format=h),
      but this is clearly wrong...

        Activity

        Hide
        James C. Wu added a comment -

        Here is my local modifications for your reference. It seem to be working for me. I can start both ldap and the kdc service.

        /**

        • @param authenticators authenticators to be used by this AuthenticationInterceptor
          */
          public void setAuthenticators( Set<Authenticator> authenticators )
          {
          if ( authenticators == null ) { this.authenticators.clear(); this.authenticatorsMapByType.clear(); return; }

        this.authenticators = authenticators;
        }

        /**

        • @param authenticators authenticators to be used by this AuthenticationInterceptor
          */
          public void setAuthenticators( Authenticator[] authenticators )
          {
          if ( authenticators == null ) { throw new IllegalArgumentException( "The given authenticators set is null" ); }

        Set<Authenticator> authenticatorsSet = new HashSet<Authenticator>();
        for (Authenticator authenticator : authenticators)

        { if (authenticator != null) authenticatorsSet.add(authenticator); }

        setAuthenticators(authenticatorsSet);
        }

        Show
        James C. Wu added a comment - Here is my local modifications for your reference. It seem to be working for me. I can start both ldap and the kdc service. /** @param authenticators authenticators to be used by this AuthenticationInterceptor */ public void setAuthenticators( Set<Authenticator> authenticators ) { if ( authenticators == null ) { this.authenticators.clear(); this.authenticatorsMapByType.clear(); return; } this.authenticators = authenticators; } /** @param authenticators authenticators to be used by this AuthenticationInterceptor */ public void setAuthenticators( Authenticator[] authenticators ) { if ( authenticators == null ) { throw new IllegalArgumentException( "The given authenticators set is null" ); } Set<Authenticator> authenticatorsSet = new HashSet<Authenticator>(); for (Authenticator authenticator : authenticators) { if (authenticator != null) authenticatorsSet.add(authenticator); } setAuthenticators(authenticatorsSet); }
        Hide
        Emmanuel Lecharny added a comment -

        A patch has been applied that fixes the issue (see DIRSERVER-1730)

        Show
        Emmanuel Lecharny added a comment - A patch has been applied that fixes the issue (see DIRSERVER-1730 )
        Hide
        Emmanuel Lecharny added a comment -

        Closed all the resolved issues

        Show
        Emmanuel Lecharny added a comment - Closed all the resolved issues
        Hide
        James C. Wu added a comment -

        I do not think this bug is fixed. After upgrade to M12, I still saw exactly the same issues that I have describe above for M11.

        Show
        James C. Wu added a comment - I do not think this bug is fixed. After upgrade to M12, I still saw exactly the same issues that I have describe above for M11.
        Hide
        James C. Wu added a comment -

        this bug is still there.

        Show
        James C. Wu added a comment - this bug is still there.
        Hide
        Emmanuel Lecharny added a comment -

        I haven't said this bug is solved in M12 It's supposed to be solved in trunk...(this will be solved in RC1, as mentionned the 'Fix version' flag).

        Could you build the server from trunk , and check that it works ?

        Show
        Emmanuel Lecharny added a comment - I haven't said this bug is solved in M12 It's supposed to be solved in trunk...(this will be solved in RC1, as mentionned the 'Fix version' flag). Could you build the server from trunk , and check that it works ?
        Hide
        James C. Wu added a comment -

        yes, I rebuild from trunk and verified that it actually works now. So this bug is fixed in trunk.

        Show
        James C. Wu added a comment - yes, I rebuild from trunk and verified that it actually works now. So this bug is fixed in trunk.

          People

          • Assignee:
            Unassigned
            Reporter:
            James C. Wu
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development