OpenConext / OpenConext-attribute-aggregation

OpenConext attribute aggregation
Apache License 2.0
1 stars 2 forks source link

Set proper Content-type header on SAML AttributeQuery #39

Closed tvdijen closed 2 years ago

tvdijen commented 2 years ago

@thijskh I tried playing with this aggregator in conjunction with SSP's simplesamlphp-module-exampleattributeserver and then hit the UnsupportedBindingException on line 109

I was wondering, since you are using SSP for SAB as well, why this never affected you. This appears to be an appropriate fix.

codecov[bot] commented 2 years ago

Codecov Report

Base: 92.79% // Head: 92.81% // Increases project coverage by +0.02% :tada:

Coverage data is based on head (3dbb213) compared to base (53a5ce8). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #39 +/- ## ============================================ + Coverage 92.79% 92.81% +0.02% Complexity 323 323 ============================================ Files 59 59 Lines 874 877 +3 Branches 48 48 ============================================ + Hits 811 814 +3 Misses 43 43 Partials 20 20 ``` | [Impacted Files](https://codecov.io/gh/OpenConext/OpenConext-attribute-aggregation/pull/39?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenConext) | Coverage Δ | | |---|---|---| | [...ava/aa/aggregators/sab/SabAttributeAggregator.java](https://codecov.io/gh/OpenConext/OpenConext-attribute-aggregation/pull/39/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenConext#diff-YWEtc2VydmVyL3NyYy9tYWluL2phdmEvYWEvYWdncmVnYXRvcnMvc2FiL1NhYkF0dHJpYnV0ZUFnZ3JlZ2F0b3IuamF2YQ==) | `84.37% <100.00%> (+1.61%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenConext). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenConext)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

thijskh commented 2 years ago

SAB does not use that module but its own attributeserver module which seems originally based on sspmod_attributes_server by Sixto Martin which does not check content types.

thijskh commented 2 years ago

I wonder how being strict about the content type relates to SAML 2.0 Bindings 3.2.3.1 lines 353-354?

tvdijen commented 2 years ago

Hmm, interesting.. Although it specifically says additional headers.

I've found out that for SOAP 1.1 the appropriate content type is text/xml and not application/soap+xml which is specific for SOAP 1.2.

SAML2 SOAP-binding is locked to SOAP 1.1

Edit: According to the second link, the Content-type header is required for SOAP, so I think it invalidates your point regarding the SAML 2.0 Bindings.. I really think they mean additional headers there.

thijskh commented 2 years ago

For more compatibility, the attribute server might also check for SOAPAction header (344-345) and process the response if either this with the correct value is found OR the mime type matches.

AA could consider to also send this SOAPAction header as per line 344?

tvdijen commented 2 years ago

The attribute server can only check if the header exists, because the value is unspecified, so it cannot really check that. I'm not really sure what we should put there, but I agree AA should send it.

tvdijen commented 2 years ago

As per paragraph 6.1.1 from the specs

The header field value of empty string ("") means that the intent of the SOAP message is provided by the HTTP Request-URI

So we might as well add the header and leave it empty..

thijskh commented 2 years ago

The value is specified in line 345

tvdijen commented 2 years ago

Ah cool! I missed that.. So many specs involved here I'll add it (if I figure out how)