Ignore unknown authncontext

[Sgtpluck]

🎫 Ticket

Link to the relevant ticket:
Ignore unknown authentication contexts
Link to 1-pager

🛠 Summary of changes

This change updates our SAML integration to allow and ignore unknown authentication context class reference (ACR) values that are sent via the request, as long as there is at least one valid AuthnContext value that we can assert. (OIDC already allowed arbitrary values to be passed in.)

SAML spec line 1820

If Comparison is set to "exact" or omitted, then the resulting authentication context in the authentication
statement MUST be the exact match of at least one of the authentication contexts specified.

This change also:

• Includes any unknown ACR values in authentication events
• Adds/updates associated tests

Open questions:
I am currently handling the case of a partner passing in only unknown ACR values as invalid.

However, we do allow SAML partners to pass in no ACR values, in which case we use their service provider defaults, so an argument could be made that if we get only unknown ACR values, we should default to the SP's defaults.

My thinking is that if they are purposefully sending ACR values, that should override any defaults we have -- if they are only sending one, unknown ACR values that is more likely a mistake. Am open to arguments in favor/against this approach!

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Update your local identity-saml-sinatra repo to include 'unknown-authn-context-value' here https://github.com/18F/identity-saml-sinatra/blob/main/app.rb#L144:
• run sinatra app with make run
• run your local main IdP with make run
• go to http://localhost:4567/
• click sign in
• observer an Authn context Unauthorized authentication context error
• kill the local IdP, and checkout this branch
• run local IdP with make run
• go to http://localhost:4567/
• click sign in
• you should now be able to log in

[jmhooper]

I'll remark here that this may be a deviation from the spec since we are accepting vectors we do not recognize that are not described by our trustmark. However, that may not be a huge deal since this API should be going away soon.

[Sgtpluck]

thanks for that note! i could probably update it to only raise when vectors of trust are passed in, if you have any feelings about it! (although i think we are deviating from the spec in some ways anyways, like having implied vectors, and as you said, it's hopefully not long for our code base.)

[zachmargolis]

indentation is kinda funky here, WDYT of breaking this out into a variable?

Suggested change

	(requested_authn_contexts -
	Saml::Idp::Constants::VALID_AUTHN_CONTEXTS).reject do |authn_context|
	authn_context.match(req_attrs_regexp)
	end.join(' ').presence
	unmatched_contexts = Saml::Idp::Constants::VALID_AUTHN_CONTEXTS).reject do |authn_context|
	authn_context.match(req_attrs_regexp)
	end
	(requested_authn_contexts - unmatched_contexts).join(' ').presence

[Sgtpluck]

sure! updated in 52a697b (i pulled it into a separate method to make it easier to read)

[ajfarkas]

I think we should have a Login-wide position on whether we primarily focus on Partners (ie, breaking errors when they pass invalid values, with the assumption they test their integrations), or on End Users (ie, be as forgiving as possible with integrations within the bounds of the relevant specifications).
These kinds of questions pop up constantly.

[ajfarkas]

🙌

[nprimak]

Some very minor nitpicks, otherwise LGTM! Always excited to help unblock partners.