Hi Marek,

Thank you for your comments and i updated code following your comments. I have some comment inline.
Could you review it again


On Tue, May 20, 2014 at 1:14 AM, Marek Posolda <mposolda@redhat.com> wrote:
Hi,

Great that you have it working on Tomcat. I have some minor comments:
- Names of your classes like SPNEGOFilter, SPNEGOLoginModule etc are
same like already existing classes inside gatein-sso. How about renaming
them to something like TomcatSPNEGOFilter, TomcatSPNEGOLoginModule etc?

I renamed these class to SPNEGOSSOFilter and SPNEGOSSOLoginModule

 

- there are 3 configuration files where is SPNEGOFilter defined and it
seems that it's needed just inside extension. So it seems that this
whole directory
https://github.com/nttuyen/gatein-spnego/tree/master/spnego-jar/src/main/resources
could be removed?

I removed all unused configuration file. Now, spnego implementation is only a jar project. 

 

- Very minor - I noticed invalid log class here:
https://github.com/nttuyen/gatein-spnego/blob/master/spnego-jar/src/main/java/org/gatein/security/sso/spnego/SPNEGOFilter.java#L50

Yes, i replace logger at SPNEGOSSOFilter to org.apache.commons.logging.Log, but SPNEGOSSOLoginModule still use org.exoplatform.services.log.Log because AbstractLoginModule require it.

 


Place where to put it: I am not sure. In current form, it can't be put
inside gatein-sso because it has dependencies on some gatein-portal
artifacts. It should be possible to remove it if you want to do some
more changes in your code and perhaps change instructions (For example
use SSOInterceptor like other stuff inside gatein-sso instead of
org.exoplatform.web.filter.Filter). So in current state, I would suggest
to put it inside some separate directory inside gatein-toolbox or inside
some separate directory in gatein-portal.

When i try make SPNEGOSSOFilter extends from SSOInterceptor instead of org.exoplatform.web.filter.Filter, i can not config this filter to listen on many url-pattern (now, that filter handler 2 url: /login and /spengosso). 
Do you have any idea?

Btw, I've tested and this implementation work well on both tomcat and jboss packaging. So, What do you think if we replace spnego implementation with this?

Thanks.

TuyenNT.


 

Marek


On 13.5.2014 12:17, Bolesław Dawidowicz wrote:
> Marek is away this week and I would prefer that he takes a look at it.
>
> On 05/13/2014 09:32 AM, Tuyen The Nguyen wrote:
>> Hi all,
>>
>> We are trying to make SPNEGO SSO work on gatein tomcat packaging.
>>
>> Unfortunately, current spnego sso implementation of gatein only works on
>> jboss because it use Jboss Negotiation library - a specific library for
>> jboss.
>>
>> So, i provide other spnego implementation for gatein tomcat packaging at
>> https://github.com/nttuyen/gatein-spnego, this implementation works as
>> an extension and i tested it on master and 3.7.x branch.
>>
>> If there some one can review and have any feedback, i would appreciate.
>> Btw, is there any idea about where this implementation should be in
>> gatein codebase?
>>
>> Thanks!
>>
>> TuyenNT.
>>
>>
>> _______________________________________________
>> gatein-dev mailing list
>> gatein-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/gatein-dev
>>
>
>
>

_______________________________________________
gatein-dev mailing list
gatein-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/gatein-dev