Uploaded image for project: 'LDAP Connector'
  1. LDAP Connector
  2. MGNLLDAP-71

Inconsistencies in loading ldap.properties (code vs comments vs doc)

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Neutral Neutral
    • 1.6.2
    • 1.5.3
    • None

      Docu for the ldap connector tells me to configure a "jndi.ldap.config" property in my magnolia.properties. I get the vague feeling this is vaguely new, as I thought it used to be configured in jaas.properties. But OK, fair enough, I'll change my config.
      However, I have to point out inconsistencies in the code with regards to that:

      1. info.magnolia.jaas.sp.ldap.LDAPUtils#getJNDIConfig throws an exception if the jndi.ldap.config property (magnolia.properties) isn't defined.
      2. info.magnolia.jaas.sp.ldap.LDAPAuthenticationModule#getJNDIConfig first tries the jaas.config property, THEN falls back to the method above.

      Both methods have a comment that says // for backword compatability, this use to be in magnolia.properties as a relative path. Typos apart, this tells me the opposite of the documentation, and that the current way of configuring would be to use jaas.config ?

      IF all calls would go to (2), then things would be consistent, and I'd believe that it's just the code comment that's poorly written.
      However, some calls go to (2), some to (1) – which essentially means that
      a) docu is correct, and code-comment is wrong
      b) most importantly, I guess, there is no backwards compatibility, and the code is just misleading.

      The issue is not so much where the calls go rather than the fact that the methods are just behaving inconsistently.

      So I would suggest the following:

      1. have one single method to do this. Keep the one in LDAPAuthenticationModule but have it strictly delegate to the LDAPUtils method.
      2. arguably we already broke backwards compatibility, but I'd rather restore it for people who have not upgraded yet ? (and fix the comment)
      3. look at calls to LDAPUtils#getJNDIConfig and fix exception handling. I've seen a few e.printStackTrace() – at the very least, log the exception in error, or better yet, let it go up stack.

      If we don't restore backwards compatibility, then http://documentation.magnolia-cms.com/releases/4-5-1.html#IfyouareusingtheLDAPConnectormodule should not say ".. is now configurable in .." but "must now be configured in ...".

      While we're at it: docu says jndi.ldap.config should be relative to the web-app. In fact, it looks it can also be an absolute path, or use \${magnolia.home} like other path properties. This should be verified and documented.

        Acceptance criteria

              mdivilek Milan Divilek
              gjoseph Magnolia International
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated:
                Resolved:

                  Bug DoR
                  Task DoD