Skip to content

Add all_emails support for SAML#5587

Merged
jmhooper merged 8 commits intomainfrom
jmhooper-saml-multiple-emails
Nov 16, 2021
Merged

Add all_emails support for SAML#5587
jmhooper merged 8 commits intomainfrom
jmhooper-saml-multiple-emails

Conversation

@jmhooper
Copy link
Contributor

@jmhooper jmhooper commented Nov 5, 2021

Why: So that SAML SPs can request all of a user's email addresses

**Why**: So that SAML SPs can request all of a user's email addresses
Comment on lines +172 to +173
attrs[:all_emails] = {
getter: ->(principal) { principal.confirmed_email_addresses.map(&:email) },
Copy link
Contributor

Choose a reason for hiding this comment

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

codeclimate is saying this is not covered by tests 😬 can we add some?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also does retuning an array here just correctly turn into an array of some kind of tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, work in progress because I need to add some tests to this one. Gonna try to mirror the pattern I used for the OIDC implementation as much as I can.

And yep, this renders a list of values:

<AttributeStatement>
  <Attribute Name="uuid" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" FriendlyName="uuid">
    <AttributeValue>b9fa616d-0806-450d-a5bd-71a00019ae08</AttributeValue>
  </Attribute>
  <Attribute Name="email" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" FriendlyName="email">
    <AttributeValue>tanja@example.net</AttributeValue>
  </Attribute>
  <Attribute Name="all_emails" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" FriendlyName="all_emails">
    <AttributeValue>tanja@example.net</AttributeValue>
    <AttributeValue>logan@example.com</AttributeValue>
  </Attribute>
  <Attribute Name="aal" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" FriendlyName="aal">
    <AttributeValue>http://idmanagement.gov/ns/assurance/aal/2</AttributeValue>
  </Attribute>
  <Attribute Name="ial" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" FriendlyName="ial">
    <AttributeValue>http://idmanagement.gov/ns/assurance/ial/1</AttributeValue>
  </Attribute>
</AttributeStatement>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis: I just pushed a test for the attribute asserter!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Test is good but can we get one that checks the full XML payload somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me take a swing at that. I know that hidden in the SAML specs somewhere there's gotta be a test that takes apart the SAML response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis: Just added a test along with a bonus test for OIDC

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +172 to +173
attrs[:all_emails] = {
getter: ->(principal) { principal.confirmed_email_addresses.map(&:email) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Test is good but can we get one that checks the full XML payload somewhere?

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +529 to +533
it 'includes all the user email addresses' do
all_emails_getter = ial1_user.asserted_attributes[:all_emails][:getter]
emails = all_emails_getter.call(user)
expect(emails.length).to eq(2)
expect(emails).to match_array(user.confirmed_email_addresses.map(&:email))
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@jmhooper jmhooper merged commit abd8023 into main Nov 16, 2021
@jmhooper jmhooper deleted the jmhooper-saml-multiple-emails branch November 16, 2021 17:45
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.

2 participants