Skip to content

Fix SAML auth context handling for IAL2 strict request#6253

Merged
aduth merged 2 commits intomainfrom
aduth-saml-auth-ial2-strict
Apr 28, 2022
Merged

Fix SAML auth context handling for IAL2 strict request#6253
aduth merged 2 commits intomainfrom
aduth-saml-auth-ial2-strict

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 26, 2022

Why: So that an IAL2 strict request behaves similar to a standard IAL2 request, particularly in how a user should be directed to enter their password to decrypt PII if encrypted.

Discovered in #6229, via failing feature specs where user would not be prompted to enter their password to decrypt profile when authenticating as IAL2 strict.

@aduth
Copy link
Contributor Author

aduth commented Apr 26, 2022

Discovered in #6229, via failing feature specs where user would not be prompted to enter their password to decrypt profile when authenticating as IAL2 strict.

For example, you can see the issue by checking out the changes of #6255 against the current main.

git checkout main
git checkout 1b3b5d45a02d31f56fb042b083b1411f03b1d1db -- spec
rspec ./spec/features/reports/authorization_count_spec.rb:162
#     Capybara::ElementNotFound:
#       Unable to find field "Password" that is not disabled

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

Great catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

nice.

@aduth
Copy link
Contributor Author

aduth commented Apr 27, 2022

I discovered an additional, similar issue with how we're not upgrading IAL1 accounts to IAL2 for IAL2 strict requests, but I filed this as a separate bug, since the changes here are sufficient to unblock me in #6229.

See: LG-6217

@aduth
Copy link
Contributor Author

aduth commented Apr 27, 2022

@solipet , I pushed a minor revision in 3cd0288 to revert the IAL2 authns' constant to its original value, and instead check explicitly for IAL2 strict when considering requested attributes. While I sorta like the idea of disambiguating "IAL2" and "IAL2 strict", we don't have a tendency of doing that anywhere else, so this feels more in line with what we're usually doing when checking for IAL2 or IAL2 strict.

Could you give it a quick look and let me know if this makes sense to you?

@aduth aduth force-pushed the aduth-saml-auth-ial2-strict branch from 12ad4c7 to 3cd0288 Compare April 27, 2022 13:52
aduth added 2 commits April 27, 2022 11:55
**Why**: So that an IAL2 strict request behaves similar to a standard IAL2 request, particularly in how a user should be directed to enter their password to decrypt PII if encrypted.

[skip changelog]
Avoid disambiguating IAL2 as meaning either "IAL2" or "IAL2 strict", since we don't seem to have a tendency to do that
@aduth aduth force-pushed the aduth-saml-auth-ial2-strict branch from 3cd0288 to 42fa86f Compare April 27, 2022 15:55
@aduth aduth requested a review from solipet April 27, 2022 18:11
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

I agree with your decision to avoid messing with the current definition of IAL2 authn contexts.

@aduth aduth merged commit 0b56e2e into main Apr 28, 2022
@aduth aduth deleted the aduth-saml-auth-ial2-strict branch April 28, 2022 17:40
peggles2 pushed a commit that referenced this pull request May 3, 2022
* Fix SAML auth context handling for IAL2 strict request

**Why**: So that an IAL2 strict request behaves similar to a standard IAL2 request, particularly in how a user should be directed to enter their password to decrypt PII if encrypted.

[skip changelog]

* Check explicitly for ial2_strict in SAML requested_attributes

Avoid disambiguating IAL2 as meaning either "IAL2" or "IAL2 strict", since we don't seem to have a tendency to do that
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants