LG-12150 add vot to the identity token#10142
Conversation
changelog: Internal, IdV, write acr_values and vtr to identites table
changelog: Internal, IdV, Add vot to identity token
| else | ||
| raise "Unknown ial #{ial}" | ||
| end | ||
| return nil unless identity.acr_values.present? |
There was a problem hiding this comment.
Currently we only return a single ACR value. I'm not sure if we want to start returning the full list because I am not sure how SPs will respond to that.
I believe the previous implementation of responding with the ACR value for the identity assurance level is probably the way to go here to ensure backwards compadibility.
There was a problem hiding this comment.
Actually we could probably remove all the IAL computation in my previous comment if we dressed up this method like so:
def acr
if resolved_authn_context_result.ialmax?
determine_ial_max_acr.name
elsif resolved_authn_context_result.identity_proofing?
Vot::LegacyComponentValues::IAL2.name
else
Vot::LegacyComponentValues::IAL1.name
end
endThere was a problem hiding this comment.
And since this didn't stack up like I expected, this is the "previous comment" I am referring to: #10142 (comment)
There was a problem hiding this comment.
This way we:
- Return
ial1orial2correctly in the IALMAX case - Return
ial1orial2even if the SP requestsloa1orloa3or if the SP makes noialrelated request - Only ever return one value in the ACR
- Correctly return the
ial1orial2ACR values even if we introduce new ACR values or change names
| context 'ial2 request' do | ||
| before do | ||
| identity.ial = 2 | ||
| identity.acr = Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF |
There was a problem hiding this comment.
I think a test where we include an AAL ACR value alongside an IAL once may be wise to cover the concerns I describe in my comment above ☝️
app/services/id_token_builder.rb
Outdated
| def id_token_claims | ||
| { | ||
| acr: acr, | ||
| vot: vot, |
There was a problem hiding this comment.
we should feature flag adding this
There was a problem hiding this comment.
also, I think we should make sure to add vtm if we add vot also (spec link)
and to be clear, I think we should have both claims behind a feature flag to start with, so that we can quiet/soft launch this feature
and ideally, I think we would only only return the VOT claims if the clients requested VOT in the first place
| end.join(' ') | ||
| end | ||
|
|
||
| def sp_requests_vot? |
There was a problem hiding this comment.
We probably also want to add a vot.present? clause here as well. That way we won't end up with a vtm claim when there is not vot.
app/services/id_token_builder.rb
Outdated
|
|
||
| def acr_ial_component_values | ||
| resolved_authn_context_result.component_values.select do |component_value| | ||
| component_value.name.include?('ial') |
There was a problem hiding this comment.
I believe that this could include an loa ACR value which may trip this up. You may want to consider something like this:
def acr_value_for_ial
if resolved_authn_context_result.identity_proofing?
Vot::LegacyComponentValues::IAL2
else
Vot::LegacyComponentValues::IAL1
end
endThis approach will return the ial2 ACR value when identity proofing is requested (this is assuming it was done correctly which the checks in the authorization controller should ensure). When identity proofing is not requested this will return the ial1 ACR value.
This will hopefully work better for us since we won't be tied to matching against the name of the ACR value and instead against what the ACR value actually does.
There was a problem hiding this comment.
Actually I thought about this some and posted a shortcut here: #10142 (comment)
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-12150
🛠 Summary of changes
This is part 3 of the ticket where we finally add the VoT to the identity token. With acr and vtr being written to the identity table we can now pull the values and use the resolved authn context to determine what we send in the id token to the service provider.