From 68eaa804c9bafdc5f798e114e9ba00161425dd71 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 14 Feb 2024 15:15:34 +0100 Subject: [PATCH] Merge pull request from GHSA-7w3c-p9j8-mq3x * 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. * Ensure password resets revoke access to Streaming API * Improve performance of deleting OAuth tokens --------- Co-authored-by: Claire --- app/lib/application_extension.rb | 20 ++++++++++++++++++++ app/models/user.rb | 10 ++++++++++ spec/models/user_spec.rb | 7 +++++++ 3 files changed, 37 insertions(+) diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb index fb442e2c2d..400c51a023 100644 --- a/app/lib/application_extension.rb +++ b/app/lib/application_extension.rb @@ -4,14 +4,34 @@ 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 + 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 95fdc431ac..f706c91eff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -344,6 +344,16 @@ 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 + 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 5ac41c0ff1..845335e873 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -420,7 +420,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(:pipelined).and_yield(redis_pipeline_stub) user.reset_password! end @@ -437,6 +440,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_pipeline_stub).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)