Re: [Pki-devel] [PATCH] 0027..0029 support external authorization LDAP server

Sorry, revising my comments based on the latest patches. On 3/12/2015 10:50 PM, Endi Sukma Dewata wrote: >>>1. The TOKEN_GROUPS probably should be a List<String> to simplify the >>>creation and the usage of the list of groups. >>> >>We are constrained by the IAuthToken interface, which has only the >>String[] method. > >In AuthToken the attributes are actually stored in a Hashtable<String, >Object>, and there's already a generic get() that returns Object, so we >probably can just add a set(String name, Object value). If the list of >groups is only kept in memory I don't see a reason to concatenate them >into a single string. Scratch the last sentence, but I think the List<String> is still better because you won't need to convert into array of Strings.

>3. The listGroups() is always called with "*" filter so it will pull all >groups in the database, then the code will execute a search operation >for each group to check if the user is a member. I think this is highly >inefficient. The listGroups() should have been called with the right >filter to get the right list of groups with a single search operation. Never mind. The new patches already addressed this issue. The comments below are still valid. >4. When constructing a DN the attribute values should be escaped with >LDAPUtil.escapeRDNValue() to prevent problems with DN special characters. > >5. When constructing LDAP filter the attribute values should be escaped >with LDAPUtil.escapeFilter() to prevent problems with LDAP filter >special characters. >

>6. If there's an exception the method should rethrow the exception so >the client will see the problem. >

>7. The array of groups can be added into a list with a single operation: > > groups.addAll(Arrays.asList(groupsArray)); >

>8. The Utils.stripQuotes() right now is called multiple times for the >same value. It can be simplified as follows: > > matched = groups.contains(Utils.stripQuotes(value)); >

One more thing: 9. I see the "gid" attribute being set in TokenAuthentication. Probably it should be changed to IAuthToken.GID so we know where it's set and used. Alternatively we can replace it with IAuthToken.GROUPS that contains just a single group, and remove IAuthToken.GID.

-- Endi S. Dewata