Decouple authentication from CASImpl

[mmoayyed]

Closes https://github.com/Jasig/cas/issues/1315

This pull introduces the following new concepts:

1. AuthenticationContext: represents the final authentication event, collected from login flows and combined into one. The context will include a composite authentication object that represents a final view on all authentication attributes and principal attributes from all events, merged into a single entity.
2. AuthenticationContextBuilder: builds AuthenticationContext. Responsible to collect authentication objects from flows, and builds the final context after reviewing all authentications.
3. AuthenticationSupervisor: an abstraction that acts as a bridge between the authentication layer and upper layers (flows for example). All components interact with a supervisor.

The idea here is to lay the ground work for MFA functionality, based on the cas-MFA extension project. Presently, there exists only a single flow and one set of credentials but as we start to add subflows to MFA to handle secondary credentials and flows, this design is needed to keep collecting credentials and authentications from all flows, and at the very end, a single TGT is issued by CASImpl. Presently, CASImpl is tied to the authentication event, so every authN event potentially creates a new TGT which is not practical.

Example snippet:

this.authenticationSupervisor.authenticate(credential);
...
...//There could be many other authenticate() calls for various types of credentials...
...
final AuthenticationContext authenticationContext = this.authenticationSupervisor.build();
final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationContext);

A supervisor is constructed as such, acting as a bridge:

private AuthenticationManager manager;
private DefaultAuthenticationContextBuilder builder;
private DefaultAuthenticationSupervisor authenticationSupervisor;

@Before
public void setup() {
    this.builder = new DefaultAuthenticationContextBuilder();
    final DefaultPrincipalElectionStrategy principalStrategy = new DefaultPrincipalElectionStrategy();
    principalStrategy.setPrincipalFactory(new DefaultPrincipalFactory());
    builder.setPrincipalElectionStrategy(principalStrategy);

    final AcceptUsersAuthenticationHandler handler = new AcceptUsersAuthenticationHandler();
    handler.setUsers(Collections.singletonMap("casuser", "Mellon"));
    this.manager = new PolicyBasedAuthenticationManager(handler, new SimpleTestUsernamePasswordAuthenticationHandler());

    this.authenticationSupervisor = new DefaultAuthenticationSupervisor();
    this.authenticationSupervisor.setAuthenticationContextBuilder(this.builder);
    this.authenticationSupervisor.setAuthenticationManager(this.manager);
}

When the final AuthenticationContext is built, we need to examine all authentication objects (and their principals) from everything that is collected. Authentication attributes and principal attributes are all merged together, and the primary principal is selected out of the many authN events. By default, the first authentication is considered to be the primary authN, whose principal is the primary principal.

[leleuj]

BTW, I guess we said we could avoid such trivial Javadoc.

[mmoayyed]

You are perfectly right. Will remove. I had a plugin configured to auto-generate docs, and it scanned everything here!

[leleuj]

I haven't reviewed the pull request thoroughfully, but my main concern is multi-threading: I'm surprised to see a singleton component which collect all authentications for all users happening at the same time or not...

[mmoayyed]

You are right; that might be a legitimate concern. I'll do a bit more research and report back.

[mmoayyed]

PR is updated. All tests pass.

Reminder that I'd like to finalize this prior to the 4.2 RC1 release, due in about a week or so.

[dima767]

And so the unsolved issue still remains - where to persist https://github.com/Jasig/cas/blob/a013bb8714f413541499ec8939e391be2243ce41/cas-server-core-authentication/src/main/java/org/jasig/cas/authentication/DefaultAuthenticationContextBuilder.java in between requests for multi-legged authentication transactions

[mmoayyed]

No that isnt persisted anywhere by the API. It's passed locally around. See the AuthenticationViaFormAction as an example.

We may have persistence issue later on as we develop subflows. I am planning to handle that with #572

[mmoayyed]

Reviewing the CAS-MFA code, I see that extension using SWF to store the primary authN as it transitions to subflows. So I am inclined to think that SWF may be good candidate for multiple legs of the authN, but we still need to investigate this with the new API here since the way we handle SWF state now across flows has changed in 4.x.

[dima767]

Sure thing - the persistence is not needed to be present in this API, of course. My main point is that currently, each SWF action e.g.: https://github.com/Unicon/cas/blob/a013bb8714f413541499ec8939e391be2243ce41/cas-server-webapp-actions/src/main/java/org/jasig/cas/web/flow/AuthenticationViaFormAction.java#L174 and for example non-interactive action also blindly assume that they are THE ONLY participants in the authentication transaction and immediately build the context and ask CAS to issue TGT. This assumption/impl would need to be re-thought and refactored to introduce a higher layer *web/SWF for example) to mediate the multi-phased authn transactions, take care of the auth ctx builder persistence in between requests in the thread safe manner for each authn session, etc. Basically a mediator/glue layer between authentication and web sso subsystems.

