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.

Sign up for GitHub

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

Refactor ial/aal determination #11160

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Refactor ial/aal determination #11160

merged 1 commit into from
Sep 3, 2024

Conversation

vrajmohan
Copy link
Member

@vrajmohan vrajmohan commented Aug 29, 2024 •
edited
Loading

Restrict ial and aal determination to the idp app.

This was unnecessarily leaking into the saml_idp gem. A subsequent PR in
that repo will remove these concerns there.

As part of the move to semantic ACR values, we will also replace the regex prefix matching for IALs and AALs with explicit enumerations.

@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch 3 times, most recently from 9912852 to 77dfbad Compare August 29, 2024 20:27
@vrajmohan vrajmohan changed the title [DO NOT REVIEW - ONLY A TEST] Refactor ial/aal determination Refactor ial/aal determination Aug 29, 2024
@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch from 77dfbad to 7761db1 Compare August 29, 2024 20:43
@vrajmohan vrajmohan marked this pull request as ready for review August 29, 2024 22:44
@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch 3 times, most recently from bb80c71 to ac888d0 Compare August 30, 2024 19:40
lmgeorge
lmgeorge reviewed Aug 30, 2024
View reviewed changes
app/models/federated_protocols/saml.rb Outdated
Comment on lines 20 to 34
requested_ial_authn_context || default_ial_authn_context
end
end

def requested_ial_authn_context
request.requested_authn_contexts.find do |classref|
IAL_PREFIX.match?(classref)
end
end

def aal
request.requested_aal_authn_context
request.requested_authn_contexts.find do |classref|
AAL_PREFIX.match?(classref)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

note to other reviewers: this code is a lift and shift from the saml_idp gem (see: PR#117: Remove ial/aal concerns) and will be replaced in the next sprint with a long term solution.

@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch from 0fe705f to ac888d0 Compare September 1, 2024 01:42
Sgtpluck
Sgtpluck reviewed Sep 3, 2024
View reviewed changes
Copy link
Member

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

this change looks good! i think we should add the LOA prefix back to avoid changing any behavior, and a couple formatting nitpicks in the tests, but just minor changes. once the LOA prefix is back i'll approve

app/controllers/concerns/saml_idp_auth_concern.rb Show resolved Hide resolved
app/controllers/concerns/saml_idp_auth_concern.rb Show resolved Hide resolved
app/controllers/saml_idp_controller.rb Show resolved Hide resolved
app/controllers/saml_idp_controller.rb
if requested_ial_acr == ::Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF
return 'ialmax'
else
saml_protocol.requested_ial_authn_context.presence || 'none'
Copy link
Member

Choose a reason for hiding this comment

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

this is not in scope to your change, but interesting that we return 'none' here instead of nil 🤔

app/services/analytics_events.rb
@@ -5845,7 +5845,7 @@ def rules_of_use_visit
# @param [String] endpoint
# @param [Boolean] idv
# @param [Boolean] finish_profile
# @param [Integer] requested_ial
# @param [String] requested_ial
Copy link
Member

Choose a reason for hiding this comment

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

nice catch on these two

app/services/attribute_asserter.rb Outdated
@@ -42,7 +42,7 @@ def build
add_vot(attrs)
else
add_aal(attrs)
add_ial(attrs) if authn_request.requested_ial_authn_context || !service_provider.ial.nil?
add_ial(attrs) if requested_ial_authn_context || !service_provider.ial.nil?
Copy link
Member

Choose a reason for hiding this comment

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

since requested_ial_authn_context is only used in the case of this conditional, i might pull the whole conditional into a method (and/or update the .nil? check to a present? check so it can be positive) but it's not a big deal really

spec/controllers/saml_idp_controller_spec.rb Outdated Show resolved Hide resolved
app/models/federated_protocols/saml.rb
@@ -2,6 +2,9 @@

module FederatedProtocols
class Saml
IAL_PREFIX = %r{^http://idmanagement.gov/ns/assurance/ial}
Copy link
Member

Choose a reason for hiding this comment

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

[suggestion] it looks like the LOA prefix is not being included. we should probably add that back as this is a refactor, not a code change

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change on the basis of https://gsa-tts.slack.com/archives/C0NGESUN5/p1724965595589029.

Copy link
Member

Choose a reason for hiding this comment

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

yes, i understand it's maybe not what we want, but if we're going to make a change that affects partner requests or responses, we have to consider it fully and not just include it in a refactor. feel free to make a ticket for it!

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell (and the tests), it only affects the logged analytics event, and doesn't affect what we send to partners.

Copy link
Member Author

@vrajmohan vrajmohan Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll open a separate issue - https://gitlab.login.gov/lg-people/Melba/backlog-fy24/-/issues/99

Copy link
Member

Choose a reason for hiding this comment

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

thanks! once you add the LOA prefix back in i'll approve

spec/models/federated_protocols/saml_spec.rb Show resolved Hide resolved
@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch 2 times, most recently from db51015 to 806feef Compare September 3, 2024 19:24
@vrajmohan
Perform ial and aal determination in the idp app
ae88f6c 
This was unnecessarily leaking into the saml_idp gem.
A separate PR (18F/saml_idp#117) in
that repo will remove these concerns from there.

See https://gitlab.login.gov/lg-people/Melba/backlog-fy24/-/issues/95

changelog: Internal, Refactoring, Refactor saml_request.requested_ial_authn_context calls to single place
@vrajmohan vrajmohan force-pushed the vm/refactor-ial-aal-determination branch from 806feef to ae88f6c Compare September 3, 2024 20:14
@vrajmohan vrajmohan merged commit 6e8b29e into main Sep 3, 2024
2 checks passed
@vrajmohan vrajmohan deleted the vm/refactor-ial-aal-determination branch September 3, 2024 21:10
@jmdembe jmdembe mentioned this pull request Sep 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sgtpluck Sgtpluck Sgtpluck approved these changes

@jmhooper jmhooper Awaiting requested review from jmhooper

@lmgeorge lmgeorge Awaiting requested review from lmgeorge

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

Successfully merging this pull request may close these issues.
3 participants
@vrajmohan @lmgeorge @Sgtpluck