Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/presenters/saml_request_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class SamlRequestPresenter
ATTRIBUTE_TO_FRIENDLY_NAME_MAP = {
email: :email,
all_emails: :all_emails,
first_name: :given_name,
middle_name: :name,
last_name: :family_name,
Expand Down
9 changes: 9 additions & 0 deletions app/services/attribute_asserter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def initialize(user:,
def build
attrs = default_attrs
add_email(attrs) if bundle.include? :email
add_all_emails(attrs) if bundle.include? :all_emails
add_bundle(attrs) if user.active_profile.present? && ial_context.ial2_or_greater?
add_verified_at(attrs) if bundle.include?(:verified_at) && ial_context.ial2_service_provider?
add_aal(attrs)
Expand Down Expand Up @@ -167,6 +168,14 @@ def add_email(attrs)
}
end

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

name_format: 'urn:oasis:names:tc:SAML:2.0:attrname-format:basic',
name_id_format: Saml::XML::Namespaces::Formats::NameId::EMAIL_ADDRESS,
}
end

def bundle
@_bundle ||= (
authn_request_bundle || service_provider.metadata[:attribute_bundle] || []
Expand Down
116 changes: 77 additions & 39 deletions spec/features/multiple_emails/sp_sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,61 +3,110 @@
feature 'signing into an SP with multiple emails enabled' do
include SamlAuthHelper

scenario 'signing in with OIDC sends the email address used to sign in' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)
context 'with the email scope' do
scenario 'signing in with OIDC sends the email address used to sign in' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)

expect(emails.count).to eq(2)

emails.each do |email|
visit_idp_from_oidc_sp(scope: 'openid email')
signin(email, user.password)
fill_in_code_with_last_phone_otp
click_submit_default
click_agree_and_continue if current_path == sign_up_completed_path

decoded_id_token = fetch_oidc_id_token_info
expect(decoded_id_token[:email]).to eq(email)
expect(decoded_id_token[:all_emails]).to be_nil

Capybara.reset_session!
end
end

expect(emails.count).to eq(2)
scenario 'signing in with SAML sends the email address used to sign in' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)

emails.each do |email|
visit_idp_from_oidc_sp
signin(email, user.password)
fill_in_code_with_last_phone_otp
click_submit_default
click_agree_and_continue if current_path == sign_up_completed_path
expect(emails.count).to eq(2)

emails.each do |email|
visit authn_request
signin(email, user.password)
fill_in_code_with_last_phone_otp
click_submit_default
click_agree_and_continue if current_path == sign_up_completed_path

xmldoc = SamlResponseDoc.new('feature', 'response_assertion')
email_from_saml_response = xmldoc.attribute_value_for('email')

expect_oidc_sp_to_receive_email(email)
expect(email_from_saml_response).to eq(email)

Capybara.reset_session!
Capybara.reset_session!
end
end
end

scenario 'signing in with SAML sends the email address used to sign in' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)
context 'with the all_emails scope' do
scenario 'signing in with OIDC sends all emails' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)

expect(emails.count).to eq(2)
expect(emails.count).to eq(2)

emails.each do |email|
visit authn_request
signin(email, user.password)
visit_idp_from_oidc_sp(scope: 'openid all_emails')
signin(emails.first, user.password)
fill_in_code_with_last_phone_otp
click_submit_default
click_agree_and_continue if current_path == sign_up_completed_path
click_agree_and_continue

xmldoc = SamlResponseDoc.new('feature', 'response_assertion')
email_from_saml_response = xmldoc.attribute_value_for('email')
decoded_id_token = fetch_oidc_id_token_info
expect(decoded_id_token[:all_emails]).to match_array(emails)
end

scenario 'signing in with SAML sends all emails' do
user = create(:user, :signed_up, :with_multiple_emails)
emails = user.reload.email_addresses.map(&:email)

expect(emails.count).to eq(2)

expect(email_from_saml_response).to eq(email)
settings = saml_settings(
overrides: {
authn_context: [
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
"#{Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF}all_emails",
],
},
)
visit authn_request(settings)
signin(emails.first, user.password)
fill_in_code_with_last_phone_otp
click_submit_default
click_agree_and_continue

xmldoc = SamlResponseDoc.new('feature', 'response_assertion')

Capybara.reset_session!
emails_from_saml_response = xmldoc.attribute_node_for('all_emails').children.map(&:text)
expect(emails_from_saml_response).to match_array(emails)
end
end

def visit_idp_from_oidc_sp
def visit_idp_from_oidc_sp(scope:)
visit openid_connect_authorize_path(
client_id: 'urn:gov:gsa:openidconnect:sp:server',
response_type: 'code',
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
scope: 'openid email',
scope: scope,
redirect_uri: 'http://localhost:7654/auth/result',
state: SecureRandom.hex,
prompt: 'select_account',
nonce: SecureRandom.hex,
)
end

def expect_oidc_sp_to_receive_email(email)
def fetch_oidc_id_token_info
redirect_uri = URI(current_url)
redirect_params = Rack::Utils.parse_query(redirect_uri.query).with_indifferent_access
code = redirect_params[:code]
Expand All @@ -83,18 +132,7 @@ def expect_oidc_sp_to_receive_email(email)

token_response = JSON.parse(page.body).with_indifferent_access
id_token = token_response[:id_token]
decoded_id_token = JWT.decode(id_token, nil, false).first.with_indifferent_access

expect(decoded_id_token[:email]).to eq(email)

access_token = token_response[:access_token]
page.driver.get api_openid_connect_userinfo_path,
{},
'HTTP_AUTHORIZATION' => "Bearer #{access_token}"

userinfo_response = JSON.parse(page.body).with_indifferent_access

expect(userinfo_response[:email]).to eq(email)
JWT.decode(id_token, nil, false).first.with_indifferent_access
end

def client_private_key
Expand Down
17 changes: 17 additions & 0 deletions spec/services/attribute_asserter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,23 @@
end
end

context 'custom bundle includes all_emails' do
before do
create(:email_address, user: user)
allow(service_provider.metadata).to receive(:[]).with(:attribute_bundle).and_return(
%w[all_emails],
)
subject.build
end

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))
Comment on lines +529 to +533

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❤️

end
end

context 'custom bundle includes email, phone' do
before do
allow(service_provider.metadata).to receive(:[]).with(:attribute_bundle).and_return(
Expand Down