Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for extension classes to post-process generated XML #321

Merged

Conversation

mauromol
Copy link
Contributor

@mauromol mauromol commented Apr 1, 2021

This change allows for any java-saml consumer to extend the standard
classes used to generate SAML messages (AuthnRequest, LogoutRequest and
LogoutResponse), as well as the metadata, and provide their own logic to
post-process the default XML produced by java-saml. Any extension class
will then be able to transform or enrich the generated XML as required,
before the framework applies encoding, encryption or signing.

This is the implementation of the idea I was thinking of when writing #265 (comment). I think this is the most non-instrusive and not too dirty way to let java-saml be extended by consumers. Of course, this is just one step: if the consumer wants to use the Auth class as well, the other problem I mentioned in the above comment must be solved (please see #265 (comment) for a proposal).

@mauromol
Copy link
Contributor Author

mauromol commented Apr 15, 2021

While using this feature myself to implement SPID, it emerged that postProcessXml may need to access input parameters, in particular when post processing outgoing messages (AuthRequest and LogoutRequest). This is made harder by the fact that postProcessXml is called at construction time: indeed, accessing the standard input parameters (which can be retrieved using the AuthnRequest and LogoutRequest getters) is not a problem, since the call to postProcessXml is made after basic class initialization has already been performed. However, if your custom extension of AuthRequest and/or LogoutRequest requires further input, this can be a problem.
If the approach described in #307 for AuthnRequest is accepted, the solution can be to pass the AuthnRequestParams input parameters to the postProcessXml (along with the original XML String): if custom input is required, a subclass of AuthnRequestParams may be implemented and postProcessXml may simply access the required information by casting the AuthnRequestParams object to the target expected type.

I can't add this change to this pull request right now, until the refactoring introduced in #307 is accepted. Here is just a preview: mauromol@c96a1d4 (but a more comprehensive change should most probably increase the AuthnRequestParams getter visibility to public, otherwise accessing the input from postProcessXml would still be impossible).
This use case, though, shows another benefit we could get by applying such a refactoring.

An alternative but more invasive approach could be the following (I use the AuthnRequest example):

  • store the AuthnRequestParams in a class field
  • provide a getter that postProcessXml may use to retrieve the input parameters

However in this case, to avoid confusion, all the input-related getters in AuthnRequest (like isForceAuthn(), isPassive(), etc.) should then delegate to the corresponding methods of the stored input parameters (and perhaps they could possibly be deprecated): in fact, to maintain backward compatibility without going too deep in changing the current API, #307 lets AuthnRequest extend AuthnRequestParams directly.

@pitbulk pitbulk force-pushed the master branch 3 times, most recently from ab7e4d7 to 3c79c8c Compare May 18, 2021 00:32
@pitbulk
Copy link
Contributor

pitbulk commented Jun 24, 2021

I'm ok with the postProcessXml idea that will allow you to extend the behavior.

But I don't see why in each class you need to recover the settings.

If you are using the Auth class, you already have the getSettings method
If you executed the constructor of AuthnRequest, SamlResponse, LogoutRequest, LogoutResponse or Metadata.
You already provided the setting object on the constructor, so you still have it.

I believe that instead of adding settings to the class and create a return method, you could simply define postProcessXml as

protected String postProcessXml(Saml2Settings settings,  final String logoutResponseXml) {
    return logoutResponseXml;
}

That way you could even inject changes on the settings object

@mauromol
Copy link
Contributor Author

I think that lowering the coupling between the message classes and the settings is a good idea, so adding the settings parameter to postProcessXml is a good solution and I can change it this way. Still it's a fact that all message classes already store the settings instance in a field, which is private and hence not accessible to subclasses. I can't look at the code right now, but I was wondering whether getting rid of this strong coupling would be viable or not.

@mauromol
Copy link
Contributor Author

As a follow up to my previous comment, let's consider AuthnRequest for instance: the settings property was added in commit ac8ae33, my change only provides a protected getter available to subclasses to retrieve it.
This said, we may shape postProcessXml method similarly to generateSubstitutor, so letting it receive the settings instance as an input parameter. If you agree on this, I will adapt this PR.

This change allows for any java-saml consumer to extend the standard
classes used to generate SAML messages (AuthnRequest, LogoutRequest and
LogoutResponse), as well as the metadata, and provide their own logic to
post-process the default XML produced by java-saml. Any extension class
will then be able to transform or enrich the generated XML as required,
before the framework applies encoding, encryption or signing.
The various SAML message and metadata object classes have now a
protected getter that allows for subclasses to access the settings
specified at construction time. This is useful to ease extension, for
instance when implementing postProcessXml, so that extensions don't need
to save their own copy of the settings.
@mauromol mauromol force-pushed the add-message-xml-post-processing branch from 9f2132f to 423618a Compare July 23, 2021 14:57
@mauromol
Copy link
Contributor Author

@pitbulk I made postProcessXml receive the settings directly, hence reverting the addition of the settings getters on all classes. I also rebased changes on current master.

@pitbulk pitbulk merged commit 8c66a14 into SAML-Toolkits:master Jul 23, 2021
@mauromol mauromol deleted the add-message-xml-post-processing branch July 24, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants