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>
This commit is contained in:
		
							parent
							
								
									3c3ef8b9d3
								
							
						
					
					
						commit
						436419cc2f
					
				
					 3 changed files with 37 additions and 0 deletions
				
			
		|  | @ -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 | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
|  | @ -438,7 +438,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 | ||||||
| 
 | 
 | ||||||
|  | @ -455,6 +458,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 a new issue