DRAFT DOCUMENT to discuss ideas
One of the goals for Milestone 2 is to achieve a rich AuthenticationException heirarchy.
The root of the heirarchy will be a generic AuthenticationException. AuthenticationExceptions will be checked exceptions (i.e. extending Exception). They will define one extra method:
getCode() will include a code to identify the exception. These are designed to be immutable so that any client receiving an exception will be able to recognize it. Our default controllers will be able to map these codes to an external messages.properties file in order to facilitate internationalization.
Tree:
Can we use the fully qualified class name of the exception to identify the exception (as the return value for getCode())?
Actually we may not need getCode() on the AuthenticationException heirarchy as these are never returned via the CAS payload (though I guess it can't hurt to have them for future use).
When we define our TicketException heirarchy we will definately need a getCode() as that will correspond to what is returned in <cas:authenticationFailure code= "?????????">
Don't we need to be able to identify AuthenticationExceptions in morbid detail (i.e., with codes) so that failures in the AuthHandler tier can be reflected with appropriate end user information in the Login page tier? As in, if certificate authentication fails because of an expired cert, it would be nice for the resulting AuthenticationException to inform web tier logic that renders the message that the cert failed because it was expired?
That would be the purpose of the exception heirarchy. There could be a CertificateExpiredAuthenticationException
Okay. I think you're saying that the fully qualified class name is functioning as the return value for getCode(). That is, if we drop getCode() from the AuthenticationException interface, we'll be okay because we are using a hierarchy in which we can identify types of authentication problems by their fully qualified class names.
So now I'm wondering whether we still get something by exposing this fully qualified class name as the return value from getCode() so that in JSPs we can write
<c:if test="exception.code eq 'org.jasig.cas.auth.support.cert.CertificateExpiredAuthenticationException'">
Authentication failed because your cert was exipired.
</c:if>
This worth anything?
We wouldn't need to do that.
We just throw the exception up. If the client of the CentralAuthenticationService (such as the LoginController) wants, it can catch a generic AuthenticationException and add errors.reject("", e.getDescription()) to the errors model or if it knows how to handle a specific AuthenticationException it can try and catch those first.
Andrew,
I'm wondering if we should have a seperate exception to handle stuff like "it would have been valid but...."
For example, AccountLocked or CertificateExpired implies that your credentials would have been correct except for this stipulation. Whereas, BadCredentials could imply totally invalid (i.e. not even found anywhere).
Or is it not worth making that fine-grained of a distinction?
Scott,
Not sure. We may not have to invent all of this. The deployer who actually needs locked accounts can invent the particular exception that carries in its payload the netid of the administrator locking the account or whatever. As I see it, we just need to make sure that it's possible for an authentication handler to differentiate between reasons for failure in a way that percolates up to the web tier such that the view can change based on why authentication is failing.
After some discussion, we propose some specific exceptions with defined funtional meaning:
AuthHandlerFailure - Thrown by the AuthHandler when it is unable to perform its function due to internal or external problems (database or network down, program failure). This exeception is also generated and logged by the AuthenticationManager when any exception not in this Hierarchy (i.e. NullPointerException) is thrown by an AuthHandler. Generally the catcher (if any) should assume there is an original Exception chained to this.
AccessProhibited - Thrown by an AuthHandler who has reason to believe that this user should not be allowed to access any resources no matter what the status of his account in other systems. The motivation is that one database knows this is an employee terminated or student expelled last week who may have a grudge and should be blocked from all IT access. Not all systems may have been updated, so this exception prevents logon even if the user was authenticated by a previous AuthHandler.
I've merged these two exceptions into the hierarchy in the page content itself, above.
Is it appropriate for one AuthenticationHandler to make statements about what another AuthenticationHandler? Should an AuthenticationHandler be able to make statements more broad then "yes/no I believe these credentials to be valid." By throwing an AccessProhibited exception we are giving one authentication handler an exceptional amount of power.
AuthHandlers have much more power when they succeed, since they can allow a remote user to logon as the most powerful user in the organization. The ability to "blackball" a user based on administrative policy is also powerful, but less easily subject to abuse or long term consequences.
In this case, the AuthHandler is saying, "yes I recognize these credentials and I know that the person represented by these credentials has had all his access privileges to the network revoked". This is not something that should be used casually, but I think it is a servide that needs to be available. I don't think we will use it in any of the distribution modules, but administrators can add it to their plug-ins if they need the function.
I envision the AuthenticationManager doing two things. First, it takes the credentials and asks the AuthHandlers about them, collecting up the Authentication objects that the handlers return and handling any exceptions they threw. Second, it takes that collection of Authentications and feeds them into a post-processor which considers them all and comes back with the since merged-together Authentication object expressing all the ways in which the user was authenticated and all the Principal information that it could glean from the handlers. And maybe it even drops some Principal information (yeah, the LDAP reported that attribute for this user, but we're not going to include it, say.) So this post-processor is a plugin into an AuthenticationManager and by default it computes a union of the Authentications reported by the individual handlers.
Maybe this post-processor would be the best place to throw a BlackballException or otherwise veto the user's authentication. This would express that we're vetoing not on the basis of credentials but on the basis of who it was that was authenticated.
Credentials are never cause to be blackballed. "Subjects" or "AuthenticationEntries" or Principals – things in the Authentication - are.
My belief is that the one making the ultimate decision on the authenticity of the principal is the only one who should be able to override the decision of any handler. A AuthenticationManager that polls many authentication handlers may decide on its own that the failure of one authentication handler is warrant enough not to continue authenticating and reject. However, an authentication handler in my opinion has no right to tell the manager that hey you know what I think these credentials are bogus so don't even bother checking anyone else.
AuthHandlers don't have their own point of view. All code is under the control of the administrators or programmers of an institution. So by providing an Exception for people to throw, we are telling Princeton or UCLA that they can protect themselves from dangerous people. This isn't one AuthHandler against another. This is 1 to 12 AuthHandlers all written by or at least adopted by the same administrator cooperating on making a decision. This is like saying that you don't want to give my index finger the ability to interfere with what my middle finger is doing.
First, nothing is stopping Princeton or UCLA from adding to the exception heirarchy to fit their needs.
Regardless of who wrote the authentication handler (whether it was the same person or different people) the ultimate responsibility of returning the Authentication object rests with the AuthenticationManager. It asks the authenticationhandlers their opinion on whether the credentials are valid or not. It does not ask it to speak for the group. If a manager chooses to automatically fail when one handler says these credentials are invalid, that's a manager implementation. However, I don't believe we should define an Exception to include that the handlers can throw that basically gives delegates to them veto power. Or even suggests that they can overrule the other authentication handlers. AuthenticationHandlers should never assume they are working with other authenticationhandlers. All they can tell you is yes/valid, no/invalid or some information about the account (i.e. locked).
Also, its not like saying you don't want your index finger to interfere with the middle finger. Your middle and index fingers can only interfere if your hand (which controls your fingers) let's you. Similarly, the authentication handlers can only affect each other if a manager chooses to allow them too. They should never assume their outcome can affect another handler.
I would recommend that we rename AuthHandlerFailure to something more related to authenticating, such as UnableToProcessCredentials or something similar.
I would also think we should rename BasicCredentialsException to something like UncategorizedAuthenticationException similar to
http://www.springframework.org/docs/api/org/springframework/dao/UncategorizedDataAccessException.html
Though this is an abstract class. I'm not sure if we need to to that.