support configurable sign algorithm

[amergey]

No description provided.

[jzheaux]

@amergey, my apologies, but I think I led you down the wrong path here by placing the algorithm in Saml2AuthenticationRequestContext.

Thinking about this further, the selected algorithm must be one that the Asserting Party can support. The place to look that up is RelyingPartyRegistration.AssertingPartyDetails.

What we need to do, then, is add a private List<String> signingMethods field to RelyingPartyRegistration.AssertingPartyDetails and then use that information to resolve the SigningSignatureParameters in OpenSamlAuthenticationRequestFactory instead of getting the setting from Saml2AuthenticationRequestContext.

Would you be able to make those changes to the PR? If not, I'd be happy to add a polish to apply them.

[amergey]

@jzheaux I have updated the PR according to your comments

[jzheaux]

Thanks for the updates, @amergey! I've left some feedback inline.

Also, will you please rebase? This file has changed since you began the PR.

[jzheaux]

It would be nice if the JavaDoc here explained how this value relates to the asserting party's <IDPSSODescriptor>.

[amergey]

I have rebased the PR, there is a test failure since, but I do not see how it is related to my changes

[jzheaux]

Thanks for the fast turnaround, @amergey!

This is starting to take shape. I've left a bit more feedback inline.

[jzheaux]

This should ensure that the list also isn't empty. You can achieve this by calling Assert.notEmpty instead. I think it would make sense to change the error message to align with that as well.

[amergey]

@jzheaux I have updated the PR according to your comments, the remaining point is related to collection for signing algorithm. I keep list because of the order, but If you still think collection is better, let me know, I will do the change

[jzheaux]

Thanks for the adjustments, @amergey. I've left one more inline comment.

In preparation for merging, will you please squash your commits and format the commit message to include the ticket number, like so:

Commit Title

Closes gh-8952

[jzheaux]

What is your reasoning for clearing the list out? build does not clear out the other lists or reset any of the other fields to their original state.

[amergey]

Good point, It was to solve the initialization issue where default algo was added in the list of the builder because of the fact that a default AssertingPartyDetails Builder is created and kept with RelyingPartyRegistration instance but after looking at this more carefully, it seems the wrong way to fix this, I will improve this

[jzheaux]

Thanks, @amergey! This is now merged into master.

Note that I added a polish of e1826a0 that changes the name of the method since "method" and "algorithm" are synonymous. So getSigningMethodAlgorithms is now getSigningAlgorithms. I also added support to the withRelyingPartyRegistration method to make sure the algorithms get copied over. Finally, I reordered the methods so that they appear in the order that those items typically appear in role descriptors.

Also, I added documentation via b8f8fab.