Improve authentication and logout request input params API #350
Conversation
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
Input parameters, which drive how the AuthnRequest should be generated, are now encapsulated into an AuthnRequestParams object which is far more extensible and help to polish the API of AuthnRequest constructors and Auth.login, which are crowded with overloadings and cannot be easily extended. Also, these input params are passed to postProcessXml: in this way, if an extension plans to use a specialization of AuthnRequestParams, it can access such a specialized instance also within a proper postProcessXml overriding.
This refactor is pretty much the same done with AuthnRequest and AuthnRequestParams: in this case, the params class encapsulates all the logout request input parameters used whenever a new logout request is created for subsequent sending. This as well allows to reduce the number of LogoutRequest constructor and Auth.logout() overloadings, allows an extension to use a customized input param object which is also passed to postProcessXml and eases Auth extensibility.
pitbulk
approved these changes
Jul 26, 2021
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.
This PR is made of:
AuthRequestrefactoring extracted from Add support for multiple Attribute Consuming Services #307LogoutRequestrefactoring picked from my own fork (for which I hadn't provided any PR yet because it was relying on other changes)Benefits of this refactoring:
AuthnRequest,LogoutRequestandAuthAPIs: all those overridings were honestly cluttering the API and they were not easily extensibleAuthnRequestandLogoutRequestgeneration process, may simply extendAuthnRequestParamsandLogoutRequestParamswithout the need to further touch the API ofAuthnRequest,LogoutRequestandAuthclasses (or to provide even more overridings); this allows, for instance, to implement NameIDPolicy AllowCreate hardcoded to true in AuthnRequest #329 by simply adding a new property toAuthnRequestParams(and of course adjust the generation process) without the need to provide new overloadingspostProcessXmlmechanism introduced in Allow for extension classes to post-process generated XML #321 (*), any consumer application which needs to extend this library, may pass toAuthnRequestandLogoutRequestconstructors a proper input params extension, which will also be passed topostProcessXml, so that an overriding of the latter will allow for proper XML manipulation and/or extension based on the actual custom input paramsAuthextensibility improvement, which leverages this rafactoring, can be provided: I already anticipated he idea in Adding attributes to the "saml:Issuer" part in the AuthnRequest #265 (comment) and I may provide a separate PR for that if this is accepted and mergeLogoutRequest: the new introduced constructors now are specialized on a single scenario (new outgoing logout request creation vs parsing of an incoming request in an IdP-initiated logout)The change is 100% backward compatible: all those
Auth.login,Auth.logout,AuthnRequestandLogoutRequestconstructor overloadings have been deprecated (to ease migration), not removed. You may then consider their removal in the future sooner or later, depending on requirements.(*) Please note that this PR also brings a change in
postProcessXmlAPI forAuthnRequestandLogoutRequest, letting it receive the input parameters as well. I think this is fine since thepostProcessXmlhas not been released yet, so we should still be on time for this.