Skip to content

Commit

Permalink
[78699] SiS validate /authenticate csp type & acr (#16241)
Browse files Browse the repository at this point in the history
* adds csps and acrs attributes to ClientConfig

* name update

* adds validation of csp type & acr against client config

* adds csps and acrs attributes to ClientConfig

* name update

* name updates and removes redundant validations

* removes duplicate test
  • Loading branch information
bramleyjl authored Apr 18, 2024
1 parent 056ccee commit d1a5670
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 14 deletions.
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)
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

0 comments on commit d1a5670

Please sign in to comment.