[mmoayyed]

Absolutely. You are perfectly right.

[dima767]

And so perhaps one more thing to re-think in this current API - the auth ctx builder stores the Authentication objects with all its state. May be a "lighter-weight" version with just enough of simple, serializable state - so it could be easily stored in between requests without a great concern for serialization issues, etc. Just thinking out loud here...

[mmoayyed]

Reviewed. It turns out that the authentication object contains everything we need to get it passed around. It's as "lightweight" as it can be for now. Also, I think if we were to create, let's say a lighter version of the Authentication object, we'd need to find a way to copy the contents of the real authentication into the light-weight version, still making sure that those copied items are serializable, which means possibly lots of transformations. That sounds like a lot of effort to me, compared to the energy it takes to figure out what in the real authentication object may cause serialization issues, and in what ways those can be solved.

Makes sense?

Reviewing our implementations here, I am more and more convinced that everything we have right now is perfectly suited for SWF. All objects are meant to be serializable and perfectly are. What I was worried about was any "external" implementations by deployments, or weird implementations of a Principal by a deployment that violates this rule, but, now I that rethink about it, that is of no concern to us really. We don't ever expect folks to reimplement the Authentication object or the Principal, and if they do, they would need to follow the design guidelines in making sure their impls are safe and sound. So I think we should be good here.

@leleuj let me know how you feel about this patch.

[leleuj]

That remains complicated from my point of view. What are we doing here: take the current authentication context from SWF, take the credentials, validate them, update the current authentication context with the new authentication and save the authentication context back again in the SWF, right?

Let me try to propose some naive implementation:

final AuthenticationContext authContext = WebUtils.getAuthenticationContextFromSWF(context);
final Authentication authentication = auhtenticationManager.authenticate(credentials);
authenticationContext.add(authentication);
WebUtils.saveAuthenticationContextToSWF(context);
final ServiceTicket serviceTicketId = this.centralAuthenticationService.grantServiceTicket(ticketGrantingTicketId, service, authenticationContext);

The principalElectionStrategy would be attached to the AuthenticationContext created in the InitialFlowSetupAction...

[leleuj]

Of course, my counter proposal could not be the final source code, but it may help to find improvments. Even for me, it's hard to read the changes and I've spent quite some time on this PR...

[mmoayyed]

final AuthenticationContext authContext = WebUtils.getAuthenticationContextFromSWF(context);
WebUtils.saveAuthenticationContextToSWF(context);

The above two calls are completely unnecessary for this PR.

Note that is no SWF involvement here. NONE. It does not save anything, nothing is retrieved from it. Just like how things work today. SWF has absolutely no role whatsoever in managing the authentication in the current master, and neither in this current PR. It is completely and utter irrelevant.

It ONLY becomes relevant once we start adding multiple flows, and multiple ways of providing credentials. That functionality is of no concern to this PR. Your patch might be relevant for a subsequent PR that attempts to "store" the context across multiple flows/legs of the authentication. It does not apply to this PR. There are no other flows. There is no need to store the context anywhere right now.

[mmoayyed]

If I were to explain this in a more palatable way:

final AuthenticationContextBuilder builder = new DefaultAuthenticationContextBuilder(
                        this.authenticationObjectsRepository.getPrincipalElectionStrategy());
final AuthenticationTransaction transaction =
        this.authenticationObjectsRepository.getAuthenticationTransactionFactory().get(credential);
this.authenticationObjectsRepository.getAuthenticationTransactionManager().handle(transaction,  builder);
final AuthenticationContext authenticationContext = builder.build(service);

• Create a local authentication context at the start of the flow. Again, remember that there is only a SINGLE flow. One way to submit credentials. In your authentication context, only ONE thing happens. No more.
• Obtain the credentials, and initiate an authentication transaction. The transaction will pre-vet the credentials.
• Ask the Transaction Manager to process that transaction.

(You may optionally repeat same 2-3, once we add subflows, and other ways to obtain credentials. Again, remember that presently there is only a SINGLE flow. One way to submit credentials. In your authentication context, only ONE thing happens)

• When you are satisfied, you finalize/build the context and instruct CASImpl to deal with it.

Now, I think the question here is: do we have a proliferation of objects/components? Are we trying to create way too many abstractions? Possibly. Here is where the idea for these abstractions comes from:

