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

Add support for multiple Attribute Consuming Services #307

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mauromol
Copy link
Contributor

@mauromol mauromol commented Mar 5, 2021

Attribute Consuming Services can now be configured in settings. They will then be parsed and added to the generated metadata. On SSO initiation a selector mechanism can be used to select the desired Attribute Consuming Service, producing the proper AttributeConsumingServiceIndex attribute in the AuthnRequest.

This is what I had in mind when I wrote #264 (comment). To quickly understand the rationale, I suggest to start to look at the changes made to the README file.
All changes are backward compatible , although I've deprecated some method overloadings because I think they are now obsolete and pollute the code (see, for instance, the so many overloadings of Auth.login() method). To better shape the login input parameters, I introduced a AuthnRequest superclass, named AuthnRequestParams that just carry information on how the AuthnRequest should be produced, without any reference to the settings, to the binding protocol or to other context-related information. Then, AuthnRequest extends AuthnRequestParams to easily use those input parameters and maintain backward compatibility. If in future any more input params are to be introduced, they can be added to AuthnRequestParams without the need to even further overload Auth.login() (I was thinking about the ability to select an Assertion Consumer Services among a set of available services, somewhat similar to what I made now with Attribute Consuming Services)
(^^^ these changes have meanwhile already been merged within the context of other PRs)

I tested my changes by adding some more tests and by using it in an application I'm writing. As I said in #264, these changes are particularly useful to properly support the Italian SPID authentication system.

I tried to adhere as much as possible to the existing styles of code formatting and javadoc, but it was not easy because I see there are multiple styles in place.

@mauromol
Copy link
Contributor Author

mauromol commented Apr 1, 2021

This needs a simple addition to properly merge with #315 (with regards to the new Attribute Consuming Services XML fragment generation), but I'm waiting for feedback before proceeding.
(^^^ done)

@mauromol
Copy link
Contributor Author

@pitbulk now that all the required refactoring is merged into master, I could rebase this PR so that the above commits describe just the new feature. As stated above, to understand what's new from a user point of view, start by looking at the updated README file.
The change is 100% backward compatible and IMHO nicely integrates with what already exists, in a non-invasive way.

@mauromol mauromol force-pushed the supportMultipleAttributeConsumingServices branch from 32eda0e to 13102ac Compare August 19, 2021 09:32
@pitbulk
Copy link
Contributor

pitbulk commented Oct 1, 2021

Now that we added factories, is easy to support this

@mauromol mauromol force-pushed the supportMultipleAttributeConsumingServices branch from 13102ac to f272092 Compare November 5, 2021 15:15
@mauromol
Copy link
Contributor Author

mauromol commented Nov 5, 2021

Hi @pitbulk , I've rebased this PR so that it applies cleanly on current master.

Yes, now that all my previous contributions have been merged, this feature may be added on top of java-saml. However, I'm not sure whether it would be so straightforward. This change indeed features:

  • the ability to specify an Attribute Consuming Service in settings: this nicely improves on top of current support for the ACS (Assertion Consumer Service) specification and allows the user to complete the whole Service Provider configuration in settings, instead of using a hybrid approach (currently, the Attribute Consuming Service must be set programmatically with Metadata(Saml2Settings, Calendar, Integer, AttributeConsumingService) instead)
  • the ability to support multiple Attribute Consuming Services: I don't know what is your typical use case of java-saml, but when dealing with authentication systems like SPID, eIDAS or other (not-so-)complex scenarios, the need to declare multiple Attribute Consuming Services is quite frequent (see How to add custom "RequestedAttributes" into SP metadata #264 for instance); this can be once again configured in settings and, in particular, in property files with a syntax that follows the one used by Spring Boot for indexed properties, so it should be quite natural for Java developers
  • a minor but IMHO useful clean up of the documentation regarding the use of the "ACS" acronym, which in SAML documentation is used for Assertion Consumer Services, not for Attribute Consuming Services

I know your concerns about keeping java-saml simple. With this regards, I can say that:

  • the change is 100% backward compatible: if you need not to use Attribute Consuming Services, you won't even notice this new feature; if you were using it to specify just a single Attribute Consuming Service programmatically, you'll still be able to do that, I just deprecated that Metadata constructor, not removed
  • if you think that the README file becomes now too complicated and verbose, I may change how I document this feature in the following way:
    • we may describe the case of a single Attribute Consuming Service in the README, which nicely mirrors what we do for the Assertion Consumer Service
    • then put a reference to a new md file which instead explains how multiple Attribute Consuming Services can be specified

The latter documentation change may also scale well if we later introduce the support for multiple Assertion Consumer Services as well.

Attribute Consuming Services can now be configured in settings. They
will then be parsed and added to the generated metadata. On SSO
initiation a selector mechanism can be used to select the desired ACS,
producing the proper AttributeConsumingServiceIndex attribute in the
AuthnRequest.
ACS is usually used to refer to the Assertion Consumer Service concept
in SAML. The ACS may also behave as an Attribute Consuming Services,
but in general the two concepts are separate. This fixes the use of the
ACS acronym for the Assertion Consumer Service only.
@mauromol mauromol force-pushed the supportMultipleAttributeConsumingServices branch from 11a883d to b12811f Compare November 28, 2021 16:23
Now Saml2Settings.checkSPSettings() also checks that, if any Attribute
Consuming Service are declared, their configuration is consistent.
When a non-indexed Attribute Consuming Service is defined in
configuration along with other indexed Services, a warning is printed to
the log to inform that the non-indexed one will be ignored.
@mauromol mauromol force-pushed the supportMultipleAttributeConsumingServices branch from b12811f to 2eb29ba Compare November 28, 2021 16:32
@bzvestey
Copy link
Contributor

@eriktalvi can you please help look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants