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.

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

Jump to bottom

Ensure SAML POST requests preserve an existing session #5624

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Ensure SAML POST requests preserve an existing session #5624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions app/controllers/concerns/saml_idp_auth_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ def current_issuer

def request_url
url = URI.parse request.original_url
url.path = remap_auth_post_path(url.path)
query_params = Rack::Utils.parse_nested_query url.query
unless query_params['SAMLRequest']
orig_saml_request = saml_request.options[:get_params][:SAMLRequest]
Expand All @@ -200,4 +201,11 @@ def request_url
url.query = Rack::Utils.build_query(query_params).presence
url.to_s
end

def remap_auth_post_path(path)
path_match = path.match(%r{/api/saml/authpost(?<year>\d{4})})
return path unless path_match.present?

"/api/saml/auth#{path_match[:year]}"
end
end
2 changes: 1 addition & 1 deletion app/controllers/saml_idp_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def identity_needs_decryption?

def capture_analytics
analytics_payload = @result.to_h.merge(
endpoint: request.env['PATH_INFO'],
endpoint: remap_auth_post_path(request.env['PATH_INFO']),
idv: identity_needs_verification?,
finish_profile: profile_needs_verification?,
)
Expand Down
14 changes: 14 additions & 0 deletions app/controllers/saml_post_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class SamlPostController < ApplicationController
after_action -> { request.session_options[:skip] = true }, only: :auth
skip_before_action :verify_authenticity_token
jmhooper marked this conversation as resolved.
Show resolved Hide resolved

def auth
path_year = request.path[-4..-1]
path_method = "api_saml_authpost#{path_year}_url"
action_url = Rails.application.routes.url_helpers.send(path_method)

form_params = params.permit(:SAMLRequest, :RelayState, :SigAlg, :Signature)

