ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <cody@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Nick Cloward <ncloward@instructure.com>
This commit is contained in:
Nick Cloward 2014-08-06 14:30:26 -06:00
parent a8b2a430b6
commit a2ea3f49aa
3 changed files with 59 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
app/controllers/pseudonym_sessions_controller.rb
View File

@ -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{^<samlp:LogoutRequest.*?<samlp:SessionIndex>(.*)</samlp:SessionIndex>}m
params['logoutRequest'] =~ %r{^<samlp:LogoutRequest.*?<samlp:SessionIndex>(.*)</samlp:SessionIndex>}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)

3
config/routes.rb
View File

@ -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

17
spec/controllers/pseudonym_sessions_controller_spec.rb
View File

@ -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