From a2ea3f49aa91df6972fe81eae37be5655b692b44 Mon Sep 17 00:00:00 2001 From: Nick Cloward Date: Wed, 6 Aug 2014 14:30:26 -0600 Subject: [PATCH] allow GET request for SAML logout fixes: CNVS-14491 Allows GET requests for logout only when a SAML request is sent. Still only allows DELETE methods for all other authentication types. Test Plan: Initiate a SAML logout, Should work. Any other authentications methods should only logout with DELETE methods. Reference test plan for g/32723. Change-Id: Ia9153927e1ec17189d0fa00084837d451a1281a3 Reviewed-on: https://gerrit.instructure.com/38837 Reviewed-by: Cody Cutrer Tested-by: Jenkins QA-Review: August Thornton Product-Review: Nick Cloward --- .../pseudonym_sessions_controller.rb | 65 ++++++++++++------- config/routes.rb | 3 +- .../pseudonym_sessions_controller_spec.rb | 17 ++++- 3 files changed, 59 insertions(+), 26 deletions(-) diff --git a/app/controllers/pseudonym_sessions_controller.rb b/app/controllers/pseudonym_sessions_controller.rb index 9c2e21c590a..b38d36f239e 100644 --- a/app/controllers/pseudonym_sessions_controller.rb +++ b/app/controllers/pseudonym_sessions_controller.rb @@ -196,14 +196,19 @@ class PseudonymSessionsController < ApplicationController # Call for some cleanups that should be run when a user logs in @user = @pseudonym.login_assertions_for_user successful_login(@user, @pseudonym) - # Otherwise re-render the login page to show the error + # Otherwise re-render the login page to show the error else unsuccessful_login t('errors.invalid_credentials', "Incorrect username and/or password") end end def destroy - logout_user_action + # Only allow DELETE method for all logout requests except for SAML. + if saml_request? + saml_logout + else + logout_user_action + end end def logout_user_action @@ -218,19 +223,19 @@ class PseudonymSessionsController < ApplicationController end account = @current_pseudonym.try(:account) || @domain_root_account - if account.saml_authentication? and session[:saml_unique_id] + if account.saml_authentication? && session[:saml_unique_id] increment_saml_stat("logout_attempt") # logout at the saml identity provider # once logged out it'll be redirected to here again if aac = account.account_authorization_configs.find_by_id(session[:saml_aac_id]) settings = aac.saml_settings(request.host_with_port) - request = Onelogin::Saml::LogOutRequest.new(settings, session) - forward_url = request.generate_request + saml_request = Onelogin::Saml::LogOutRequest.new(settings, session) + forward_url = saml_request.generate_request if aac.debugging? && aac.debug_get(:logged_in_user_id) == @current_user.id aac.debug_set(:logout_request_id, request.id) aac.debug_set(:logout_to_idp_url, forward_url) - aac.debug_set(:logout_to_idp_xml, request.request_xml) + aac.debug_set(:logout_to_idp_xml, saml_request.request_xml) aac.debug_set(:debugging, t('debug.logout_redirect', "LogoutRequest sent to IdP")) end @@ -269,7 +274,7 @@ class PseudonymSessionsController < ApplicationController # NOT SUPPORTED without redis return render :text => "NOT SUPPORTED", :status => :method_not_allowed elsif params['logoutRequest'] && - params['logoutRequest'] =~ %r{^(.*)}m + params['logoutRequest'] =~ %r{^(.*)}m # we *could* validate the timestamp here, but the whole request is easily spoofed anyway, so there's no # point. all the security is in the ticket being secret and non-predictable return render :text => "OK", :status => :ok if Pseudonym.release_cas_ticket($1) @@ -286,7 +291,7 @@ class PseudonymSessionsController < ApplicationController end def saml_consume - if @domain_root_account.account_authorization_configs.any? { |aac| aac.saml_authentication? } && params[:SAMLResponse] + if saml_request? # Break up the SAMLResponse into chunks for logging (a truncated version was probably already # logged with the request when using syslog) chunks = params[:SAMLResponse].scan(/.{1,1024}/) @@ -428,10 +433,10 @@ class PseudonymSessionsController < ApplicationController end def saml_logout - if @domain_root_account.account_authorization_configs.any? { |aac| aac.saml_authentication? } && params[:SAMLResponse] + if saml_request? increment_saml_stat("logout_response_received") response = saml_logout_response(params[:SAMLResponse]) - if aac = @domain_root_account.account_authorization_configs.find_by_idp_entity_id(response.issuer) + if aac = @domain_root_account.account_authorization_configs.find_by_idp_entity_id(response.issuer) settings = aac.saml_settings(request.host_with_port) response.process(settings) @@ -443,8 +448,20 @@ class PseudonymSessionsController < ApplicationController aac.debug_set(:debugging, t('debug.logout_redirect_from_idp', "Received LogoutResponse from IdP")) end end + + logout_user_action + return end - logout_user_action + + @message = 'SAML Logout request requires a SAMLResponse parameter on a SAML enabled account.' + respond_to do |format| + format.html { render :template => 'shared/errors/400_message', :status => :bad_request } + format.json { render :json => { message: @message }, :status => :bad_request } + end + end + + def saml_request? + @domain_root_account.account_authorization_configs.any? { |aac| aac.saml_authentication? } && params[:SAMLResponse] end def saml_response(raw_response, settings=nil) @@ -512,7 +529,7 @@ class PseudonymSessionsController < ApplicationController drift = 30 # give them 5 minutes to enter an OTP sent via SMS drift = 300 if session[:pending_otp_communication_channel_id] || - (!session[:pending_otp_secret_key] && @current_user.otp_communication_channel_id) + (!session[:pending_otp_secret_key] && @current_user.otp_communication_channel_id) if !force_fail && ROTP::TOTP.new(secret_key).verify_with_drift(verification_code, drift) if session[:pending_otp_secret_key] @@ -527,13 +544,13 @@ class PseudonymSessionsController < ApplicationController old_cookie = cookies['canvas_otp_remember_me'] old_cookie = nil unless @current_user.validate_otp_secret_key_remember_me_cookie(old_cookie) cookies['canvas_otp_remember_me'] = { - :value => @current_user.otp_secret_key_remember_me_cookie(now, old_cookie, request.remote_ip), - :expires => now + 30.days, - :domain => otp_remember_me_cookie_domain, - :httponly => true, - :secure => CanvasRails::Application.config.session_options[:secure], - :path => '/login' - } + :value => @current_user.otp_secret_key_remember_me_cookie(now, old_cookie, request.remote_ip), + :expires => now + 30.days, + :domain => otp_remember_me_cookie_domain, + :httponly => true, + :secure => CanvasRails::Application.config.session_options[:secure], + :path => '/login' + } end if session.delete(:pending_otp) successful_login(@current_user, @current_pseudonym, true) @@ -572,7 +589,7 @@ class PseudonymSessionsController < ApplicationController if !otp_passed mfa_settings = @current_user.mfa_settings if (@current_user.otp_secret_key && mfa_settings == :optional) || - mfa_settings == :required + mfa_settings == :required session[:pending_otp] = true return otp_login(true) end @@ -580,10 +597,10 @@ class PseudonymSessionsController < ApplicationController if pseudonym.account_id == Account.site_admin.id && Account.site_admin.account_authorization_config.try(:delegated_authentication?) cookies['canvas_sa_delegated'] = { - :value => '1', - :domain => otp_remember_me_cookie_domain, - :httponly => true, - :secure => CanvasRails::Application.config.session_options[:secure] + :value => '1', + :domain => otp_remember_me_cookie_domain, + :httponly => true, + :secure => CanvasRails::Application.config.session_options[:secure] } end session[:require_terms] = true if @domain_root_account.require_acceptance_of_terms?(@current_user) diff --git a/config/routes.rb b/config/routes.rb index ea83537a471..2fd4f6fcff8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -604,6 +604,7 @@ CanvasRails::Application.routes.draw do match 'login' => 'pseudonym_sessions#new', :as => :login, :via => :get match 'login' => 'pseudonym_sessions#create', :via => :post match 'logout' => 'pseudonym_sessions#destroy', :as => :logout, :via => :delete + match 'logout' => 'pseudonym_sessions#saml_logout', :via => [:get, :post] match 'login/cas' => 'pseudonym_sessions#new', :as => :cas_login, :via => :get match 'login/cas' => 'pseudonym_sessions#cas_logout', :as => :cas_logout, :via => :post match 'login/otp' => 'pseudonym_sessions#otp_login', :as => :otp_login, :via => [:get, :post] @@ -762,7 +763,7 @@ CanvasRails::Application.routes.draw do match 'object_snippet' => 'context#object_snippet', :as => :object_snippet, :via => :post match 'saml_consume' => 'pseudonym_sessions#saml_consume', :as => :saml_consume - match 'saml_logout' => 'pseudonym_sessions#saml_logout', :as => :saml_logout + match 'saml_logout' => 'pseudonym_sessions#saml_logout', :as => :saml_logout, :via => [:get, :post, :delete] match 'saml_meta_data' => 'accounts#saml_meta_data', :as => :saml_meta_data # Routes for course exports diff --git a/spec/controllers/pseudonym_sessions_controller_spec.rb b/spec/controllers/pseudonym_sessions_controller_spec.rb index 094edc33128..4383e5c4888 100644 --- a/spec/controllers/pseudonym_sessions_controller_spec.rb +++ b/spec/controllers/pseudonym_sessions_controller_spec.rb @@ -449,10 +449,25 @@ describe PseudonymSessionsController do @stub_hash[:issuer] = "nobody eh" get_saml_consume end + + it "should return bad request if a SAMLResponse parameter is not provided" do + controller.expects(:logout_user_action).never + get 'saml_logout' + response.status.should == 400 + end end end end + context "/saml_logout" do + it "should return bad request if SAML is not configured for account" do + controller.expects(:logout_user_action).never + controller.request.env['canvas.domain_root_account'] = @account + get 'saml_logout', :SAMLResponse => "foo", :RelayState => "/courses" + response.status.should == 400 + end + end + context "login attributes" do before :once do @unique_id = 'foo' @@ -753,7 +768,7 @@ describe PseudonymSessionsController do user_session(@user, @pseudonym) PseudonymSession.find.stubs(:destroy) session[:cas_session] = true - post 'destroy' + delete 'destroy' response.should be_redirect response.location.should match %r{^https://localhost/cas/logout} end