Merge pull request #891 from Retrospring/revoke-twitter-on-unauthorized

Revoke Twitter connection when the token is revoked

app/controllers/services_controller.rb

@@ -2,6 +2,7 @@
 
 class ServicesController < ApplicationController
   before_action :authenticate_user!
+  before_action :mark_notifications_as_read, only: %i[index]
 
   def index
     @services = current_user.services
@@ -59,4 +60,10 @@ class ServicesController < ApplicationController
   def omniauth_hash
     request.env["omniauth.auth"]
   end
+
+  def mark_notifications_as_read
+    Notification::ServiceTokenExpired
+      .where(recipient: current_user, new: true)
+      .update_all(new: false) # rubocop:disable Rails/SkipsModelValidations
+  end
 end

app/javascript/styles/components/_notifications.scss

@@ -8,6 +8,7 @@
   &__user,
   &__text {
     margin-bottom: 0;
+    white-space: normal;
   }
 
   &__icon {
@@ -36,7 +37,7 @@
   &-dropdown {
     min-width: 400px;
 
-    & .dropdown-item:hover, 
+    & .dropdown-item:hover,
     & .dropdown-item:active {
       background: transparent;
       color: RGB(var(--body-text));
@@ -55,4 +56,4 @@
       margin-bottom: 0;
     }
   }
-}
+}

app/models/notification/service_token_expired.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class Notification::ServiceTokenExpired < Notification
+end

app/models/notification/twitter_token_expired.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class Notification::TwitterTokenExpired < Notification::ServiceTokenExpired
+end

app/models/user/expired_service_connection.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+# stub model for notifying about expired service connections
+class User::ExpiredServiceConnection < User
+end

app/models/user/expired_twitter_service_connection.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class User::ExpiredTwitterServiceConnection < User::ExpiredServiceConnection
+end

app/views/notifications/type/_expiredtwitterserviceconnection.html.haml

@@ -0,0 +1,8 @@
+.media.notification
+  .notification__icon
+    %i.fa.fa-2x.fa-fw.fa-twitter
+  .media-body
+    %h6.media-heading.notification__user
+      = t(".heading")
+    .notification__text
+      = t(".text_html", settings_sharing: link_to(t(".settings_services"), services_path))

app/workers/share_worker.rb

@@ -8,11 +8,10 @@ class ShareWorker
   # @param user_id [Integer] the user id
   # @param answer_id [Integer] the user id
   # @param service [String] the service to post to
-  def perform(user_id, answer_id, service)
-    service_type = "Services::#{service.camelize}"
-    user_service = User.find(user_id).services.find_by(type: service_type)
+  def perform(user_id, answer_id, service) # rubocop:disable Metrics/AbcSize
+    @user_service = User.find(user_id).services.find_by(type: "Services::#{service.camelize}")
 
-    user_service.post(Answer.find(answer_id))
+    @user_service.post(Answer.find(answer_id))
   rescue ActiveRecord::RecordNotFound
     logger.info "Tried to post answer ##{answer_id} for user ##{user_id} to #{service.titleize} but the user/answer/service did not exist (likely deleted), will not retry."
     # The question to be posted was deleted
@@ -23,11 +22,22 @@ class ShareWorker
     logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the account is suspended."
   rescue Twitter::Error::Unauthorized
     # User's Twitter token has expired or been revoked
-    # TODO: Notify user if this happens (https://github.com/Retrospring/retrospring/issues/123)
-    logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has exired or been revoked."
+    logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has expired or been revoked."
+    revoke_and_notify(user_id, service)
   rescue => e
     logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}"
     Sentry.capture_exception(e)
     raise
   end
+
+  def revoke_and_notify(user_id, service)
+    @user_service.destroy
+
+    Notification::ServiceTokenExpired.create(
+      target_id:    user_id,
+      target_type:  "User::Expired#{service.camelize}ServiceConnection",
+      recipient_id: user_id,
+      new:          true
+    )
+  end
 end

config/locales/views.en.yml

@@ -352,6 +352,10 @@ en:
           link_text: "your comment"
       follow:
         heading_html: "followed you %{time} ago"
+      expiredtwitterserviceconnection:
+        heading: "Twitter connection expired"
+        text_html: "If you would like to continue automatically sharing your answers to Twitter, head to %{settings_sharing} and re-connect your account."
+        settings_services: "Sharing Settings"
   services:
     index:
       title: "Service Settings"

spec/controllers/services_controller_spec.rb

@@ -1,77 +1,124 @@
-require 'rails_helper'
+# frozen_string_literal: true
+
+require "rails_helper"
 
 describe ServicesController, type: :controller do
-  context 'successful Twitter sign in' do
-    let(:user) { FactoryBot.create(:user) }
+  describe "#index" do
+    subject { get :index }
 
-    before do
-      sign_in user
-      OmniAuth.config.test_mode = true
-      OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new({
-                                                                     'provider' => 'twitter',
-                                                                     'uid' => '12',
-                                                                     'info' => { 'nickname' => 'jack' },
-                                                                     'credentials' => { 'token' => 'AAAA', 'secret' => 'BBBB' }
-                                                                   })
-      request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter]
-    end
+    context "user signed in" do
+      let(:user) { FactoryBot.create(:user) }
 
-    after do
-      OmniAuth.config.mock_auth[:twitter] = nil
-    end
+      before { sign_in user }
 
