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.

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

[78699] SiS validate /authenticate csp type & acr #16241

Merged
merged 17 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
4 changes: 2 additions & 2 deletions app/controllers/v0/sign_in_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,13 @@ def validate_authorize_params(type, client_id, acr, operation)
if client_config(client_id).blank?
raise SignIn::Errors::MalformedParamsError.new message: 'Client id is not valid'
end
unless SignIn::Constants::Auth::CSP_TYPES.include?(type)
unless client_config(client_id).valid_credential_service_provider?(type)
dickdavis marked this conversation as resolved.
Show resolved Hide resolved
raise SignIn::Errors::MalformedParamsError.new message: 'Type is not valid'
end
unless SignIn::Constants::Auth::OPERATION_TYPES.include?(operation)
raise SignIn::Errors::MalformedParamsError.new message: 'Operation is not valid'
end
unless SignIn::Constants::Auth::ACR_VALUES.include?(acr)
unless client_config(client_id).valid_service_level?(acr)
raise SignIn::Errors::MalformedParamsError.new message: 'ACR is not valid'
end
end
Expand Down
11 changes: 11 additions & 0 deletions app/models/sign_in/client_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ class ClientConfig < ApplicationRecord
validates :client_id, presence: true, uniqueness: true
validates :logout_redirect_uri, presence: true, if: :cookie_auth?
validates :access_token_attributes, inclusion: { in: Constants::AccessToken::USER_ATTRIBUTES }
validates :service_levels, presence: true, inclusion: { in: Constants::Auth::ACR_VALUES, allow_nil: false }
validates :credential_service_providers, presence: true,
inclusion: { in: Constants::Auth::CSP_TYPES, allow_nil: false }

def self.valid_client_id?(client_id:)
find_by(client_id:).present?
Expand Down Expand Up @@ -49,6 +52,14 @@ def va_terms_enforced?
enforced_terms == Constants::Auth::VA_TERMS
end

def valid_credential_service_provider?(type)
credential_service_providers.include?(type)
end

def valid_service_level?(acr)
service_levels.include?(acr)
end

private

def appropriate_mock_environment?
Expand Down
29 changes: 18 additions & 11 deletions spec/controllers/v0/sign_in_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
get(:authorize, params: authorize_params)
end

let!(:client_config) { create(:client_config, authentication:, pkce:) }
let!(:client_config) do
create(:client_config, authentication:, pkce:, credential_service_providers:, service_levels:)
end
let(:authorize_params) do
{}.merge(type)
.merge(code_challenge)
Expand All @@ -26,6 +28,8 @@
let(:code_challenge_method) { { code_challenge_method: 'some-code-challenge-method' } }
let(:client_id) { { client_id: client_id_value } }
let(:pkce) { true }
let(:credential_service_providers) { %w[idme logingov dslogon mhv] }
let(:service_levels) { %w[loa1 loa3 ial1 ial2 min] }
let(:client_id_value) { client_config.client_id }
let(:authentication) { SignIn::Constants::Auth::COOKIE }
let(:client_state) { {} }
Expand Down Expand Up @@ -228,9 +232,10 @@
it_behaves_like 'error response'
end

context 'when type param is given but not in CSP_TYPES' do
let(:type_value) { 'some-undefined-type' }
context 'when type param is given but not in client credential_service_providers' do
let(:type_value) { 'idme' }
let(:type) { { type: type_value } }
let(:credential_service_providers) { ['logingov'] }
let(:expected_error) { 'Type is not valid' }

it_behaves_like 'error response'
Expand All @@ -245,21 +250,22 @@
it_behaves_like 'error response'
end

context 'and acr param is given but not in ACR_VALUES' do
let(:acr_value) { 'some-undefiend-acr' }
context 'and acr param is given but not in client service_levels' do
let(:acr_value) { 'ial1' }
let(:service_levels) { ['ial2'] }
let(:expected_error) { 'ACR is not valid' }

it_behaves_like 'error response'
end

context 'and acr param is given and in ACR_VALUES but not valid for logingov' do
context 'and acr param is given and in client service_levels but not valid for logingov' do
let(:acr_value) { 'loa1' }
let(:expected_error) { 'Invalid ACR for logingov' }

it_behaves_like 'error response'
end

context 'and acr param is given and in ACR_VALUES and valid for logingov' do
context 'and acr param is given and in client service_levels and valid for logingov' do
let(:acr_value) { 'ial1' }

context 'and code_challenge_method is not given' do
Expand Down Expand Up @@ -408,21 +414,22 @@
it_behaves_like 'error response'
end

