Make SamlResponse more extensible#318
Merged
pitbulk merged 2 commits intoApr 30, 2021
Merged
Conversation
The query and queryAssertion methods are really useful when creating extensions of SamlResponse that need to expose other information that the plain java-saml library does not (like the response issue instant, or any custom contents of the AuthnContext in the assertion auth statement). Having these two methods private forces the extender to reimplement them from scratch, probably by copy-and-paste, which is clearly sub-optimal.
If a subclass needs to extend isValid(String), it must be able to set the validation exception. In fact, overriding any of the non-private validateXXX methods is trivial, because a ValidationError can simply be thrown, but if additional validation is required, which is not covered by any of those validateXXX methods, a subclass may need to override isValid(String) directly, and this does not throw ValidationError, but rather catches it so that it can be subsequently returned by getValidationException() to the consumer. Hence, a setter with at least protected visibility is needed for subclasses to do the same.
pitbulk
approved these changes
Apr 30, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The query and queryAssertion methods are really useful when creating
extensions of SamlResponse that need to expose other information that
the plain java-saml library does not (like the response issue instant,
or any custom contents of the AuthnContext in the assertion auth
statement). Having these two methods private forces the extender to
reimplement them from scratch, probably by copy-and-paste, which is
clearly sub-optimal.
A concrete example: the Italian SPID authentication system expects the "authentication level" to be specified within the
AuthnContextClassRefelement. Also, it recommends to track the response issue instant, so when using java-saml to implement a SPID SP, these two pieces of information should be extracted from the response.Please note I also removed a clear repetition in the
querymethod implementation, by delegating togetSAMLResponseDocument()to retrieve the non-encryptedDocumentinstance.