Ensure push subscription is immediately removed when application is revoked (#7548)

* Ensure push subscription is immediately removed when application is revoked

* When token is revoked from app, unsubscribe too

diff --git a/app/controllers/oauth/authorized_applications_controller.rb b/app/controllers/oauth/authorized_applications_controller.rb
index 395fbc51bcfcd82f576737c56e977f7fd4f329ae..0c28d194bc82c8c1643ba0aaffd827f19b97d42b 100644 (file)
--- a/app/controllers/oauth/authorized_applications_controller.rb
+++ b/app/controllers/oauth/authorized_applications_controller.rb
@@ -8,6 +8,11 @@ class Oauth::AuthorizedApplicationsController < Doorkeeper::AuthorizedApplicatio
 
   include Localized
 
+  def destroy
+    Web::PushSubscription.unsubscribe_for(params[:id], current_resource_owner)
+    super
+  end
+
   private
 
   def store_current_location

diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb
new file mode 100644 (file)
index 0000000..fa6d58f
--- /dev/null
+++ b/app/controllers/oauth/tokens_controller.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class Oauth::TokensController < Doorkeeper::TokensController
+  def revoke
+    unsubscribe_for_token if authorized? && token.accessible?
+    super
+  end
+
+  private
+
+  def unsubscribe_for_token
+    Web::PushSubscription.where(access_token_id: token.id).delete_all
+  end
+end

diff --git a/app/models/web/push_subscription.rb b/app/models/web/push_subscription.rb
index 7da3428fea47d31aa31097a0a6efc4fd9ddd0e66..867bc9519130174457d0c2121f6687c14538ce72 100644 (file)
--- a/app/models/web/push_subscription.rb
+++ b/app/models/web/push_subscription.rb
@@ -50,6 +50,15 @@ class Web::PushSubscription < ApplicationRecord
                                end
   end
 
+  class << self
+    def unsubscribe_for(application_id, resource_owner)
+      access_token_ids = Doorkeeper::AccessToken.where(application_id: application_id, resource_owner_id: resource_owner.id, revoked_at: nil)
+                                                .pluck(:id)
+
+      where(access_token_id: access_token_ids).delete_all
+    end
+  end
+
   private
 
   def push_payload(message, ttl = 5.minutes.seconds)

diff --git a/config/routes.rb b/config/routes.rb
index bd9d092261c69a269dff67d2eaf6ca743ca16856..3042b5ea0a897ab84f87bf0b7624ffa4ea811a78 100644 (file)
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -14,7 +14,9 @@ Rails.application.routes.draw do
   end
 
   use_doorkeeper do
-    controllers authorizations: 'oauth/authorizations', authorized_applications: 'oauth/authorized_applications'
+    controllers authorizations: 'oauth/authorizations',
+                authorized_applications: 'oauth/authorized_applications',
+                tokens: 'oauth/tokens'
   end
 
   get '.well-known/host-meta', to: 'well_known/host_meta#show', as: :host_meta, defaults: { format: 'xml' }

diff --git a/spec/controllers/oauth/authorized_applications_controller_spec.rb b/spec/controllers/oauth/authorized_applications_controller_spec.rb
index f967b507f05a455134c3ae24dde587704a79c563..901e538e95cb6862ca0001f821b083d4db42f2ee 100644 (file)
--- a/spec/controllers/oauth/authorized_applications_controller_spec.rb
+++ b/spec/controllers/oauth/authorized_applications_controller_spec.rb
@@ -39,4 +39,24 @@ describe Oauth::AuthorizedApplicationsController do
       include_examples 'stores location for user'
     end
   end
+
+  describe 'DELETE #destroy' do
+    let!(:user) { Fabricate(:user) }
+    let!(:application) { Fabricate(:application) }
+    let!(:access_token) { Fabricate(:accessible_access_token, application: application, resource_owner_id: user.id) }
+    let!(:web_push_subscription) { Fabricate(:web_push_subscription, user: user, access_token: access_token) }
+
+    before do
+      sign_in user, scope: :user
+      post :destroy, params: { id: application.id }
+    end
+
+    it 'revokes access tokens for the application' do
+      expect(Doorkeeper::AccessToken.where(application: application).first.revoked_at).to_not be_nil
+    end
+
+    it 'removes subscriptions for the application\'s access tokens' do
+      expect(Web::PushSubscription.where(user: user).count).to eq 0
+    end
+  end
 end

diff --git a/spec/controllers/oauth/tokens_controller_spec.rb b/spec/controllers/oauth/tokens_controller_spec.rb
new file mode 100644 (file)
index 0000000..ba8e367
--- /dev/null
+++ b/spec/controllers/oauth/tokens_controller_spec.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Oauth::TokensController, type: :controller do
+  describe 'POST #revoke' do
+    let!(:user) { Fabricate(:user) }
+    let!(:access_token) { Fabricate(:accessible_access_token, resource_owner_id: user.id) }
+    let!(:web_push_subscription) { Fabricate(:web_push_subscription, user: user, access_token: access_token) }
+
+    before do
+      post :revoke, params: { token: access_token.token }
+    end
+
+    it 'revokes the token' do
+      expect(access_token.reload.revoked_at).to_not be_nil
+    end
+
+    it 'removes web push subscription for token' do
+      expect(Web::PushSubscription.where(access_token: access_token).count).to eq 0
+    end
+  end
+end

diff --git a/spec/fabricators/web_push_subscription_fabricator.rb b/spec/fabricators/web_push_subscription_fabricator.rb
index 72d11b77cc1c298ae5e20ca9697108b12deda787..97f90675de0a53d76ba9d85f25cf32fe814bfe25 100644 (file)
--- a/spec/fabricators/web_push_subscription_fabricator.rb
+++ b/spec/fabricators/web_push_subscription_fabricator.rb
@@ -1,4 +1,4 @@
-Fabricator(:web_push_subscription) do
+Fabricator(:web_push_subscription, from: Web::PushSubscription) do
   endpoint   Faker::Internet.url
   key_p256dh Faker::Internet.password
   key_auth   Faker::Internet.password

diff --git a/spec/fabricators/web_setting_fabricator.rb b/spec/fabricators/web_setting_fabricator.rb
index e5136829b9d0a88e4a095ec0ee75cc2da23eeb11..369b86bc18a56e793616149eb4ba5becfa081826 100644 (file)
--- a/spec/fabricators/web_setting_fabricator.rb
+++ b/spec/fabricators/web_setting_fabricator.rb
@@ -1,3 +1,2 @@
-Fabricator('Web::Setting') do
-
+Fabricator(:web_setting, from: Web::Setting) do
 end