|Summary:||Loosely couple SingleSignOn Valve and Authenticators|
|Product:||Tomcat 5||Reporter:||Brian Stansberry <bstansberry>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
Patch with a diff and new interface SSOValve
Patch to allow subclassing
Patch to allow subclassing of SingleSignOnEntry
Description Brian Stansberry 2004-04-08 17:36:32 UTC
Attached is a patch that loosens the coupling between the SingleSignOn valve implementation and the various Authenticator classes. Right now, all authenticators check for an instance of the SingleSignOn class in the pipeline, and interact directly with it. In the patch, SingleSignOn implements interface SSOValve, and all external classes interact with it through the interface. This will allow application developers to deploy custom SSO implementations without having to replace the entire TC5 Authenticator infrastructure. This follows the pattern used by other TC components. I've been messing around with trying to use the SingleSignOn valve in a cluster (see enhancement request 28039) and can easily see how different implementations would be appropriate for different environments. My 28039 patch attacks the problem by adding a property to SingleSignOn where deployers can specify use of an implementation of a new interface SSOClusterManager. I think that's a valid approach, but loosening the coupling between SingleSignOn and Authenticators will provide greater flexibility.
Comment 1 Brian Stansberry 2004-04-08 17:37:30 UTC
Created attachment 11188 [details] Patch with a diff and new interface SSOValve
Comment 2 Remy Maucherat 2004-04-10 15:11:55 UTC
Your two related patches seem useful. I'll look at them.
Comment 3 Brian Stansberry 2004-04-10 18:06:06 UTC
Great. I'm sorry to say the two patches conflict (not logically, but in the sense that they are diffs to the same code base), but if one is committed I'll gladly redo the other.
Comment 4 Remy Maucherat 2004-04-25 14:55:00 UTC
After reviewing, I am not going to apply this patch or the other. I don't see the point of adding complexity and additional interfaces when SSO is used through a standard Valve (SingleSignOn), which can easily be extended to implement the needed clustering support.
Comment 5 Brian Stansberry 2004-04-26 05:48:15 UTC
I thought you might feel that way about the 28039 patch, which is one reason why I wrote the interface proposal ;-) And, it's true an interface isn't absolutely needed; subclassing would work. The fact that SingleSignOnEntry is not public (and I'm not arguing it should be) is a big stumbling block in easily subclassing SingleSignOn, but getting around that problem in a subclass is no harder than writing a whole new implementation of an interface. But, to get subclassing to work effectively, a couple things need to be changed: 1) AuthenticatorBase.reauthenticateFromSSO() invokes SingleSignOn.lookup(). This method returns a SingleSignOnEntry, which is a package protected class. This effectively precludes subclassing this method. 2) SingleSignOn.update() is package protected, again preventing subclassing. I'm attaching another patch that addresses these two issues by: 1) Changing the way AuthenticatorBase.reauthenticateFromSSO() works so it does not need to call SingleSignOn.lookup(). lookup() is now only called internally in SingleSignOn, so someone who wished to write a subclass could just remove any calls to it, write their own lookup algorithm, and replace SingleSignOnEntry with their own class. 2) Makes SingleSignOn.update() protected. Maybe 3rd time's the charm? ;-)
Comment 6 Brian Stansberry 2004-04-26 05:49:01 UTC
Created attachment 11330 [details] Patch to allow subclassing
Comment 7 Remy Maucherat 2004-04-26 16:44:59 UTC
I'm all for making the entry public. I think I'm going to apply your patch.
Comment 8 Remy Maucherat 2004-04-26 21:58:01 UTC
This is committed. I don't see the point of not allowing to extend the entry as well, so I also patched it.
Comment 9 Brian Stansberry 2004-05-15 18:41:48 UTC
The constructor in SingleSignOnEntry is package protected, so the class cannot be extended. The attached patch makes the c'tor protected.