Add factories as an extension mechanism for Auth

[mauromol]

As anticipated in #350 and in #265 (comment), this is my proposal to enhance the Auth class to make it use custom extensions of the standard SAML message classes (AuthnRequest, SamlResponse, LogoutRequest and LogoutResponse) when needed, by introducing specialized factories for the purpose.
The change is 100% backward compatible: when no factory is explicitly set on Auth, default factory instances are used such that instances of the default toolkit classes will be used, just as before.

The changes introduced in #350 make this work quite well because now the parameters that drive the behaviour of the outgoing message classes are encapsulated into input param objects that can be passed to the factories and the whole APIs is quite consistent.
The only part where there's some discrepancy is the case of the creation of an outgoing LogoutResponse object. This is because there's no "LogoutResponseParams" class and such a logout response object is not built on construction (like it happens for all the other outgoing messages, like AuthnRequest or LogoutRequest for the outgoing case), but with LogoutResponse.build(). This complicates specifying a proper factory a little bit for just that case, see here:

auth.setAuthnRequestFactory(AuthnRequestEx::new); // to make it build custom AuthnRequests
auth.setSamlResponseFactory(SamlResponseEx::new); // to make it build custom SamlResponses
auth.setOutgoingLogoutRequestFactory(LogoutRequestEx::new); // to make it build custom outgoing LogoutRequests
auth.setReceivedLogoutRequestFactory(LogoutRequestEx::new); // to make it build custom incoming LogoutRequests
auth.setOutgoingLogoutResponseFactory((sett, nothing) -> new LogoutResponseEx(settings, null)); // to make it build custom outgoing LogoutResponses
auth.setReceivedLogoutResponseFactory(LogoutResponseEx::new); // to make it build custom incoming LogoutResponses

To improve that specific case, we may either:

• declare a specialized factory just for that case, instead of using the SamlOutgoingMessageFactory: this just moves the asymmetry, yet improving the java-saml consumer experience in case a custom outgoing logout response implementation is needed
• or else, try to align the API for this case to the others, and hence:
  • introduce a LogoutResponseParams object; this may carry on information like the inResponseTo and the SamlResponseStatus/statusCode
  • add a LogoutResponse constructor overloading that takes settings and a LogoutResponseParams as input; this overloading would cover the "outgoing" case and would do what LogoutResponse.build() currently does; indeed, in this case the HttpRequest parameter of the only constructor we currently have is useless; there would be a symmetry with the different LogoutRequest constructors, one of which is used to cover the "incoming" case, the other one for the "ougoing" case instead
  • deprecate build() (it becomes useless and even detrimental if inadvertitedly used when the LogoutResponse is received), but keep it just to maintain backward compatibility
  • once this is done, the outgoing logout response factory would become just a SamlOutgoingMessageFactory<LogoutResponseParams, LogoutResponse> and it could be set with a straightforward:

auth.setOutgoingLogoutResponseFactory(LogoutResponseEx::new);

Of course, the second solution is much better and improves the API coherence. If you agree with this approach, I may integrate it in this PR.

[mauromol]

Hmmm... those tests aren't failing for me in Eclipse even if I'm using Java 8. I suspect it's due to compiler differences. I will have a look.

[mauromol]

@pitbulk Hi Sixto, did you have a change to look at this?

[pitbulk]

Can you move all the factories inside its own folder?
toolkit/src/main/java/com/onelogin/saml2/factory

In the second approach, maybe the new constructor can call the build method, so we keep it

[mauromol]

Can you move all the factories inside its own folder?
toolkit/src/main/java/com/onelogin/saml2/factory

Done in 784e26a.

In the second approach, maybe the new constructor can call the build method, so we keep it

Yes, this is what I had in mind. So, let me implement it in that way. Coming back soon :-)

[mauromol]

Ok @pitbulk, here it is the API enhancement for LogoutResponse we were talking about. I think it was worth the effort.

In the end, I did not make the new LogoutResponse constructor delegate to build(), because I would have lost the input parameters instance (which might be extended by the client), breaking the extensibility mechanism offered by postProcessXml, so I opted to simply copy the build(String, SamlResponseStatus) code into the new LogoutResponse constructor. The good news is that now those deprecated build() overloadings can be easily removed in the future without the need to touch other non-deprecated code.

Please note two things:

1. just like Improve authentication and logout request input params API #350, this PR brings a change in postProcessXml API for LogoutResponse, letting it receive the input parameters as well. Again, I think this is fine since the postProcessXml has not been released yet, so we should still be on time for this
2. the API is almost completely backward compatible, but there's a tiny API compile-time breakage: indeed, previously the API to build an outgoing LogoutResponse required something like this:

// null is the HTTP request - it's not needed to build an outgoing LogoutResponse
LogoutResponse response = new LogoutResponse(settings, null);
response.build();

Now calling the LogoutResponse constructor with a null second parameter becomes ambiguous, so that code should be changed to:

LogoutResponse response = new LogoutResponse(settings, (HttpRequest) null);
response.build();

or replaced with the new recommended constructor:

LogoutResponse response = new LogoutResponse(settings, new LogoutResponseParams());
// no need to call build() any longer

I'm not sure what happens at runtime with precompiled consumers: I think that the binding to the LogoutResponse(Saml2Settings, HttpRequest) constructor for those calls remains valid (because its compiled in), so I would expect no break at runtime for such consumers.

I think this is an acceptable compromise, especially because I expect most consumers just use the Auth class to drive the logout process, so the change is completely transparent in these cases.

[mauromol]

I just verified that binary compatibility is ok. To test this, I did the following:

• wrote the following code against current stable java-saml 2.7.0:

Saml2Settings settings = new Saml2Settings();
LogoutResponse r = new LogoutResponse(settings, null);
r.build();
System.out.println(DateTimeFormatter.ISO_INSTANT.format(r.getIssueInstant().getTime().toInstant()));

• compiled against current stable java-saml 2.7.0 and prepared a corresponding JAR
• ran the code: I get the issue instant printed (i.e.: now)
• replaced java-saml 2.7.0 jars on the classpath with 2.7.1-SNAPSHOT jars compiled from this branch
• re-ran the previous code (without recompiling)
• the code still runs fine, printing the issue instant (no ambiguous call exceptions or such)

So, as I expected, binary compatibility is maintained: consumer code that needs to be recompiled just needs to fix the mentioned compilation error with one of the solutions I described in my previous comment.

[pitbulk]

Now devs using the Auth object will see 6 new methods
(setAuthnRequestFactory, setSamlResponseFactory, setOutgoingLogoutRequestFactory, setReceivedLogoutRequestFactory, setOutgoingLogoutResponseFactory, setReceivedLogoutResponseFactory)

that don't gonna need if they are using the toolkit as it is.

I wonder if there is any other way to allow to extend the auth class, without adding all such amount of methods and new attributes:

	private static final SamlOutgoingMessageFactory<AuthnRequestParams, AuthnRequest> DEFAULT_AUTHN_REQUEST_FACTORY = AuthnRequest::new;
	private static final SamlReceivedMessageFactory<SamlResponse> DEFAULT_SAML_RESPONSE_FACTORY = SamlResponse::new;
	private static final SamlOutgoingMessageFactory<LogoutRequestParams, LogoutRequest> DEFAULT_OUTGOING_LOGOUT_REQUEST_FACTORY = LogoutRequest::new;
	private static final SamlReceivedMessageFactory<LogoutRequest> DEFAULT_RECEIVED_LOGOUT_REQUEST_FACTORY = LogoutRequest::new;
	private static final SamlOutgoingMessageFactory<LogoutResponseParams, LogoutResponse> DEFAULT_OUTGOING_LOGOUT_RESPONSE_FACTORY = LogoutResponse::new;
	private static final SamlReceivedMessageFactory<LogoutResponse> DEFAULT_RECEIVED_LOGOUT_RESPONSE_FACTORY = LogoutResponse::new;
	
	private SamlOutgoingMessageFactory<AuthnRequestParams, AuthnRequest> authnRequestFactory = DEFAULT_AUTHN_REQUEST_FACTORY;
	private SamlReceivedMessageFactory<SamlResponse> samlResponseFactory = DEFAULT_SAML_RESPONSE_FACTORY;
	private SamlOutgoingMessageFactory<LogoutRequestParams, LogoutRequest> outgoingLogoutRequestFactory = DEFAULT_OUTGOING_LOGOUT_REQUEST_FACTORY;
	private SamlReceivedMessageFactory<LogoutRequest> receivedLogoutRequestFactory = DEFAULT_RECEIVED_LOGOUT_REQUEST_FACTORY;
	private SamlOutgoingMessageFactory<LogoutResponseParams, LogoutResponse> outgoingLogoutResponseFactory = DEFAULT_OUTGOING_LOGOUT_RESPONSE_FACTORY;
	private SamlReceivedMessageFactory<LogoutResponse> receivedLogoutResponseFactory = DEFAULT_RECEIVED_LOGOUT_RESPONSE_FACTORY;

Maybe a hash attribute containing all the default factories?
A unique method able to handle all the new factory declaration?

[mauromol]

Well, the above default implementations and corresponding fields are not actually visible to toolkit consumers, as they are private implementation details. I decided to declare the default implementations as static constants because so I can use them as a safe default in case someone sets null on the corresponding factory, which would of course cause a NullPointerException at runtime otherwise.

There are 6 of them because, indeed, there are 6 cases to handle in the worst case scenario (that is: you need to cusomise ALL the messages): AuthnRequest+Response, +2 LogoutRequest and +2 LogoutResponse scenarios (incoming/outgoing, depending on whether the logout process is SP-initiated or IdP-initiated).