• An AuthenticationContext is the playing ground. It's local to every request, from start to finish. It's as if you go to the gym, and you reserve the gym for a couple of hours from when you enter until when you exit. If that makes sense, then we need something to build this context for us. Thus AuthenticationContextBuilder.
• Once you are in the gym, you pass the front desk and present your credentials by swiping a card into a reader. The clerk will take your credentials, process them and gives you a pass to the court. You enter the general hallway.

The reader is the AuthenticationTransactionManager.
The act of you submitting credentials and swipping it is an AuthenticationTransaction
The clerk is the AuthenticationManager.
Your pass to the gym is an Authentication object.

• Once you are in the general hallway, you attempt to enter the basketball court. Another clerk stands at the door. You present your team membership card by swiping a card into a reader. The clerk will take your credentials, process them and gives you a pass to the gym. You enter the court.

The reader is the AuthenticationTransactionManager.
The act of you submitting credentials and swipping it is an AuthenticationTransaction
The clerk is the AuthenticationManager.
Your pass to the gym is an Authentication object.

Note that your credentials are different this time.

• You are then ready to exit the gym. The gym (AuthenticationContext) knows this about you:
• You presented credentials to the first desk. Were successful. We know who you are, your address, your name, your email, etc.
• You presented credentials to the clerk at the court door. Were successful. We know you play basketball.

So the gym will provide a comprehensive view of ALL of your activity and ALL of who you are. That's where the finalized AuthenticationContext comes in.
And you do this in the present patch via AuthenticationContextBuilder#build()

Note that again, in the present patch you do not enter the basketball court. There is absolutely no way for you to that via CAS. The act of submitting credentials takes place ONCE, and it's the only thing you can do. No SWF involvement; no need to record what you did, for the time being.

[mmoayyed]

Also, please note that I am in no way advocating for this patch to go in :) My main objective is ensure none of this is too complicated, and that we all understand the same story. That's the most important bit. If the story makes sense, then we can write it in a way that makes sense to all.

[dima767]

And so perhaps instead of a separate factory for transaction, we could use a simpler, static "factory method" e.g. AuthenticationTransaction tx = AuthenticationTransaction.for(credential)

[leleuj]

Sorry to be so "painful". And you're right: SWF is not related here.

Trying to pushing my ideas once again:

1. I think credentials cleaning should go into the AuthenticationManager: it's a few lines of source code and I don't see where they could be used otherwise. So I would remove all the stuffs around: AuthenticationTransactionManager and AuthenticationTransactionFactory

2. We need to have some authentication context, add authentications to it and then generate a final authentication (using the PrincipalElectionStrategy).

3. Do the AuthenticationContext really need to carry the Service?

Let's try to rewrite this part. I assume the PrincipalElectionStragegy is injected in the class as well as AuthenticationManager (maybe this is where we need your repository to gather both):

Authentication authentication = authenticationManager.authenticate(credential);
AuthenticationContext context = new DefaultAuthenticationContext();
context.collect(authentication);
Authentication result = context.buildFinalAuthentication(principalElectionStrategy);

Does it make a little more sense?

[mmoayyed]

Yes, thank you both. Sounds like we are getting closer to the finish line.

Based on your review, I am inclined to do away with factories to reduce the object noise. Factory methods might work better as Dima suggests. I am inclined to keep the core abstractions for now but I'll know more once I see how the removal of factories and turning them into methods might help with the rest. So I'll carry forward with a new update and we then continue once more.

@leleuj on your #3, yes we do. Note that the Service can be optional for the context because you CAN generate a TGT without going anywhere but this is absolutely required for a number of additional use cases which may not be too obvious, so I shall explain:

1. A service in the context helps to enforce ABAC at the time of generating a TGT. We have ABAC enforced at ST validation time which is when the AuthZ rules kick in. This should really also be done at TGT time, and we could only do this if we knew what the service (optionally) was. So we need it there.
2. On the roadmap, there is a request to implement authentication-per-service features. We'd want to accommodate a use case where one could say: This application should ONLY use this handler, and that application in the registry should ONLY use this other handler. I don't know the specific implementations of how that would turn out, but I do know for sure that for this feature, we need the service in the context.

So yes, it's needed.

[mmoayyed]

Here is another pass at the API based on the collective feedback:

final AuthenticationContextBuilder builder = new DefaultAuthenticationContextBuilder(
      this.authenticationSystemSupport.getPrincipalElectionStrategy());
final AuthenticationTransaction transaction = AuthenticationTransaction.wrap(new ClientCredential(credentials));
this.authenticationSystemSupport.getAuthenticationTransactionManager().handle(transaction,  builder);
final AuthenticationContext authenticationContext = builder.build(service);
final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationContext);

