Community
Participate
Working Groups
Right now, there is a stand-alone interface called ILoginContext. This is overkill, and should be something like an EclipseLoginContext that simply extends javax.security.auth.login.LoginContext and adds Eclipse-ey things like event management, etc.
Created attachment 89192 [details] Proposed patch This patch makes an EquinoxLoginContext that adds the following functionality above and beyond what Java provides: 1. Events 2. Declarative registration of CallbackHandlers via extendion points 3. Automatic login when getSubject() called Issues: 1. getSubject signature does not have LoginException. It's a bit strange. One thing we could do is remove LoginException from both SecurePlatform.createContext and LoginContext.getSubject and return an empty subject. This is a common pattern for an 'anonymous' user 2. Should EquinoxLoginContext be internal? Move listener registration to another API? Then the only API is createContext (which deals with configURL and platform start/stop) and an API for registering listeners. Everything else is standard JAAS. Oleg, thoughts?
Hmm I don't really see the point here - the API size remains the same (minus one interface, plus one class), the functionality is mostly copied from internal class into a new API class. If we go this way it might be better to eliminate SecurePlatform class and do SecurePlatformInternal#addConfigURL, SecurePlatformInternal#start() in the constructors of EquinoxLoginContext? Details: - Javadoc is needed for EquinoxLoginContext and its methods - EquinoxLoginContext - is it a final class? - there is quite a bit of functionality exposed in this class for an API - SecurePlatform factory methods returns LoginContext which then needs to be explicitly downcast by users to EquinoxLoginContext - EquinoxLoginContext#getSubject() - as you said, some meaningful treatment of exception needs to be added :-) (To account for potential API modifications in future I usually try to use a "pattern" where API factory produces an object imlpmenting API interface. As the interface is described as non-implementable by users, it can be expanded in future. This factor+interface approach allows clean separation between API and implementation - whatever is not in the Javadoc or method signatures can be modifed at will later.)
Created attachment 89208 [details] Proposed patch Remove EquinoxLoginContext as API... This is how we could make the API footprint lighter.
What happens if callback handler is null? The "old" way(SecureContext): CallbackHandler callbackHandler = SecurePlatformInternal.getInstance().loadCallbackHandler(configName); if (callbackHandler == null) loginContext = new LoginContext(configName); else loginContext = new LoginContext(configName, callbackHandler); The "new" way(EquinoxLoginContext) : public EquinoxLoginContext(String name) throws LoginException { super(name, getCallbackHandler(name)); } That would trigger an exception if getCallbackHandler() is null.
Created attachment 89264 [details] Proposed patch How's this... we only have two constructors, and SecurePlatform becomes the factory object responsible for connecting the logincontext and the callbackhandler.
I guess at some point it becomes a question of preferences. While this will work, I am still not sold on the new approach being better. Three points why I think so: 1) Consider if we decide to add isLoggedIn() method to equinox LoginContext. With the proposed approach the only place to stick it on would be SecurePlatform - something like: SecurePlatform#isLoggedIn(LoginContext) similar to the register/unregister listeners you are proposing. So SecurePlatform will be growing instead of our LoginContext subclass - which is not logical. 2) Another point is that we can't change methods provided by LoginContext - the equinox getSubject() implementation in the patch really needs to be able to throw an exception, but we can't do it as LoginContext.getSubject() does not. 3) In general, interfaces are better then classes for expandibility. Half the problems with those patches stems from the fact that LoginContext is a class and not an interface. As a class, it has a set of constructors that don't fit our needs - so we can't create "EquinoxLoginContext extends LoginContext" as there is no "clean" way to construct EquinoxLoginContext. To me, this is a "just-in-time" example of design to avoid. (It is possible to create a "EquinoxLoginContext extends LoginContext" with a dummy properties for superclass and all method calls going to an internal EquinoxLoginContext. However, due to the way LoginContext works, the dummy configuration provider and login modules would have to be added for this to work. While this is possible, such solution would be rather fragile.) =========== Some details about the patch: EquinoxLoginContext login(), logout() - throw new LoginException(SecAuthMessages.loginFailure); + throw loginException; getSubject(): consider return super.getSubject() if login fails, not new Subject() (see Javadoc for LoginContext - returns null or user-specified) our doc for it should be: getSubject() attempts to perform login if context is not in a logged-in status SecurePlatform createContext() internals could be moved into SecurePlatformInternal along with getCallbackHandler() method code style - we usually don't put {} brackets around one-line statements unless they help with something Javadoc what are the differences between LoginContext returned by SecurePlatform and "standard" one? When user should call one or another?
Created attachment 89511 [details] Alternative approach (draft patch) This patch illustrates approach with having API EquinoxLoginContext extending LoginContext. It allows elimination of both SecurePlatform and ISecureContext. However, if we take this approach we potentially loose some flexibility.
Ouch, the LoginContext(String name, Subject subject, CallbackHandler callbackHandler, Configuration config) is only available on Java 1.5. The dummy configuration probably could be build in the ConfigurationFederator.
Moving to the security component
As suggested by Oleg, adding a toLoginContext() or a getLoginContext() method to ISecureContext would be sufficient to expose the LoginContext.
Added ISecureContext#getLoginContext().
Renamed ISecureContext to ILoginContext and SecurePlatform to LoginContextFactory. I think this is sufficient compromise between extending JAAS or implementing our own thing - consistency in naming should make the interface intent clear.
