From 989c67d29d379f872f23a513f43d5100630c1b12 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 5 Nov 2021 21:14:35 +0100 Subject: Fix handling announcements with links (#16941) Broken since #15827 --- app/models/status.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/status.rb b/app/models/status.rb index 3acf759f9..c7f761bc6 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -338,7 +338,7 @@ class Status < ApplicationRecord def from_text(text) return [] if text.blank? - text.scan(FetchLinkCardService::URL_PATTERN).map(&:first).uniq.filter_map do |url| + text.scan(FetchLinkCardService::URL_PATTERN).map(&:second).uniq.filter_map do |url| status = begin if TagManager.instance.local_url?(url) ActivityPub::TagManager.instance.uri_to_resource(url, Status) -- cgit From 87085a5152011b2f5595feba2a6c4d56a2b425f0 Mon Sep 17 00:00:00 2001 From: Claire Date: Sat, 6 Nov 2021 00:12:25 +0100 Subject: Fix AccountNote not having a maximum length (#16942) --- app/models/account_note.rb | 1 + app/workers/move_worker.rb | 8 +++- .../api/v1/accounts/notes_controller_spec.rb | 48 ++++++++++++++++++++++ spec/workers/move_worker_spec.rb | 41 +++++++++++++----- 4 files changed, 86 insertions(+), 12 deletions(-) create mode 100644 spec/controllers/api/v1/accounts/notes_controller_spec.rb (limited to 'app/models') diff --git a/app/models/account_note.rb b/app/models/account_note.rb index bf61df923..b338bc92f 100644 --- a/app/models/account_note.rb +++ b/app/models/account_note.rb @@ -17,4 +17,5 @@ class AccountNote < ApplicationRecord belongs_to :target_account, class_name: 'Account' validates :account_id, uniqueness: { scope: :target_account_id } + validates :comment, length: { maximum: 2_000 } end diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb index cc2c17d32..4a900e3b8 100644 --- a/app/workers/move_worker.rb +++ b/app/workers/move_worker.rb @@ -53,10 +53,16 @@ class MoveWorker new_note = AccountNote.find_by(account: note.account, target_account: @target_account) if new_note.nil? - AccountNote.create!(account: note.account, target_account: @target_account, comment: [text, note.comment].join("\n")) + begin + AccountNote.create!(account: note.account, target_account: @target_account, comment: [text, note.comment].join("\n")) + rescue ActiveRecord::RecordInvalid + AccountNote.create!(account: note.account, target_account: @target_account, comment: note.comment) + end else new_note.update!(comment: [text, note.comment, "\n", new_note.comment].join("\n")) end + rescue ActiveRecord::RecordInvalid + nil rescue => e @deferred_error = e end diff --git a/spec/controllers/api/v1/accounts/notes_controller_spec.rb b/spec/controllers/api/v1/accounts/notes_controller_spec.rb new file mode 100644 index 000000000..0a2957fed --- /dev/null +++ b/spec/controllers/api/v1/accounts/notes_controller_spec.rb @@ -0,0 +1,48 @@ +require 'rails_helper' + +describe Api::V1::Accounts::NotesController do + render_views + + let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'write:accounts') } + let(:account) { Fabricate(:account) } + let(:comment) { 'foo' } + + before do + allow(controller).to receive(:doorkeeper_token) { token } + end + + describe 'POST #create' do + subject do + post :create, params: { account_id: account.id, comment: comment } + end + + context 'when account note has reasonable length' do + let(:comment) { 'foo' } + + it 'returns http success' do + subject + expect(response).to have_http_status(200) + end + + it 'updates account note' do + subject + expect(AccountNote.find_by(account_id: user.account.id, target_account_id: account.id).comment).to eq comment + end + end + + context 'when account note exceends allowed length' do + let(:comment) { 'a' * 2_001 } + + it 'returns 422' do + subject + expect(response).to have_http_status(422) + end + + it 'does not create account note' do + subject + expect(AccountNote.where(account_id: user.account.id, target_account_id: account.id).exists?).to be_falsey + end + end + end +end diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb index 8ab4f182f..82449b0c7 100644 --- a/spec/workers/move_worker_spec.rb +++ b/spec/workers/move_worker_spec.rb @@ -9,7 +9,8 @@ describe MoveWorker do let(:source_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } let(:target_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } let(:local_user) { Fabricate(:user) } - let!(:account_note) { Fabricate(:account_note, account: local_user.account, target_account: source_account) } + let(:comment) { 'old note prior to move' } + let!(:account_note) { Fabricate(:account_note, account: local_user.account, target_account: source_account, comment: comment) } let(:block_service) { double } @@ -26,19 +27,37 @@ describe MoveWorker do end shared_examples 'user note handling' do - it 'copies user note' do - subject.perform(source_account.id, target_account.id) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + context 'when user notes are short enough' do + it 'copies user note with prelude' do + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + end + + it 'merges user notes when needed' do + new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + end end - it 'merges user notes when needed' do - new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + context 'when user notes are too long' do + let(:comment) { 'abc' * 333 } - subject.perform(source_account.id, target_account.id) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + it 'copies user note without prelude' do + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + end + + it 'keeps user notes unchanged' do + new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + end end end -- cgit From 6da135a493cc039d92bb5925c2a1ef66025623bf Mon Sep 17 00:00:00 2001 From: Claire Date: Sat, 6 Nov 2021 00:13:58 +0100 Subject: Fix reviving revoked sessions and invalidating login (#16943) Up until now, we have used Devise's Rememberable mechanism to re-log users after the end of their browser sessions. This mechanism relies on a signed cookie containing a token. That token was stored on the user's record, meaning it was shared across all logged in browsers, meaning truly revoking a browser's ability to auto-log-in involves revoking the token itself, and revoking access from *all* logged-in browsers. We had a session mechanism that dynamically checks whether a user's session has been disabled, and would log out the user if so. However, this would only clear a session being actively used, and a new one could be respawned with the `remember_user_token` cookie. In practice, this caused two issues: - sessions could be revived after being closed from /auth/edit (security issue) - auto-log-in would be disabled for *all* browsers after logging out from one of them This PR removes the `remember_token` mechanism and treats the `_session_id` cookie/token as a browser-specific `remember_token`, fixing both issues. --- app/controllers/auth/passwords_controller.rb | 1 - app/controllers/auth/registrations_controller.rb | 3 -- app/controllers/auth/sessions_controller.rb | 3 -- app/models/user.rb | 2 +- config/initializers/devise.rb | 39 ++++++++++++++++++++++-- 5 files changed, 37 insertions(+), 11 deletions(-) (limited to 'app/models') diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index 5db2668f7..2996c0431 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -10,7 +10,6 @@ class Auth::PasswordsController < Devise::PasswordsController super do |resource| if resource.errors.empty? resource.session_activations.destroy_all - resource.forget_me! end end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index a3114ab25..3c1730f25 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Auth::RegistrationsController < Devise::RegistrationsController - include Devise::Controllers::Rememberable include RegistrationSpamConcern layout :determine_layout @@ -30,8 +29,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController super do |resource| if resource.saved_change_to_encrypted_password? resource.clear_other_sessions(current_session.session_id) - resource.forget_me! - remember_me(resource) end end end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index d48abb707..0184bfb52 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class Auth::SessionsController < Devise::SessionsController - include Devise::Controllers::Rememberable - layout 'auth' skip_before_action :require_no_authentication, only: [:create] @@ -150,7 +148,6 @@ class Auth::SessionsController < Devise::SessionsController clear_attempt_from_session user.update_sign_in!(request, new_sign_in: true) - remember_me(user) sign_in(user) flash.delete(:notice) diff --git a/app/models/user.rb b/app/models/user.rb index 4059c96b5..c4dec4813 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -64,7 +64,7 @@ class User < ApplicationRecord devise :two_factor_backupable, otp_number_of_backup_codes: 10 - devise :registerable, :recoverable, :rememberable, :validatable, + devise :registerable, :recoverable, :validatable, :confirmable include Omniauthable diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index ef612e177..5232e6cfd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,3 +1,5 @@ +require 'devise/strategies/authenticatable' + Warden::Manager.after_set_user except: :fetch do |user, warden| if user.session_active?(warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']) session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'] @@ -72,17 +74,48 @@ module Devise mattr_accessor :ldap_uid_conversion_replace @@ldap_uid_conversion_replace = nil - class Strategies::PamAuthenticatable - def valid? - super && ::Devise.pam_authentication + module Strategies + class PamAuthenticatable + def valid? + super && ::Devise.pam_authentication + end + end + + class SessionActivationRememberable < Authenticatable + def valid? + @session_cookie = nil + session_cookie.present? + end + + def authenticate! + resource = SessionActivation.find_by(session_id: session_cookie)&.user + + unless resource + cookies.delete('_session_id') + return pass + end + + if validate(resource) + success!(resource) + end + end + + private + + def session_cookie + @session_cookie ||= cookies.signed['_session_id'] + end end end end +Warden::Strategies.add(:session_activation_rememberable, Devise::Strategies::SessionActivationRememberable) + Devise.setup do |config| config.warden do |manager| manager.default_strategies(scope: :user).unshift :two_factor_ldap_authenticatable if Devise.ldap_authentication manager.default_strategies(scope: :user).unshift :two_factor_pam_authenticatable if Devise.pam_authentication + manager.default_strategies(scope: :user).unshift :session_activation_rememberable manager.default_strategies(scope: :user).unshift :two_factor_authenticatable manager.default_strategies(scope: :user).unshift :two_factor_backupable end -- cgit