On 21.5.2014 09:55, Tuyen The Nguyen wrote:
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(a)redhat.com
<mailto: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/...
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/...
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?
yes, ATM it just doesn't support many url-pattern :) Feel
free to open
JIRA in GTNSSO. But anyway, if you really want 2 separate URLs to be
mapped to same filter instead of use 2 filters, it should be easy to
workaround with map to "/*" and use some code like:
if (!requestURI.equals("/login") &&
!requestURI.equals("/spnegosso")) {
chain.doFilter(request, response);
return;
}
Hack, but should work:)
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?
I am not sure about this TBH. Fact is that
jboss-negotiation was written
before SPNEGO support has been added into Sun JDK (It's available from
JDK6 and later AFAIK), so theoretically now it should be fine to rely on
functionality provided by JDK. But our customers may use different JDK
(for example IBM JDK) on various platforms and it's possible that SPNEGO
provided by Java won't work everywhere... So I would still prefer to use
jboss-negotiation and old implementation for JBoss.
Btv. 2 more things I did not noticed earlier (sorry for that):
1) You are using sun specific Base64Encoder and Base64Decoder. I am
quite sure that this won't work on all platforms (like IBM JDK). We have
org.gatein.common.util.Base64 in gatein-common. How about switching to
use this one instead?
2) Right now, you are doing SPNEGO server login in filter constructor so
it's cached if I understand correctly. I think it's not good to cache
the LoginContext of the server login. For example if credential of the
server principal and keytabs change or if Kerberos server is
unavailable, you will still have cached instance of server subject. IMO
server subject should be re-logged during each login process.
I am sorry for late response :/
Marek
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(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org>
>>
https://lists.jboss.org/mailman/listinfo/gatein-dev
>>
>
>
>
_______________________________________________
gatein-dev mailing list
gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org>
https://lists.jboss.org/mailman/listinfo/gatein-dev