From 8378b72ebacc51e5e090faa527462b801e4c2803 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 19 May 2018 21:05:08 +0200 Subject: [PATCH] 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 --- .../authorized_applications_controller.rb | 5 ++++ app/controllers/oauth/tokens_controller.rb | 14 +++++++++++ app/models/web/push_subscription.rb | 9 ++++++++ config/routes.rb | 4 +++- ...authorized_applications_controller_spec.rb | 20 ++++++++++++++++ .../oauth/tokens_controller_spec.rb | 23 +++++++++++++++++++ .../web_push_subscription_fabricator.rb | 2 +- spec/fabricators/web_setting_fabricator.rb | 3 +-- 8 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 app/controllers/oauth/tokens_controller.rb create mode 100644 spec/controllers/oauth/tokens_controller_spec.rb diff --git a/app/controllers/oauth/authorized_applications_controller.rb b/app/controllers/oauth/authorized_applications_controller.rb index 395fbc51b..0c28d194b 100644 --- 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 index 000000000..fa6d58f25 --- /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 7da3428fe..867bc9519 100644 --- 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 bd9d09226..3042b5ea0 100644 --- 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 f967b507f..901e538e9 100644 --- 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 index 000000000..ba8e367a6 --- /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 72d11b77c..97f90675d 100644 --- 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 e5136829b9..369b86bc1 100644 --- 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