From 92c9092abda58ee27e28faaea7ea84b89b45f9e4 Mon Sep 17 00:00:00 2001 From: Free Speech Forever Date: Mon, 15 Feb 2021 23:26:00 +0000 Subject: [PATCH] Avoid redundant OAuth queries when not signed in If you aren't signed in, you don't have an auth token. When you don't have an auth token, React was sending the headers "Authorization: Bearer null" This caused 5 Doorkeeper token lookups using WHERE "oauth_access_tokens"."token" = 'null' on the Explore page (the root of the app when not signed in). --- Gemfile | 4 +- Gemfile.lock | 20 ++++++---- app/javascript/gabsocial/actions/lists.js | 46 +++++++++++----------- app/javascript/gabsocial/api.js | 28 +++++++------ app/models/account_moderation_note.rb | 5 --- app/models/account_verification_request.rb | 4 +- app/models/application_record.rb | 2 +- app/models/report.rb | 5 --- app/models/report_note.rb | 5 --- app/models/session_activation.rb | 21 ++++------ config/database.yml | 4 +- config/initializers/devise.rb | 8 ++++ config/initializers/omniauth.rb | 7 ++++ 13 files changed, 82 insertions(+), 77 deletions(-) diff --git a/Gemfile b/Gemfile index 80b5e867..3d2af797 100644 --- a/Gemfile +++ b/Gemfile @@ -30,7 +30,9 @@ gem 'charlock_holmes', '~> 0.7.6' gem 'iso-639' gem 'chewy', '~> 5.0' gem 'cld3', '~> 3.2.4' -gem 'devise', '~> 4.6' +git 'https://github.com/freespeech4ever/devise.git', branch: 'gab2' do + gem 'devise' +end gem 'devise-two-factor', '~> 3.0' group :pam_authentication, optional: true do diff --git a/Gemfile.lock b/Gemfile.lock index f4215c20..d0fca141 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,15 @@ +GIT + remote: https://github.com/freespeech4ever/devise.git + revision: 4009905531f28ebd7ecab22f898b5d6180eefb4a + branch: gab2 + specs: + devise (4.7.3) + bcrypt (~> 3.0) + orm_adapter (~> 0.1) + railties (>= 4.1.0) + responders + warden (~> 1.2.3) + GIT remote: https://github.com/rtomayko/posix-spawn revision: 58465d2e213991f8afb13b984854a49fcdcc980c @@ -183,12 +195,6 @@ GEM rake (> 10, < 14) ruby-statistics (>= 2.1) thor (>= 0.19, < 2) - devise (4.7.3) - bcrypt (~> 3.0) - orm_adapter (~> 0.1) - railties (>= 4.1.0) - responders - warden (~> 1.2.3) devise-two-factor (3.1.0) activesupport (< 6.1) attr_encrypted (>= 1.3, < 4, != 2) @@ -696,7 +702,7 @@ DEPENDENCIES climate_control (~> 0.2) concurrent-ruby derailed_benchmarks - devise (~> 4.6) + devise! devise-two-factor (~> 3.0) devise_pam_authenticatable2 (~> 9.2) doorkeeper (~> 5.1) diff --git a/app/javascript/gabsocial/actions/lists.js b/app/javascript/gabsocial/actions/lists.js index f943c5cf..1dfaf8a7 100644 --- a/app/javascript/gabsocial/actions/lists.js +++ b/app/javascript/gabsocial/actions/lists.js @@ -51,7 +51,7 @@ export const LIST_ADDER_LISTS_FETCH_SUCCESS = 'LIST_ADDER_LISTS_FETCH_SUCCESS' export const LIST_ADDER_LISTS_FETCH_FAIL = 'LIST_ADDER_LISTS_FETCH_FAIL' /** - * + * */ export const fetchList = (id) => (dispatch, getState) => { if (!me) return @@ -82,7 +82,7 @@ const fetchListFail = (id, error) => ({ }) /** - * + * */ export const fetchLists = () => (dispatch, getState) => { return new Promise((resolve, reject) => { @@ -109,12 +109,12 @@ const fetchListsSuccess = (lists) => ({ const fetchListsFail = (error) => ({ type: LISTS_FETCH_FAIL, - showToast: true, + showToast: false, error, }) /** - * + * */ export const submitListEditor = (shouldReset) => (dispatch, getState) => { const listId = getState().getIn(['listEditor', 'listId']) @@ -128,7 +128,7 @@ export const submitListEditor = (shouldReset) => (dispatch, getState) => { } /** - * + * */ export const setupListEditor = (listId) => (dispatch, getState) => { dispatch({ @@ -140,7 +140,7 @@ export const setupListEditor = (listId) => (dispatch, getState) => { } /** - * + * */ export const changeListEditorTitle = (value) => ({ type: LIST_EDITOR_TITLE_CHANGE, @@ -148,7 +148,7 @@ export const changeListEditorTitle = (value) => ({ }) /** - * + * */ export const createList = (title, shouldReset) => (dispatch, getState) => { if (!me) return @@ -181,7 +181,7 @@ export const createListFail = (error) => ({ }) /** - * + * */ export const updateList = (id, title, shouldReset) => (dispatch, getState) => { if (!me) return @@ -220,7 +220,7 @@ export const resetListEditor = () => ({ }) /** - * + * */ export const deleteList = (id) => (dispatch, getState) => { if (!me) return @@ -251,7 +251,7 @@ export const deleteListFail = (id, error) => ({ }) /** - * + * */ export const fetchListAccounts = (listId) => (dispatch, getState) => { if (!me) return @@ -284,7 +284,7 @@ export const fetchListAccountsFail = (id, error) => ({ }) /** - * + * */ export const fetchListSuggestions = (q) => (dispatch, getState) => { if (!me) return @@ -303,7 +303,7 @@ export const fetchListSuggestions = (q) => (dispatch, getState) => { } /** - * + * */ const fetchListSuggestionsReady = (query, accounts) => ({ type: LIST_EDITOR_SUGGESTIONS_READY, @@ -312,14 +312,14 @@ const fetchListSuggestionsReady = (query, accounts) => ({ }) /** - * + * */ export const clearListSuggestions = () => ({ type: LIST_EDITOR_SUGGESTIONS_CLEAR, }) /** - * + * */ export const changeListSuggestions = (value) => ({ type: LIST_EDITOR_SUGGESTIONS_CHANGE, @@ -327,14 +327,14 @@ export const changeListSuggestions = (value) => ({ }) /** - * + * */ export const addToListEditor = accountId => (dispatch, getState) => { dispatch(addToList(getState().getIn(['listEditor', 'listId']), accountId)) } /** - * + * */ export const addToList = (listId, accountId) => (dispatch, getState) => { if (!me) return @@ -368,14 +368,14 @@ const addToListFail = (listId, accountId, error) => ({ }) /** - * + * */ export const removeFromListEditor = accountId => (dispatch, getState) => { dispatch(removeFromList(getState().getIn(['listEditor', 'listId']), accountId)) } /** - * + * */ export const removeFromList = (listId, accountId) => (dispatch, getState) => { if (!me) return @@ -409,14 +409,14 @@ const removeFromListFail = (listId, accountId, error) => ({ }) /** - * + * */ export const resetListAdder = () => ({ type: LIST_ADDER_RESET, }) /** - * + * */ export const setupListAdder = accountId => (dispatch, getState) => { dispatch({ @@ -428,7 +428,7 @@ export const setupListAdder = accountId => (dispatch, getState) => { } /** - * + * */ export const fetchAccountLists = (accountId) => (dispatch, getState) => { if (!me) return @@ -459,14 +459,14 @@ const fetchAccountListsFail = (id, error) => ({ }) /** - * + * */ export const addToListAdder = (listId) => (dispatch, getState) => { dispatch(addToList(listId, getState().getIn(['listAdder', 'accountId']))) } /** - * + * */ export const removeFromListAdder = (listId) => (dispatch, getState) => { dispatch(removeFromList(listId, getState().getIn(['listAdder', 'accountId']))) diff --git a/app/javascript/gabsocial/api.js b/app/javascript/gabsocial/api.js index 9d07fbaa..19a80fa1 100644 --- a/app/javascript/gabsocial/api.js +++ b/app/javascript/gabsocial/api.js @@ -25,16 +25,20 @@ function setCSRFHeader() { ready(setCSRFHeader); -export default getState => axios.create({ - headers: Object.assign(csrfHeader, getState ? { - 'Authorization': `Bearer ${getState().getIn(['meta', 'access_token'], '')}`, - } : {}), +export default getState => { + const authToken = getState ? getState().getIn(['meta', 'access_token'], '') : null; - transformResponse: [function (data) { - try { - return JSON.parse(data); - } catch (Exception) { - return data; - } - }], -}); + return axios.create({ + headers: Object.assign(csrfHeader, authToken ? { + 'Authorization': `Bearer ${authToken}}`, + } : {}), + + transformResponse: [function (data) { + try { + return JSON.parse(data); + } catch (Exception) { + return data; + } + }], + }); +}; diff --git a/app/models/account_moderation_note.rb b/app/models/account_moderation_note.rb index c3e8c253..22e312bb 100644 --- a/app/models/account_moderation_note.rb +++ b/app/models/account_moderation_note.rb @@ -15,11 +15,6 @@ class AccountModerationNote < ApplicationRecord belongs_to :account belongs_to :target_account, class_name: 'Account' - connects_to database: { - writing: :master, - reading: :master - } - scope :latest, -> { reorder('created_at DESC') } validates :content, presence: true, length: { maximum: 500 } diff --git a/app/models/account_verification_request.rb b/app/models/account_verification_request.rb index 7fb3b0a4..96139596 100644 --- a/app/models/account_verification_request.rb +++ b/app/models/account_verification_request.rb @@ -15,8 +15,8 @@ class AccountVerificationRequest < ApplicationRecord connects_to database: { - writing: :master, - reading: :master + writing: :primary, + reading: :primary } LIMIT = 4.megabytes diff --git a/app/models/application_record.rb b/app/models/application_record.rb index e1e79acc..d603a56e 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -2,7 +2,7 @@ class ApplicationRecord < ActiveRecord::Base connects_to database: { - writing: :master, + writing: :primary, reading: :slave1 } self.abstract_class = true diff --git a/app/models/report.rb b/app/models/report.rb index 71f8ff7e..5d43d9b1 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -29,11 +29,6 @@ class Report < ApplicationRecord validates :comment, length: { maximum: 1000 } - connects_to database: { - writing: :master, - reading: :master - } - def local? false # Force uri_for to use uri attribute end diff --git a/app/models/report_note.rb b/app/models/report_note.rb index fa330f2a..b96a4361 100644 --- a/app/models/report_note.rb +++ b/app/models/report_note.rb @@ -19,9 +19,4 @@ class ReportNote < ApplicationRecord validates :content, presence: true, length: { maximum: 500 } - connects_to database: { - writing: :master, - reading: :master - } - end diff --git a/app/models/session_activation.rb b/app/models/session_activation.rb index 4a2a25c4..95626140 100644 --- a/app/models/session_activation.rb +++ b/app/models/session_activation.rb @@ -23,11 +23,6 @@ class SessionActivation < ApplicationRecord to: :access_token, allow_nil: true - connects_to database: { - writing: :master, - reading: :master - } - def detection @detection ||= Browser.new(user_agent) end @@ -45,7 +40,9 @@ class SessionActivation < ApplicationRecord class << self def active?(id) - id && where(session_id: id).exists? + ActiveRecord::Base.connected_to(role: :writing) do + id && where(session_id: id).exists? + end end def activate(**options) @@ -61,19 +58,16 @@ class SessionActivation < ApplicationRecord def deactivate(id) return unless id - ActiveRecord::Base.connected_to(role: :writing) do - conn = ActiveRecord::Base.connection - conn.exec_query "delete from session_activations where session_id = '#{id}'" - end + where(session_id: id).destroy_all end def purge_old order('created_at desc').offset(Rails.configuration.x.max_session_activations).destroy_all end - def exclusive(id) - where('session_id != ?', id).destroy_all - end + #def exclusive(id) + # where('session_id != ?', id).destroy_all + #end end private @@ -93,6 +87,5 @@ class SessionActivation < ApplicationRecord expires_in: Doorkeeper.configuration.access_token_expires_in, use_refresh_token: Doorkeeper.configuration.refresh_token_enabled?) end - self.access_token end end diff --git a/config/database.yml b/config/database.yml index 03878e8d..3a831ec7 100644 --- a/config/database.yml +++ b/config/database.yml @@ -7,7 +7,7 @@ default: &default prepared_statements: <%= ENV['PREPARED_STATEMENTS'] || 'false' %> development: - master: + primary: <<: *default url: <%= ENV['DB_MASTER_URL'] %> slave1: @@ -39,7 +39,7 @@ test: # port: <%= ENV['DB_PORT'] || 5432 %> # prepared_statements: <%= ENV['PREPARED_STATEMENTS'] || 'true' %> production: - master: + primary: <<: *default url: <%= ENV['DB_MASTER_URL'] %> slave1: diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 2e18f789..2149e2c9 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -76,6 +76,14 @@ module Devise end Devise.setup do |config| + + config.warden_hook_save_wrapper = Proc.new do |hook| + # ensure the writable connection is used to avoid read-only write errors + ApplicationRecord.connected_to(role: :writing) do + hook.call + end + end + config.warden do |manager| manager.default_strategies(scope: :user).unshift :ldap_authenticatable if Devise.ldap_authentication manager.default_strategies(scope: :user).unshift :pam_authenticatable if Devise.pam_authentication diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index e8d7697a..47a45aea 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -7,6 +7,13 @@ Devise.setup do |config| options = {} options[:redirect_at_sign_in] = ENV['OAUTH_REDIRECT_AT_SIGN_IN'] == 'true' + config.warden_hook_save_wrapper = Proc.new do |hook| + # ensure the writable connection is used to avoid read-only write errors + ApplicationRecord.connected_to(role: :writing) do + hook.call + end + end + # CAS strategy if ENV['CAS_ENABLED'] == 'true' cas_options = options