• I removed the AuthenticationTransactionFactory and added a factory method to wrap credentials. I would have opted for for, except that I can't seeing how it's a keyword.
• The sanitation of credentials is now moved to the AuthenticationTransaction, since the factory is gone.
• As discussed, since we need the Service in the context, I opted to keep the AuthenticationContext around and the builder that goes along with that as the carrier.
• I also do agree with Dima, that I wouldnt want upper layers to have access to the AuthenticationManager. The TransactionManager is the right intermediate glue here.

Note that this API does not have to be "perfect". It has to be functional. It has to support MFA. It can always be revved later as we don't really consider any of this an "extension point".

[dima767]

Based on several productive iterations over this, looks good to me. +1

On Sat, Dec 19, 2015 at 04:45 Misagh Moayyed notifications@github.com
wrote:

In
cas-server-core-web/src/main/java/org/jasig/cas/web/flow/AbstractNonInteractiveCredentialsAction.java
#1343 (comment):

         try {

•            final AuthenticationContextBuilder builder = new DefaultAuthenticationContextBuilder(
  

Here is another pass at the API based on the collective feedback:

final AuthenticationContextBuilder builder = new DefaultAuthenticationContextBuilder

(
this.authenticationSystemSupport.getPrincipalElectionStrategy());final AuthenticationTransaction transaction = AuthenticationTransaction.wrap(new ClientCredential(credentials));this.authenticationSystemSupport.getAuthenticationTransactionManager().handle(transaction, builder);

final AuthenticationContext authenticationContext = builder.build(service);

final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationContext);

• I removed the AuthenticationTransactionFactory and added a factory
  method to wrap credentials. I would have opted for for, except that I
  can't seeing how it's a keyword.
• The sanitation of credentials is now moved to the
  AuthenticationTransaction, since the factory is gone.
• As discussed, since we need the Service in the context, I opted to
  keep the AuthenticationContext around and the builder that goes along
  with that as the carrier.
• I also do agree with Dima, that I wouldnt want upper layers to have
  access to the AuthenticationManager. The TransactionManager is the
  right intermediate glue here.

Note that this API does not have to be "perfect". It has to be functional.
It has to support MFA. It can always be revved later as we don't really
consider any of this an "extension point".

—
Reply to this email directly or view it on GitHub
https://github.com/Jasig/cas/pull/1343/files#r48090894.

[dima767]

And so my personal, final proposal for refactoring for this PR is:

• Get rid of AutheticationTransactionFactory and create a static factory method on transaction class itself: AuthenticationTransaction::for(Credential)
• Rename AuthenticationObjectsRepository to for example AuthenticationSystemSupport to clearly communicate that this is a "helper" type. And that class would for now carry and expose AuthenticationTransactionManager and PrincipalElectionStrategy

And so I would summarize my own understanding of the current abstractions and so we synchronize all our thinking to make sure we are all on the same page:

The main abstractions are:

• AuthenticationTransaction - a unit of work wrapping presented Credential for the system to process
• AuthenticationTransactionManager - an entity that is responsible of receiving AuthenticationTransaction(s) and "processing it
• AuthenticationContextBuilder - a "carrier" for all the processed authentication transactions by the transaction manager, as well as the entity that has the knowledge of how to produce a final authentication view from all these transaction steps when asked
• AuthenticationContext - a definitive, final view of the successful authentication event (sum of all the authentication transactions). This is the "glue" object between Authn and Web SSO subsystems. It is passed to the web SSO subsystem (CASImpl) to produce web SSO tokens (TGT) - only when authentication event is successfully completed. See how CASimpl API is now fully de-coupled from authentication concerns?!

Also note that these new authentication API types serve as the "higher level of abstraction" for the client layers to use (web, SWF... any other client layer for that matter) as opposed to the "lower level" API - which would represent real "workers" in this subsystem i.e. AuthenticationManager, AuthenticationHandler, etc. Now the upper layers like SWF (yet to be developed) - would not deal with AuthenticationManager directly - only via this higher level authentication transaction API.

Does this sound reasonable? I personally like this de-coupling and think it will enable very flexible mfa architecture, REST authentication architecture, etc. in the CAS server.

[mmoayyed]

@leleuj what do you think about the recent mods to this PR? Are you ok with the changeset, or shall we iterate more?

[mmoayyed]

Heads up. Merging by Monday morning, prepping for RC1, unless there are other reported issues.

[leleuj]

I guess we iterate enough for the time being. We'll revisite the design if we feel the need later on. Rebase the PR with the master and go ahead with the merge.