From 887b779b406e7cb301606d99ffb3b5faaf09f8a1 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 21 Jul 2021 12:00:13 +0000 Subject: [PATCH] Follow up - ca56de3648815d18e39b593ed0f600f3cd6c2214 - Maintenance: Updated to Rails 6.0.4 and the new Zeitwerk autoloader: Fixed deprecation of autoloading in initializers causing namespace lookup errors. --- app/models/session.rb | 13 ++++ config/initializers/db_preflight_check.rb | 2 +- config/initializers/logo.rb | 13 ++-- config/initializers/models_searchable.rb | 12 ++-- config/initializers/session_store.rb | 2 +- .../action_dispatch/middleware/cookies.rb | 17 +++++ lib/core_ext/rack/session/abstract/id.rb | 20 ++++++ lib/core_ext/rack/utils.rb | 22 +++++++ .../initializer/db_preflight_check.rb | 2 +- .../initializer/db_preflight_check/base.rb | 2 +- .../initializer/db_preflight_check/mysql2.rb | 2 +- .../initializer/db_preflight_check/nulldb.rb | 2 +- .../db_preflight_check/postgresql.rb | 2 +- .../application/initializer/session_store.rb | 24 ++++--- .../initializer/session_store_spec.rb | 31 ---------- spec/requests/external_credentials_spec.rb | 34 +++++++--- spec/requests/session_spec.rb | 62 +++++++++++++++++++ 17 files changed, 194 insertions(+), 68 deletions(-) create mode 100644 lib/core_ext/action_dispatch/middleware/cookies.rb create mode 100644 lib/core_ext/rack/session/abstract/id.rb create mode 100644 lib/core_ext/rack/utils.rb delete mode 100644 spec/lib/zammad/application/initializer/session_store_spec.rb diff --git a/app/models/session.rb b/app/models/session.rb index 0ac92f9c7..3f7e95741 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -2,4 +2,17 @@ class Session < ActiveRecord::SessionStore::Session include Session::SetsPersistentFlag + + def self.secure_flag? + # enable runtime change support in test/develop environments + return https? if !Rails.env.production? + + @secure_flag ||= https? + rescue ActiveRecord::NoDatabaseError, ActiveRecord::StatementInvalid + false + end + + def self.https? + Setting.get('http_type') == 'https' + end end diff --git a/config/initializers/db_preflight_check.rb b/config/initializers/db_preflight_check.rb index c28a31e78..115ce26ee 100644 --- a/config/initializers/db_preflight_check.rb +++ b/config/initializers/db_preflight_check.rb @@ -1,6 +1,6 @@ # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ # Rails' constant auto-loading resolves to 'rails/initializable' instead -require_dependency 'zammad/application/initializer/db_preflight_check' +require 'zammad/application/initializer/db_preflight_check' Zammad::Application::Initializer::DbPreflightCheck.perform diff --git a/config/initializers/logo.rb b/config/initializers/logo.rb index 2163a7753..b06c146e2 100644 --- a/config/initializers/logo.rb +++ b/config/initializers/logo.rb @@ -1,9 +1,12 @@ # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ -return if !ActiveRecord::Base.connected? +Rails.application.reloader.to_prepare do -# sync logo to fs / only if settings already exists -return if ActiveRecord::Base.connection.tables.exclude?('settings') -return if Setting.column_names.exclude?('state_current') + next if !ActiveRecord::Base.connected? -StaticAssets.sync + # sync logo to fs / only if settings already exists + next if ActiveRecord::Base.connection.tables.exclude?('settings') + next if Setting.column_names.exclude?('state_current') + + StaticAssets.sync +end diff --git a/config/initializers/models_searchable.rb b/config/initializers/models_searchable.rb index 394e54b02..13f50fe7d 100644 --- a/config/initializers/models_searchable.rb +++ b/config/initializers/models_searchable.rb @@ -2,10 +2,12 @@ # update settings for searchable models -begin - return if !Setting.exists?(name: 'models_searchable') +Rails.application.reloader.to_prepare do + begin + next if !Setting.exists?(name: 'models_searchable') - Setting.set('models_searchable', Models.searchable.map(&:to_s)) -rescue ActiveRecord::StatementInvalid - nil + Setting.set('models_searchable', Models.searchable.map(&:to_s)) + rescue ActiveRecord::StatementInvalid + nil + end end diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 9f611fda9..32a148e89 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -1,6 +1,6 @@ # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ # Rails' constant auto-loading resolves to 'rails/initializable' instead -require_dependency 'zammad/application/initializer/session_store' +require 'zammad/application/initializer/session_store' Zammad::Application::Initializer::SessionStore.perform diff --git a/lib/core_ext/action_dispatch/middleware/cookies.rb b/lib/core_ext/action_dispatch/middleware/cookies.rb new file mode 100644 index 000000000..5f36a4be0 --- /dev/null +++ b/lib/core_ext/action_dispatch/middleware/cookies.rb @@ -0,0 +1,17 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'action_dispatch/middleware/cookies' + +module ActionDispatch + class Cookies + class CookieJar + + alias original_write_cookie? write_cookie? + + # https://github.com/rails/rails/blob/v6.0.4/actionpack/lib/action_dispatch/middleware/cookies.rb#L447-L449 + def write_cookie?(cookie) + original_write_cookie?(cookie.merge(secure: ::Session.secure_flag?)) + end + end + end +end diff --git a/lib/core_ext/rack/session/abstract/id.rb b/lib/core_ext/rack/session/abstract/id.rb new file mode 100644 index 000000000..7e68c1a6b --- /dev/null +++ b/lib/core_ext/rack/session/abstract/id.rb @@ -0,0 +1,20 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rack/session/abstract/id' + +module Rack + module Session + module Abstract + class Persisted + + alias original_security_matches? security_matches? + + # https://github.com/rack/rack/blob/2.2.3/lib/rack/session/abstract/id.rb#L363-L366 + def security_matches?(request, options) + options[:secure] = ::Session.secure_flag? + original_security_matches?(request, options) + end + end + end + end +end diff --git a/lib/core_ext/rack/utils.rb b/lib/core_ext/rack/utils.rb new file mode 100644 index 000000000..39b5caa27 --- /dev/null +++ b/lib/core_ext/rack/utils.rb @@ -0,0 +1,22 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rack/utils' + +module Rack + module Utils + + module_function + + singleton_class.alias_method :original_add_cookie_to_header, :add_cookie_to_header + + # https://github.com/rack/rack/blob/2.2.3/lib/rack/session/utils.rb#L223-L262 + def add_cookie_to_header(header, key, value) + + if value.is_a?(Hash) + value[:secure] = ::Session.secure_flag? + end + + original_add_cookie_to_header(header, key, value) + end + end +end diff --git a/lib/zammad/application/initializer/db_preflight_check.rb b/lib/zammad/application/initializer/db_preflight_check.rb index 5ff55d0e4..7338e57c2 100644 --- a/lib/zammad/application/initializer/db_preflight_check.rb +++ b/lib/zammad/application/initializer/db_preflight_check.rb @@ -7,7 +7,7 @@ require 'zammad/application/initializer/db_preflight_check/nulldb' module Zammad class Application - class Initializer + module Initializer module DbPreflightCheck def self.perform adapter.perform diff --git a/lib/zammad/application/initializer/db_preflight_check/base.rb b/lib/zammad/application/initializer/db_preflight_check/base.rb index 272910e07..bb3c46067 100644 --- a/lib/zammad/application/initializer/db_preflight_check/base.rb +++ b/lib/zammad/application/initializer/db_preflight_check/base.rb @@ -13,7 +13,7 @@ module Zammad class Application - class Initializer + module Initializer module DbPreflightCheck module Base def check_version_compatibility diff --git a/lib/zammad/application/initializer/db_preflight_check/mysql2.rb b/lib/zammad/application/initializer/db_preflight_check/mysql2.rb index f69cf0d4a..01ff8dee1 100644 --- a/lib/zammad/application/initializer/db_preflight_check/mysql2.rb +++ b/lib/zammad/application/initializer/db_preflight_check/mysql2.rb @@ -12,7 +12,7 @@ module Zammad class Application - class Initializer + module Initializer module DbPreflightCheck module Mysql2 extend Base diff --git a/lib/zammad/application/initializer/db_preflight_check/nulldb.rb b/lib/zammad/application/initializer/db_preflight_check/nulldb.rb index cff45f70a..0ab00bd3f 100644 --- a/lib/zammad/application/initializer/db_preflight_check/nulldb.rb +++ b/lib/zammad/application/initializer/db_preflight_check/nulldb.rb @@ -2,7 +2,7 @@ module Zammad class Application - class Initializer + module Initializer module DbPreflightCheck module Nulldb # no-op diff --git a/lib/zammad/application/initializer/db_preflight_check/postgresql.rb b/lib/zammad/application/initializer/db_preflight_check/postgresql.rb index c321ad19c..f163b7a81 100644 --- a/lib/zammad/application/initializer/db_preflight_check/postgresql.rb +++ b/lib/zammad/application/initializer/db_preflight_check/postgresql.rb @@ -12,7 +12,7 @@ module Zammad class Application - class Initializer + module Initializer module DbPreflightCheck module Postgresql extend Base diff --git a/lib/zammad/application/initializer/session_store.rb b/lib/zammad/application/initializer/session_store.rb index 689e42ffb..35090de9b 100644 --- a/lib/zammad/application/initializer/session_store.rb +++ b/lib/zammad/application/initializer/session_store.rb @@ -6,24 +6,28 @@ module Zammad class Application - class Initializer + module Initializer module SessionStore STORE_TYPE = :active_record_store # default: :cookie_store SESSION_KEY = "_zammad_session_#{Digest::MD5.hexdigest(Rails.root.to_s)[5..15]}".freeze # default: '_zammad_session' def self.perform - ActionDispatch::Session::ActiveRecordStore.session_class = Session + # it's important to register the session store at initialization time + # otherwise the store won't be used + # ATTENTION: Rails/Rack Cookie handling was customized to call `Session.secure_flag?` + # instead of accessing the `:secure` key (default Rack/Rails behavior). + # See: lib/core_ext/action_dispatch/middleware/cookies.rb + # See: lib/core_ext/rack/session/abstract/id.rb + # See: lib/core_ext/rack/session/utils.rb Rails.application.config.session_store STORE_TYPE, - key: SESSION_KEY, - secure: secure? - end + key: SESSION_KEY - def self.secure? - Setting.get('http_type') == 'https' - rescue ActiveRecord::StatementInvalid - false + # once the application is initialized and we can access the models + # we need to update the session_class + Rails.application.reloader.to_prepare do + ActionDispatch::Session::ActiveRecordStore.session_class = Session + end end - private_class_method :secure? end end end diff --git a/spec/lib/zammad/application/initializer/session_store_spec.rb b/spec/lib/zammad/application/initializer/session_store_spec.rb deleted file mode 100644 index 06735e4c9..000000000 --- a/spec/lib/zammad/application/initializer/session_store_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ - -require 'rails_helper' - -RSpec.describe Zammad::Application::Initializer::SessionStore do - describe '.perform' do - context 'for HTTP deployment' do - before { Setting.set('http_type', 'http') } - - # Why not use the "change" matcher in this example? - # - # This initializer is already run when the application is loaded for testing. - # Since the test env always uses http, the :secure option is already set to false. - it 'adds { secure: false } to application session options' do - described_class.perform - - expect(Rails.application.config.session_options).to include(secure: false) - end - end - - context 'for HTTPS deployment' do - before { Setting.set('http_type', 'https') } - - it 'adds { secure: true } to application session options' do - expect { described_class.perform } - .to change(Rails.application.config, :session_options) - .to include(secure: true) - end - end - end -end diff --git a/spec/requests/external_credentials_spec.rb b/spec/requests/external_credentials_spec.rb index 4cb2a4ca5..2a7713073 100644 --- a/spec/requests/external_credentials_spec.rb +++ b/spec/requests/external_credentials_spec.rb @@ -192,7 +192,7 @@ RSpec.describe 'External Credentials', type: :request do shared_examples 'for failure cases' do it 'responds with the appropriate status and error message' do - send(*endpoint, as: :json, params: try(:params) || {}) + send(*endpoint, as: :json, params: try(:params) || {}, headers: headers) expect(response).to have_http_status(status) expect(json_response).to include('error' => error_message) @@ -415,7 +415,14 @@ RSpec.describe 'External Credentials', type: :request do let!(:twitter_credential) { create(:twitter_credential) } - before { get '/api/v1/external_credentials/twitter/link_account', as: :json } + # Rails / Rack needs to detect that the request comes via HTTPS as well + let(:headers) do + { + 'X-Forwarded-Proto' => 'https' + } + end + + before { get '/api/v1/external_credentials/twitter/link_account', as: :json, headers: headers } include_examples 'for failure cases' do let(:status) { :unprocessable_entity } @@ -458,7 +465,14 @@ RSpec.describe 'External Credentials', type: :request do let(:oauth_token) { 'DyhnyQAAAAAA9CNXAAABcSxAexs' } let(:oauth_verifier) { '15DOeRkjP4JkOSVqULkTKA1SCuIPP105' } - before { get '/api/v1/external_credentials/twitter/link_account', as: :json } + # Rails / Rack needs to detect that the request comes via HTTPS as well + let(:headers) do + { + 'X-Forwarded-Proto' => 'https' + } + end + + before { get '/api/v1/external_credentials/twitter/link_account', as: :json, headers: headers } context 'if Twitter account has already been added' do let!(:channel) { create(:twitter_channel, custom_options: channel_options) } @@ -472,12 +486,12 @@ RSpec.describe 'External Credentials', type: :request do end it 'uses the existing channel' do - expect { send(*endpoint, as: :json, params: params) } + expect { send(*endpoint, as: :json, params: params, headers: headers) } .not_to change(Channel, :count) end it 'updates channel properties' do - expect { send(*endpoint, as: :json, params: params) } + expect { send(*endpoint, as: :json, params: params, headers: headers) } .to change { channel.reload.options[:user][:name] } .and change { channel.reload.options[:auth][:external_credential_id] } .and change { channel.reload.options[:auth][:oauth_token] } @@ -485,7 +499,7 @@ RSpec.describe 'External Credentials', type: :request do end it 'subscribes to webhooks' do - send(*endpoint, as: :json, params: params) + send(*endpoint, as: :json, params: params, headers: headers) expect(WebMock) .to have_requested(:post, "https://api.twitter.com/1.1/account_activity/all/#{twitter_credential.credentials[:env]}/subscriptions.json") @@ -496,7 +510,7 @@ RSpec.describe 'External Credentials', type: :request do end it 'creates a new channel' do - expect { send(*endpoint, as: :json, params: params) } + expect { send(*endpoint, as: :json, params: params, headers: headers) } .to change(Channel, :count).by(1) expect(Channel.last.options) @@ -506,19 +520,19 @@ RSpec.describe 'External Credentials', type: :request do end it 'redirects to the newly created channel' do - send(*endpoint, as: :json, params: params) + send(*endpoint, as: :json, params: params, headers: headers) expect(response).to redirect_to(%r{/#channels/twitter/#{Channel.last.id}$}) end it 'clears the :request_token session variable' do - send(*endpoint, as: :json, params: params) + send(*endpoint, as: :json, params: params, headers: headers) expect(session[:request_token]).to be(nil) end it 'subscribes to webhooks' do - send(*endpoint, as: :json, params: params) + send(*endpoint, as: :json, params: params, headers: headers) expect(WebMock) .to have_requested(:post, "https://api.twitter.com/1.1/account_activity/all/#{twitter_credential.credentials[:env]}/subscriptions.json") diff --git a/spec/requests/session_spec.rb b/spec/requests/session_spec.rb index 45a42e02b..7ec168eae 100644 --- a/spec/requests/session_spec.rb +++ b/spec/requests/session_spec.rb @@ -4,6 +4,68 @@ require 'rails_helper' RSpec.describe 'Sessions endpoints', type: :request do + describe 'GET /' do + + let(:headers) { {} } + let(:session_key) { Zammad::Application::Initializer::SessionStore::SESSION_KEY } + + before do + Setting.set('http_type', http_type) + + get '/', headers: headers + end + + context "when Setting 'http_type' is set to 'https'" do + + let(:http_type) { 'https' } + + context "when it's not an HTTPS request" do + + it 'sets no Cookie' do + expect(response.header['Set-Cookie']).to be_nil + end + end + + context "when it's an HTTPS request" do + + let(:headers) do + { + 'X-Forwarded-Proto' => 'https' + } + end + + it "sets Cookie with 'secure' flag" do + expect(response.header['Set-Cookie']).to include(session_key).and include('; secure;') + end + end + end + + context "when Setting 'http_type' is set to 'http'" do + + let(:http_type) { 'http' } + + context "when it's not an HTTPS request" do + + it 'sets Cookie' do + expect(response.header['Set-Cookie']).to include(session_key).and not_include('; secure;') + end + end + + context "when it's an HTTPS request" do + + let(:headers) do + { + 'X-Forwarded-Proto' => 'https' + } + end + + it "sets Cookie without 'secure' flag" do + expect(response.header['Set-Cookie']).to include(session_key).and not_include('; secure;') + end + end + end + end + describe 'GET /signshow' do context 'user logged in' do