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

Jump to bottom

Multiple of the same Attribute Nodes now retain all attribute values #59

Merged
merged 3 commits into from Jul 5, 2022
Merged

Multiple of the same Attribute Nodes now retain all attribute values #59

merged 3 commits into from Jul 5, 2022

Conversation

adam-james-v
Copy link

This is related to metabase/metabase#20744

If an IdP sends a response containing some Attribute nodes as follows:

...
      <saml:Attribute Name="member_of" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string">test-group1</saml:AttributeValue>
      </saml:Attribute>
      <saml:Attribute Name="member_of" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string">test-group2</saml:AttributeValue>
      </saml:Attribute>
...

then previously the first value was discarded as only the last node/value is merged into the attributes map in Assertion->map.

Now, attribute nodes are grouped and their attribute value nodes are concatenated.

@adam-james-v
Multiple of the same Attribute Nodes now retain all attribute values
2ef3aa1 
Prior, if a response had multiple Attribute Nodes with the same name,
any values only within the last node would be kept.

Now, attribute nodes are grouped and their attribute value nodes are concatenated.
@codecov
Copy link

codecov bot commented Jun 16, 2022 •
edited

Codecov Report

Merging #59 (28d3068) into master (501c736) will increase coverage by 0.19%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   74.72%   74.92%   +0.19%     
==========================================
  Files          10       10              
  Lines         645      646       +1     
  Branches       91       90       -1     
==========================================
+ Hits          482      484       +2     
  Misses         72       72              
+ Partials       91       90       -1
Impacted Files Coverage Δ
src/saml20_clj/sp/response.clj 64.32% <83.33%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 501c736...28d3068. Read the comment docs.

@adam-james-v adam-james-v requested a review from a team June 16, 2022 17:59
camsaul
camsaul reviewed Jun 16, 2022
View reviewed changes
deps.edn Outdated Show resolved Hide resolved
camsaul
camsaul requested changes Jun 16, 2022
View reviewed changes
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but please don't specify the Clojure version since this is a lib. Once you remove that line re-request me and I'll merge this and cut a new point release

@camsaul camsaul merged commit e5eb42a into master Jul 5, 2022
@camsaul camsaul deleted the multiple-attribute-nodes-keep-attribute-values branch July 5, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsaul camsaul camsaul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@adam-james-v @camsaul