Properly halt rails filter when not authorized

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 \

app/controllers/api/application_controller.rb

@@ -1,6 +1,9 @@
 require_relative '../../../lib/cartodb/stats/editor_apis'
 
 class Api::ApplicationController < ApplicationController
+
+  include Carto::ControllerHelper
+
   protect_from_forgery with: :null_session
 
   # Don't force org urls

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, with: :rescue_from_carto_error
 
   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 @@ class ApplicationController < ActionController::Base
   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,7 +68,7 @@ class ApplicationController < ActionController::Base
         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, false) unless user.nil?
             @current_viewer = user
           end
         end
@@ -104,22 +108,6 @@ class ApplicationController < ActionController::Base
     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
@@ -127,7 +115,7 @@ class ApplicationController < ActionController::Base
   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 +267,21 @@ class ApplicationController < ActionController::Base
 
   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 +292,7 @@ class ApplicationController < ActionController::Base
   # 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)

app/controllers/carto/api/grantable_presenter.rb

@@ -33,4 +33,3 @@ module Carto
 
   end
 end
-

app/controllers/carto/api/grantables_controller.rb

@@ -46,5 +46,3 @@ module Carto
 
   end
 end
-
-

app/models/organization.rb

@@ -405,6 +405,8 @@ class Organization < Sequel::Model
   end
 
   def admin?(user)
+    return false unless user
+
     user.belongs_to_organization?(self) && user.organization_admin?
   end
 

lib/carto/authentication_manager.rb

@@ -0,0 +1,29 @@
+module Carto
+  class AuthenticationManager
+
+    def self.validate_session(warden_context, request, user, reset_session_on_error = true)
+      if session_security_token_valid?(warden_context, user)
+        true
+      else
+        request.reset_session if reset_session_on_error
+        false
+      end
+    end
+
+    def self.session_security_token_valid?(warden_context, user)
+      session = warden_context.session(user.username)
+
+      return false unless session.key?(:sec_token)
+
+      if session[:sec_token] != user.security_token
+        user.user_multifactor_auths.any? ? (return false) : (raise Carto::ExpiredSessionError)
+      end
+
+      true
+    rescue Warden::NotAuthenticated
+      false
+    end
+    private_class_method :session_security_token_valid?
+
+  end
+end

lib/carto/errors.rb

@@ -33,6 +33,12 @@ module Carto
     end
   end
 
+  class ExpiredSessionError < CartoError
+    def initialize(message = "Your session has expired.", status = 401)
+      super(message, status)
+    end
+  end
+
   class UnauthorizedError < CartoError
     def initialize(message = "You don't have permission to access that resource", status = 403)
       super(message, status)

spec/lib/carto/authentication_manager_spec.rb

@@ -0,0 +1,63 @@
+require_relative '../../spec_helper_min.rb'
+require_relative '../../../lib/carto/authentication_manager.rb'
+
+module Carto
+  describe AuthenticationManager do
+
+    describe '::validate_session' do
+      subject { described_class.validate_session(warden_context, request, user) }
+
+      let!(:user) { FactoryGirl.create(:user) }
+      let(:valid_session) { { sec_token: user.security_token } }
+      let(:warden_context) { mock }
+      let(:request) { mock }
+
+      context 'when session is valid' do
+        before { warden_context.expects(:session).returns(valid_session) }
+
+        it { should be_true }
+      end
+
+      context 'when no session' do
+        before do
+          request.expects(:reset_session)
+          warden_context.expects(:session).returns({})
+        end
+
+        it { should be_false }
+      end
+
+      context 'when session was invalidated' do
+        let(:session) { { sec_token: 'old-security-token' } }
+
+        before { warden_context.expects(:session).returns(session) }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(Carto::ExpiredSessionError)
+        end
+      end
+
+      context 'when authenticating with a valid method and no session' do
+        before do
+          request.expects(:reset_session)
+          warden_context.expects(:session).raises(Warden::NotAuthenticated)
+        end
+
+        it { should be_false }
+      end
+
+      context "when security token does not match but using multifactor authentication" do
+        let(:session) { { sec_token: 'old-security-token' } }
+
+        before do
+          FactoryGirl.create(:totp, :active, user_id: user.id)
+          request.expects(:reset_session)
+          warden_context.expects(:session).returns(session)
+        end
+
+        it { should be_false }
+      end
+    end
+
+  end
+end

spec/requests/carto/api/organization_users_controller_spec.rb

@@ -100,6 +100,7 @@ describe Carto::Api::OrganizationUsersController do
 
   before(:each) do
     @old_soft_limits = soft_limits(@organization.owner)
+
     @old_whitelisted_email_domains = @organization.whitelisted_email_domains
   end
 
@@ -954,6 +955,24 @@ describe Carto::Api::OrganizationUsersController do
       last_response.status.should == 401
     end
 
+    it 'returns 401 when session is not valid' do
+      organization = create_organization_with_owner
+      user = FactoryGirl.create(:valid_user, organization: organization, org_admin: true)
+
+      login_response = post_session(user: user, password: 'kkkkkkkkk', organization: organization)
+      set_cookies_for_next_request(login_response)
+
+      get api_v2_organization_users_index_url(id_or_name: organization.name)
+      last_response.status.should == 200
+
+      user.invalidate_all_sessions!
+
+      set_cookies_for_next_request(login_response)
+
+      get api_v2_organization_users_index_url(id_or_name: organization.name)
+      last_response.status.should == 401
+    end
+
     it 'should list users' do
       login(@organization.owner)
 

spec/requests/sessions_controller_spec.rb

@@ -863,6 +863,7 @@ describe SessionsController do
 
           login
           get multifactor_authentication_session_url
+          cookies["_cartodb_session"] = response.cookies["_cartodb_session"]
           post multifactor_authentication_verify_code_url(user_id: @user.id, code: code)
           expect_login
         end

spec/spec_helper.rb

@@ -113,3 +113,25 @@ end
 def login_page_response?(response)
   response.status == 200 && response.body.include?("title=\"Email or username\"")
 end
+
+def post_session(params = {})
+  host! "#{params[:user].username}.localhost.lan"
+
+  request_params = { email: params[:user].email, password: params[:password] }
+  request_params[:user_domain] = params[:organization].name if params[:organization]
+
+  post(create_session_url(request_params))
+end
+
+def parse_set_cookie_header(header)
+  kv_pairs = header.split(/\s*;\s*/).map do |attr|
+    k, v = attr.split '='
+    [ k, v || nil ]
+  end
+  Hash[ kv_pairs ]
+end
+
+def set_cookies_for_next_request(previous_response)
+  received_cookies = parse_set_cookie_header(previous_response.headers["Set-Cookie"])
+  received_cookies.each { |key, value| cookies[key] = value }
+end