From 15d13bff49a91b82af53cf3f6c0374bbe2ac9c86 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Mon, 14 Jan 2019 16:08:17 -0700 Subject: [PATCH] rip out ruby-saml-mod gem Change-Id: Id6c71716a3060747b817e7c031487746aa170cc7 Reviewed-on: https://gerrit.instructure.com/177869 Tested-by: Jenkins QA-Review: Tucker Mcknight Product-Review: Cody Cutrer Reviewed-by: Rob Orton --- Gemfile.d/app.rb | 1 - app/controllers/login/saml_controller.rb | 305 +++++------------- app/models/authentication_provider/saml.rb | 60 +--- .../authentication_providers_presenter.rb | 4 +- config/initializers/saml.rb | 4 +- ...035651_add_saml_requested_authn_context.rb | 2 +- lib/external_auth_observation/saml.rb | 53 ++- spec/apis/auth_spec.rb | 10 +- .../controllers/login/saml_controller_spec.rb | 114 +------ .../authentication_provider/saml_spec.rb | 62 ---- ...authentication_providers_presenter_spec.rb | 14 +- 11 files changed, 137 insertions(+), 492 deletions(-) diff --git a/Gemfile.d/app.rb b/Gemfile.d/app.rb index ad4d180a1a0..5001639adaf 100644 --- a/Gemfile.d/app.rb +++ b/Gemfile.d/app.rb @@ -112,7 +112,6 @@ gem 'ritex', '1.0.1', require: false gem 'rotp', '3.3.1', require: false gem 'net-ldap', '0.16.1', require: false gem 'ruby-duration', '3.2.3', require: false -gem 'ruby-saml-mod', '0.3.8' gem 'saml2', '3.0.1' gem 'nokogiri-xmlsec-instructure', '0.9.6', require: false gem 'rubycas-client', '2.3.9', require: false diff --git a/app/controllers/login/saml_controller.rb b/app/controllers/login/saml_controller.rb index aaf696edd90..3fcf8223d1c 100644 --- a/app/controllers/login/saml_controller.rb +++ b/app/controllers/login/saml_controller.rb @@ -27,7 +27,6 @@ class Login::SamlController < ApplicationController def new increment_saml_stat("login_attempt") - session[:saml2_processing] = false if Canvas::Plugin.value_to_boolean(params[:saml2_processing], ignore_unrecognized: true) == false redirect_to delegated_auth_redirect_uri(aac.generate_authn_request_redirect(host: request.host_with_port, parent_registration: session[:parent_registration], relay_state: Rails.env.development? && params[:RelayState])) @@ -37,25 +36,20 @@ class Login::SamlController < ApplicationController login_error_message = t("There was a problem logging in at %{institution}", institution: @domain_root_account.display_name) - saml2_processing = true - saml2_processing = false if session[:saml2_processing] == false - saml2_processing = false if @domain_root_account.settings[:process_saml_responses_with_saml2] == false - - legacy_response = Onelogin::Saml::Response.new(params[:SAMLResponse]) response, relay_state = SAML2::Bindings::HTTP_POST.decode(request.request_parameters) increment_saml_stat('login_response_received') aac = @domain_root_account.authentication_providers.active. where(auth_type: 'saml'). - where(idp_entity_id: legacy_response.issuer). + where(idp_entity_id: response.issuer&.id). first if aac.nil? - logger.error "Attempted SAML login for #{legacy_response.issuer} on account without that IdP" + logger.error "Attempted SAML login for #{response.issuer&.id} on account without that IdP" flash[:delegated_message] = if @domain_root_account.auth_discovery_url t("Canvas did not recognize your identity provider") elsif response.issuer - t("Canvas is not configured to receive logins from %{issuer}.", issuer: legacy_response.issuer) + t("Canvas is not configured to receive logins from %{issuer}.", issuer: response.issuer.id) else t("The institution you logged in from is not configured on this account.") end @@ -71,142 +65,26 @@ class Login::SamlController < ApplicationController end encrypted_xml = response.to_s if debugging - settings = aac.saml_settings(request.host_with_port) - aac.sp_metadata(request.host_with_port).valid_response?(response, aac.idp_metadata, ignore_audience_condition: aac.settings['ignore_audience_condition']) - legacy_response.process(settings) unless saml2_processing if debugging aac.debug_set(:debugging, t('debug.redirect_from_idp', "Received LoginResponse from IdP")) aac.debug_set(:idp_response_encoded, params[:SAMLResponse]) - aac.debug_set(:idp_response_xml_encrypted, saml2_processing ? encrypted_xml : legacy_response.xml) - aac.debug_set(:idp_response_xml_decrypted, saml2_processing ? response.to_s : legacy_response.decrypted_document.to_s) - aac.debug_set(:idp_in_response_to, saml2_processing ? response.try(:in_response_to) : legacy_response.in_response_to) - aac.debug_set(:idp_login_destination, saml2_processing ? response.destination : legacy_response.destination) + aac.debug_set(:idp_response_xml_encrypted, encrypted_xml) + aac.debug_set(:idp_response_xml_decrypted, response.to_s) + aac.debug_set(:idp_in_response_to, response.try(:in_response_to)) + aac.debug_set(:idp_login_destination, response.destination) aac.debug_set(:login_to_canvas_success, 'false') - unless saml2_processing - aac.debug_set(:fingerprint_from_idp, legacy_response.fingerprint_from_idp) - end end - if !saml2_processing && legacy_response.is_valid? && !response.errors.empty? - logger.warn("Response valid via legacy SAML processing from #{legacy_response.issuer}, but invalid according to SAML2 processing: #{response.errors.join("\n")}") - unless aac.settings['first_saml_error'] - aac.settings['first_saml_error'] = response.errors.join("\n") - aac.save! - end - end - - if saml2_processing - # yes, a lot of this is duplicated from below, but there are also a lot of changes, - # and it's easier to not interweave them so the legacy code can be easily stripped - # in the future - assertion = response.assertions.first - # yes, they could be _that_ busted that we put a dangling rescue here. - provider_attributes = assertion&.attribute_statements&.first&.to_h || {} rescue {} - subject_name_id = assertion&.subject&.name_id - unique_id = if aac.login_attribute == 'NameID' - subject_name_id&.id - else - provider_attributes[aac.login_attribute] - end - if unique_id && aac.strip_domain_from_login_attribute? - unique_id = unique_id.split('@', 2)[0] - end - - logger.info "Attempting SAML2 login for #{aac.login_attribute} #{unique_id} in account #{@domain_root_account.id}" - - unless response.errors.empty? - increment_saml_stat("errors.invalid_response") - if debugging - aac.debug_set(:is_valid_login_response, 'false') - aac.debug_set(:login_response_validation_error, response.errors.join("\n")) - end - logger.error "Failed to verify SAML signature: #{response.errors.join("\n")}" - flash[:delegated_message] = login_error_message - return redirect_to login_url - end - - aac.debug_set(:is_valid_login_response, 'true') if debugging - - # for parent using self-registration to observe a student - # the student is logged out after validation - # and registration process resumed - if session[:parent_registration] - expected_unique_id = session[:parent_registration][:observee][:unique_id] - session[:parent_registration][:unique_id_match] = (expected_unique_id == unique_id) - saml = ExternalAuthObservation::Saml.new(@domain_root_account, request, response) - redirect_to saml.logout_url - return - end - - reset_session_for_login - - pseudonym = @domain_root_account.pseudonyms.for_auth_configuration(unique_id, aac) - if !pseudonym && aac.jit_provisioning? - pseudonym = aac.provision_user(unique_id, provider_attributes) - elsif pseudonym - aac.apply_federated_attributes(pseudonym, provider_attributes) - end - - if pseudonym - # Successful login and we have a user - @domain_root_account.pseudonym_sessions.create!(pseudonym, false) - user = pseudonym.login_assertions_for_user - - if debugging - aac.debug_set(:login_to_canvas_success, 'true') - aac.debug_set(:logged_in_user_id, user.id) - end - increment_saml_stat("normal.login_success") - - session[:saml_unique_id] = unique_id - session[:name_id] = subject_name_id&.id - session[:name_identifier_format] = subject_name_id&.format - session[:name_qualifier] = subject_name_id&.name_qualifier - session[:sp_name_qualifier] = subject_name_id&.sp_name_qualifier - session[:session_index] = assertion.authn_statements.first&.session_index - session[:login_aac] = aac.id - - if relay_state.present? && (uri = URI.parse(relay_state) rescue nil) - if uri.host - # allow relay_state's to other (trusted) domains, by tacking on a session token - target_account = Account.find_by_domain(uri.host) - if target_account && - target_account != @domain_root_account && - pseudonym.works_for_account?(target_account, true) - token = SessionToken.new(pseudonym.global_id, - current_user_id: pseudonym.global_user_id).to_s - uri.query.concat('&') if uri.query - uri.query ||= '' - uri.query.concat("session_token=#{token}") - session[:return_to] = uri.to_s - end - elsif uri.path[0] == '/' - # otherwise, absolute paths on the same domain are okay - session[:return_to] = relay_state - end - end - successful_login(user, pseudonym) - else - unknown_user_url = @domain_root_account.unknown_user_url.presence || login_url - increment_saml_stat("errors.unknown_user") - message = "Received SAML login request for unknown user: #{unique_id} redirecting to: #{unknown_user_url}." - logger.warn message - aac.debug_set(:canvas_login_fail_message, message) if debugging - flash[:delegated_message] = t("Canvas doesn't have an account for user: %{user}", - user: unique_id) - redirect_to unknown_user_url - end - - return - end - - provider_attributes = legacy_response.saml_attributes + assertion = response.assertions.first + # yes, they could be _that_ busted that we put a dangling rescue here. + provider_attributes = assertion&.attribute_statements&.first&.to_h || {} rescue {} + subject_name_id = assertion&.subject&.name_id unique_id = if aac.login_attribute == 'NameID' - legacy_response.name_id + subject_name_id&.id else provider_attributes[aac.login_attribute] end @@ -214,94 +92,89 @@ class Login::SamlController < ApplicationController unique_id = unique_id.split('@', 2)[0] end - logger.info "Attempting SAML login for #{aac.login_attribute} #{unique_id} in account #{@domain_root_account.id}" + logger.info "Attempting SAML2 login for #{aac.login_attribute} #{unique_id} in account #{@domain_root_account.id}" - if legacy_response.is_valid? - aac.debug_set(:is_valid_login_response, 'true') if debugging - - if legacy_response.success_status? - # for parent using self-registration to observe a student - # the student is logged out after validation - # and registration process resumed - if session[:parent_registration] - expected_unique_id = session[:parent_registration][:observee][:unique_id] - session[:parent_registration][:unique_id_match] = (expected_unique_id == unique_id) - saml = ExternalAuthObservation::Saml.new(@domain_root_account, request, legacy_response) - redirect_to saml.logout_url - return - end - - reset_session_for_login - - pseudonym = @domain_root_account.pseudonyms.for_auth_configuration(unique_id, aac) - if !pseudonym && aac.jit_provisioning? - pseudonym = aac.provision_user(unique_id, provider_attributes) - elsif pseudonym - aac.apply_federated_attributes(pseudonym, provider_attributes) - end - - if pseudonym - # Successful login and we have a user - @domain_root_account.pseudonym_sessions.create!(pseudonym, false) - user = pseudonym.login_assertions_for_user - - if debugging - aac.debug_set(:login_to_canvas_success, 'true') - aac.debug_set(:logged_in_user_id, user.id) - end - increment_saml_stat("normal.login_success") - - session[:saml_unique_id] = unique_id - session[:name_id] = legacy_response.name_id - session[:name_identifier_format] = legacy_response.name_identifier_format - session[:name_qualifier] = legacy_response.name_qualifier - session[:sp_name_qualifier] = legacy_response.sp_name_qualifier - session[:session_index] = legacy_response.session_index - session[:return_to] = params[:RelayState] if params[:RelayState] && params[:RelayState] =~ /\A\/(\z|[^\/])/ - session[:login_aac] = aac.id - - successful_login(user, pseudonym) - else - unknown_user_url = @domain_root_account.unknown_user_url.presence || login_url - increment_saml_stat("errors.unknown_user") - message = "Received SAML login request for unknown user: #{unique_id} redirecting to: #{unknown_user_url}." - logger.warn message - aac.debug_set(:canvas_login_fail_message, message) if debugging - flash[:delegated_message] = t("Canvas doesn't have an account for user: %{user}", - user: unique_id) - redirect_to unknown_user_url - end - elsif legacy_response.auth_failure? - increment_saml_stat("normal.login_failure") - message = "Failed to log in correctly at IdP" - logger.warn message - aac.debug_set(:canvas_login_fail_message, message) if debugging - flash[:delegated_message] = login_error_message - redirect_to login_url - elsif legacy_response.no_authn_context? - increment_saml_stat("errors.no_authn_context") - message = "Attempted SAML login for unsupported authn_context at IdP." - logger.warn message - aac.debug_set(:canvas_login_fail_message, message) if debugging - flash[:delegated_message] = login_error_message - redirect_to login_url - else - increment_saml_stat("errors.unexpected_response_status") - message = "Unexpected SAML status code - status code: #{legacy_response.status_code || ''} - Status Message: #{legacy_response.status_message || ''}" - logger.warn message - aac.debug_set(:canvas_login_fail_message, message) if debugging - flash[:delegated_message] = login_error_message - redirect_to login_url - end - else + unless response.errors.empty? increment_saml_stat("errors.invalid_response") if debugging aac.debug_set(:is_valid_login_response, 'false') - aac.debug_set(:login_response_validation_error, legacy_response.validation_error) + aac.debug_set(:login_response_validation_error, response.errors.join("\n")) end - logger.error "Failed to verify SAML signature: #{legacy_response.validation_error}" + logger.error "Failed to verify SAML signature: #{response.errors.join("\n")}" flash[:delegated_message] = login_error_message - redirect_to login_url + return redirect_to login_url + end + + aac.debug_set(:is_valid_login_response, 'true') if debugging + + # for parent using self-registration to observe a student + # the student is logged out after validation + # and registration process resumed + if session[:parent_registration] + expected_unique_id = session[:parent_registration][:observee][:unique_id] + session[:parent_registration][:unique_id_match] = (expected_unique_id == unique_id) + saml = ExternalAuthObservation::Saml.new(@domain_root_account, request, response) + redirect_to saml.logout_url + return + end + + reset_session_for_login + + pseudonym = @domain_root_account.pseudonyms.for_auth_configuration(unique_id, aac) + if !pseudonym && aac.jit_provisioning? + pseudonym = aac.provision_user(unique_id, provider_attributes) + elsif pseudonym + aac.apply_federated_attributes(pseudonym, provider_attributes) + end + + if pseudonym + # Successful login and we have a user + @domain_root_account.pseudonym_sessions.create!(pseudonym, false) + user = pseudonym.login_assertions_for_user + + if debugging + aac.debug_set(:login_to_canvas_success, 'true') + aac.debug_set(:logged_in_user_id, user.id) + end + increment_saml_stat("normal.login_success") + + session[:saml_unique_id] = unique_id + session[:name_id] = subject_name_id&.id + session[:name_identifier_format] = subject_name_id&.format + session[:name_qualifier] = subject_name_id&.name_qualifier + session[:sp_name_qualifier] = subject_name_id&.sp_name_qualifier + session[:session_index] = assertion.authn_statements.first&.session_index + session[:login_aac] = aac.id + + if relay_state.present? && (uri = URI.parse(relay_state) rescue nil) + if uri.host + # allow relay_state's to other (trusted) domains, by tacking on a session token + target_account = Account.find_by_domain(uri.host) + if target_account && + target_account != @domain_root_account && + pseudonym.works_for_account?(target_account, true) + token = SessionToken.new(pseudonym.global_id, + current_user_id: pseudonym.global_user_id).to_s + uri.query.concat('&') if uri.query + uri.query ||= '' + uri.query.concat("session_token=#{token}") + session[:return_to] = uri.to_s + end + elsif uri.path[0] == '/' + # otherwise, absolute paths on the same domain are okay + session[:return_to] = relay_state + end + end + successful_login(user, pseudonym) + else + unknown_user_url = @domain_root_account.unknown_user_url.presence || login_url + increment_saml_stat("errors.unknown_user") + message = "Received SAML login request for unknown user: #{unique_id} redirecting to: #{unknown_user_url}." + logger.warn message + aac.debug_set(:canvas_login_fail_message, message) if debugging + flash[:delegated_message] = t("Canvas doesn't have an account for user: %{user}", + user: unique_id) + redirect_to unknown_user_url end end diff --git a/app/models/authentication_provider/saml.rb b/app/models/authentication_provider/saml.rb index 0ebdd6c316d..635355b3d26 100644 --- a/app/models/authentication_provider/saml.rb +++ b/app/models/authentication_provider/saml.rb @@ -24,14 +24,7 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated end def self.enabled?(_account = nil) - @enabled - end - - begin - require 'onelogin/saml' - @enabled = true - rescue LoadError - @enabled = false + true end def self.recognized_params @@ -84,7 +77,6 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated }, { idp_in_response_to: -> { t("IdP InResponseTo") }, idp_login_destination: -> { t("IdP LoginResponse destination") }, - fingerprint_from_idp: -> { t("IdP certificate fingerprint") }, is_valid_login_response: -> { t("Canvas thinks response is valid") }, login_response_validation_error: -> { t("Validation Error") }, login_to_canvas_success: -> { t("User succesfully logged into Canvas") }, @@ -223,6 +215,10 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated settings['sig_alg'] = value end + def self.name_id_formats + SAML2::NameID::Format.constants.map { |const| SAML2::NameID::Format.const_get(const, false) }.sort_by(&:downcase) + end + def populate_from_metadata(entity) idps = entity.identity_providers raise "Must provide exactly one IDPSSODescriptor; found #{idps.length}" unless idps.length == 1 @@ -231,7 +227,7 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated self.log_in_url = idp.single_sign_on_services.find { |ep| ep.binding == SAML2::Bindings::HTTPRedirect::URN }.try(:location) self.log_out_url = idp.single_logout_services.find { |ep| ep.binding == SAML2::Bindings::HTTPRedirect::URN }.try(:location) self.certificate_fingerprint = idp.signing_keys.map(&:fingerprint).join(' ').presence || idp.keys.first&.fingerprint - self.identifier_format = (idp.name_id_formats & Onelogin::Saml::NameIdentifiers::ALL_IDENTIFIERS).first + self.identifier_format = (idp.name_id_formats & self.class.name_id_formats).first self.settings[:signing_certificates] = idp.signing_keys.map(&:x509) case idp.want_authn_requests_signed? when true @@ -266,23 +262,6 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated end end - def saml_settings(current_host=nil) - return nil unless self.auth_type == 'saml' - - unless @saml_settings - @saml_settings = self.class.onelogin_saml_settings_for_account(self.account, current_host) - - @saml_settings.idp_sso_target_url = self.log_in_url - @saml_settings.idp_slo_target_url = self.log_out_url - @saml_settings.idp_cert_fingerprint = (certificate_fingerprint || '').split.presence - @saml_settings.name_identifier_format = self.identifier_format - @saml_settings.requested_authn_context = self.requested_authn_context - @saml_settings.logger = logger - end - - @saml_settings - end - # construct a metadata doc to represent the IdP # TODO: eventually store the actual metadata we got from the IdP def idp_metadata @@ -420,33 +399,6 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated remove_instance_variable(:@key) if instance_variable_defined?(:@key) end - def self.onelogin_saml_settings_for_account(account, current_host=nil) - app_config = ConfigFile.load('saml') || {} - domains = HostUrl.context_hosts(account, current_host) - - settings = Onelogin::Saml::Settings.new - settings.sp_slo_url = "#{HostUrl.protocol}://#{domains.first}/login/saml/logout" - settings.assertion_consumer_service_url = domains.flat_map do |domain| - [ - "#{HostUrl.protocol}://#{domain}/login/saml" - ] - end - settings.tech_contact_name = app_config[:tech_contact_name] || 'Webmaster' - settings.tech_contact_email = app_config[:tech_contact_email] || '' - - settings.issuer = saml_default_entity_id_for_account(account) - - encryption = app_config[:encryption] - if encryption.is_a?(Hash) - settings.xmlsec_certificate = resolve_saml_key_path(Array.wrap(encryption[:certificate]).first) - settings.xmlsec_privatekey = resolve_saml_key_path(encryption[:private_key]) - - settings.xmlsec_additional_privatekeys = Array(encryption[:additional_private_keys]).map { |apk| resolve_saml_key_path(apk) }.compact - end - - settings - end - def self.resolve_saml_key_path(path) return nil unless path diff --git a/app/presenters/authentication_providers_presenter.rb b/app/presenters/authentication_providers_presenter.rb index 3d40b17c658..954021c3353 100644 --- a/app/presenters/authentication_providers_presenter.rb +++ b/app/presenters/authentication_providers_presenter.rb @@ -100,14 +100,14 @@ class AuthenticationProvidersPresenter def saml_identifiers return [] unless saml_enabled? - Onelogin::Saml::NameIdentifiers::ALL_IDENTIFIERS + AuthenticationProvider::SAML.name_id_formats end def login_attribute_for(config) saml_login_attributes.invert[config.login_attribute] end - def saml_authn_contexts(base = Onelogin::Saml::AuthnContexts::ALL_CONTEXTS) + def saml_authn_contexts(base = SAML2::AuthnStatement::Classes.constants.map { |const| SAML2::AuthnStatement::Classes.const_get(const, false) }) return [] unless saml_enabled? [["No Value", nil]] + base.sort end diff --git a/config/initializers/saml.rb b/config/initializers/saml.rb index 1acdb9076f1..c82717eb2a4 100644 --- a/config/initializers/saml.rb +++ b/config/initializers/saml.rb @@ -17,8 +17,6 @@ Rails.configuration.to_prepare do require 'saml2' - require 'onelogin/saml' - Onelogin::Saml.config[:max_message_size] = - SAML2.config[:max_message_size] = 1.megabyte + SAML2.config[:max_message_size] = 1.megabyte end diff --git a/db/migrate/20120127035651_add_saml_requested_authn_context.rb b/db/migrate/20120127035651_add_saml_requested_authn_context.rb index c4e1eed5d49..8a7a527ce32 100644 --- a/db/migrate/20120127035651_add_saml_requested_authn_context.rb +++ b/db/migrate/20120127035651_add_saml_requested_authn_context.rb @@ -27,7 +27,7 @@ class AddSamlRequestedAuthnContext < ActiveRecord::Migration[4.2] AuthenticationProvider.where(auth_type: "saml").each do |aac| # This was the hard-coded value before - aac.requested_authn_context = Onelogin::Saml::AuthnContexts::PASSWORD_PROTECTED_TRANSPORT + aac.requested_authn_context = SAML2::AuthnStatement::Classes::PASSWORD_PROTECTED_TRANSPORT aac.save! end AuthenticationProvider.reset_column_information diff --git a/lib/external_auth_observation/saml.rb b/lib/external_auth_observation/saml.rb index c79c336b841..2bc2fcf4ee7 100644 --- a/lib/external_auth_observation/saml.rb +++ b/lib/external_auth_observation/saml.rb @@ -17,50 +17,35 @@ module ExternalAuthObservation class Saml - attr_accessor :request, :response, :saml_settings, :account_auth_config + attr_accessor :request, :response, :account_auth_config def initialize(account, request, response) @request = request @response = response @account_auth_config = account.authentication_providers.where(parent_registration: true).first - @saml_settings = account_auth_config.saml_settings(request.host_with_port) end def logout_url - if response.is_a?(SAML2::Response) - aac = @account_auth_config - idp = aac.idp_metadata.identity_providers.first - name_id = response.assertions.first.subject.name_id + aac = @account_auth_config + idp = aac.idp_metadata.identity_providers.first + name_id = response.assertions.first.subject.name_id - logout_request = SAML2::LogoutRequest.initiate( - idp, - SAML2::NameID.new(aac.entity_id), - SAML2::NameID.new(name_id.id, - name_id.format, - name_qualifier: name_id.name_qualifier, - sp_name_qualifier: name_id.sp_name_qualifier), - response.assertions.first.authn_statements.first&.session_index - ) + logout_request = SAML2::LogoutRequest.initiate( + idp, + SAML2::NameID.new(aac.entity_id), + SAML2::NameID.new(name_id.id, + name_id.format, + name_qualifier: name_id.name_qualifier, + sp_name_qualifier: name_id.sp_name_qualifier), + response.assertions.first.authn_statements.first&.session_index + ) - # sign the request - private_key = AuthenticationProvider::SAML.private_key - private_key = nil if aac.sig_alg.nil? - SAML2::Bindings::HTTPRedirect.encode(logout_request, - private_key: private_key, - sig_alg: aac.sig_alg) - else - saml_request = Onelogin::Saml::LogoutRequest.generate( - response.name_qualifier, - response.sp_name_qualifier, - response.name_id, - response.name_identifier_format, - response.session_index, - saml_settings - ) - forward_url = saml_request.forward_url - uri = URI(forward_url) - uri.to_s - end + # sign the request + private_key = AuthenticationProvider::SAML.private_key + private_key = nil if aac.sig_alg.nil? + SAML2::Bindings::HTTPRedirect.encode(logout_request, + private_key: private_key, + sig_alg: aac.sig_alg) end end end diff --git a/spec/apis/auth_spec.rb b/spec/apis/auth_spec.rb index 1de568a61aa..d80682f5d51 100644 --- a/spec/apis/auth_spec.rb +++ b/spec/apis/auth_spec.rb @@ -194,16 +194,8 @@ describe "API Authentication", type: :request do skip("requires SAML extension") unless AuthenticationProvider::SAML.enabled? account_with_saml(account: Account.default) flow do - allow_any_instance_of(Onelogin::Saml::Response).to receive(:settings=) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:logger=) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:is_valid?).and_return(true) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:success_status?).and_return(true) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:name_id).and_return('test1@example.com') - allow_any_instance_of(Onelogin::Saml::Response).to receive(:name_qualifier).and_return(nil) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:session_index).and_return(nil) - allow_any_instance_of(Onelogin::Saml::Response).to receive(:issuer).and_return("saml_entity") - allow_any_instance_of(Onelogin::Saml::Response).to receive(:trusted_roots).and_return([]) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new('test1@example.com') diff --git a/spec/controllers/login/saml_controller_spec.rb b/spec/controllers/login/saml_controller_spec.rb index 84597363bb7..84193c0abef 100644 --- a/spec/controllers/login/saml_controller_spec.rb +++ b/spec/controllers/login/saml_controller_spec.rb @@ -36,12 +36,8 @@ describe Login::SamlController do @pseudonym.account = account2 @pseudonym.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', - issuer: "saml_entity", - ) - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) @@ -78,21 +74,6 @@ describe Login::SamlController do @pseudonym.account = account1 @pseudonym.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', - is_valid?: true, - success_status?: true, - name_id: unique_id, - name_identifier_format: nil, - name_qualifier: nil, - sp_name_qualifier: nil, - session_index: nil, - process: nil, - issuer: "such a lie", - saml_attributes: {}, - used_key: nil - ) - ) allow(SAML2::Bindings::HTTP_POST).to receive(:decode).and_return( [double('response2', errors: [], @@ -111,10 +92,8 @@ describe Login::SamlController do account = account_with_saml - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) @@ -160,10 +139,8 @@ describe Login::SamlController do ap.federated_attributes = { 'display_name' => 'eduPersonNickname' } ap.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) @@ -194,10 +171,8 @@ describe Login::SamlController do ap.federated_attributes = { 'display_name' => 'eduPersonNickname' } ap.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@pseudonym.unique_id) @@ -224,10 +199,8 @@ describe Login::SamlController do @pseudonym.account = account1 @pseudonym.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity" ) - ) saml_response = SAML2::Response.new + saml_response.issuer = SAML2::NameID.new('saml_entity') saml_response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) @@ -247,10 +220,8 @@ describe Login::SamlController do account = account_with_saml user_with_pseudonym(active_all: 1, account: account) - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) saml_response = SAML2::Response.new + saml_response.issuer = SAML2::NameID.new('saml_entity') saml_response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@pseudonym.unique_id) @@ -274,10 +245,8 @@ describe Login::SamlController do account = account_with_saml user_with_pseudonym(active_all: 1, account: account) - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) saml_response = SAML2::Response.new + saml_response.issuer = SAML2::NameID.new('saml_entity') saml_response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@pseudonym.unique_id) @@ -298,10 +267,8 @@ describe Login::SamlController do account = account_with_saml user_with_pseudonym(active_all: 1, account: account) - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) saml_response = SAML2::Response.new + saml_response.issuer = SAML2::NameID.new('saml_entity') saml_response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@pseudonym.unique_id) @@ -322,10 +289,8 @@ describe Login::SamlController do account = account_with_saml user_with_pseudonym(active_all: 1, account: account) - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) saml_response = SAML2::Response.new + saml_response.issuer = SAML2::NameID.new('saml_entity') saml_response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@pseudonym.unique_id) @@ -356,20 +321,6 @@ describe Login::SamlController do @aac2.log_in_url = "https://example.com/idp1/sso" @aac2.log_out_url = "https://example.com/idp1/slo" @aac2.save! - - @stub_hash = { - issuer: @aac2.idp_entity_id, - is_valid?: true, - success_status?: true, - name_id: @unique_id, - name_identifier_format: nil, - name_qualifier: nil, - sp_name_qualifier: nil, - session_index: nil, - process: nil, - saml_attributes: {}, - used_key: nil - } end it "sends the AuthnRequest by entity id" do @@ -380,8 +331,8 @@ describe Login::SamlController do end it "should saml_consume login with multiple authorization configs" do - allow(Onelogin::Saml::Response).to receive(:new).and_return(double('response', @stub_hash)) response = SAML2::Response.new + response.issuer = SAML2::NameID.new(@aac2.idp_entity_id) response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@unique_id) @@ -423,27 +374,10 @@ describe Login::SamlController do @aac2.log_out_url = "https://example.com/idp2/slo" @aac2.position = nil @aac2.save! - - @stub_hash = { - issuer: @aac2.idp_entity_id, - is_valid?: true, - success_status?: true, - name_id: @unique_id, - name_identifier_format: nil, - name_qualifier: nil, - sp_name_qualifier: nil, - session_index: nil, - process: nil, - saml_attributes: {}, - used_key: nil - } end context "#create" do def post_create - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', @stub_hash) - ) allow_any_instance_of(SAML2::Entity).to receive(:valid_response?) controller.request.env['canvas.domain_root_account'] = @account @@ -451,10 +385,8 @@ describe Login::SamlController do end it "finds the SAML config by entity_id" do - expect_any_instantiation_of(@aac1).to receive(:saml_settings).never - expect_any_instantiation_of(@aac2).to receive(:saml_settings) - response = SAML2::Response.new + response.issuer = SAML2::NameID.new(@aac2.idp_entity_id) response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@unique_id) @@ -469,7 +401,6 @@ describe Login::SamlController do end it "redirects to login screen with message if no AAC found" do - @stub_hash[:issuer] = "hahahahahahaha" allow(SAML2::Bindings::HTTP_POST).to receive(:decode).and_return( [double('response2', errors: [], issuer: double(id: "hahahahahahaha")), nil] ) @@ -514,9 +445,6 @@ describe Login::SamlController do context "logging out" do before do - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', @stub_hash) - ) controller.request.env['canvas.domain_root_account'] = @account post :create, params: {:SAMLResponse => "foo", :RelayState => "/courses"} end @@ -636,10 +564,8 @@ SAML @aac.login_attribute = 'eduPersonPrincipalName_stripped' @aac.save - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@unique_id) @@ -659,12 +585,8 @@ SAML @aac.login_attribute = nil @aac.save - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', - issuer: "saml_entity", - ) - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(@unique_id) @@ -692,12 +614,8 @@ SAML @pseudonym.account = account @pseudonym.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', - issuer: "saml_entity", - ) - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) @@ -721,10 +639,8 @@ SAML @pseudonym.account = account @pseudonym.save! - allow(Onelogin::Saml::Response).to receive(:new).and_return( - double('response', issuer: "saml_entity") - ) response = SAML2::Response.new + response.issuer = SAML2::NameID.new('saml_entity') response.assertions << (assertion = SAML2::Assertion.new) assertion.subject = SAML2::Subject.new assertion.subject.name_id = SAML2::NameID.new(unique_id) diff --git a/spec/models/authentication_provider/saml_spec.rb b/spec/models/authentication_provider/saml_spec.rb index ccfa76c161d..38af1d5f002 100644 --- a/spec/models/authentication_provider/saml_spec.rb +++ b/spec/models/authentication_provider/saml_spec.rb @@ -25,68 +25,6 @@ describe AuthenticationProvider::SAML do @file_that_exists = File.expand_path(__FILE__) end - it "should load encryption settings" do - ConfigFile.stub('saml', { - :entity_id => 'http://www.example.com/saml2', - :encryption => { - :private_key => @file_that_exists, - :certificate => @file_that_exists - } - }) - - s = @account.authentication_providers.build(:auth_type => 'saml').saml_settings - - expect(s).to be_encryption_configured - end - - it "should load the tech contact settings" do - ConfigFile.stub('saml', { - :tech_contact_name => 'Admin Dude', - :tech_contact_email => 'admindude@example.com', - }) - - s = @account.authentication_providers.build(:auth_type => 'saml').saml_settings - - expect(s.tech_contact_name).to eq 'Admin Dude' - expect(s.tech_contact_email).to eq 'admindude@example.com' - end - - it "should allow additional private keys to be set" do - ConfigFile.stub('saml', { - :entity_id => 'http://www.example.com/saml2', - :encryption => { - :private_key => @file_that_exists, - :certificate => @file_that_exists, - :additional_private_keys => [ - @file_that_exists - ] - } - }) - - s = @account.authentication_providers.build(:auth_type => 'saml').saml_settings - - expect(s.xmlsec_additional_privatekeys).to eq [@file_that_exists] - end - - it "should allow some additional private keys to be set when not all exist" do - file_that_does_not_exist = '/tmp/i_am_not_a_private_key' - ConfigFile.stub('saml', { - :entity_id => 'http://www.example.com/saml2', - :encryption => { - :private_key => @file_that_exists, - :certificate => @file_that_exists, - :additional_private_keys => [ - @file_that_exists, - file_that_does_not_exist - ] - } - }) - - s = @account.authentication_providers.build(:auth_type => 'saml').saml_settings - - expect(s.xmlsec_additional_privatekeys).to eq [@file_that_exists] - end - it "should set the entity_id with the current domain" do allow(HostUrl).to receive(:default_host).and_return('bob.cody.instructure.com') @aac = @account.authentication_providers.create!(:auth_type => "saml") diff --git a/spec/presenters/authentication_providers_presenter_spec.rb b/spec/presenters/authentication_providers_presenter_spec.rb index e92a150ff7f..9fd099e9892 100644 --- a/spec/presenters/authentication_providers_presenter_spec.rb +++ b/spec/presenters/authentication_providers_presenter_spec.rb @@ -64,10 +64,9 @@ describe AuthenticationProvidersPresenter do expect(presenter.saml_identifiers).to be_empty end - it "is the list from Onelogin::Saml::NameIdentifiers" do + it "is the list from the SAML2 gem" do allow(AuthenticationProvider::SAML).to receive(:enabled?).and_return(true) - expected = Onelogin::Saml::NameIdentifiers::ALL_IDENTIFIERS - expect(presenter.saml_identifiers).to eq(expected) + expect(presenter.saml_identifiers).to eq(AuthenticationProvider::SAML.name_id_formats) end end @@ -83,14 +82,7 @@ describe AuthenticationProvidersPresenter do allow(AuthenticationProvider::SAML).to receive(:enabled?).and_return(true) end - it "has each value from Onelogin" do - contexts = presenter.saml_authn_contexts - Onelogin::Saml::AuthnContexts::ALL_CONTEXTS.each do |context| - expect(contexts).to include(context) - end - end - - it "sorts OneLogin values" do + it "sorts the gem values" do contexts = presenter.saml_authn_contexts(['abc', 'xyz', 'bcd']) expect(contexts.index('bcd') < contexts.index('xyz')).to be(true) end