Fix edge case in SAML logout

NEWS.md

@@ -118,6 +118,7 @@ sudo make install
 - Free users can't create datasets due to default state was private [16223](https://github.com/CartoDB/cartodb/pull/16223)
 - Improve visibility over SAML errors [#16243](https://github.com/CartoDB/cartodb/pull/16243)
 - SAML adjustments [#16246](https://github.com/CartoDB/cartodb/pull/16246)
+- Retrieve user email for SAML logout before closing CARTO session [#16248](https://github.com/CartoDB/cartodb/pull/16248)
 
 4.44.0 (2020-11-20)
 -------------------

app/controllers/sessions_controller.rb

@@ -418,9 +418,10 @@ class SessionsController < ApplicationController
       redirect_to default_logout_url
     # SP-initiated logout
     else
-      user_email = current_user.email # Save email for afterwards (current_user is cleared by cdb_logout)
+      user_email = current_user&.email # Save email for afterwards (current_user is cleared by cdb_logout)
       cdb_logout # Close session in CARTO first, in case somthing goes wrong in the IDP
-      redirect_to saml_service.sp_logout_request(user_email)
+      redirect_url = user_email ? saml_service.sp_logout_request(user_email) : default_logout_url
+      redirect_to(redirect_url)
     end
   end
 

spec/factories/organizations.rb

@@ -34,6 +34,20 @@ FactoryBot.define do
       end
     end
 
+    trait :saml_enabled do
+      auth_saml_configuration do
+        {
+          issuer: 'localhost.lan',
+          idp_sso_target_url: 'https://example.com/saml/signon/',
+          idp_slo_target_url: 'https://example.com/saml/signon/',
+          idp_cert_fingerprint: '',
+          assertion_consumer_service_url: 'https://localhost.lan/saml/finalize',
+          name_identifier_format: '',
+          email_attribute: 'username'
+        }.stringify_keys
+      end
+    end
+
     factory :organization_whitelist_carto, class: 'Carto::Organization' do
       whitelisted_email_domains { ['carto.com'] }
       auth_username_password_enabled { true }

spec/requests/sessions_controller_spec.rb

@@ -383,9 +383,9 @@ describe SessionsController do
     describe 'SAML logout' do
       it 'calls SamlService#sp_logout_request from user-initiated logout' do
         stub_saml_service(@user)
-        SessionsController.any_instance.expects(:authenticate!).with(:saml, scope: @user.username).returns(@user).once
 
-        post create_session_url(user_domain: user_domain, SAMLResponse: 'xx')
+        host! "#{@user.username}.localhost.lan"
+        post create_session_url(email: @user.email, password: password)
 
         # needs returning an url to do a redirection
         Carto::SamlService.any_instance.stubs(:sp_logout_request).returns('http://carto.com').once
@@ -394,9 +394,9 @@ describe SessionsController do
 
       it 'does not call SamlService#sp_logout_request if logout URL is not configured' do
         stub_saml_service(@user)
-        SessionsController.any_instance.expects(:authenticate!).with(:saml, scope: @user.username).returns(@user).once
 
-        post create_session_url(user_domain: user_domain, SAMLResponse: 'xx')
+        host! "#{@user.username}.localhost.lan"
+        post create_session_url(email: @user.email, password: password)
 
         # needs returning an url to do a redirection
         Carto::SamlService.any_instance.stubs(:logout_url_configured?).returns(false)
@@ -437,12 +437,28 @@ describe SessionsController do
   end
 
   describe 'SAML authentication' do
+    let(:password) { '12345678' }
+    let(:organization) do
+      create(
+        :organization_with_users, :saml_enabled,
+        quota_in_bytes: 1.gigabytes,
+        viewer_seats: 20
+      )
+    end
+    let(:user) do
+      create(
+        :carto_user,
+        organization_id: organization.id,
+        password: password,
+        password_confirmation: password,
+        factory_bot_context: { only_db_setup: true }
+      )
+    end
+
     def setup_saml_organization
-      @organization = create(:saml_organization, quota_in_bytes: 1.gigabytes, viewer_seats: 20)
-      @admin_user = create_admin_user(@organization)
-      @user = create(:carto_user)
-      @user.organization_id = @organization.id
-      @user.save
+      @organization = organization
+      @admin_user = @organization.owner
+      @user = user
     end
 
     def cleanup