-    subject { get :create, params: { provider: 'twitter' } }
-
-    context 'no services connected' do
-      it 'creates a service integration' do
-        expect { subject }.to change { Service.count }.by(1)
-      end
-    end
-
-    context 'a user has a service connected' do
-      let(:other_user) { FactoryBot.create(:user) }
-      let!(:service) { Services::Twitter.create(user: other_user, uid: 12) }
-
-      it 'shows an error when trying to attach a service account which is already connected' do
+      it "renders the services settings page with no services" do
         subject
-        expect(flash[:error]).to eq("The Twitter account you are trying to connect is already connected to another #{APP_CONFIG['site_name']} account. If you are unable to disconnect the account yourself, please send us a Direct Message on Twitter: @retrospring.")
+        expect(response).to render_template("index")
+        expect(controller.instance_variable_get(:@services)).to be_empty
+      end
+
+      context "user has a service token expired notification" do
+        let(:notification) do
+          Notification::ServiceTokenExpired.create(
+            target_id:    user.id,
+            target_type:  "User::ExpiredTwitterServiceConnection",
+            recipient_id: user.id,
+            new:          true
+          )
+        end
+
+        it "marks the notification as read" do
+          expect { subject }.to change { notification.reload.new }.from(true).to(false)
+        end
+      end
+
+      context "user has Twitter connected" do
+        before do
+          Services::Twitter.create(user:, uid: 12)
+        end
+
+        it "renders the services settings page" do
+          subject
+          expect(response).to render_template("index")
+          expect(controller.instance_variable_get(:@services)).not_to be_empty
+        end
       end
     end
   end
 
-  context '#update' do
-    subject { patch :update, params: params }
+  describe "#create" do
+    subject { get :create, params: { provider: "twitter" } }
 
-    context 'not signed in' do
+    context "successful Twitter sign in" do
+      let(:user) { FactoryBot.create(:user) }
+
+      before do
+        sign_in user
+        OmniAuth.config.test_mode = true
+        OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new({
+                                                                       "provider"    => "twitter",
+                                                                       "uid"         => "12",
+                                                                       "info"        => { "nickname" => "jack" },
+                                                                       "credentials" => { "token" => "AAAA", "secret" => "BBBB" }
+                                                                     })
+        request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter]
+      end
+
+      after do
+        OmniAuth.config.mock_auth[:twitter] = nil
+      end
+
+      context "no services connected" do
+        it "creates a service integration" do
+          expect { subject }.to change { Service.count }.by(1)
+        end
+      end
+
+      context "a user has a service connected" do
+        let(:other_user) { FactoryBot.create(:user) }
+        let!(:service) { Services::Twitter.create(user: other_user, uid: 12) }
+
+        it "shows an error when trying to attach a service account which is already connected" do
+          subject
+          expect(flash[:error]).to eq("The Twitter account you are trying to connect is already connected to another #{APP_CONFIG['site_name']} account. If you are unable to disconnect the account yourself, please send us a Direct Message on Twitter: @retrospring.")
+        end
+      end
+    end
+  end
+
+  describe "#update" do
+    subject { patch :update, params: }
+
+    context "not signed in" do
       let(:params) { { id: 1 } }
 
-      it 'redirects to sign in page' do
+      it "redirects to sign in page" do
         subject
         expect(response).to redirect_to(new_user_session_path)
       end
     end
 
-    context 'user with Twitter connection' do
+    context "user with Twitter connection" do
       before { sign_in user }
 
       let(:user) { FactoryBot.create(:user) }
-      let(:service) { Services::Twitter.create(user: user, uid: 12) }
-      let(:params) { { id: service.id, service: { post_tag: post_tag } } }
+      let(:service) { Services::Twitter.create(user:, uid: 12) }
+      let(:params) { { id: service.id, service: { post_tag: } } }
 
-      context 'tag is valid' do
-        let(:post_tag) { '#askaraccoon' }
+      context "tag is valid" do
+        let(:post_tag) { "#askaraccoon" }
 
-        it 'updates a service connection' do
-          expect { subject }.to change { service.reload.post_tag }.to('#askaraccoon')
+        it "updates a service connection" do
+          expect { subject }.to change { service.reload.post_tag }.to("#askaraccoon")
           expect(response).to redirect_to(services_path)
           expect(flash[:success]).to eq("Service updated successfully.")
         end
       end
 
-      context 'tag is too long' do
-        let(:post_tag) { 'a' * 21 }  # 1 character over the limit
+      context "tag is too long" do
+        let(:post_tag) { "a" * 21 }  # 1 character over the limit
 
-        it 'shows an error' do
+        it "shows an error" do
           subject
           expect(response).to redirect_to(services_path)
           expect(flash[:error]).to eq("Unable to update service.")

spec/workers/share_worker_spec.rb

@@ -56,7 +56,19 @@ describe ShareWorker do
         allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized)
         subject
         ShareWorker.drain
-        expect(Sidekiq.logger).to have_received(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has exired or been revoked.")
+        expect(Sidekiq.logger).to have_received(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has expired or been revoked.")
+      end
+
+      it "revokes the service connection when Twitter::Error::Unauthorized is raised" do
+        allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized)
+        subject
+        expect { ShareWorker.drain }.to change { Services::Twitter.count }.by(-1)
+      end
+
+      it "sends the user a notification when Twitter::Error::Unauthorized is raised" do
+        allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized)
+        subject
+        expect { ShareWorker.drain }.to change { Notification::ServiceTokenExpired.count }.by(1)
       end
 
       it "handles Twitter::Error::Forbidden" do