CartoDB · simon-contreras-deel · May 8, 2020 · Apr 28, 2020 · Apr 30, 2020 · Apr 30, 2020
diff --git a/Makefile b/Makefile
@@ -76,6 +76,7 @@ WORKING_SPECS_1 = \
 	spec/lib/carto/visualization_migrator_spec.rb \
 	spec/lib/carto/http/client_spec.rb \
 	spec/lib/carto/table_utils_spec.rb \
+	spec/lib/carto/authentication_manager_spec.rb \
 	spec/helpers/uuidhelper_spec.rb \
 	spec/helpers/url_validator_spec.rb \
 	spec/models/carto/data_import_spec.rb \

diff --git a/NEWS.md b/NEWS.md
@@ -25,6 +25,7 @@ Development
 - /api/v3/me endpoint returns less public data ([#5627](https://github.com/CartoDB/cartodb-platform/issues/5627))
 - Retrieve IPs before adding or removing to avoid inconsistencies ([#15643](https://github.com/CartoDB/cartodb/pull/15643))
 - Faster geometry types calculation for big datasets ([#15654](https://github.com/CartoDB/cartodb/pull/15654))
+- Return error for requests whose authentication was succeeding with an expired session ([#15637](https://github.com/CartoDB/cartodb/pull/15637))
 
 4.37.0 (2020-04-24)
 -------------------

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -1,8 +1,11 @@
 require_relative '../../lib/cartodb/profiler.rb'
+require_dependency 'carto/authentication_manager'
 require_dependency 'carto/http_header_authentication'
 
 class ApplicationController < ActionController::Base
   include UrlHelper
+  include Carto::ControllerHelper
+
   protect_from_forgery
 
   helper :all
@@ -26,6 +29,7 @@ class ApplicationController < ActionController::Base
 
   rescue_from NoHTML5Compliant, :with => :no_html5_compliant
   rescue_from ActiveRecord::RecordNotFound, RecordNotFound, with: :render_404
+  rescue_from(Carto::ExpiredSessionError) { |e| rescue_from_carto_error(e) }
 
   ME_ENDPOINT_COOKIE = :_cartodb_base_url
   IGNORE_PATHS_FOR_CHECK_USER_STATE = %w(maintenance_mode lockout login logout unauthenticated multifactor_authentication).freeze
@@ -50,7 +54,7 @@ def self.ssl_allowed(*_splat)
   def current_viewer
     if @current_viewer.nil?
       if current_user && env["warden"].authenticated?(current_user.username)
-        @current_viewer = current_user if validate_session(current_user)
+        @current_viewer = current_user if Carto::AuthenticationManager.validate_session(warden, request, current_user)
       else
         authenticated_usernames = request.session.to_hash.select { |k, _|
           k.start_with?("warden.user") && !k.end_with?(".session")
@@ -64,13 +68,16 @@ def current_viewer
         if current_user_present.nil?
           unless authenticated_usernames.first.nil?
             user = ::User.where(username: authenticated_usernames.first).first
-            validate_session(user, false) unless user.nil?
+            Carto::AuthenticationManager.validate_session(warden, request, user) unless user.nil?
             @current_viewer = user
           end
         end
       end
     end
     @current_viewer
+  rescue Carto::ExpiredSessionError => e
+    request.reset_session
+    not_authorized(e)
   end
 
   protected
@@ -104,30 +111,14 @@ def update_session_security_token(user)
     warden.session(user.username)[:sec_token] = user.security_token
   end
 
-  def session_security_token_valid?(user)
-    warden.session(user.username).key?(:sec_token) &&
-      warden.session(user.username)[:sec_token] == user.security_token
-  rescue Warden::NotAuthenticated
-    false
-  end
-
-  def validate_session(user = current_user, reset_session_on_error = true)
-    if session_security_token_valid?(user)
-      true
-    else
-      reset_session if reset_session_on_error
-      false
-    end
-  end
-
   def is_https?
     request.protocol == 'https://'
   end
 
   def http_header_authentication
     authenticate(:http_header_authentication, scope: CartoDB.extract_subdomain(request))
     if current_user
-      validate_session(current_user)
+      Carto::AuthenticationManager.validate_session(warden, request, current_user)
     else
       authenticator = Carto::HttpHeaderAuthentication.new
       if authenticator.autocreation_enabled?
@@ -279,21 +270,21 @@ def multifactor_authentication_required?(user = current_viewer)
 
   def login_required
     is_auth = authenticated?(CartoDB.extract_subdomain(request))
-    is_auth ? validate_session(current_user) : not_authorized
+    is_auth ? Carto::AuthenticationManager.validate_session(warden, request, current_user) : not_authorized
   end
 
   def login_required_any_user
-    current_viewer ? validate_session(current_viewer) : not_authorized
+    current_viewer ? Carto::AuthenticationManager.validate_session(warden, request, current_viewer) : not_authorized
   end
 
   def api_authorization_required
     authenticate!(:auth_api, :api_authentication, scope: CartoDB.extract_subdomain(request))
-    validate_session(current_user)
+    Carto::AuthenticationManager.validate_session(warden, request, current_user)
   end
 
   def any_api_authorization_required
     authenticate!(:any_auth_api, :api_authentication, scope: CartoDB.extract_subdomain(request))
-    validate_session(current_user)
+    Carto::AuthenticationManager.validate_session(warden, request, current_user)
   end
 
   def engine_required
@@ -304,7 +295,7 @@ def engine_required
   # but doesn't break the request if can't authenticate
   def optional_api_authorization
     got_auth = authenticate(:auth_api, :api_authentication, scope: CartoDB.extract_subdomain(request))
-    validate_session(current_user) if got_auth
+    Carto::AuthenticationManager.validate_session(warden, request, current_user) if got_auth
   end
 
   def redirect_or_forbidden(path, error)
@@ -342,15 +333,16 @@ def render_locked_owner
     end
   end
 
-  def not_authorized
+  def not_authorized(exception = nil)
     respond_to do |format|
       format.html do
         session[:return_to] = request.url
         redirect_to CartoDB.url(self, 'login', keep_base_url: true)
         return
       end
       format.json do
-        head :unauthorized
+        render(json: { errors: exception&.message }, status: :unauthorized)
+        return
       end
     end
   end

diff --git a/app/controllers/carto/admin/mobile_apps_controller.rb b/app/controllers/carto/admin/mobile_apps_controller.rb
@@ -3,7 +3,6 @@
 require_dependency 'helpers/organization_notifications_helper'
 
 class Carto::Admin::MobileAppsController < Admin::AdminController
-  include Carto::ControllerHelper
   include MobileAppsHelper
   include AvatarHelper
   include OrganizationNotificationsHelper

diff --git a/app/controllers/carto/api/analyses_controller.rb b/app/controllers/carto/api/analyses_controller.rb
@@ -4,7 +4,6 @@
 module Carto
   module Api
     class AnalysesController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       include Carto::UUIDHelper
       include Carto::Builder::BuilderUsersModule
 

diff --git a/app/controllers/carto/api/api_keys_controller.rb b/app/controllers/carto/api/api_keys_controller.rb
@@ -1,7 +1,6 @@
 require_relative 'paged_searcher'
 
 class Carto::Api::ApiKeysController < ::Api::ApplicationController
-  include Carto::ControllerHelper
   include Carto::UUIDHelper
   include Carto::Api::PagedSearcher
   include Carto::Api::AuthApiAuthentication

diff --git a/app/controllers/carto/api/dbdirect_certificates_controller.rb b/app/controllers/carto/api/dbdirect_certificates_controller.rb
@@ -3,7 +3,6 @@
 module Carto
   module Api
     class DbdirectCertificatesController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       extend Carto::DefaultRescueFroms
 
       skip_before_filter :verify_authenticity_token, only: [:create], if: :zip_formatted_request?

diff --git a/app/controllers/carto/api/dbdirect_ips_controller.rb b/app/controllers/carto/api/dbdirect_ips_controller.rb
@@ -1,7 +1,6 @@
 module Carto
   module Api
     class DbdirectIpsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       extend Carto::DefaultRescueFroms
 
       ssl_required :show, :update, :destroy

diff --git a/app/controllers/carto/api/grantable_presenter.rb b/app/controllers/carto/api/grantable_presenter.rb
@@ -33,4 +33,3 @@ def model_presenter
 
   end
 end
-
diff --git a/app/controllers/carto/api/grantables_controller.rb b/app/controllers/carto/api/grantables_controller.rb
@@ -5,7 +5,6 @@ module Api
 
     class GrantablesController < ::Api::ApplicationController
       include PagedSearcher
-      include Carto::ControllerHelper
 
       respond_to :json
 
@@ -46,5 +45,3 @@ def load_organization
 
   end
 end
-
-
diff --git a/app/controllers/carto/api/groups_controller.rb b/app/controllers/carto/api/groups_controller.rb
@@ -6,7 +6,6 @@ module Api
 
     class GroupsController < ::Api::ApplicationController
       include PagedSearcher
-      include Carto::ControllerHelper
 
       ssl_required :index, :show, :create, :update, :destroy, :add_users, :remove_users
 

diff --git a/app/controllers/carto/api/layers_controller.rb b/app/controllers/carto/api/layers_controller.rb
@@ -4,8 +4,6 @@
 module Carto
   module Api
     class LayersController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :show, :layers_by_map, :custom_layers_by_user, :map_index, :user_index, :map_show, :user_show,
                    :map_create, :user_create, :map_update, :user_update, :map_destroy, :user_destroy
 

diff --git a/app/controllers/carto/api/legends_controller.rb b/app/controllers/carto/api/legends_controller.rb
@@ -1,8 +1,6 @@
 module Carto
   module Api
     class LegendsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :index, :show, :create, :update, :destroy
 
       before_filter :load_layer,

diff --git a/app/controllers/carto/api/mapcaps_controller.rb b/app/controllers/carto/api/mapcaps_controller.rb
@@ -5,7 +5,6 @@
 module Carto
   module Api
     class MapcapsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       include Carto::Builder::BuilderUsersModule
 
       ssl_required :show, :create, :destroy, :index

diff --git a/app/controllers/carto/api/maps_controller.rb b/app/controllers/carto/api/maps_controller.rb
@@ -1,8 +1,6 @@
 module Carto
   module Api
     class MapsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :show, :update
 
       before_filter :load_map, :owners_only

diff --git a/app/controllers/carto/api/metrics_controller.rb b/app/controllers/carto/api/metrics_controller.rb
@@ -1,8 +1,6 @@
 module Carto
   module Api
     class MetricsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :create
 
       skip_before_filter :api_authorization_required

diff --git a/app/controllers/carto/api/multifactor_authentication_controller.rb b/app/controllers/carto/api/multifactor_authentication_controller.rb
@@ -1,7 +1,6 @@
 module Carto
   module Api
     class MultifactorAuthenticationController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       extend  Carto::DefaultRescueFroms
       include OrganizationUsersHelper
 

diff --git a/app/controllers/carto/api/multifactor_auths_controller.rb b/app/controllers/carto/api/multifactor_auths_controller.rb
@@ -1,6 +1,4 @@
 class Carto::Api::MultifactorAuthsController < ::Api::ApplicationController
-  include Carto::ControllerHelper
-
   ssl_required
 
   before_action :load_user

diff --git a/app/controllers/carto/api/organization_assets_controller.rb b/app/controllers/carto/api/organization_assets_controller.rb
@@ -1,8 +1,6 @@
 module Carto
   module Api
     class OrganizationAssetsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :index, :show, :create, :destroy
 
       before_filter :load_organization,

diff --git a/app/controllers/carto/api/organization_notifications_controller.rb b/app/controllers/carto/api/organization_notifications_controller.rb
@@ -1,7 +1,6 @@
 module Carto
   module Api
     class OrganizationNotificationsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       extend Carto::DefaultRescueFroms
 
       ssl_required :create, :destroy

diff --git a/app/controllers/carto/api/organizations_controller.rb b/app/controllers/carto/api/organizations_controller.rb
@@ -5,7 +5,6 @@ module Api
     class OrganizationsController < ::Api::ApplicationController
       include OrganizationsHelper
       include PagedSearcher
-      include Carto::ControllerHelper
 
       ssl_required :users
 

diff --git a/app/controllers/carto/api/overlays_controller.rb b/app/controllers/carto/api/overlays_controller.rb
@@ -3,8 +3,6 @@
 module Carto
   module Api
     class OverlaysController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :index, :show, :create, :update, :destroy
 
       before_filter :logged_users_only

diff --git a/app/controllers/carto/api/permissions_controller.rb b/app/controllers/carto/api/permissions_controller.rb
@@ -1,5 +1,4 @@
 class Carto::Api::PermissionsController < ::Api::ApplicationController
-  include Carto::ControllerHelper
   extend Carto::DefaultRescueFroms
 
   ssl_required :update

diff --git a/app/controllers/carto/api/public/apps_controller.rb b/app/controllers/carto/api/public/apps_controller.rb
@@ -2,7 +2,6 @@
 require_relative '../paged_searcher'
 
 class Carto::Api::Public::AppsController < Carto::Api::Public::ApplicationController
-  include Carto::ControllerHelper
   include Carto::Api::VisualizationSearcher
   include Carto::Api::PagedSearcher
 

diff --git a/app/controllers/carto/api/public/custom_visualizations_controller.rb b/app/controllers/carto/api/public/custom_visualizations_controller.rb
@@ -2,7 +2,6 @@
 require_relative '../paged_searcher'
 
 class Carto::Api::Public::CustomVisualizationsController < Carto::Api::Public::ApplicationController
-  include Carto::ControllerHelper
   include Carto::Api::VisualizationSearcher
   include Carto::Api::PagedSearcher
 

diff --git a/app/controllers/carto/api/public/data_observatory_controller.rb b/app/controllers/carto/api/public/data_observatory_controller.rb
@@ -2,7 +2,6 @@ module Carto
   module Api
     module Public
       class DataObservatoryController < Carto::Api::Public::ApplicationController
-        include Carto::ControllerHelper
         include Carto::Api::PagedSearcher
         extend Carto::DefaultRescueFroms
 

diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb
@@ -6,7 +6,6 @@ module Api
     module Public
       class DatasetsController < Carto::Api::Public::ApplicationController
         include Carto::Api::PagedSearcher
-        include Carto::ControllerHelper
         extend Carto::DefaultRescueFroms
 
         ssl_required

diff --git a/app/controllers/carto/api/public/federated_tables_controller.rb b/app/controllers/carto/api/public/federated_tables_controller.rb
@@ -3,7 +3,6 @@ module Api
     module Public
       class FederatedTablesController < Carto::Api::Public::ApplicationController
         include Carto::Api::PagedSearcher
-        include Carto::ControllerHelper
         extend Carto::DefaultRescueFroms
 
         VALID_ORDER_PARAMS_FEDERATED_SERVER = %i(federated_server_name).freeze

diff --git a/app/controllers/carto/api/public/oauth_apps_controller.rb b/app/controllers/carto/api/public/oauth_apps_controller.rb
@@ -6,7 +6,6 @@ module Api
     module Public
       class OauthAppsController < Carto::Api::Public::ApplicationController
         include Carto::Api::PagedSearcher
-        include Carto::ControllerHelper
         extend Carto::DefaultRescueFroms
 
         ssl_required

diff --git a/app/controllers/carto/api/received_notifications_controller.rb b/app/controllers/carto/api/received_notifications_controller.rb
@@ -1,7 +1,6 @@
 module Carto
   module Api
     class ReceivedNotificationsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
       extend Carto::DefaultRescueFroms
 
       ssl_required :update

diff --git a/app/controllers/carto/api/search_preview_controller.rb b/app/controllers/carto/api/search_preview_controller.rb
@@ -1,8 +1,6 @@
 require_dependency 'carto/controller_helper'
 
 class Carto::Api::SearchPreviewController < ::Api::ApplicationController
-  include Carto::ControllerHelper
-
   ssl_required
 
   before_filter :login_required

diff --git a/app/controllers/carto/api/states_controller.rb b/app/controllers/carto/api/states_controller.rb
@@ -1,8 +1,6 @@
 module Carto
   module Api
     class StatesController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :update
 
       before_filter :load_visualization,

diff --git a/app/controllers/carto/api/static_notifications_controller.rb b/app/controllers/carto/api/static_notifications_controller.rb
@@ -3,8 +3,6 @@
 module Carto
   module Api
     class StaticNotificationsController < ::Api::ApplicationController
-      include Carto::ControllerHelper
-
       ssl_required :update
       before_filter :load_static_notifications, only: [:update]