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

FI-3220 Add scope and aud parameters as optional inputs for authorization code redirect test #16

Merged
merged 5 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
24 changes: 23 additions & 1 deletion lib/udap_security_test_kit/authorization_code_redirect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,25 @@ class AuthorizationCodeRedirectTest < Inferno::Test
title: 'Client ID',
description: 'Client ID as registered with the authorization server.'

input :udap_authorization_code_request_scopes,
title: 'Scope Parameter for Authorization Request',
description: %(
A list of space-separated scopes to include in the authorization request. If included, these may be equal
to or a subset of the scopes requested during registration.
If empty, scope will be omitted as a parameter to the authorization endpoint.
),
optional: true

input :udap_authorization_code_request_aud,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to just use a checkbox input or configuration option for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I originally attempted a checkbox but then there would have been two inputs for it: the checkbox input to use/not use the aud param and then the udap_fhir_base_url (originally input in the Discovery group) to get the actual value needed for the request. I thought it may be kind of confusing when running from the authorization and authentication group level (group 1.3), to have an input for the aud param value and then also a checkbox to opt to use it even though the value is already present, as opposed to "if you want to use this parameter, provide an input value for it; if it's empty the parameter won't be used," which felt simpler.

That said, I didn't really love any options I thought of for this input, and am open to cleaner suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be a distinct input from the base url. So I think I would vote for a checkbox plus base url input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the tests to use the checkbox and base url inputs.

title: "Audience ('aud') Parameter for Authorization Request",
description: %(
If included, should be the same as the base FHIR URL. If empty, 'aud' parameter will be omitted as
a parameter to the authorization endpoint.
),
optional: true

output :udap_authorization_code_state
output :udap_authorization_redirect_url

receives_request :redirect

Expand Down Expand Up @@ -59,7 +77,9 @@ def authorization_url_builder(url, params)
'response_type' => 'code',
'client_id' => udap_client_id,
'redirect_uri' => config.options[:redirect_uri],
'state' => udap_authorization_code_state
'state' => udap_authorization_code_state,
'scope' => udap_authorization_code_request_scopes,
'aud' => udap_authorization_code_request_aud
}.compact

authorization_url = authorization_url_builder(
Expand All @@ -69,6 +89,8 @@ def authorization_url_builder(url, params)

info("Inferno redirecting browser to #{authorization_url}.")

output udap_authorization_redirect_url: authorization_url

wait(
identifier: udap_authorization_code_state,
message: wait_message(authorization_url)
Expand Down
123 changes: 86 additions & 37 deletions spec/udap_security_test_kit/authorization_code_redirect_test_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@
}
end

let(:inputs_scope_aud) do
{
udap_authorization_code_request_scopes: 'patient/Condition.rs',
udap_authorization_code_request_aud: 'http://example.com'
}
end

def run(runnable, inputs = {})
test_run_params = { test_session_id: test_session.id }.merge(runnable.reference_hash)
test_run = Inferno::Repositories::TestRuns.new.create(test_run_params)
Expand All @@ -31,54 +38,96 @@ def run(runnable, inputs = {})
Inferno::TestRunner.new(test_session:, test_run:).run(runnable)
end

it 'waits and then passes when it receives a request with the correct state' do
allow(test).to receive(:parent).and_return(Inferno::TestGroup)
result = run(test, inputs)
expect(result.result).to eq('wait')
context "when optional 'scope' and 'aud' inputs are omitted" do
it 'waits and then passes when it receives a request with the correct state' do
allow(test).to receive(:parent).and_return(Inferno::TestGroup)
result = run(test, inputs)
expect(result.result).to eq('wait')

state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
get "/custom/udap_security/redirect?state=#{state}"
state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
get "/custom/udap_security/redirect?state=#{state}"

result = results_repo.find(result.id)
expect(result.result).to eq('pass')
end
result = results_repo.find(result.id)
expect(result.result).to eq('pass')
end

it 'continues to wait when it receives a request with the incorrect state' do
result = run(test, inputs)
expect(result.result).to eq('wait')
it 'continues to wait when it receives a request with the incorrect state' do
result = run(test, inputs)
expect(result.result).to eq('wait')

state = SecureRandom.uuid
get "/custom/smart/redirect?state=#{state}"
state = SecureRandom.uuid
get "/custom/smart/redirect?state=#{state}"

result = results_repo.find(result.id)
expect(result.result).to eq('wait')
end
result = results_repo.find(result.id)
expect(result.result).to eq('wait')
end

it 'fails if the authorization url is invalid' do
inputs[:udap_authorization_endpoint] = 'invalid'
result = run(test, inputs)
expect(result.result).to eq('fail')
expect(result.result_message).to match(/is not a valid URI/)
end
it 'fails if the authorization url is invalid' do
inputs[:udap_authorization_endpoint] = 'invalid'
result = run(test, inputs)
expect(result.result).to eq('fail')
expect(result.result_message).to match(/is not a valid URI/)
end

it "persists the incoming 'redirect' request" do
allow(test).to receive(:parent).and_return(Inferno::TestGroup)
run(test, inputs)
state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
url = "/custom/udap_security/redirect?state=#{state}"
get url

request = requests_repo.find_named_request(test_session.id, 'redirect')
expect(request.url).to end_with(url)
end

it "persists the 'udap_authorization_code_state' output" do
result = run(test, inputs)
expect(result.result).to eq('wait')

it "persists the incoming 'redirect' request" do
allow(test).to receive(:parent).and_return(Inferno::TestGroup)
run(test, inputs)
state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
url = "/custom/udap_security/redirect?state=#{state}"
get url
state = result.result_message.match(/a state of `(.*)`/)[1]
persisted_state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')

request = requests_repo.find_named_request(test_session.id, 'redirect')
expect(request.url).to end_with(url)
expect(persisted_state).to eq(state)
end

it "omits 'scope' and 'aud' values from authorization redirect url" do
result = run(test, inputs)
expect(result.result).to eq('wait')

authorization_url = session_data_repo.load(
test_session_id: test_session.id,
name: 'udap_authorization_redirect_url'
)

expect(authorization_url).to_not include('scope')
expect(authorization_url).to_not include('aud')
end
end

it "persists the 'udap_authorization_code_state' output" do
result = run(test, inputs)
expect(result.result).to eq('wait')
context "when optional 'scope' and 'aud' inputs are provided" do
it 'waits and then passes when it receives a request with the correct state' do
allow(test).to receive(:parent).and_return(Inferno::TestGroup)
result = run(test, inputs.merge(inputs_scope_aud))
expect(result.result).to eq('wait')

state = result.result_message.match(/a state of `(.*)`/)[1]
persisted_state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
state = session_data_repo.load(test_session_id: test_session.id, name: 'udap_authorization_code_state')
get "/custom/udap_security/redirect?state=#{state}"

expect(persisted_state).to eq(state)
result = results_repo.find(result.id)
expect(result.result).to eq('pass')
end

it "includes 'scope' and 'aud' values in authorization redirect url" do
result = run(test, inputs.merge(inputs_scope_aud))
expect(result.result).to eq('wait')

authorization_url = session_data_repo.load(
test_session_id: test_session.id,
name: 'udap_authorization_redirect_url'
)

expect(authorization_url).to include('scope')
expect(authorization_url).to include('aud')
end
end
end
Loading