From 1eda25f186ad274bd50896dd5eb62bd9a734bbc7 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 29 Jan 2024 12:02:41 +0100 Subject: [PATCH 01/12] Fix insufficient origin validation --- .../concerns/signature_verification.rb | 2 +- app/helpers/jsonld_helper.rb | 4 ++-- app/lib/activitypub/activity.rb | 2 +- app/lib/activitypub/linked_data_signature.rb | 2 +- .../activitypub/fetch_remote_account_service.rb | 2 +- .../activitypub/fetch_remote_actor_service.rb | 6 +++--- .../activitypub/fetch_remote_key_service.rb | 17 ++--------------- .../activitypub/fetch_remote_status_service.rb | 8 ++++---- .../activitypub/process_account_service.rb | 2 +- app/services/fetch_resource_service.rb | 10 +++++++++- .../activitypub/linked_data_signature_spec.rb | 4 ++-- .../fetch_remote_account_service_spec.rb | 2 +- .../fetch_remote_actor_service_spec.rb | 2 +- .../fetch_remote_key_service_spec.rb | 2 +- spec/services/fetch_resource_service_spec.rb | 10 +++++----- spec/services/resolve_url_service_spec.rb | 1 + 16 files changed, 36 insertions(+), 40 deletions(-) diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 35391e64c4..92f1eb5a16 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -266,7 +266,7 @@ module SignatureVerification stoplight_wrap_request { ResolveAccountService.new.call(key_id.delete_prefix('acct:'), suppress_errors: false) } elsif !ActivityPub::TagManager.instance.local_uri?(key_id) account = ActivityPub::TagManager.instance.uri_to_actor(key_id) - account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false, suppress_errors: false) } + account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, suppress_errors: false) } account end rescue Mastodon::PrivateNetworkAddressError => e diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index ce3ff094f6..1b840f566b 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -155,8 +155,8 @@ module JsonLdHelper end end - def fetch_resource(uri, id, on_behalf_of = nil) - unless id + def fetch_resource(uri, id_is_known, on_behalf_of = nil) + unless id_is_known json = fetch_resource_without_id_validation(uri, on_behalf_of) return if !json.is_a?(Hash) || unsupported_uri_scheme?(json['id']) diff --git a/app/lib/activitypub/activity.rb b/app/lib/activitypub/activity.rb index 51384ef984..322f3e27ad 100644 --- a/app/lib/activitypub/activity.rb +++ b/app/lib/activitypub/activity.rb @@ -154,7 +154,7 @@ class ActivityPub::Activity if object_uri.start_with?('http') return if ActivityPub::TagManager.instance.local_uri?(object_uri) - ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true, on_behalf_of: @account.followers.local.first, request_id: @options[:request_id]) + ActivityPub::FetchRemoteStatusService.new.call(object_uri, on_behalf_of: @account.followers.local.first, request_id: @options[:request_id]) elsif @object['url'].present? ::FetchRemoteStatusService.new.call(@object['url'], request_id: @options[:request_id]) end diff --git a/app/lib/activitypub/linked_data_signature.rb b/app/lib/activitypub/linked_data_signature.rb index faea63e8f1..9459fdd8b7 100644 --- a/app/lib/activitypub/linked_data_signature.rb +++ b/app/lib/activitypub/linked_data_signature.rb @@ -19,7 +19,7 @@ class ActivityPub::LinkedDataSignature return unless type == 'RsaSignature2017' creator = ActivityPub::TagManager.instance.uri_to_actor(creator_uri) - creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false) if creator&.public_key.blank? + creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri) if creator&.public_key.blank? return if creator.nil? diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 567dd8a14a..7b083d889b 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -2,7 +2,7 @@ class ActivityPub::FetchRemoteAccountService < ActivityPub::FetchRemoteActorService # Does a WebFinger roundtrip on each call, unless `only_key` is true - def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) + def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) actor = super return actor if actor.nil? || actor.is_a?(Account) diff --git a/app/services/activitypub/fetch_remote_actor_service.rb b/app/services/activitypub/fetch_remote_actor_service.rb index 8df8c75876..86a134bb4e 100644 --- a/app/services/activitypub/fetch_remote_actor_service.rb +++ b/app/services/activitypub/fetch_remote_actor_service.rb @@ -10,15 +10,15 @@ class ActivityPub::FetchRemoteActorService < BaseService SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze # Does a WebFinger roundtrip on each call, unless `only_key` is true - def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) + def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) return if domain_not_allowed?(uri) return ActivityPub::TagManager.instance.uri_to_actor(uri) if ActivityPub::TagManager.instance.local_uri?(uri) @json = begin if prefetched_body.nil? - fetch_resource(uri, id) + fetch_resource(uri, true) else - body_to_json(prefetched_body, compare_id: id ? uri : nil) + body_to_json(prefetched_body, compare_id: uri) end rescue Oj::ParseError raise Error, "Error parsing JSON-LD document #{uri}" diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index 8eb97c1e66..e96b5ad3bb 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/app/services/activitypub/fetch_remote_key_service.rb @@ -6,23 +6,10 @@ class ActivityPub::FetchRemoteKeyService < BaseService class Error < StandardError; end # Returns actor that owns the key - def call(uri, id: true, prefetched_body: nil, suppress_errors: true) + def call(uri, suppress_errors: true) raise Error, 'No key URI given' if uri.blank? - if prefetched_body.nil? - if id - @json = fetch_resource_without_id_validation(uri) - if actor_type? - @json = fetch_resource(@json['id'], true) - elsif uri != @json['id'] - raise Error, "Fetched URI #{uri} has wrong id #{@json['id']}" - end - else - @json = fetch_resource(uri, id) - end - else - @json = body_to_json(prefetched_body, compare_id: id ? uri : nil) - end + @json = fetch_resource(uri, false) raise Error, "Unable to fetch key JSON at #{uri}" if @json.nil? raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?(@json) diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index a491b32b26..5a3eeeaf4e 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -8,14 +8,14 @@ class ActivityPub::FetchRemoteStatusService < BaseService DISCOVERIES_PER_REQUEST = 1000 # Should be called when uri has already been checked for locality - def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil) + def call(uri, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil) return if domain_not_allowed?(uri) @request_id = request_id || "#{Time.now.utc.to_i}-status-#{uri}" @json = if prefetched_body.nil? - fetch_resource(uri, id, on_behalf_of) + fetch_resource(uri, true, on_behalf_of) else - body_to_json(prefetched_body, compare_id: id ? uri : nil) + body_to_json(prefetched_body, compare_id: uri) end return unless supported_context? @@ -65,7 +65,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService def account_from_uri(uri) actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true, request_id: @request_id) if actor.nil? || actor.possibly_stale? + actor = ActivityPub::FetchRemoteAccountService.new.call(uri, request_id: @request_id) if actor.nil? || actor.possibly_stale? actor end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 8fc0989a3f..9e787ace50 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -277,7 +277,7 @@ class ActivityPub::ProcessAccountService < BaseService def moved_account account = ActivityPub::TagManager.instance.uri_to_resource(@json['movedTo'], Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], id: true, break_on_redirect: true, request_id: @options[:request_id]) + account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], break_on_redirect: true, request_id: @options[:request_id]) account end diff --git a/app/services/fetch_resource_service.rb b/app/services/fetch_resource_service.rb index a3406e5a57..71c6cca790 100644 --- a/app/services/fetch_resource_service.rb +++ b/app/services/fetch_resource_service.rb @@ -48,7 +48,15 @@ class FetchResourceService < BaseService body = response.body_with_limit json = body_to_json(body) - [json['id'], { prefetched_body: body, id: true }] if supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteActorService::SUPPORTED_TYPES) || expected_type?(json)) + return unless supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteActorService::SUPPORTED_TYPES) || expected_type?(json)) + + if json['id'] != @url + return if terminal + + return process(json['id'], terminal: true) + end + + [@url, { prefetched_body: body }] elsif !terminal link_header = response['Link'] && parse_link_header(response) diff --git a/spec/lib/activitypub/linked_data_signature_spec.rb b/spec/lib/activitypub/linked_data_signature_spec.rb index 97268eea6d..1af45673c0 100644 --- a/spec/lib/activitypub/linked_data_signature_spec.rb +++ b/spec/lib/activitypub/linked_data_signature_spec.rb @@ -56,7 +56,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do allow(ActivityPub::FetchRemoteKeyService).to receive(:new).and_return(service_stub) - allow(service_stub).to receive(:call).with('http://example.com/alice', id: false) do + allow(service_stub).to receive(:call).with('http://example.com/alice') do sender.update!(public_key: old_key) sender end @@ -64,7 +64,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do it 'fetches key and returns creator' do expect(subject.verify_actor!).to eq sender - expect(service_stub).to have_received(:call).with('http://example.com/alice', id: false).once + expect(service_stub).to have_received(:call).with('http://example.com/alice').once end end diff --git a/spec/services/activitypub/fetch_remote_account_service_spec.rb b/spec/services/activitypub/fetch_remote_account_service_spec.rb index ac7484d96d..f33a928da6 100644 --- a/spec/services/activitypub/fetch_remote_account_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do end describe '#call' do - let(:account) { subject.call('https://example.com/alice', id: true) } + let(:account) { subject.call('https://example.com/alice') } shared_examples 'sets profile data' do it 'returns an account' do diff --git a/spec/services/activitypub/fetch_remote_actor_service_spec.rb b/spec/services/activitypub/fetch_remote_actor_service_spec.rb index 93d31b69d5..944a2f8b1c 100644 --- a/spec/services/activitypub/fetch_remote_actor_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_actor_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe ActivityPub::FetchRemoteActorService, type: :service do end describe '#call' do - let(:account) { subject.call('https://example.com/alice', id: true) } + let(:account) { subject.call('https://example.com/alice') } shared_examples 'sets profile data' do it 'returns an account' do diff --git a/spec/services/activitypub/fetch_remote_key_service_spec.rb b/spec/services/activitypub/fetch_remote_key_service_spec.rb index e210d20ec7..0b14da4f44 100644 --- a/spec/services/activitypub/fetch_remote_key_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_key_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe ActivityPub::FetchRemoteKeyService, type: :service do end describe '#call' do - let(:account) { subject.call(public_key_id, id: false) } + let(:account) { subject.call(public_key_id) } context 'when the key is a sub-object from the actor' do before do diff --git a/spec/services/fetch_resource_service_spec.rb b/spec/services/fetch_resource_service_spec.rb index 0f1068471f..78037a06ce 100644 --- a/spec/services/fetch_resource_service_spec.rb +++ b/spec/services/fetch_resource_service_spec.rb @@ -57,7 +57,7 @@ RSpec.describe FetchResourceService, type: :service do let(:json) do { - id: 1, + id: 'http://example.com/foo', '@context': ActivityPub::TagManager::CONTEXT, type: 'Note', }.to_json @@ -83,27 +83,27 @@ RSpec.describe FetchResourceService, type: :service do let(:content_type) { 'application/activity+json; charset=utf-8' } let(:body) { json } - it { is_expected.to eq [1, { prefetched_body: body, id: true }] } + it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] } end context 'when content type is ld+json with profile' do let(:content_type) { 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"' } let(:body) { json } - it { is_expected.to eq [1, { prefetched_body: body, id: true }] } + it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] } end context 'when link header is present' do let(:headers) { { 'Link' => '; rel="alternate"; type="application/activity+json"' } } - it { is_expected.to eq [1, { prefetched_body: json, id: true }] } + it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] } end context 'when content type is text/html' do let(:content_type) { 'text/html' } let(:body) { '' } - it { is_expected.to eq [1, { prefetched_body: json, id: true }] } + it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] } end end end diff --git a/spec/services/resolve_url_service_spec.rb b/spec/services/resolve_url_service_spec.rb index bcfb9dbfb0..5270cc10dd 100644 --- a/spec/services/resolve_url_service_spec.rb +++ b/spec/services/resolve_url_service_spec.rb @@ -139,6 +139,7 @@ describe ResolveURLService, type: :service do stub_request(:get, url).to_return(status: 302, headers: { 'Location' => status_url }) body = ActiveModelSerializers::SerializableResource.new(status, serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter).to_json stub_request(:get, status_url).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' }) + stub_request(:get, uri).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' }) end it 'returns status by url' do From 6229edcd1a5e1f348656164120bc1a555a0f1d17 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 6 Feb 2024 10:20:23 +0100 Subject: [PATCH 02/12] Update dependency nokogiri to v1.16.2 [SECURITY] (#29106) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6cf0504b51..782556926c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -465,7 +465,7 @@ GEM net-smtp (0.4.0.1) net-protocol nio4r (2.5.9) - nokogiri (1.16.0) + nokogiri (1.16.2) mini_portile2 (~> 2.8.2) racc (~> 1.4) oj (3.16.3) From f29cc95b57a180395ed3a50a7e0c75551b04925b Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 13 Feb 2024 08:56:46 +0100 Subject: [PATCH 03/12] Update dependency sidekiq-unique-jobs to v7.1.33 (#29175) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 782556926c..2faaae0f09 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -711,7 +711,7 @@ GEM rufus-scheduler (~> 3.2) sidekiq (>= 6, < 8) tilt (>= 1.4.0) - sidekiq-unique-jobs (7.1.31) + sidekiq-unique-jobs (7.1.33) brpoplpush-redis_script (> 0.1.1, <= 2.0.0) concurrent-ruby (~> 1.0, >= 1.0.5) redis (< 5.0) From 47ebc6d6e67e75c20b4d90d46eca1c228a913001 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 13 Feb 2024 19:11:47 +0100 Subject: [PATCH 04/12] Disable administrative doorkeeper routes (#29187) --- config/initializers/doorkeeper.rb | 9 +- .../requests/disabled_oauth_endpoints_spec.rb | 83 +++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 spec/requests/disabled_oauth_endpoints_spec.rb diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index fe3871d2e7..f9d47a205c 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -21,9 +21,14 @@ Doorkeeper.configure do user unless user&.otp_required_for_login? end - # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. + # Doorkeeper provides some administrative interfaces for managing OAuth + # Applications, allowing creation, edit, and deletion of applications from the + # server. At present, these administrative routes are not integrated into + # Mastodon, and as such, we've disabled them by always return a 403 forbidden + # response for them. This does not affect the ability for users to manage + # their own OAuth Applications. admin_authenticator do - current_user&.admin? || redirect_to(new_user_session_url) + head 403 end # Authorization Code expiration time (default 10 minutes). diff --git a/spec/requests/disabled_oauth_endpoints_spec.rb b/spec/requests/disabled_oauth_endpoints_spec.rb new file mode 100644 index 0000000000..7c2c09f380 --- /dev/null +++ b/spec/requests/disabled_oauth_endpoints_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'Disabled OAuth routes' do + # These routes are disabled via the doorkeeper configuration for + # `admin_authenticator`, as these routes should only be accessible by server + # administrators. For now, these routes are not properly designed and + # integrated into Mastodon, so we're disabling them completely + describe 'GET /oauth/applications' do + it 'returns 403 forbidden' do + get oauth_applications_path + + expect(response).to have_http_status(403) + end + end + + describe 'POST /oauth/applications' do + it 'returns 403 forbidden' do + post oauth_applications_path + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/new' do + it 'returns 403 forbidden' do + get new_oauth_application_path + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + get oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'PATCH /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + patch oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'PUT /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + put oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'DELETE /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + delete oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/:id/edit' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + get edit_oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end +end From cc9a75b9abc82608a7ebb25c3d9ab100470a230c Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Feb 2024 13:12:13 +0100 Subject: [PATCH 05/12] Add `sidekiq_unique_jobs:delete_all_locks` task and disable `sidekiq-unique-jobs` UI by default (#29199) --- config/routes.rb | 2 +- lib/tasks/sidekiq_unique_jobs.rake | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 lib/tasks/sidekiq_unique_jobs.rake diff --git a/config/routes.rb b/config/routes.rb index d4cd27a491..ccb5d90242 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'sidekiq_unique_jobs/web' +require 'sidekiq_unique_jobs/web' if ENV['ENABLE_SIDEKIQ_UNIQUE_JOBS_UI'] == true require 'sidekiq-scheduler/web' class RedirectWithVary < ActionDispatch::Routing::PathRedirect diff --git a/lib/tasks/sidekiq_unique_jobs.rake b/lib/tasks/sidekiq_unique_jobs.rake new file mode 100644 index 0000000000..bedc8fe4c6 --- /dev/null +++ b/lib/tasks/sidekiq_unique_jobs.rake @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +namespace :sidekiq_unique_jobs do + task delete_all_locks: :environment do + digests = SidekiqUniqueJobs::Digests.new + digests.delete_by_pattern('*', count: digests.count) + + expiring_digests = SidekiqUniqueJobs::ExpiringDigests.new + expiring_digests.delete_by_pattern('*', count: expiring_digests.count) + end +end From 4ec2f2f2d6a07b4f18f24e42ed918f289f0a183a Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Sat, 14 Oct 2023 21:53:50 +0200 Subject: [PATCH 06/12] Ensure destruction of OAuth Applications notifies streaming Due to doorkeeper using a dependent: delete_all relationship, the destroy of an OAuth Application bypassed the existing AccessTokenExtension callbacks for announcing destructing of access tokens. --- app/lib/application_extension.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb index fb442e2c2d..9245e00c14 100644 --- a/app/lib/application_extension.rb +++ b/app/lib/application_extension.rb @@ -4,14 +4,29 @@ module ApplicationExtension extend ActiveSupport::Concern included do + include Redisable + has_many :created_users, class_name: 'User', foreign_key: 'created_by_application_id', inverse_of: :created_by_application validates :name, length: { maximum: 60 } validates :website, url: true, length: { maximum: 2_000 }, if: :website? validates :redirect_uri, length: { maximum: 2_000 } + + # The relationship used between Applications and AccessTokens is using + # dependent: delete_all, which means the ActiveRecord callback in + # AccessTokenExtension is not run, so instead we manually announce to + # streaming that these tokens are being deleted. + before_destroy :push_to_streaming_api, prepend: true end def confirmation_redirect_uri redirect_uri.lines.first.strip end + + def push_to_streaming_api + # TODO: #28793 Combine into a single topic + access_tokens.in_batches.each do |token| + redis.publish("timeline:access_token:#{token.id}", Oj.dump(event: :kill)) + end + end end From 2668b6d19a3b080cb253db7c2d982ea0f7baf40c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 17 Jan 2024 20:38:21 +0100 Subject: [PATCH 07/12] Ensure password resets revoke access to Streaming API --- app/models/user.rb | 7 +++++++ spec/models/user_spec.rb | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index cc56c2f54e..39f7b3b0cf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -342,6 +342,13 @@ class User < ApplicationRecord Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch| batch.update_all(revoked_at: Time.now.utc) Web::PushSubscription.where(access_token_id: batch).delete_all + + # Revoke each access token for the Streaming API, since `update_all`` + # doesn't trigger ActiveRecord Callbacks: + # TODO: #28793 Combine into a single topic + batch.each do |token| + redis.publish("timeline:access_token:#{token.id}", Oj.dump(event: :kill)) + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 213022e830..051e920454 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -439,6 +439,7 @@ RSpec.describe User do let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) } before do + allow(redis).to receive_messages(publish: nil) user.reset_password! end @@ -455,6 +456,10 @@ RSpec.describe User do expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0 end + it 'revokes streaming access for all access tokens' do + expect(redis).to have_received(:publish).with("timeline:access_token:#{access_token.id}", Oj.dump(event: :kill)).once + end + it 'removes push subscriptions' do expect(Web::PushSubscription.where(user: user).or(Web::PushSubscription.where(access_token: access_token)).count).to eq 0 expect { web_push_subscription.reload }.to raise_error(ActiveRecord::RecordNotFound) From edd0a947610c0fe128456650af1b945208f270e6 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 12 Feb 2024 13:29:36 +0100 Subject: [PATCH 08/12] Improve performance of deleting OAuth tokens --- app/lib/application_extension.rb | 9 +++++++-- app/models/user.rb | 7 +++++-- spec/models/user_spec.rb | 6 ++++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb index 9245e00c14..400c51a023 100644 --- a/app/lib/application_extension.rb +++ b/app/lib/application_extension.rb @@ -25,8 +25,13 @@ module ApplicationExtension def push_to_streaming_api # TODO: #28793 Combine into a single topic - access_tokens.in_batches.each do |token| - redis.publish("timeline:access_token:#{token.id}", Oj.dump(event: :kill)) + payload = Oj.dump(event: :kill) + access_tokens.in_batches do |tokens| + redis.pipelined do |pipeline| + tokens.ids.each do |id| + pipeline.publish("timeline:access_token:#{id}", payload) + end + end end end end diff --git a/app/models/user.rb b/app/models/user.rb index 39f7b3b0cf..388be31fab 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -346,8 +346,11 @@ class User < ApplicationRecord # Revoke each access token for the Streaming API, since `update_all`` # doesn't trigger ActiveRecord Callbacks: # TODO: #28793 Combine into a single topic - batch.each do |token| - redis.publish("timeline:access_token:#{token.id}", Oj.dump(event: :kill)) + payload = Oj.dump(event: :kill) + redis.pipelined do |pipeline| + batch.ids.each do |id| + pipeline.publish("timeline:access_token:#{id}", payload) + end end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 051e920454..3cc804af43 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -438,8 +438,10 @@ RSpec.describe User do let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) } let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) } + let(:redis_pipeline_stub) { instance_double(Redis::Namespace, publish: nil) } + before do - allow(redis).to receive_messages(publish: nil) + allow(redis).to receive(:pipelined).and_yield(redis_pipeline_stub) user.reset_password! end @@ -457,7 +459,7 @@ RSpec.describe User do end it 'revokes streaming access for all access tokens' do - expect(redis).to have_received(:publish).with("timeline:access_token:#{access_token.id}", Oj.dump(event: :kill)).once + expect(redis_pipeline_stub).to have_received(:publish).with("timeline:access_token:#{access_token.id}", Oj.dump(event: :kill)).once end it 'removes push subscriptions' do From 22c71fb83d8cb160342ee981fc12e56b70c6ec20 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 9 Feb 2024 14:38:32 +0100 Subject: [PATCH 09/12] Prevent different identities from a same SSO provider from accessing a same account --- app/models/concerns/user/omniauthable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb index 113bfda230..10baa890d8 100644 --- a/app/models/concerns/user/omniauthable.rb +++ b/app/models/concerns/user/omniauthable.rb @@ -51,7 +51,7 @@ module User::Omniauthable user = User.find_by(email: email) if email_is_verified - return user unless user.nil? + return user unless user.nil? && !Identity.exists?(provider: auth.provider, user_id: user.id) user = User.new(user_params_from_auth(email, auth)) From 561a397592b06438831f819e6dc31b81a067168b Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 9 Feb 2024 15:06:07 +0100 Subject: [PATCH 10/12] Lock auth provider changes behind `ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH=true` --- app/models/concerns/user/omniauthable.rb | 42 +++++++++++++++++++----- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb index 10baa890d8..d1855c7283 100644 --- a/app/models/concerns/user/omniauthable.rb +++ b/app/models/concerns/user/omniauthable.rb @@ -29,6 +29,7 @@ module User::Omniauthable # Note that this may leave zombie accounts (with no associated identity) which # can be cleaned up at a later date. user = signed_in_resource || identity.user + user ||= reattach_for_oauth(auth) user ||= create_for_oauth(auth) if identity.user.nil? @@ -39,19 +40,33 @@ module User::Omniauthable user end + def reattach_for_oauth(auth) + # If allowed, check if a user exists with the provided email address, + # and return it if they does not have an associated identity with the + # current authentication provider. + + # This can be used to provide a choice of alternative auth providers + # or provide smooth gradual transition between multiple auth providers, + # but this is discouraged because any insecure provider will put *all* + # local users at risk, regardless of which provider they registered with. + + return unless ENV['ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH'] == 'true' + + email, email_is_verified = email_from_oauth(auth) + return unless email_is_verified + + user = User.find_by(email: email) + return if user.nil? || Identity.exists?(provider: auth.provider, user_id: user.id) + + user + end + def create_for_oauth(auth) - # Check if the user exists with provided email. If no email was provided, + # Create a user for the given auth params. If no email was provided, # we assign a temporary email and ask the user to verify it on # the next step via Auth::SetupController.show - strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy - assume_verified = strategy&.security&.assume_email_is_verified - email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified - email = auth.info.verified_email || auth.info.email - - user = User.find_by(email: email) if email_is_verified - - return user unless user.nil? && !Identity.exists?(provider: auth.provider, user_id: user.id) + email, email_is_verified = email_from_oauth(auth) user = User.new(user_params_from_auth(email, auth)) @@ -68,6 +83,15 @@ module User::Omniauthable private + def email_from_oauth(auth) + strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy + assume_verified = strategy&.security&.assume_email_is_verified + email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified + email = auth.info.verified_email || auth.info.email + + [email, email_is_verified] + end + def user_params_from_auth(email, auth) { email: email || "#{TEMP_EMAIL_PREFIX}-#{auth.uid}-#{auth.provider}.com", From 53d44373604bdecc8955de76a1765aeb28e594a3 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 9 Feb 2024 15:24:17 +0100 Subject: [PATCH 11/12] Rename methods to avoid confusion between OAuth and OmniAuth --- .../auth/omniauth_callbacks_controller.rb | 2 +- app/models/concerns/user/omniauthable.rb | 22 +++++++++---------- app/models/identity.rb | 2 +- spec/models/identity_spec.rb | 6 ++--- spec/requests/omniauth_callbacks_spec.rb | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 707b50ef9e..9b83de945b 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -7,7 +7,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController def self.provides_callback_for(provider) define_method provider do @provider = provider - @user = User.find_for_oauth(request.env['omniauth.auth'], current_user) + @user = User.find_for_omniauth(request.env['omniauth.auth'], current_user) if @user.persisted? record_login_activity diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb index d1855c7283..396a0598f8 100644 --- a/app/models/concerns/user/omniauthable.rb +++ b/app/models/concerns/user/omniauthable.rb @@ -19,18 +19,18 @@ module User::Omniauthable end class_methods do - def find_for_oauth(auth, signed_in_resource = nil) + def find_for_omniauth(auth, signed_in_resource = nil) # EOLE-SSO Patch auth.uid = (auth.uid[0][:uid] || auth.uid[0][:user]) if auth.uid.is_a? Hashie::Array - identity = Identity.find_for_oauth(auth) + identity = Identity.find_for_omniauth(auth) # If a signed_in_resource is provided it always overrides the existing user # to prevent the identity being locked with accidentally created accounts. # Note that this may leave zombie accounts (with no associated identity) which # can be cleaned up at a later date. user = signed_in_resource || identity.user - user ||= reattach_for_oauth(auth) - user ||= create_for_oauth(auth) + user ||= reattach_for_auth(auth) + user ||= create_for_auth(auth) if identity.user.nil? identity.user = user @@ -40,7 +40,9 @@ module User::Omniauthable user end - def reattach_for_oauth(auth) + private + + def reattach_for_auth(auth) # If allowed, check if a user exists with the provided email address, # and return it if they does not have an associated identity with the # current authentication provider. @@ -52,7 +54,7 @@ module User::Omniauthable return unless ENV['ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH'] == 'true' - email, email_is_verified = email_from_oauth(auth) + email, email_is_verified = email_from_auth(auth) return unless email_is_verified user = User.find_by(email: email) @@ -61,12 +63,12 @@ module User::Omniauthable user end - def create_for_oauth(auth) + def create_for_auth(auth) # Create a user for the given auth params. If no email was provided, # we assign a temporary email and ask the user to verify it on # the next step via Auth::SetupController.show - email, email_is_verified = email_from_oauth(auth) + email, email_is_verified = email_from_auth(auth) user = User.new(user_params_from_auth(email, auth)) @@ -81,9 +83,7 @@ module User::Omniauthable user end - private - - def email_from_oauth(auth) + def email_from_auth(auth) strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy assume_verified = strategy&.security&.assume_email_is_verified email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified diff --git a/app/models/identity.rb b/app/models/identity.rb index c95a68a6f6..77821b78fa 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -17,7 +17,7 @@ class Identity < ApplicationRecord validates :uid, presence: true, uniqueness: { scope: :provider } validates :provider, presence: true - def self.find_for_oauth(auth) + def self.find_for_omniauth(auth) find_or_create_by(uid: auth.uid, provider: auth.provider) end end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 7022454443..d5a2ffbc86 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -3,19 +3,19 @@ require 'rails_helper' RSpec.describe Identity do - describe '.find_for_oauth' do + describe '.find_for_omniauth' do let(:auth) { Fabricate(:identity, user: Fabricate(:user)) } it 'calls .find_or_create_by' do allow(described_class).to receive(:find_or_create_by) - described_class.find_for_oauth(auth) + described_class.find_for_omniauth(auth) expect(described_class).to have_received(:find_or_create_by).with(uid: auth.uid, provider: auth.provider) end it 'returns an instance of Identity' do - expect(described_class.find_for_oauth(auth)).to be_instance_of described_class + expect(described_class.find_for_omniauth(auth)).to be_instance_of described_class end end end diff --git a/spec/requests/omniauth_callbacks_spec.rb b/spec/requests/omniauth_callbacks_spec.rb index 0d37c41140..b478ca1ce6 100644 --- a/spec/requests/omniauth_callbacks_spec.rb +++ b/spec/requests/omniauth_callbacks_spec.rb @@ -96,7 +96,7 @@ describe 'OmniAuth callbacks' do context 'when a user cannot be built' do before do - allow(User).to receive(:find_for_oauth).and_return(User.new) + allow(User).to receive(:find_for_omniauth).and_return(User.new) end it 'redirects to the new user signup page' do From 4084b738a599bab0432046b31731dad2827c8d3c Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Feb 2024 15:57:49 +0100 Subject: [PATCH 12/12] Fix OmniAuth tests (#29201) --- spec/requests/omniauth_callbacks_spec.rb | 35 ++++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/spec/requests/omniauth_callbacks_spec.rb b/spec/requests/omniauth_callbacks_spec.rb index b478ca1ce6..1e488b3f43 100644 --- a/spec/requests/omniauth_callbacks_spec.rb +++ b/spec/requests/omniauth_callbacks_spec.rb @@ -39,16 +39,33 @@ describe 'OmniAuth callbacks' do Fabricate(:user, email: 'user@host.example') end - it 'matches the existing user, creates an identity, and redirects to root path' do - expect { subject } - .to not_change(User, :count) - .and change(Identity, :count) - .by(1) - .and change(LoginActivity, :count) - .by(1) + context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is set to true' do + around do |example| + ClimateControl.modify ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH: 'true' do + example.run + end + end - expect(Identity.find_by(user: User.last).uid).to eq('123') - expect(response).to redirect_to(root_path) + it 'matches the existing user, creates an identity, and redirects to root path' do + expect { subject } + .to not_change(User, :count) + .and change(Identity, :count) + .by(1) + .and change(LoginActivity, :count) + .by(1) + + expect(Identity.find_by(user: User.last).uid).to eq('123') + expect(response).to redirect_to(root_path) + end + end + + context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is not set to true' do + it 'does not match the existing user or create an identity' do + expect { subject } + .to not_change(User, :count) + .and not_change(Identity, :count) + .and not_change(LoginActivity, :count) + end end end