Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ gem 'rqrcode'
gem 'ruby-progressbar'
gem 'ruby-saml'
gem 'safe_target_blank', '>= 1.0.2'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.16.0-18f'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.17.0-18f'
gem 'scrypt'
gem 'simple_form', '>= 5.0.2'
gem 'stringex', require: false
Expand Down
8 changes: 4 additions & 4 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ GIT

GIT
remote: https://github.com/18F/saml_idp.git
revision: f82bd9cd1682d1645abe98e1fe5e6261f53a8279
tag: 0.16.0-18f
revision: f10c8ba1b4e10ba983a79b1d0fd39cadca95a728
tag: 0.17.0-18f
specs:
saml_idp (0.16.0.pre.18f)
saml_idp (0.17.0.pre.18f)
activesupport
builder
faraday
Expand Down Expand Up @@ -408,7 +408,7 @@ GEM
pg_query (2.1.3)
google-protobuf (>= 3.19.2)
phonelib (0.6.54)
pkcs11 (0.3.3)
pkcs11 (0.3.4)
premailer (1.15.0)
addressable
css_parser (>= 1.6.0)
Expand Down
6 changes: 4 additions & 2 deletions app/services/saml_endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ def x509_certificate

def saml_metadata
config = SamlIdp.config.dup
config.single_service_post_location = config.single_service_post_location + suffix
config.single_logout_service_post_location = config.single_logout_service_post_location + suffix
config.single_service_post_location += suffix
config.single_logout_service_post_location += suffix
config.remote_logout_service_post_location += suffix

SamlIdp::MetadataBuilder.new(
config,
x509_certificate,
Expand Down
1 change: 1 addition & 0 deletions config/initializers/saml_idp.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
config.attribute_service_location = "#{api_base}/saml/attributes"
config.single_service_post_location = "#{api_base}/saml/auth"
config.single_logout_service_post_location = "#{api_base}/saml/logout"
config.remote_logout_service_post_location = "#{api_base}/saml/remotelogout"

# Name ID
config.name_id.formats =
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
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/remotelogout#{suffix}" => 'saml_idp#remotelogout', via: %i[get post delete]
post "/api/saml/remotelogout#{suffix}" => 'saml_idp#remotelogout'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the other actions since we really only support the POST binding and not the Redirect binding.

# JS-driven POST redirect route to preserve existing session
post "/api/saml/auth#{suffix}" => 'saml_post#auth'
# actual SAML handling POST route
Expand Down
12 changes: 6 additions & 6 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
result = { service_provider: nil, saml_request_valid: false }
expect(@analytics).to receive(:track_event).with('Remote Logout initiated', result)

delete :remotelogout, params: { SAMLRequest: 'foo' }
post :remotelogout, params: { SAMLRequest: 'foo' }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This actually wasn't necessary to keep the tests passing so I also added request specs to ensure that we're only permitting POST requests.

end

let(:agency) { create(:agency) }
Expand Down Expand Up @@ -240,7 +240,7 @@
),
).to eq(true)

delete :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))
post :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))

expect(response).to be_ok
expect(session_accessor.load).to be_empty
Expand Down Expand Up @@ -277,7 +277,7 @@
),
).to eq(true)

delete :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))
post :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))

expect(response).to be_bad_request
end
Expand Down Expand Up @@ -308,7 +308,7 @@
),
).to eq(true)

delete :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))
post :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))

expect(response).to be_bad_request
end
Expand Down Expand Up @@ -339,13 +339,13 @@
),
).to eq(true)

delete :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))
post :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))

expect(response).to be_bad_request
end

it 'rejects requests from a wrong cert' do
delete :remotelogout, params: UriService.params(
post :remotelogout, params: UriService.params(
OneLogin::RubySaml::Logoutrequest.new.create(wrong_cert_settings),
)

Expand Down
19 changes: 16 additions & 3 deletions spec/features/saml/multiple_endpoints_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,30 @@
expect(cert_base64).to eq(Base64.strict_encode64(endpoint_cert.to_der))
end

it 'includes the correct auth url, and no SingleLogoutService urls' do
it 'includes the correct auth url' do
visit endpoint_metadata_path
document = REXML::Document.new(page.html)
auth_node = REXML::XPath.first(document, '//SingleSignOnService')
logout_node = REXML::XPath.first(document, '//SingleLogoutService')

expect(auth_node.attributes['Location']).to include(
['/api/saml/auth', endpoint_suffix].join(''),
)
end

expect(logout_node).to be_nil
it 'includes the front-channel logout url' do
visit endpoint_metadata_path
document = REXML::Document.new(page.html)
logout_nodes = REXML::XPath.match(document, '//SingleLogoutService')
expect(logout_nodes.count { |n| n['Location'].match?(%r{/api/saml/logout\d{4}}) }).
to eq(2)
end

it 'includes the remote logout url' do
visit endpoint_metadata_path
document = REXML::Document.new(page.html)
logout_nodes = REXML::XPath.match(document, '//SingleLogoutService')
expect(logout_nodes.count { |n| n['Location'].match?(%r{/api/saml/remotelogout\d{4}}) }).
to eq(1)
end
end
end
18 changes: 0 additions & 18 deletions spec/requests/saml_post_spec.rb

This file was deleted.

40 changes: 40 additions & 0 deletions spec/requests/saml_requests_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require 'rails_helper'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed this file, the first describe block is old.


RSpec.describe 'SAML requests', 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

describe '/api/saml/remotelogout' do
let(:remote_slo_url) do
saml_settings.idp_slo_target_url.gsub('logout', 'remotelogout')
end

it 'does not accept GET requests' do
get remote_slo_url
expect(response.status).to eq(404)
end

it 'does not accept DELETE requests' do
delete remote_slo_url
expect(response.status).to eq(404)
end

it 'accepts POST requests' do
post remote_slo_url
# fails (:bad_request) without SAMLRequest param but not 404
expect(response.status).to eq(400)
end
end
end
3 changes: 3 additions & 0 deletions spec/services/saml_endpoint_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@
expect(result.configurator.single_logout_service_post_location).to match(
%r{api/saml/logout2022\Z},
)
expect(result.configurator.remote_logout_service_post_location).to match(
%r{api/saml/remotelogout2022\Z},
)
end
end
end