Bug 28286 - Loosely couple SingleSignOn Valve and Authenticators
Summary: Loosely couple SingleSignOn Valve and Authenticators
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 5.0.22
Hardware: All other
: P3 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2004-04-08 17:36 UTC by Brian Stansberry
Modified: 2004-11-16 19:05 UTC (History)
0 users

Patch with a diff and new interface SSOValve (6.65 KB, application/zip)
2004-04-08 17:37 UTC, Brian Stansberry
Patch to allow subclassing (8.95 KB, patch)
2004-04-26 05:49 UTC, Brian Stansberry
Details | Diff
Patch to allow subclassing of SingleSignOnEntry (1.06 KB, patch)
2004-05-15 18:42 UTC, Brian Stansberry
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 

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.
Comment 10 Brian Stansberry 2004-05-15 18:42:49 UTC
Created attachment 11566 [details]
Patch to allow subclassing of SingleSignOnEntry