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

[Endi Sukma Dewata]

On 3/12/2015 3:18 PM, Fraser Tweedale wrote:
> New patch attached (squashed some bugs and cleaned up the
> implementation considerably ; squashed back down into one commit).
>
> Other comments inline below.

>> 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.

>> 2. I'm not quite clear the purpose of this enhancement. If it's meant to be
>> a general-purpose directory-based authentication plugin, it would make sense
>> to have a fully configurable parameters for retrieving the group
>> information. However, if this is only to be used for Dogtag authentication,
>> there's already a user-group subsystem that can provide the information. See
>> PKIRealm.getRoles().
>>
> It is apparently needed for retrieving groups from external
> directories.  https://fedorahosted.org/pki/ticket/1174

OK.

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.

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));

-- 
Endi S. Dewata