Indeed, the only changes that consumers will see are 6 new setters, which can be safely ignored if they just want to use the default implementation like they do right now.

As an alternative approach we may provide a factory that is able to create all message instances. Let's call it SamlMessageFactory. I expect it to have n methods like createAuthnRequest, createSamlResponse, createLogoutRequest and so on. We may establish that if one of these methods returns null, the default SAML message implementation will be used for that. Java 8 default methods or an adapter base class could be provided to allow the consumer to easily create SamlMessageFactory implementations that just implement the desired message creations, without the need to implement all of them.

This may reduce the code in Auth, but indeed will make the specification of new factories a little more complicated from a java-saml consumer point of view and prevents the use of Java 8 lambdas. Right now, if I just want to provide an AuthnRequest and a SamlResponse extensions, I have to do:

Auth auth = new Auth(request, response);
auth.setAuthnRequestFactory(MyOwnAuthnRequest::new);
auth.setSamlResponseFactory(MyOwnSamlResponse::new);
auth.login();

With the alternative approach I need to:

1. declare a SamlMessageFactory implementation:

public class MySamlMessageFactory implements SamlMessageFactory {
  @Override
  public AuthnRequest createAuthnRequest(Saml2Settings settings, AuthnRequestParams params) {
    return new MyOwnAuthnRequest(settings, params);
  }

  @Override
  public SamlResponse createSamlResponse(Saml2Settings settings, HttpRequest request) {
    return new MyOwnSamlResponse(settings, request);
  }
}

2. set this implementation on the Auth object:

Auth auth = new Auth(request, response);
auth.setSamlMessageFactory(new MySamlMessageFactory());
auth.login();

I save one setter call on Auth but I have a lot more to code the MySamlMessageFactory class.
Indeed, since the typical case will be (I guess) thay you need to provide just one or two custom message implementations, I believe the current approach is much quicker and leaner from a consumer point of view rather then the alternative approach, even if it means some more (indeed very simple) code added to Auth.

Some other alternatives? Just one setter on Auth that receives a map of factory instances, each of them keyed by a conventional "name" (like "AUTHN_REQUEST_FACTORY", "RESPONSE_FACTORY" and such)? Maybe, but we are somewhat reducing type-safety...

After all I am still personally preferring the current approach.
But tell me what you prefer. The SamlMessageFactory is also valid, although more verbose for consumers.

[pitbulk]

A developer that doesn't need to extend the classes at all will be confused to see those new 6 methods, that my opinion.

I believe that someone extending the current classes and adding already its own code, will be ok with the extra effort required by the SamlMessageFactory approach.

The "map of factory instances" seems also an acceptable approach for me.

So, I basically prioritize the usability of the majority of developers, that don't gonna extend the classses, while allowing others to extend the classes, but not maybe in the easiest form for them, but they will already touch the code, and know about SAML since they are extending the classes, so I believe its ok.

[mauromol]

If you believe that the number of SAML messages actually handled by the Auth class may increase in the future (Attribute Authority support?), the SamlMessageFactory approach will be more scalable, indeed. If, instead, the number of handled messages is unlikely to grow, it will be unlikely that we need to add even more setters (apart from those 6 ones) in the future, if we maintain the current approach...
So, once again, perhaps you have a better insight on future plans...

[mauromol]

I believe that someone extending the current classes and adding already its own code, will be ok with the extra effort required by the SamlMessageFactory approach.

Ok, so I will explore this path. I personally prefer this much more than the map approach...

[pitbulk]

One of the main goals of the OneLogin SAML toolkits is to keep them simple.

There are already other implementations covering the whole SAML spec, but its settings and code are kinda complex... so our goaI is to offer a simple implementation that follows the standard and allows you to cover the majority of use cases.

That said, we are open to allow others to build more complex implementations using our project as a base, and here is where the factory extension appears, but we need to keep the focus on simplicity.

[mauromol]

That said, we are open to allow others to build more complex implementations using our project as a base, and here is where the factory extension appears, but we need to keep the focus on simplicity.

Simplicity is the reason why I chose java-saml myself, so I agree with this :-) Indeed, in all my PRs I strive to maintain it as simple and backward compatible as possible, while still allowing it to be reasonably powerful when needed.

I will get back soon on this one.

[pitbulk]

Yes, I appreciate your huge effort in the contributions you did.

[mauromol]

Here it is @pitbulk. Let me know what you think.

Maybe I can try to squash some commits, if you prefer to simplify the history. The only problem is that it's probably worth to keep some of the above commits on their own and there's a bit of overlapping between the first approach implementation and, for instance, the change in LogoutResponse API.

[mauromol]

@pitbulk I have made the small adjustments to the README changes required by #359. Now this PR is complete from my side and applies the single factory class approach.

Please let me know what you think.

[mauromol]

@pitbulk do you think this can be merged in its current shape? :-)