context 'and acr param is given but not in ACR_VALUES' do
let(:acr_value) { 'some-undefiend-acr' }
context 'and acr param is given but not in client service_levels' do
let(:acr_value) { 'loa1' }
let(:service_levels) { ['loa3'] }
let(:expected_error) { 'ACR is not valid' }

it_behaves_like 'error response'
end

context 'and acr param is given and in ACR_VALUES but not valid for type' do
context 'and acr param is given and in client service_levels but not valid for type' do
let(:acr_value) { 'ial1' }
let(:expected_error) { "Invalid ACR for #{type_value}" }

it_behaves_like 'error response'
end

context 'and acr param is given and in ACR_VALUES and valid for type' do
context 'and acr param is given and in client service_levels and valid for type' do
let(:acr_value) { 'loa1' }

context 'and code_challenge_method is not given' do
Expand Down
106 changes: 105 additions & 1 deletion spec/models/sign_in/client_config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
certificates:,
access_token_attributes:,
enforced_terms:,
terms_of_use_url:)
terms_of_use_url:,
service_levels:,
credential_service_providers:)
end
let(:client_id) { 'some-client-id' }
let(:authentication) { SignIn::Constants::Auth::API }
Expand All @@ -32,6 +34,8 @@
let(:access_token_attributes) { [] }
let(:enforced_terms) { SignIn::Constants::Auth::VA_TERMS }
let(:terms_of_use_url) { 'some-terms-of-use-url' }
let(:service_levels) { %w[loa1 loa3 ial1 ial2 min] }
let(:credential_service_providers) { %w[idme logingov dslogon mhv] }

describe 'validations' do
subject { client_config }
Expand Down Expand Up @@ -217,6 +221,66 @@
end
end

describe '#credential_service_providers' do
context 'when credential_service_providers is empty' do
let(:credential_service_providers) { [] }
let(:expected_error_message) { "Validation failed: Credential service providers can't be blank" }
let(:expected_error) { ActiveRecord::RecordInvalid }

it 'raises validation error' do
expect { subject }.to raise_error(expected_error, expected_error_message)
end
end

context 'when credential_service_providers contain values not included in CSP_TYPES constant' do
let(:credential_service_providers) { %w[idme logingov dslogon mhv bad_csp] }
let(:expected_error_message) { 'Validation failed: Credential service providers is not included in the list' }
let(:expected_error) { ActiveRecord::RecordInvalid }

it 'raises validation error' do
expect { subject }.to raise_error(expected_error, expected_error_message)
end
end

context 'when all credential_service_providers values are included in CSP_TYPES constant' do
let(:credential_service_providers) { SignIn::Constants::Auth::CSP_TYPES }

it 'does not raise validation error' do
expect { subject }.not_to raise_error
end
end
end

describe '#service_levels' do
context 'when service_levels is empty' do
let(:service_levels) { [] }
let(:expected_error_message) { "Validation failed: Service levels can't be blank" }
let(:expected_error) { ActiveRecord::RecordInvalid }

it 'raises validation error' do
expect { subject }.to raise_error(expected_error, expected_error_message)
end
end

context 'when service_levels contain values not included in ACR_VALUES constant' do
let(:service_levels) { %w[loa1 loa3 ial1 ial2 min bad_acr] }
let(:expected_error_message) { 'Validation failed: Service levels is not included in the list' }
let(:expected_error) { ActiveRecord::RecordInvalid }

it 'raises validation error' do
expect { subject }.to raise_error(expected_error, expected_error_message)
end
end

context 'when all service_levels values are included in ACR_VALUES constant' do
let(:service_levels) { SignIn::Constants::Auth::ACR_VALUES }

it 'does not raise validation error' do
expect { subject }.not_to raise_error
end
end
end

describe '#enforced_terms' do
context 'when enforced_terms is arbitrary' do
let(:enforced_terms) { 'some-enforced-terms' }
Expand Down Expand Up @@ -378,6 +442,46 @@
end
end

describe '#valid_credential_service_provider?' do
subject { client_config.valid_credential_service_provider?(type) }

context 'when type is included in csps' do
let(:type) { 'idme' }

it 'returns true' do
expect(subject).to be(true)
end
end

context 'when type is not included in csps' do
let(:type) { 'bad_csp' }

it 'returns false' do
expect(subject).to be(false)
end
end
end

describe '#valid_service_level?' do
subject { client_config.valid_service_level?(acr) }

context 'when acr is included in acrs' do
let(:acr) { 'loa1' }

it 'returns true' do
expect(subject).to be(true)
end
end

context 'when acr is not included in acrs' do
let(:acr) { 'bad_acr' }

it 'returns false' do
expect(subject).to be(false)
end
end
end

describe '#mock_auth?' do
subject { client_config.mock_auth? }

Expand Down
Loading