Skip to content

feat: LG13235 consume eipp data from sinatra #10618

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

Merged
KeithNava merged 11 commits into from
May 21, 2024
Merged

feat: LG13235 consume eipp data from sinatra #10618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
40bc058
feat: add eipp to vot parser
KeithNava May 13, 2024
bd3d18e
feat: update authn context resolver tests for ipp
KeithNava May 13, 2024
90403c3
feat: add enhanced_ipp_required required to service provider
KeithNava May 15, 2024
8c0f520
feat: update oidc connect parsing logic for enhance_ipp
KeithNava May 16, 2024
ac8717e
feat: default saml parsing for eipp to false
KeithNava May 16, 2024
91093cb
feat: update ordering of params in sp session
KeithNava May 16, 2024
bece22c
feat: update spec for service provider session
KeithNava May 16, 2024
fddc65f
fix: merge conflicts
KeithNava May 16, 2024
3d24a05
feat: add vot parser method for sp session
KeithNava May 16, 2024
4d9bbdf
Revert "feat: add enhanced_ipp_required required to service provider"
KeithNava May 20, 2024
5107a9b
feat: lintfix
KeithNava May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/services/vot/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class ParseException < StandardError; end
:identity_proofing?,
:biometric_comparison?,
:ialmax?,
:enhanced_ipp?,
) do
def self.no_sp_result
self.new(
Expand All @@ -22,6 +23,7 @@ def self.no_sp_result
identity_proofing?: false,
biometric_comparison?: false,
ialmax?: false,
enhanced_ipp?: false,
)
end

Expand Down Expand Up @@ -86,6 +88,7 @@ def expand_components_with_initial_components(initial_components)
identity_proofing?: requirement_list.include?(:identity_proofing),
biometric_comparison?: requirement_list.include?(:biometric_comparison),
ialmax?: requirement_list.include?(:ialmax),
enhanced_ipp?: requirement_list.include?(:enhanced_ipp),
)
end

Expand Down
6 changes: 6 additions & 0 deletions app/services/vot/supported_component_values.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ module SupportedComponentValues
implied_component_values: [P1],
requirements: [:biometric_comparison],
).freeze
Pe = ComponentValue.new(
Copy link
Copy Markdown
Contributor

@svalexander svalexander May 14, 2024

Choose a reason for hiding this comment

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

what does Pe mean here?

Copy link
Copy Markdown
Contributor Author

@KeithNava KeithNava May 14, 2024

Choose a reason for hiding this comment

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

"Proofing Enhanced?" 😆 I really put it out there for feedback. I initially had it as "P2" since it was a step up from the P1 component. I'm really happy to change it to whatever is more intuitive though.

Copy link
Copy Markdown
Contributor

@svalexander svalexander May 14, 2024

Choose a reason for hiding this comment

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

ahh ok, I wonder what the convention was here? It isn't obvious to me what any of the other names in the file mean either.

Copy link
Copy Markdown
Contributor

@jmhooper jmhooper May 20, 2024

Choose a reason for hiding this comment

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

I don't think that P2 is the way to go. This isn't necessarily as step up from the base proofing experience since it is scoped to in-person. For the same reason proofing with a biometric was named Pb instead of P2.

The naming convention for vectors of trust is described in [RFC 8485]. They are an upper-case letter followed by a lower-case letter or a number.

name: 'Pe',
description: 'Enhanced In Person Proofing is required',
implied_component_values: [P1],
Copy link
Copy Markdown
Contributor

@svalexander svalexander May 14, 2024

Choose a reason for hiding this comment

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

also what does P1 signify?

Copy link
Copy Markdown
Contributor Author

@KeithNava KeithNava May 14, 2024

Choose a reason for hiding this comment

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

I was understanding the "P" to signify proofing related components and "1" was because it was the initial component in the proofing category...I could be wrong though 😅

Copy link
Copy Markdown
Contributor

@svalexander svalexander May 14, 2024

Choose a reason for hiding this comment

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

hmm ok, i guess i wonder what implied_component_values is supposed to be? This file is totally new to me

Copy link
Copy Markdown
Contributor

@gina-yamada gina-yamada May 14, 2024

Choose a reason for hiding this comment

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

The description in this PR is super helpful!

The P component represents identity proofing. It contains the following values:
1: Identity proofing is performed

requirements: [:enhanced_ipp],
).freeze

NAME_HASH = constants.map do |constant|
component_value = const_get(constant)
Expand Down
1 change: 1 addition & 0 deletions spec/policies/service_provider_mfa_policy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
identity_proofing?: false,
biometric_comparison?: false,
ialmax?: false,
enhanced_ipp?: false,
)
end
let(:auth_methods_session) { AuthMethodsSession.new(user_session: {}) }
Expand Down
23 changes: 23 additions & 0 deletions spec/services/authn_context_resolver_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@
expect(result.identity_proofing?).to eq(true)
expect(result.biometric_comparison?).to eq(true)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end

it 'parses the vtr param for enhanced ipp' do
vtr = ['Pe']

result = AuthnContextResolver.new(
service_provider: nil,
vtr: vtr,
acr_values: nil,
).resolve

expect(result.component_values.map(&:name).join('.')).to eq('C1.C2.P1.Pe')
expect(result.aal2?).to eq(true)
expect(result.phishing_resistant?).to eq(false)
expect(result.hspd12?).to eq(false)
expect(result.identity_proofing?).to eq(true)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(true)
end

it 'ignores any acr_values params that are passed' do
Expand Down Expand Up @@ -59,6 +79,7 @@
expect(result.identity_proofing?).to eq(false)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end

it 'properly parses an ACR value without an AAL ACR' do
Expand All @@ -79,6 +100,7 @@
expect(result.identity_proofing?).to eq(false)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end

it 'properly parses an ACR value without an IAL ACR' do
Expand All @@ -99,6 +121,7 @@
expect(result.identity_proofing?).to eq(false)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end
end

Expand Down
18 changes: 18 additions & 0 deletions spec/services/vot/parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
expect(result.identity_proofing?).to eq(false)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end
end

Expand All @@ -38,6 +39,22 @@
expect(result.identity_proofing?).to eq(true)
expect(result.biometric_comparison?).to eq(true)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end

it 'adds the Enhanced In Person Proofing components' do
vector_of_trust = 'Pe'

result = Vot::Parser.new(vector_of_trust:).parse

expect(result.component_values.map(&:name).join('.')).to eq('C1.C2.P1.Pe')
expect(result.aal2?).to eq(true)
expect(result.phishing_resistant?).to eq(false)
expect(result.hspd12?).to eq(false)
expect(result.identity_proofing?).to eq(true)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(true)
end
end

Expand Down Expand Up @@ -77,6 +94,7 @@
expect(result.identity_proofing?).to eq(true)
expect(result.biometric_comparison?).to eq(false)
expect(result.ialmax?).to eq(false)
expect(result.enhanced_ipp?).to eq(false)
end
end
end
Expand Down