Skip to content

Commit

Permalink
Merge pull request from GHSA-7w3c-p9j8-mq3x
Browse files Browse the repository at this point in the history
* 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: Emelia Smith <ThisIsMissEm@users.noreply.github.com>
  • Loading branch information
ClearlyClaire and ThisIsMissEm committed Feb 14, 2024
1 parent 2f6518c commit d807b39
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 0 deletions.
20 changes: 20 additions & 0 deletions app/lib/application_extension.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,32 @@ module ApplicationExtension
extend ActiveSupport::Concern

included do
include Redisable

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
10 changes: 10 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,16 @@ def revoke_access!
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

Expand Down
7 changes: 7 additions & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,10 @@ def fabricate
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

Expand All @@ -455,6 +458,10 @@ def fabricate
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
end
Expand Down

0 comments on commit d807b39

Please sign in to comment.