Getting response attributes from Saml2AuthenticatedPrincipal

[ghost]

It's not possible to read attribute values from SAML response after successful login because Saml2AuthenticatedPrincipal doesn't have getAttributes() method like OAuth2AuthenticatedPrincipal does. I've added the getAttributes() method to the interface. The map is being populated in OpenSamlAuthenticationProvider.

With this patch, I won't have to parse SAML response the second time by myself if I want to read some attribute value.

fixes gh-8661

[jzheaux]

Thanks for such a quick response, @kostic017!

I've left some feedback inline.

[jzheaux]

In addition to the inline feedback, would you also please add unit tests that show the feature working? I'd recommend tests that show how attributes statements of various arrangements and data types will work.

[jzheaux]

Thanks for the edits, @kostic017! I've left a bit more feedback. Most importantly, could you add some unit tests to prove that the functionality works as intended?

[jzheaux]

Thanks for the updates, @kostic017! It's nice to see this taking shape.

I've left some additional feedback inline.

[jzheaux]

I've been putting some thought into this method, and I've got a question for you.

While the SAML spec allows multiple AttributeValues for a single Attribute, it also encourages that it be one-to-one. Because of that, I imagine that most applications will want to do:

String email = principal.getAttribute("email");

instead of:

List<String> emails = principal.getAttribute("email");
String email = emails == null || emails.isEmpty() ? null : emails.get(0);

What would be your expectation?

If you'd expect to do the first option, we might want to change this method to:

List<A> values = getAttributes().get(name);
return (A) CollectionUtils.firstElement(values);

and possibly rename it to getFirstAttribute.

If you'd expect the second option, the signature should probably change to <A> List<A> getAttribute(String)

[ghost]

Yeah, I prefer the first option, but for one user attribute (tenant) in my app I'm expecting multiple values. Maybe we should provide both options?

[ghost]

I forgot about getAttributes method. I could use it in that one case, if we only provide the first option.

[jzheaux]

I agree that to get the underlying list you can call getAttributes().get("tenant").

Since there is the possibility of adding both somewhere down the road, I'm leaning more heavily now towards renaming this method to getFirstAttribute. I believe this aligns nicely with MultiValueMap which is similar in concept and also has a method called getFirst().

[ghost]

Hm, after changing getAttributes return type, I can't do this anymore

List<String> tenants = (List<String>) principal.getAttributes().get("tenant");

since the compiler doesn't allow casting from List<Object> to List<String>.

And this doesn't look like a nice thing to work with:

List<Object> tenants = principal.getAttributes().get("tenant");

If I had a method like this

public <A> List<A> getAttributeValues(String name) {
    return (List<A>) getAttributes().get(name);
}

I could do

List<String> tenants = principal.getAttributeValues("tenant");

[ghost]

On the other hand, what if those values have different types... I'm so used to working with only one value type.

[jzheaux]

Good catch! A couple thoughts come to mind:

1. Since this caught you and I off-guard, it would be nice if there was a test in SimpleSaml2AuthenticatedPrincipalTests that demonstrated the various behaviors. That way our expectations are codified in a test.
2. I think adding a List getter makes sense. An application can set the reference type to List<Object> if their attribute contains multiple types.

Also, I'd recommend that the interface stick with getAttribute and getFirstAttribute. The reason for that is it aligns with similar Spring Security support for OAuth 2.0, which uses getClaim and getAttribute. As another example, this is the same convention followed by the Servlet spec with HttpServletRequest#getParameter, #getAttribute, and #getHeader.

If you agree, then the contract would become:

Map<String, List<Object>> getAttributes();

<A> List<A> getAttribute(String name);

<A> A getFirstAttribute(String name);

[ghost]

@jzheaux I think you added duplicate label by mistake.

[jzheaux]

Thanks for the additional changes, @kostic017.

In preparation for merging, will you please do three more things?

• Make sure that any file you modified has a copyright header that extends to 2020?
• Squash your commits into one
• Format your final commit message to include the phrase Closes gh-xxx in the commit body, e.g.

Add SAML Attribute Support

Closes gh-8661

[jzheaux]

Regarding the duplicate label, it "duplicates" the linked issue. When looking at the tickets completed in a milestone, it helps remove some noise from the report.

That said, I think I should have probably waited to add it until after everything was merged, so as to create less confusion.

[ghost]

NP. All done now.

[jzheaux]

Excellent, @kostic017! This is now merged into master via eed3322.

I also applied a small polish via 360db53 and some documentation via 8cbdcfe