From 4b94e9c65efd227c58f8be9b3db6b667d403ed3c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 19 May 2018 14:46:47 +0200 Subject: Improve payload format of Web Push API now that it's open (#7521) > Good lord what is happening in there Previously the contents of the Web Push API payloads closely resembled the structure of JavaScript's [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification). But now that the API is open to non-browser apps, and given that there is no required coupling between contents of the payload and a Notification object, here is how I changed the payload: ```json { "access_token": "...", "preferred_locale": "en", "notification_id": "12345", "notification_type": "follow", "title": "So and so followed you", "body": "This is my bio", "icon": "https://example.com/avatar.png" } ``` The title, body and icon attributes are included as a fallback so you can construct a minimal notification if you cannot perform a network request to the API to get more data. --- app/models/web/push_subscription.rb | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'app/models') diff --git a/app/models/web/push_subscription.rb b/app/models/web/push_subscription.rb index df549c6d3..7da3428fe 100644 --- a/app/models/web/push_subscription.rb +++ b/app/models/web/push_subscription.rb @@ -21,8 +21,8 @@ class Web::PushSubscription < ApplicationRecord has_one :session_activation def push(notification) - I18n.with_locale(associated_user.locale || I18n.default_locale) do - push_payload(message_from(notification), 48.hours.seconds) + I18n.with_locale(associated_user&.locale || I18n.default_locale) do + push_payload(payload_for_notification(notification), 48.hours.seconds) end end @@ -46,16 +46,13 @@ class Web::PushSubscription < ApplicationRecord @associated_access_token = if access_token_id.nil? find_or_create_access_token.token else - access_token + access_token.token end end private def push_payload(message, ttl = 5.minutes.seconds) - # TODO: Make sure that the payload does not - # exceed 4KB - Webpush::PayloadTooLarge - Webpush.payload_send( message: Oj.dump(message), endpoint: endpoint, @@ -70,16 +67,20 @@ class Web::PushSubscription < ApplicationRecord ) end - def message_from(notification) - serializable_resource = ActiveModelSerializers::SerializableResource.new(notification, serializer: Web::NotificationSerializer, scope: self, scope_name: :current_push_subscription) - serializable_resource.as_json + def payload_for_notification(notification) + ActiveModelSerializers::SerializableResource.new( + notification, + serializer: Web::NotificationSerializer, + scope: self, + scope_name: :current_push_subscription + ).as_json end def find_or_create_access_token Doorkeeper::AccessToken.find_or_create_for( Doorkeeper::Application.find_by(superapp: true), session_activation.user_id, - Doorkeeper::OAuth::Scopes.from_string('read write follow'), + Doorkeeper::OAuth::Scopes.from_string('read write follow push'), Doorkeeper.configuration.access_token_expires_in, Doorkeeper.configuration.refresh_token_enabled? ) -- cgit From 8378b72ebacc51e5e090faa527462b801e4c2803 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 19 May 2018 21:05:08 +0200 Subject: 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 --- .../oauth/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 +++++++++++++++++++ spec/controllers/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 (limited to 'app/models') 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 -- cgit