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 <claire.github-309c@sitedethib.com>shrike
parent
554e2a019e
commit
68eaa804c9
|
@ -4,14 +4,34 @@ module ApplicationExtension
|
||||||
extend ActiveSupport::Concern
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
included do
|
included do
|
||||||
|
include Redisable
|
||||||
|
|
||||||
has_many :created_users, class_name: 'User', foreign_key: 'created_by_application_id', inverse_of: :created_by_application
|
has_many :created_users, class_name: 'User', foreign_key: 'created_by_application_id', inverse_of: :created_by_application
|
||||||
|
|
||||||
validates :name, length: { maximum: 60 }
|
validates :name, length: { maximum: 60 }
|
||||||
validates :website, url: true, length: { maximum: 2_000 }, if: :website?
|
validates :website, url: true, length: { maximum: 2_000 }, if: :website?
|
||||||
validates :redirect_uri, length: { maximum: 2_000 }
|
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
|
end
|
||||||
|
|
||||||
def confirmation_redirect_uri
|
def confirmation_redirect_uri
|
||||||
redirect_uri.lines.first.strip
|
redirect_uri.lines.first.strip
|
||||||
end
|
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
|
end
|
||||||
|
|
|
@ -344,6 +344,16 @@ class User < ApplicationRecord
|
||||||
Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch|
|
Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch|
|
||||||
batch.update_all(revoked_at: Time.now.utc)
|
batch.update_all(revoked_at: Time.now.utc)
|
||||||
Web::PushSubscription.where(access_token_id: batch).delete_all
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -420,7 +420,10 @@ RSpec.describe User do
|
||||||
let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) }
|
let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) }
|
||||||
let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) }
|
let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) }
|
||||||
|
|
||||||
|
let(:redis_pipeline_stub) { instance_double(Redis::Namespace, publish: nil) }
|
||||||
|
|
||||||
before do
|
before do
|
||||||
|
allow(redis).to receive(:pipelined).and_yield(redis_pipeline_stub)
|
||||||
user.reset_password!
|
user.reset_password!
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -437,6 +440,10 @@ RSpec.describe User do
|
||||||
expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0
|
expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0
|
||||||
end
|
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
|
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::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)
|
expect { web_push_subscription.reload }.to raise_error(ActiveRecord::RecordNotFound)
|
||||||
|
|
Loading…
Reference in New Issue