render locals: { action_url: action_url, form_params: form_params }, layout: false
end
end
2 changes: 1 addition & 1 deletion app/services/saml_endpoint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def suffix
@suffix ||= begin
suffixes = self.class.endpoint_configs.map { |config| config[:suffix] }
suffixes.find do |suffix|
request.path.match(/(metadata|auth|logout)#{suffix}$/)
request.path.match(/(metadata|auth(post)?|logout)#{suffix}$/)
end
end
end
Expand Down
30 changes: 30 additions & 0 deletions app/views/saml_post/auth.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<!DOCTYPE html>
jmhooper marked this conversation as resolved.
Show resolved Hide resolved
<html>
<head>
<meta charset="utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1" />
<%= csrf_meta_tags %>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= javascript_packs_with_chunks_tag 'saml-post' %>
</head>
<body>
<div class="container tablet:padding-y-6">
<div class="card margin-x-auto sm-col-6">
<h1 class="margin-y-0">
<%= t('saml_idp.shared.saml_post_binding.heading') %>
</h1>

<p>
<%= t('saml_idp.shared.saml_post_binding.no_js') %>
</p>

<%= form_tag action_url, id: 'saml-post-binding' do %>
<% form_params.each do |key, val| %>
<%= hidden_field_tag(key, val) %>
<% end %>
<%= submit_tag t('forms.buttons.submit.default'), class: 'usa-button usa-button--wide usa-button--big' %>
<% end %>
</div>
</div>
</body>
</html>
6 changes: 5 additions & 1 deletion config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
SamlEndpoint.suffixes.each do |suffix|
get "/api/saml/metadata#{suffix}" => 'saml_idp#metadata', format: false
match "/api/saml/logout#{suffix}" => 'saml_idp#logout', via: %i[get post delete]
match "/api/saml/auth#{suffix}" => 'saml_idp#auth', via: %i[get post]
# JS-driven POST redirect route to preserve existing session
post "/api/saml/auth#{suffix}" => 'saml_post#auth'
# actual SAML handling POST route
post "/api/saml/authpost#{suffix}" => 'saml_idp#auth'
get "/api/saml/auth#{suffix}" => 'saml_idp#auth'
end

post '/api/service_provider' => 'service_provider#update'
Expand Down
11 changes: 9 additions & 2 deletions spec/controllers/saml_idp_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,11 +565,18 @@ def name_id_version(format_urn)
ial2: false,
ial2_strict: false,
ialmax: false,
request_url: @stored_request_url,
request_url: @stored_request_url.gsub('authpost', 'auth'),
request_id: sp_request_id,
requested_attributes: ['email'],
)
end

it 'correctly sets the request URL' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmhooper @zachmargolis so this test works (e.g. fails when we don't update the path in the concern) but only when the routes are reordered a little so that the get comes after the post. I think this has to do with the fact that RSpec looks for a match in the routes file but doesn't check the HTTP verb, so if the get comes first it actually sets the request path to /api/saml/auth2021, even though this controller action is handled by /api/saml/authpost2021 when post is used.

post :auth, params: { 'SAMLRequest' => @saml_request }
session_request_url = session[:sp][:request_url]

expect(session_request_url).to match(%r{/api/saml/auth\d{4}})
end
end

context 'service provider is valid' do
Expand All @@ -589,7 +596,7 @@ def name_id_version(format_urn)
ial2: false,
ial2_strict: false,
ialmax: false,
request_url: @saml_request.request.original_url,
request_url: @saml_request.request.original_url.gsub('authpost', 'auth'),
request_id: sp_request_id,
requested_attributes: ['email'],
)
Expand Down
35 changes: 35 additions & 0 deletions spec/controllers/saml_post_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'rails_helper'

describe SamlPostController do
describe 'POST /api/saml/auth' do
render_views
include ActionView::Helpers::FormTagHelper

let(:form_action_regex) { /<form.+action=".+\/api\/saml\/authpost\d{4}.+"/ }
let(:saml_request) { 'abc123' }
let(:relay_state) { 'def456' }
let(:sig_alg) { 'aes256' }
let(:signature) { 'xyz789' }

it 'renders the appropriate form' do
post :auth, params: {
'SAMLRequest' => saml_request,
'RelayState' => relay_state,
'SigAlg' => sig_alg,
'Signature' => signature,
}

expect(response.body).to match(form_action_regex)
expect(response.body).to match(hidden_field_tag('SAMLRequest', saml_request))
expect(response.body).to match(hidden_field_tag('RelayState', relay_state))
expect(response.body).to match(hidden_field_tag('SigAlg', sig_alg))
expect(response.body).to match(hidden_field_tag('Signature', signature))
end

it 'does not render extra parameters' do
post :auth, params: { 'Foo' => 'bar' }

expect(response.body).not_to match(hidden_field_tag('Foo', 'bar'))
end
end
end
18 changes: 18 additions & 0 deletions spec/requests/saml_post_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
require 'rails_helper'

RSpec.describe 'SAML POST handling', type: :request do
include SamlAuthHelper

describe 'POST /api/saml/auth' do
let(:cookie_regex) { /\A(?<cookie>\w+)=/ }

it 'does not set a session cookie' do
post saml_settings.idp_sso_target_url
new_cookies = response.header['Set-Cookie'].split("\n").map do |c|
cookie_regex.match(c)[:cookie]
end

expect(new_cookies).not_to include('_upaya_session')
end
end
end
4 changes: 2 additions & 2 deletions spec/support/saml_auth_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ def authn_request_post_params(settings = saml_settings, params = {})

def post_saml_authn_request(settings = saml_settings, params = {})
saml_authn_params = authn_request_post_params(settings, params)
response = page.driver.post(saml_settings.idp_sso_target_url, saml_authn_params)
visit response.location
page.driver.post(saml_settings.idp_sso_target_url, saml_authn_params)
click_button(t('forms.buttons.submit.default'))
end

def login_and_confirm_sp(user)
Expand Down