New patch attached (squashed some bugs and cleaned up the implementation considerably ; squashed back down into one commit). Other comments inline below. On Thu, Mar 12, 2015 at 09:40:20AM +0700, Endi Sukma Dewata wrote:
On 3/12/2015 8:12 AM, Fraser Tweedale wrote:
On Wed, Mar 11, 2015 at 02:04:56PM -0400, Ade Lee wrote:
Looks good in general.
I notice that your patch adds the use of the Vector class. Vector is old and synchronized - which can slow things down unnecessarily. Use ArrayList or similar instead.
Ade
Roger that; I will switch to ArrayList. For now you can all s/Vector/ArrayList/g in your heads while you review this patch :)
Quick comments:
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.
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 Cheers, Fraser
I may have more comments later.
-- Endi S. Dewata