Skip to content

Jmax/lg 12264 replace uses of aal level requested in sp session#10139

Closed
jmax-gsa wants to merge 20 commits intomainfrom
jmax/LG-12264-replace-uses-of-aal_level_requested-in-sp_session
Closed

Jmax/lg 12264 replace uses of aal level requested in sp session#10139
jmax-gsa wants to merge 20 commits intomainfrom
jmax/LG-12264-replace-uses-of-aal_level_requested-in-sp_session

Conversation

@jmax-gsa
Copy link
Contributor

🎫 Ticket

LG-12264

🛠 Summary of changes

  • Replaced all uses of sp_session[:aal_level_requested] with new Vot::Parser::Result#aal_level_requested method
  • Updated feature specs as necessary to align with that change.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Verify that all specs pass.

@jmax-gsa jmax-gsa requested review from a team, jmhooper and solipet February 22, 2024 19:31
ial: parsed_vot&.ial_value_requested,
ial2: parsed_vot&.ial2_requested?,
ialmax: parsed_vot&.ialmax_requested?,
aal_level_requested: parsed_vot&.aal_level_requested,
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit makes a modification to the way these are set, but it does not change their usage. I don't see any changes in this pull request that removes the places we inspect sp_session[:aal_level_requested] and replaces them with resolved_authn_context_result.aal2? or resolved_authn_context.phishing_resistant?.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just looked an it looks like there are 2 places that the requested AAL is meaningful:

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.

A couple of requests - since this work was broken up into individual tickets for each property, we should restrict this to just stop reading from sp_session[:aal_level_requested]


def requested_aal
sp_session[:aal_level_requested] || 1
AuthnContextResolver.new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the AAL level is ultimately used to determine the remember device cookie expiration:

def mfa_expiration_interval
aal_1_expiration = IdentityConfig.store.remember_device_expiration_hours_aal_1.hours
aal_2_expiration = IdentityConfig.store.remember_device_expiration_minutes_aal_2.minutes
return aal_2_expiration if sp_aal > 1
return aal_2_expiration if sp_ial > 1
return aal_2_expiration if requested_aal > 1
aal_1_expiration
end

I think instead of copying the old value we could use AuthnContextResolver in the remember device cookie expiration computation:

  def mfa_expiration_interval
    aal_1_expiration = IdentityConfig.store.remember_device_expiration_hours_aal_1.hours
    aal_2_expiration = IdentityConfig.store.remember_device_expiration_minutes_aal_2.minutes
    return aal_2_expiration if sp_aal > 1
    return aal_2_expiration if sp_ial > 1
    # return aal_2_expiration if requested_aal > 1
    return aal_2_expiration if resolved_authn_context_result.aal2?

    aal_1_expiration
  end

Of course we'll need to define resolved_authn_context_result here for that to work. Alternatively we could pass it in. If we pass it in the next 2 paragraphs become irrelevant.

I also noticed you are passing nil here for the service_provider arg. The service provider is available in this class as sp. Setting it will make sure we are getting results that include the SP defaults. If we do that well we can also remove those sp_aal and sp_ial checks in #mfa_expiration_interval

Finally, we probably want to make sure we are reading vtr and acr_values from sp_session instead of from sp_request. They shouldn't be different but I think we want to make sure we are looking at the same place as the application controller.

If we pass the resolved_authn_context_result we can still make the changes to remove those sp_aal and sp_ial checks in #mfa_expiration_interval. The result that comes from the application controller includes the SP defaults when it is setting aal2?.


def remember_device_default
sp_aal < 2
def remember_device_default(authorization_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we made the authentication context an argument to the initializer we could pass it in in the application controller and not need to worry about passing it into all of the methods that will need to consume it.

@jmax-gsa jmax-gsa closed this Feb 27, 2024
@jmax-gsa jmax-gsa deleted the jmax/LG-12264-replace-uses-of-aal_level_requested-in-sp_session branch January 13, 2025 15:09
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.

3 participants