Merge pull request #16253 from CartoDB/feature/ch145527/check-user-saml

[Reef] Set up SSO: Check whether the user was created via SAML to logout
CartoDB · Apr 14, 2021 · a149992 · a149992
2 parents acddb5f + 4576277
commit a149992
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 79 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -120,6 +120,7 @@ sudo make install
 - 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)
+- SAML logout only for users who were created via SAML [#16253](https://github.com/CartoDB/cartodb/pull/16253)
 
 4.44.0 (2020-11-20)
 -------------------

diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
@@ -90,7 +90,7 @@ def create
   end
 
   def destroy
-    saml_authentication? && saml_service.try(:logout_url_configured?) ? saml_logout : do_logout
+    saml_authentication? && saml_service.try(:logout_url_configured?) && saml_user? ? saml_logout : do_logout
   end
 
   def show
@@ -301,6 +301,12 @@ def github_config
     end
   end
 
+  def saml_user?
+    subdomain = CartoDB.extract_subdomain(request)
+    user = Carto::User.find_by(username: subdomain)
+    user.created_via == Carto::UserCreation::CREATED_VIA_SAML unless user.nil?
+  end
+
   private
 
   def mfa_request?

diff --git a/app/models/carto/helpers/user_commons.rb b/app/models/carto/helpers/user_commons.rb
@@ -457,4 +457,12 @@ def delete_in_central
     true
   end
 
+  def created_via
+    @created_via || user_creation.try(:created_via)
+  end
+
+  def user_creation
+    @user_creation ||= Carto::UserCreation.find_by(user_id: id)
+  end
+
 end
diff --git a/app/models/user.rb b/app/models/user.rb
@@ -1251,8 +1251,7 @@ def invitation_token=(invitation_token)
   end
 
   def created_with_invitation?
-    user_creation = get_user_creation
-    user_creation && user_creation.invitation_token
+    user_creation&.invitation_token
   end
 
   def destroy_cascade
@@ -1297,14 +1296,7 @@ def destroy_shared_with
   end
 
   def get_invitation_token_from_user_creation
-    user_creation = get_user_creation
-    if !user_creation.nil? && user_creation.has_valid_invitation?
-      user_creation.invitation_token
-    end
-  end
-
-  def get_user_creation
-    @user_creation ||= Carto::UserCreation.find_by_user_id(id)
+    user_creation.invitation_token if user_creation&.has_valid_invitation?
   end
 
   def quota_dates(options)
@@ -1384,10 +1376,6 @@ def valid_email_domain?(email)
     Carto::EmailDomainValidator.validate_domain(email, organization.whitelisted_email_domains)
   end
 
-  def created_via
-    @created_via || get_user_creation.try(:created_via)
-  end
-
   def sync_master_key
     master_key = api_keys.master.first
     return unless master_key

diff --git a/script/ci/cloudbuild-build-master.yaml b/script/ci/cloudbuild-build-master.yaml
@@ -119,7 +119,7 @@ steps:
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}:latest -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:latest -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}  -t ${_DOCKER_IMAGE_NAME}-resque:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}--${SHORT_SHA}   -f Dockerfile.resque .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:latest -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-subscriber:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG}--${SHORT_SHA}  -f Dockerfile.subscriber .        
-  waitFor: ['copy-private-files']
+  waitFor: ['copy-cartodb-repo']
 
 # Checkout onprem licensing gear
 - name: 'gcr.io/cloud-builders/git'
@@ -130,22 +130,16 @@ steps:
   volumes:
   - name: 'ssh'
     path: /root/.ssh
-  waitFor: ['copy-cartodb-repo']
+  waitFor: ['build-cartodb']
 
 # Build onprem image
 - name: gcr.io/cloud-builders/docker
   id: build-cartodb-onprem
   dir: /workspace/cartodb-onprem
   entrypoint: /bin/bash
   args:
-    - -cx
+    - -cex
     - |
-      docker pull ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}
-      if [ $? -ne 0 ]
-      then
-        docker pull ${_DOCKER_IMAGE_NAME}:latest
-      fi
-      set -e
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg GRUNT_ENV=production --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}-onprem:latest -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-onprem:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
   waitFor: ['checkout-licensing-gear']
 

diff --git a/script/ci/cloudbuild-build-pr-branch.yaml b/script/ci/cloudbuild-build-pr-branch.yaml
@@ -119,7 +119,7 @@ steps:
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}:latest -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:latest -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}  -t ${_DOCKER_IMAGE_NAME}-resque:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}--${SHORT_SHA}   -f Dockerfile.resque .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:latest -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-subscriber:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG}--${SHORT_SHA}  -f Dockerfile.subscriber .        
-  waitFor: ['copy-private-files']
+  waitFor: ['copy-cartodb-repo']
 
 # Checkout onprem licensing gear
 - name: 'gcr.io/cloud-builders/git'
@@ -130,22 +130,16 @@ steps:
   volumes:
   - name: 'ssh'
     path: /root/.ssh
-  waitFor: ['copy-cartodb-repo']
+  waitFor: ['build-cartodb']
 
 # Build onprem image
 - name: gcr.io/cloud-builders/docker
   id: build-cartodb-onprem
   dir: /workspace/cartodb-onprem
   entrypoint: /bin/bash
   args:
-    - -cx
+    - -cex
     - |
-      docker pull ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}
-      if [ $? -ne 0 ]
-      then
-        docker pull ${_DOCKER_IMAGE_NAME}:latest
-      fi
-      set -e
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg GRUNT_ENV=production --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}-onprem:latest -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-onprem:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
   waitFor: ['checkout-licensing-gear']
 

diff --git a/script/ci/cloudbuild-onprem-dev-tests.yaml b/script/ci/cloudbuild-onprem-dev-tests.yaml
@@ -173,7 +173,7 @@ steps:
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}:latest -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:latest -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}  -t ${_DOCKER_IMAGE_NAME}-resque:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-resque:${_BRANCH_TAG}--${SHORT_SHA}   -f Dockerfile.resque .
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:latest -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-subscriber:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-subscriber:${_BRANCH_TAG}--${SHORT_SHA}  -f Dockerfile.subscriber .        
-  waitFor: ['copy-private-files']
+  waitFor: ['copy-cartodb-repo']
 
 # Checkout onprem licensing gear
 - name: 'gcr.io/cloud-builders/git'
@@ -184,22 +184,16 @@ steps:
   volumes:
   - name: 'ssh'
     path: /root/.ssh
-  waitFor: ['copy-cartodb-repo']
+  waitFor: ['build-cartodb']
 
 # Build onprem image
 - name: gcr.io/cloud-builders/docker
   id: build-cartodb-onprem
   dir: /workspace/cartodb-onprem
   entrypoint: /bin/bash
   args:
-    - -cx
+    - -cex
     - |
-      docker pull ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG}
-      if [ $? -ne 0 ]
-      then
-        docker pull ${_DOCKER_IMAGE_NAME}:latest
-      fi
-      set -e
       docker build --label="org.opencontainers.image.created=$$(date --rfc-3339=seconds)" --label=org.opencontainers.image.revision=${COMMIT_SHA} --build-arg GRUNT_ENV=production --build-arg COMPILE_ASSETS=true --build-arg BUNDLE_JOBS=4 -t ${_DOCKER_IMAGE_NAME}-onprem:latest -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG} -t ${_DOCKER_IMAGE_NAME}-onprem:${SHORT_SHA} -t ${_DOCKER_IMAGE_NAME}-onprem:${_BRANCH_TAG}--${SHORT_SHA} --cache-from ${_DOCKER_IMAGE_NAME}:${_BRANCH_TAG} --cache-from ${_DOCKER_IMAGE_NAME}:latest .
   waitFor: ['checkout-licensing-gear']
 

diff --git a/spec/requests/sessions_controller_spec.rb b/spec/requests/sessions_controller_spec.rb
@@ -408,12 +408,14 @@ def stub_saml_service(user)
 
       it 'calls SamlService#idp_logout_request if SAMLRequest is present' do
         # needs returning an url to do a redirection
+        Carto::SamlService.any_instance.stubs(:logout_url_configured?).returns(true)
         Carto::SamlService.any_instance.stubs(:idp_logout_request).returns('http://carto.com').once
         get logout_url(user_domain: user_domain, SAMLRequest: 'xx')
       end
 
       it 'calls SamlService#process_logout_response if SAMLResponse is present' do
         # needs returning an url to do a redirection
+        Carto::SamlService.any_instance.stubs(:logout_url_configured?).returns(true)
         Carto::SamlService.any_instance.stubs(:process_logout_response).returns('http://carto.com').once
         get logout_url(user_domain: user_domain, SAMLResponse: 'xx')
       end
@@ -456,11 +458,26 @@ def stub_saml_service(user)
         factory_bot_context: { only_db_setup: true }
       )
     end
+    let(:saml_user) do
+      user = create(
+        :carto_user,
+        organization_id: organization.id,
+        password: password,
+        password_confirmation: password,
+        factory_bot_context: { only_db_setup: true }
+      )
+      create(
+        :user_creation,
+        user_id: user.id,
+        created_via: Carto::UserCreation::CREATED_VIA_SAML
+      )
+      user
+    end
 
     def setup_saml_organization
       @organization = organization
       @admin_user = @organization.owner
-      @user = user
+      @user = saml_user
     end
 
     def cleanup
@@ -488,43 +505,7 @@ def create_admin_user(organization)
       admin_user
     end
 
-    describe 'domainful' do
-      it_behaves_like 'SAML'
-      it_behaves_like 'SAML no MFA'
-
-      let(:user_domain) { nil }
-
-      before(:each) do
-        stub_domainful(@organization.name)
-      end
-
-      before(:all) { setup_saml_organization }
-
-      after(:all) do
-        cleanup
-      end
-    end
-
-    describe 'subdomainless' do
-      it_behaves_like 'SAML'
-      it_behaves_like 'SAML no MFA'
-
-      let(:user_domain) { @organization.name }
-
-      before(:each) do
-        stub_subdomainless
-      end
-
-      before(:all) { setup_saml_organization }
-
-      after(:all) do
-        cleanup
-      end
-    end
-
     describe 'user with MFA' do
-      it_behaves_like 'SAML'
-
       it "redirects to multifactor_authentication" do
         # we use this to avoid generating the static assets in CI
         Admin::VisualizationsController.any_instance.stubs(:render).returns('')