From 54e312d269a2db46019d64e5a474c323df4d88e1 Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 10 Oct 2023 15:52:18 +0200
Subject: [PATCH 1/8] ADD controller for ActivityPub subscription to releases
 actor

This is the third and last part of the merging the overarching
subscription MR at
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132460 .

This provides the controller, route and service used to create
subscriptions.

We add the `inbox` endpoint as specified by ActivityPub on our
`releases` actor to receive various post requests. The only requests we
accept are Follow and unfollow (Undo > Follow) ones. If we receive an
other kind of activity (that could be, for example, that someone
commented on our release post in the Fediverse), we silently discard it.

The service's role is to create the blank subscription and to queue the
background job that will resolve the various URLs and prepare the
subscription.
---
 .../activity_pub/application_controller.rb    |   2 +
 .../projects/releases_controller.rb           |  45 ++-
 .../activity_pub/releases_subscription.rb     |   2 +-
 .../projects/releases_subscription_service.rb |  74 ++++
 config/routes/activity_pub.rb                 |   1 +
 .../projects/releases_controller_spec.rb      | 181 +++++++++-
 .../releases_subscription_service_spec.rb     | 315 ++++++++++++++++++
 7 files changed, 611 insertions(+), 9 deletions(-)
 create mode 100644 app/services/activity_pub/projects/releases_subscription_service.rb
 create mode 100644 spec/services/activity_pub/projects/releases_subscription_service_spec.rb

diff --git a/app/controllers/activity_pub/application_controller.rb b/app/controllers/activity_pub/application_controller.rb
index f9c2b14fe7701c..70cf881c857a04 100644
--- a/app/controllers/activity_pub/application_controller.rb
+++ b/app/controllers/activity_pub/application_controller.rb
@@ -8,6 +8,8 @@ class ApplicationController < ::ApplicationController
     skip_before_action :authenticate_user!
     after_action :set_content_type
 
+    protect_from_forgery with: :null_session
+
     def can?(object, action, subject = :global)
       Ability.allowed?(object, action, subject)
     end
diff --git a/app/controllers/activity_pub/projects/releases_controller.rb b/app/controllers/activity_pub/projects/releases_controller.rb
index 7c4c2a0322be69..e612331ffbfc59 100644
--- a/app/controllers/activity_pub/projects/releases_controller.rb
+++ b/app/controllers/activity_pub/projects/releases_controller.rb
@@ -5,15 +5,32 @@ module Projects
     class ReleasesController < ApplicationController
       feature_category :release_orchestration
 
+      before_action :enforce_payload, only: :inbox
+
       def index
         opts = {
-          inbox: nil,
+          inbox: inbox_project_releases_url(@project),
           outbox: outbox_project_releases_url(@project)
         }
 
         render json: ActivityPub::ReleasesActorSerializer.new.represent(@project, opts)
       end
 
+      def inbox
+        service = ReleasesSubscriptionService.new(project, payload)
+
+        success = if follow? || unfollow?
+                    follow? ? service.follow : service.unfollow
+                  else
+                    true
+                  end
+
+        response = { success: success }
+        response[:errors] = service.errors unless success
+
+        render json: response
+      end
+
       def outbox
         serializer = ActivityPub::ReleasesOutboxSerializer.new.with_pagination(request, response)
         render json: serializer.represent(releases)
@@ -24,6 +41,32 @@ def outbox
       def releases(params = {})
         ReleasesFinder.new(@project, current_user, params).execute
       end
+
+      def enforce_payload
+        return if payload
+
+        head :unprocessable_entity
+        false
+      end
+
+      def payload
+        @payload ||= begin
+          Gitlab::Json.parse(request.body.read)
+        rescue JSON::ParserError
+          nil
+        end
+      end
+
+      def follow?
+        payload['type'] == 'Follow'
+      end
+
+      def unfollow?
+        undo = payload['type'] == 'Undo'
+        object = payload['object']
+        follow = object.present? && object.is_a?(Hash) && object['type'] == 'Follow'
+        undo && follow
+      end
     end
   end
 end
diff --git a/app/models/activity_pub/releases_subscription.rb b/app/models/activity_pub/releases_subscription.rb
index a6304f1fc3544b..5c730c7555a3b3 100644
--- a/app/models/activity_pub/releases_subscription.rb
+++ b/app/models/activity_pub/releases_subscription.rb
@@ -11,7 +11,7 @@ class ReleasesSubscription < ApplicationRecord
     validates :payload, json_schema: { filename: 'activity_pub_follow_payload' }, allow_blank: true
     validates :subscriber_url, presence: true, uniqueness: { case_sensitive: false, scope: :project_id },
       public_url: true
-    validates :subscriber_inbox_url, uniqueness: { case_sensitive: false, scope: :project_id },
+    validates :subscriber_inbox_url, uniqueness: { case_sensitive: false, scope: :project_id, allow_nil: true },
       public_url: { allow_nil: true }
     validates :shared_inbox_url, public_url: { allow_nil: true }
 
diff --git a/app/services/activity_pub/projects/releases_subscription_service.rb b/app/services/activity_pub/projects/releases_subscription_service.rb
new file mode 100644
index 00000000000000..6a89063a53aea2
--- /dev/null
+++ b/app/services/activity_pub/projects/releases_subscription_service.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+module ActivityPub
+  module Projects
+    class ReleasesSubscriptionService
+      attr_reader :project, :payload, :errors, :subscription
+
+      def initialize(project, payload)
+        @project = project
+        @payload = payload
+        @errors = []
+      end
+
+      def follow
+        unless subscriber_url
+          errors << "You need to provide an actor id for your subscriber"
+          return false
+        end
+
+        return true if previous_subscription.present?
+
+        @subscription = ReleasesSubscription.new(
+          subscriber_url: subscriber_url,
+          subscriber_inbox_url: subscriber_inbox_url,
+          payload: payload,
+          project: project
+        )
+
+        unless subscription.save
+          errors.concat(subscription.errors.full_messages)
+          return false
+        end
+
+        enqueue_subscription
+        true
+      end
+
+      def unfollow
+        unless subscriber_url
+          errors << "You need to provide an actor id for your unsubscribe activity"
+          return false
+        end
+
+        return true unless previous_subscription.present?
+
+        previous_subscription.destroy
+      end
+
+      private
+
+      def subscriber_url
+        return false unless payload['actor']
+        return payload['actor'] if payload['actor'].is_a?(String)
+        return unless payload['actor'].is_a?(Hash) && payload['actor']['id'].is_a?(String)
+
+        payload['actor']['id']
+      end
+
+      def subscriber_inbox_url
+        return unless payload['actor'].is_a?(Hash)
+
+        payload['actor']['inbox']
+      end
+
+      def previous_subscription
+        @previous_subscription ||= ReleasesSubscription.find_by_subscriber_url(subscriber_url)
+      end
+
+      def enqueue_subscription
+        ReleasesSubscriptionWorker.perform_async(subscription.id)
+      end
+    end
+  end
+end
diff --git a/config/routes/activity_pub.rb b/config/routes/activity_pub.rb
index f400d722e7612c..a967889a0ad4be 100644
--- a/config/routes/activity_pub.rb
+++ b/config/routes/activity_pub.rb
@@ -21,6 +21,7 @@
             resources :releases, only: :index do
               collection do
                 get 'outbox'
+                post 'inbox'
               end
             end
           end
diff --git a/spec/controllers/activity_pub/projects/releases_controller_spec.rb b/spec/controllers/activity_pub/projects/releases_controller_spec.rb
index 8719756b2604b3..c6c2a76234cd26 100644
--- a/spec/controllers/activity_pub/projects/releases_controller_spec.rb
+++ b/spec/controllers/activity_pub/projects/releases_controller_spec.rb
@@ -11,13 +11,15 @@
   let_it_be(:release_1) { create(:release, project: project, released_at: Time.zone.parse('2018-10-18')) }
   let_it_be(:release_2) { create(:release, project: project, released_at: Time.zone.parse('2019-10-19')) }
 
+  let(:request_body) { '' }
+
   before_all do
     project.add_developer(developer)
   end
 
   shared_examples 'common access controls' do
     it 'renders a 200' do
-      get(action, params: params)
+      perform_action(verb, action, params, request_body)
 
       expect(response).to have_gitlab_http_status(:ok)
     end
@@ -27,7 +29,7 @@
 
       context 'when user is not logged in' do
         it 'renders a 404' do
-          get(action, params: params)
+          perform_action(verb, action, params, request_body)
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
@@ -39,7 +41,7 @@
         end
 
         it 'still renders a 404' do
-          get(action, params: params)
+          perform_action(verb, action, params, request_body)
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
@@ -52,7 +54,7 @@
       end
 
       it 'renders a 404' do
-        get(action, params: params)
+        perform_action(verb, action, params, request_body)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
@@ -64,7 +66,7 @@
       end
 
       it 'renders a 404' do
-        get(action, params: params)
+        perform_action(verb, action, params, request_body)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
@@ -83,9 +85,10 @@
 
   describe 'GET #index' do
     before do
-      get(action, params: params)
+      perform_action(verb, action, params)
     end
 
+    let(:verb) { :get }
     let(:action) { :index }
     let(:params) { { namespace_id: project.namespace, project_id: project } }
 
@@ -99,9 +102,10 @@
 
   describe 'GET #outbox' do
     before do
-      get(action, params: params)
+      perform_action(verb, action, params)
     end
 
+    let(:verb) { :get }
     let(:action) { :outbox }
     let(:params) { { namespace_id: project.namespace, project_id: project, page: page } }
 
@@ -131,4 +135,167 @@
       end
     end
   end
+
+  describe 'POST #inbox' do
+    before do
+      allow(ActivityPub::Projects::ReleasesSubscriptionService).to receive(:new) { service }
+    end
+
+    let(:verb) { :post }
+    let(:action) { :inbox }
+    let(:params) { { namespace_id: project.namespace, project_id: project } }
+    let(:service) do
+      instance_double(ActivityPub::Projects::ReleasesSubscriptionService, follow: true, unfollow: true,
+        errors: ['an error'])
+    end
+
+    context 'with a follow activity' do
+      before do
+        perform_action(verb, action, params, request_body)
+      end
+
+      let(:request_body) do
+        {
+          "@context": "https://www.w3.org/ns/activitystreams",
+          id: "http://localhost:3001/6233e6c2-d285-4aa4-bd71-ddf1824d87f8",
+          type: "Follow",
+          actor: "http://localhost:3001/users/admin",
+          object: "http://127.0.0.1:3000/flightjs/Flight/-/releases"
+        }.to_json
+      end
+
+      it_behaves_like 'common access controls'
+
+      context 'with successful subscription initialization' do
+        it 'calls the subscription service' do
+          expect(service).to have_received :follow
+        end
+
+        it 'returns a successful response' do
+          expect(json_response['success']).to be_truthy
+        end
+
+        it 'does not fill any error' do
+          expect(json_response).not_to have_key 'errors'
+        end
+      end
+
+      context 'with unsuccessful subscription initialization' do
+        let(:service) do
+          instance_double(ActivityPub::Projects::ReleasesSubscriptionService, follow: false, errors: ['an error'])
+        end
+
+        it 'calls the subscription service' do
+          expect(service).to have_received :follow
+        end
+
+        it 'returns a successful response' do
+          expect(json_response['success']).to be_falsey
+        end
+
+        it 'fills an error' do
+          expect(json_response['errors']).to include 'an error'
+        end
+      end
+    end
+
+    context 'with an unfollow activity' do
+      before do
+        perform_action(verb, action, params, request_body)
+      end
+
+      let(:service) do
+        instance_double(ActivityPub::Projects::ReleasesSubscriptionService, unfollow: true, errors: ['an error'])
+      end
+
+      let(:request_body) do
+        {
+          "@context": "https://www.w3.org/ns/activitystreams",
+          id: "http://localhost:3001/users/admin#follows/8/undo",
+          type: "Undo",
+          actor: "http://localhost:3001/users/admin",
+          object: {
+            id: "http://localhost:3001/d4358269-71a9-4746-ac16-9a909f12ee5b",
+            type: "Follow",
+            actor: "http://localhost:3001/users/admin",
+            object: "http://127.0.0.1:3000/flightjs/Flight/-/releases"
+          }
+        }.to_json
+      end
+
+      it_behaves_like 'common access controls'
+
+      context 'with successful unfollow' do
+        it 'calls the subscription service' do
+          expect(service).to have_received :unfollow
+        end
+
+        it 'returns a successful response' do
+          expect(json_response['success']).to be_truthy
+        end
+
+        it 'does not fill any error' do
+          expect(json_response).not_to have_key 'errors'
+        end
+      end
+
+      context 'with unsuccessful unfollow' do
+        let(:service) do
+          instance_double(ActivityPub::Projects::ReleasesSubscriptionService, unfollow: false, errors: ['an error'])
+        end
+
+        it 'calls the subscription service' do
+          expect(service).to have_received :unfollow
+        end
+
+        it 'returns a successful response' do
+          expect(json_response['success']).to be_falsey
+        end
+
+        it 'fills an error' do
+          expect(json_response['errors']).to include 'an error'
+        end
+      end
+    end
+
+    context 'with an unknown activity' do
+      before do
+        perform_action(verb, action, params, request_body)
+      end
+
+      let(:request_body) do
+        {
+          "@context": "https://www.w3.org/ns/activitystreams",
+          id: "http://localhost:3001/6233e6c2-d285-4aa4-bd71-ddf1824d87f8",
+          type: "Like",
+          actor: "http://localhost:3001/users/admin",
+          object: "http://127.0.0.1:3000/flightjs/Flight/-/releases"
+        }.to_json
+      end
+
+      it 'does not call the subscription service' do
+        expect(service).not_to have_received :follow
+        expect(service).not_to have_received :unfollow
+      end
+
+      it 'returns a successful response' do
+        expect(json_response['success']).to be_truthy
+      end
+
+      it 'does not fill any error' do
+        expect(json_response).not_to have_key 'errors'
+      end
+    end
+
+    context 'with no activity' do
+      it 'renders a 422' do
+        perform_action(verb, action, params, request_body)
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+      end
+    end
+  end
+end
+
+def perform_action(verb, action, params, body = nil)
+  send(verb, action, params: params, body: body)
 end
diff --git a/spec/services/activity_pub/projects/releases_subscription_service_spec.rb b/spec/services/activity_pub/projects/releases_subscription_service_spec.rb
new file mode 100644
index 00000000000000..7e52b386a8cce9
--- /dev/null
+++ b/spec/services/activity_pub/projects/releases_subscription_service_spec.rb
@@ -0,0 +1,315 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ActivityPub::Projects::ReleasesSubscriptionService, feature_category: :release_orchestration do
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) }
+
+  describe '#follow' do
+    let(:service) { described_class.new(project, payload) }
+    let(:payload) { nil }
+
+    before do
+      allow(ActivityPub::Projects::ReleasesSubscriptionWorker).to receive(:perform_async)
+    end
+
+    context 'with a valid payload' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#follow-1',
+          type: 'Follow',
+          actor: actor,
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      let(:actor) { 'https://example.com/new-actor' }
+
+      shared_examples 'valid follow request' do
+        it 'sets the subscriber url' do
+          service.follow
+          expect(ActivityPub::ReleasesSubscription.last.subscriber_url).to eq 'https://example.com/new-actor'
+        end
+
+        it 'sets the payload' do
+          service.follow
+          expect(ActivityPub::ReleasesSubscription.last.payload).to eq payload
+        end
+
+        it 'sets the project' do
+          service.follow
+          expect(ActivityPub::ReleasesSubscription.last.project_id).to eq project.id
+        end
+
+        it 'saves the subscription' do
+          expect { service.follow }.to change { ActivityPub::ReleasesSubscription.count }.by(1)
+        end
+
+        it 'returns true' do
+          expect(service.follow).to be_truthy
+        end
+      end
+
+      context 'when there is no subscription for that actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(nil)
+        end
+
+        context 'when only the actor id is provided' do
+          it_behaves_like 'valid follow request'
+
+          it 'queues the subscription job' do
+            service.follow
+            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async)
+          end
+        end
+
+        context 'when the actor inbox is provided' do
+          let(:actor) { { id: 'https://example.com/new-actor', inbox: 'https://example.com/new-actor/inbox' } }
+
+          it_behaves_like 'valid follow request'
+
+          it 'sets the inbox url' do
+            service.follow
+            expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/new-actor/inbox'
+          end
+
+          it 'does not queue the subscription job' do
+            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+          end
+        end
+      end
+
+      context 'when there is already a subscription for that actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url) { existing_subscription }
+        end
+
+        it 'does not save the subscription' do
+          expect { service.follow }.not_to change { ActivityPub::ReleasesSubscription.count }
+        end
+
+        it 'does not queue the subscription job' do
+          service.follow
+          expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+        end
+
+        it 'returns true' do
+          expect(service.follow).to be_truthy
+        end
+      end
+    end
+
+    shared_examples 'invalid follow request' do
+      it 'does not save the subscription' do
+        expect { service.follow }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'does not queue the subscription job' do
+        service.follow
+        expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+      end
+
+      it 'sets an error' do
+        service.follow
+        expect(service.errors).not_to be_empty
+      end
+
+      it 'returns false' do
+        expect(service.follow).to be_falsey
+      end
+    end
+
+    context 'when actor is missing' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+
+    context 'when actor is an object with no id attribute' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          actor: { type: 'Person' },
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+
+    context 'when actor is neither a string nor an object' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          actor: 27.13,
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+  end
+
+  describe '#unfollow' do
+    let(:service) { described_class.new(project, payload) }
+    let(:payload) { nil }
+
+    context 'with a valid payload' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          type: 'Undo',
+          actor: actor,
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: actor,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      let(:actor) { 'https://example.com/new-actor' }
+
+      context 'when there is a subscription for this actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(existing_subscription)
+          allow(existing_subscription).to receive(:destroy).and_return(true)
+        end
+
+        it 'deletes the subscription' do
+          service.unfollow
+          expect(existing_subscription).to have_received(:destroy)
+        end
+
+        it 'returns true' do
+          expect(service.unfollow).to be_truthy
+        end
+      end
+
+      context 'when there is no subscription for this actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(nil)
+        end
+
+        it 'does not delete anything' do
+          expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
+        end
+
+        it 'returns true' do
+          expect(service.unfollow).to be_truthy
+        end
+      end
+    end
+
+    shared_examples 'invalid unfollow request' do
+      it 'does not delete anything' do
+        expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'sets an error' do
+        service.unfollow
+        expect(service.errors).not_to be_empty
+      end
+
+      it 'returns false' do
+        expect(service.unfollow).to be_falsey
+      end
+    end
+
+    context 'when actor is missing' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context 'when actor is an object with no id attribute' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          actor: { type: 'Person' },
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: { type: 'Person' },
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context 'when actor is neither a string nor an object' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          actor: 27.13,
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: 27.13,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context "when actor tries to delete someone else's subscription" do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/actor#unfollow-1',
+          type: 'Undo',
+          actor: 'https://example.com/nasty-actor',
+          object: {
+            id: 'https://example.com/actor#follow-1',
+            type: 'Follow',
+            actor: existing_subscription.subscriber_url,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it 'does not delete anything' do
+        expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'returns true' do
+        expect(service.unfollow).to be_truthy
+      end
+    end
+  end
+end
-- 
GitLab


From 298666d11acf9357de53c8e1435108c9d4579351 Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 31 Oct 2023 14:43:07 +0100
Subject: [PATCH 2/8] REFACTOR for Patrick's review

Splitted the subscription service into a follow service and an unfollow
service, and extracted a method in the controller.

Additionally, I realized there was a functional problem with the
unfollow, in which the subscription deletion was not scoped to the
project, this is fixed.
---
 .../projects/releases_controller.rb           |  16 +-
 .../activity_pub/releases_subscription.rb     |   4 +-
 .../projects/releases_follow_service.rb       |  43 +++
 .../projects/releases_subscription_service.rb |  47 +--
 .../projects/releases_unfollow_service.rb     |  18 +
 .../projects/releases_controller_spec.rb      |  37 +-
 .../releases_subscription_spec.rb             |  14 +-
 .../projects/releases_follow_service_spec.rb  | 166 +++++++++
 .../releases_subscription_service_spec.rb     | 315 ------------------
 .../releases_unfollow_service_spec.rb         | 158 +++++++++
 10 files changed, 428 insertions(+), 390 deletions(-)
 create mode 100644 app/services/activity_pub/projects/releases_follow_service.rb
 create mode 100644 app/services/activity_pub/projects/releases_unfollow_service.rb
 create mode 100644 spec/services/activity_pub/projects/releases_follow_service_spec.rb
 delete mode 100644 spec/services/activity_pub/projects/releases_subscription_service_spec.rb
 create mode 100644 spec/services/activity_pub/projects/releases_unfollow_service_spec.rb

diff --git a/app/controllers/activity_pub/projects/releases_controller.rb b/app/controllers/activity_pub/projects/releases_controller.rb
index e612331ffbfc59..eeff96a5ef7047 100644
--- a/app/controllers/activity_pub/projects/releases_controller.rb
+++ b/app/controllers/activity_pub/projects/releases_controller.rb
@@ -17,13 +17,8 @@ def index
       end
 
       def inbox
-        service = ReleasesSubscriptionService.new(project, payload)
-
-        success = if follow? || unfollow?
-                    follow? ? service.follow : service.unfollow
-                  else
-                    true
-                  end
+        service = inbox_service
+        success = service ? service.execute : true
 
         response = { success: success }
         response[:errors] = service.errors unless success
@@ -67,6 +62,13 @@ def unfollow?
         follow = object.present? && object.is_a?(Hash) && object['type'] == 'Follow'
         undo && follow
       end
+
+      def inbox_service
+        return ReleasesFollowService.new(project, payload) if follow?
+        return ReleasesUnfollowService.new(project, payload) if unfollow?
+
+        nil
+      end
     end
   end
 end
diff --git a/app/models/activity_pub/releases_subscription.rb b/app/models/activity_pub/releases_subscription.rb
index 5c730c7555a3b3..0a4293b2bdea71 100644
--- a/app/models/activity_pub/releases_subscription.rb
+++ b/app/models/activity_pub/releases_subscription.rb
@@ -15,8 +15,8 @@ class ReleasesSubscription < ApplicationRecord
       public_url: { allow_nil: true }
     validates :shared_inbox_url, public_url: { allow_nil: true }
 
-    def self.find_by_subscriber_url(subscriber_url)
-      find_by('LOWER(subscriber_url) = ?', subscriber_url.downcase)
+    def self.find_by_project_and_subscriber(project_id, subscriber_url)
+      find_by('project_id = ? AND LOWER(subscriber_url) = ?', project_id, subscriber_url.downcase)
     end
   end
 end
diff --git a/app/services/activity_pub/projects/releases_follow_service.rb b/app/services/activity_pub/projects/releases_follow_service.rb
new file mode 100644
index 00000000000000..626f2da7092bfb
--- /dev/null
+++ b/app/services/activity_pub/projects/releases_follow_service.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module ActivityPub
+  module Projects
+    class ReleasesFollowService < ReleasesSubscriptionService
+      def execute
+        unless subscriber_url
+          errors << "You need to provide an actor id for your subscriber"
+          return false
+        end
+
+        return true if previous_subscription.present?
+
+        @subscription = ReleasesSubscription.new(
+          subscriber_url: subscriber_url,
+          subscriber_inbox_url: subscriber_inbox_url,
+          payload: payload,
+          project: project
+        )
+
+        unless subscription.save
+          errors.concat(subscription.errors.full_messages)
+          return false
+        end
+
+        enqueue_subscription
+        true
+      end
+
+      private
+
+      def subscriber_inbox_url
+        return unless payload['actor'].is_a?(Hash)
+
+        payload['actor']['inbox']
+      end
+
+      def enqueue_subscription
+        ReleasesSubscriptionWorker.perform_async(subscription.id)
+      end
+    end
+  end
+end
diff --git a/app/services/activity_pub/projects/releases_subscription_service.rb b/app/services/activity_pub/projects/releases_subscription_service.rb
index 6a89063a53aea2..df7b0f2f0d51f2 100644
--- a/app/services/activity_pub/projects/releases_subscription_service.rb
+++ b/app/services/activity_pub/projects/releases_subscription_service.rb
@@ -11,39 +11,8 @@ def initialize(project, payload)
         @errors = []
       end
 
-      def follow
-        unless subscriber_url
-          errors << "You need to provide an actor id for your subscriber"
-          return false
-        end
-
-        return true if previous_subscription.present?
-
-        @subscription = ReleasesSubscription.new(
-          subscriber_url: subscriber_url,
-          subscriber_inbox_url: subscriber_inbox_url,
-          payload: payload,
-          project: project
-        )
-
-        unless subscription.save
-          errors.concat(subscription.errors.full_messages)
-          return false
-        end
-
-        enqueue_subscription
-        true
-      end
-
-      def unfollow
-        unless subscriber_url
-          errors << "You need to provide an actor id for your unsubscribe activity"
-          return false
-        end
-
-        return true unless previous_subscription.present?
-
-        previous_subscription.destroy
+      def execute
+        raise "not implemented: abstract class, do not use directly."
       end
 
       private
@@ -56,18 +25,8 @@ def subscriber_url
         payload['actor']['id']
       end
 
-      def subscriber_inbox_url
-        return unless payload['actor'].is_a?(Hash)
-
-        payload['actor']['inbox']
-      end
-
       def previous_subscription
-        @previous_subscription ||= ReleasesSubscription.find_by_subscriber_url(subscriber_url)
-      end
-
-      def enqueue_subscription
-        ReleasesSubscriptionWorker.perform_async(subscription.id)
+        @previous_subscription ||= ReleasesSubscription.find_by_project_and_subscriber(project.id, subscriber_url)
       end
     end
   end
diff --git a/app/services/activity_pub/projects/releases_unfollow_service.rb b/app/services/activity_pub/projects/releases_unfollow_service.rb
new file mode 100644
index 00000000000000..df5dcefbb872ce
--- /dev/null
+++ b/app/services/activity_pub/projects/releases_unfollow_service.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module ActivityPub
+  module Projects
+    class ReleasesUnfollowService < ReleasesSubscriptionService
+      def execute
+        unless subscriber_url
+          errors << "You need to provide an actor id for your unsubscribe activity"
+          return false
+        end
+
+        return true unless previous_subscription.present?
+
+        previous_subscription.destroy
+      end
+    end
+  end
+end
diff --git a/spec/controllers/activity_pub/projects/releases_controller_spec.rb b/spec/controllers/activity_pub/projects/releases_controller_spec.rb
index c6c2a76234cd26..4102789ee431ac 100644
--- a/spec/controllers/activity_pub/projects/releases_controller_spec.rb
+++ b/spec/controllers/activity_pub/projects/releases_controller_spec.rb
@@ -138,15 +138,20 @@
 
   describe 'POST #inbox' do
     before do
-      allow(ActivityPub::Projects::ReleasesSubscriptionService).to receive(:new) { service }
+      allow(ActivityPub::Projects::ReleasesFollowService).to receive(:new) { follow_service }
+      allow(ActivityPub::Projects::ReleasesUnfollowService).to receive(:new) { unfollow_service }
     end
 
     let(:verb) { :post }
     let(:action) { :inbox }
     let(:params) { { namespace_id: project.namespace, project_id: project } }
-    let(:service) do
-      instance_double(ActivityPub::Projects::ReleasesSubscriptionService, follow: true, unfollow: true,
-        errors: ['an error'])
+
+    let(:follow_service) do
+      instance_double(ActivityPub::Projects::ReleasesFollowService, execute: true, errors: ['an error'])
+    end
+
+    let(:unfollow_service) do
+      instance_double(ActivityPub::Projects::ReleasesUnfollowService, execute: true, errors: ['an error'])
     end
 
     context 'with a follow activity' do
@@ -168,7 +173,7 @@
 
       context 'with successful subscription initialization' do
         it 'calls the subscription service' do
-          expect(service).to have_received :follow
+          expect(follow_service).to have_received :execute
         end
 
         it 'returns a successful response' do
@@ -181,12 +186,12 @@
       end
 
       context 'with unsuccessful subscription initialization' do
-        let(:service) do
-          instance_double(ActivityPub::Projects::ReleasesSubscriptionService, follow: false, errors: ['an error'])
+        let(:follow_service) do
+          instance_double(ActivityPub::Projects::ReleasesFollowService, execute: false, errors: ['an error'])
         end
 
         it 'calls the subscription service' do
-          expect(service).to have_received :follow
+          expect(follow_service).to have_received :execute
         end
 
         it 'returns a successful response' do
@@ -204,8 +209,8 @@
         perform_action(verb, action, params, request_body)
       end
 
-      let(:service) do
-        instance_double(ActivityPub::Projects::ReleasesSubscriptionService, unfollow: true, errors: ['an error'])
+      let(:unfollow_service) do
+        instance_double(ActivityPub::Projects::ReleasesSubscriptionService, execute: true, errors: ['an error'])
       end
 
       let(:request_body) do
@@ -227,7 +232,7 @@
 
       context 'with successful unfollow' do
         it 'calls the subscription service' do
-          expect(service).to have_received :unfollow
+          expect(unfollow_service).to have_received :execute
         end
 
         it 'returns a successful response' do
@@ -240,12 +245,12 @@
       end
 
       context 'with unsuccessful unfollow' do
-        let(:service) do
-          instance_double(ActivityPub::Projects::ReleasesSubscriptionService, unfollow: false, errors: ['an error'])
+        let(:unfollow_service) do
+          instance_double(ActivityPub::Projects::ReleasesUnfollowService, execute: false, errors: ['an error'])
         end
 
         it 'calls the subscription service' do
-          expect(service).to have_received :unfollow
+          expect(unfollow_service).to have_received :execute
         end
 
         it 'returns a successful response' do
@@ -274,8 +279,8 @@
       end
 
       it 'does not call the subscription service' do
-        expect(service).not_to have_received :follow
-        expect(service).not_to have_received :unfollow
+        expect(follow_service).not_to have_received :execute
+        expect(unfollow_service).not_to have_received :execute
       end
 
       it 'returns a successful response' do
diff --git a/spec/models/activity_pub/releases_subscription_spec.rb b/spec/models/activity_pub/releases_subscription_spec.rb
index 0c873a5c18ae1d..35c231a7c2dbb2 100644
--- a/spec/models/activity_pub/releases_subscription_spec.rb
+++ b/spec/models/activity_pub/releases_subscription_spec.rb
@@ -55,23 +55,25 @@
     end
   end
 
-  describe '.find_by_subscriber_url' do
+  describe '.find_by_project_and_subscriber' do
     let_it_be(:subscription) { create(:activity_pub_releases_subscription) }
 
     it 'returns a record if arguments match' do
-      result = described_class.find_by_subscriber_url(subscription.subscriber_url)
+      result = described_class.find_by_project_and_subscriber(subscription.project_id,
+        subscription.subscriber_url)
 
       expect(result).to eq(subscription)
     end
 
-    it 'returns a record if arguments match case insensitively' do
-      result = described_class.find_by_subscriber_url(subscription.subscriber_url.upcase)
+    it 'returns a record if subscriber url matches case insensitively' do
+      result = described_class.find_by_project_and_subscriber(subscription.project_id,
+        subscription.subscriber_url.upcase)
 
       expect(result).to eq(subscription)
     end
 
-    it 'returns nil if project does not match' do
-      result = described_class.find_by_subscriber_url('I really should not exist')
+    it 'returns nil if project and url do not match' do
+      result = described_class.find_by_project_and_subscriber(0, 'I really should not exist')
 
       expect(result).to be(nil)
     end
diff --git a/spec/services/activity_pub/projects/releases_follow_service_spec.rb b/spec/services/activity_pub/projects/releases_follow_service_spec.rb
new file mode 100644
index 00000000000000..fce92dcf044aa5
--- /dev/null
+++ b/spec/services/activity_pub/projects/releases_follow_service_spec.rb
@@ -0,0 +1,166 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ActivityPub::Projects::ReleasesFollowService, feature_category: :release_orchestration do
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) }
+
+  describe '#execute' do
+    let(:service) { described_class.new(project, payload) }
+    let(:payload) { nil }
+
+    before do
+      allow(ActivityPub::Projects::ReleasesSubscriptionWorker).to receive(:perform_async)
+    end
+
+    context 'with a valid payload' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#follow-1',
+          type: 'Follow',
+          actor: actor,
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      let(:actor) { 'https://example.com/new-actor' }
+
+      shared_examples 'valid follow request' do
+        it 'sets the subscriber url' do
+          service.execute
+          expect(ActivityPub::ReleasesSubscription.last.subscriber_url).to eq 'https://example.com/new-actor'
+        end
+
+        it 'sets the payload' do
+          service.execute
+          expect(ActivityPub::ReleasesSubscription.last.payload).to eq payload
+        end
+
+        it 'sets the project' do
+          service.execute
+          expect(ActivityPub::ReleasesSubscription.last.project_id).to eq project.id
+        end
+
+        it 'saves the subscription' do
+          expect { service.execute }.to change { ActivityPub::ReleasesSubscription.count }.by(1)
+        end
+
+        it 'returns true' do
+          expect(service.execute).to be_truthy
+        end
+      end
+
+      context 'when there is no subscription for that actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil)
+        end
+
+        context 'when only the actor id is provided' do
+          it_behaves_like 'valid follow request'
+
+          it 'queues the subscription job' do
+            service.execute
+            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async)
+          end
+        end
+
+        context 'when the actor inbox is provided' do
+          let(:actor) { { id: 'https://example.com/new-actor', inbox: 'https://example.com/new-actor/inbox' } }
+
+          it_behaves_like 'valid follow request'
+
+          it 'sets the inbox url' do
+            service.execute
+            expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/new-actor/inbox'
+          end
+
+          it 'does not queue the subscription job' do
+            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+          end
+        end
+      end
+
+      context 'when there is already a subscription for that actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber) { existing_subscription }
+        end
+
+        it 'does not save the subscription' do
+          expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count }
+        end
+
+        it 'does not queue the subscription job' do
+          service.execute
+          expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+        end
+
+        it 'returns true' do
+          expect(service.execute).to be_truthy
+        end
+      end
+    end
+
+    shared_examples 'invalid follow request' do
+      it 'does not save the subscription' do
+        expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'does not queue the subscription job' do
+        service.execute
+        expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
+      end
+
+      it 'sets an error' do
+        service.execute
+        expect(service.errors).not_to be_empty
+      end
+
+      it 'returns false' do
+        expect(service.execute).to be_falsey
+      end
+    end
+
+    context 'when actor is missing' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+
+    context 'when actor is an object with no id attribute' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          actor: { type: 'Person' },
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+
+    context 'when actor is neither a string nor an object' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor',
+          type: 'Follow',
+          actor: 27.13,
+          object: 'https://localhost/our/project/-/releases'
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid follow request'
+    end
+  end
+end
diff --git a/spec/services/activity_pub/projects/releases_subscription_service_spec.rb b/spec/services/activity_pub/projects/releases_subscription_service_spec.rb
deleted file mode 100644
index 7e52b386a8cce9..00000000000000
--- a/spec/services/activity_pub/projects/releases_subscription_service_spec.rb
+++ /dev/null
@@ -1,315 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ActivityPub::Projects::ReleasesSubscriptionService, feature_category: :release_orchestration do
-  let_it_be(:project) { create(:project, :public) }
-  let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) }
-
-  describe '#follow' do
-    let(:service) { described_class.new(project, payload) }
-    let(:payload) { nil }
-
-    before do
-      allow(ActivityPub::Projects::ReleasesSubscriptionWorker).to receive(:perform_async)
-    end
-
-    context 'with a valid payload' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor#follow-1',
-          type: 'Follow',
-          actor: actor,
-          object: 'https://localhost/our/project/-/releases'
-        }.with_indifferent_access
-      end
-
-      let(:actor) { 'https://example.com/new-actor' }
-
-      shared_examples 'valid follow request' do
-        it 'sets the subscriber url' do
-          service.follow
-          expect(ActivityPub::ReleasesSubscription.last.subscriber_url).to eq 'https://example.com/new-actor'
-        end
-
-        it 'sets the payload' do
-          service.follow
-          expect(ActivityPub::ReleasesSubscription.last.payload).to eq payload
-        end
-
-        it 'sets the project' do
-          service.follow
-          expect(ActivityPub::ReleasesSubscription.last.project_id).to eq project.id
-        end
-
-        it 'saves the subscription' do
-          expect { service.follow }.to change { ActivityPub::ReleasesSubscription.count }.by(1)
-        end
-
-        it 'returns true' do
-          expect(service.follow).to be_truthy
-        end
-      end
-
-      context 'when there is no subscription for that actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(nil)
-        end
-
-        context 'when only the actor id is provided' do
-          it_behaves_like 'valid follow request'
-
-          it 'queues the subscription job' do
-            service.follow
-            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async)
-          end
-        end
-
-        context 'when the actor inbox is provided' do
-          let(:actor) { { id: 'https://example.com/new-actor', inbox: 'https://example.com/new-actor/inbox' } }
-
-          it_behaves_like 'valid follow request'
-
-          it 'sets the inbox url' do
-            service.follow
-            expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/new-actor/inbox'
-          end
-
-          it 'does not queue the subscription job' do
-            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
-          end
-        end
-      end
-
-      context 'when there is already a subscription for that actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url) { existing_subscription }
-        end
-
-        it 'does not save the subscription' do
-          expect { service.follow }.not_to change { ActivityPub::ReleasesSubscription.count }
-        end
-
-        it 'does not queue the subscription job' do
-          service.follow
-          expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
-        end
-
-        it 'returns true' do
-          expect(service.follow).to be_truthy
-        end
-      end
-    end
-
-    shared_examples 'invalid follow request' do
-      it 'does not save the subscription' do
-        expect { service.follow }.not_to change { ActivityPub::ReleasesSubscription.count }
-      end
-
-      it 'does not queue the subscription job' do
-        service.follow
-        expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
-      end
-
-      it 'sets an error' do
-        service.follow
-        expect(service.errors).not_to be_empty
-      end
-
-      it 'returns false' do
-        expect(service.follow).to be_falsey
-      end
-    end
-
-    context 'when actor is missing' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor',
-          type: 'Follow',
-          object: 'https://localhost/our/project/-/releases'
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid follow request'
-    end
-
-    context 'when actor is an object with no id attribute' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor',
-          type: 'Follow',
-          actor: { type: 'Person' },
-          object: 'https://localhost/our/project/-/releases'
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid follow request'
-    end
-
-    context 'when actor is neither a string nor an object' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor',
-          type: 'Follow',
-          actor: 27.13,
-          object: 'https://localhost/our/project/-/releases'
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid follow request'
-    end
-  end
-
-  describe '#unfollow' do
-    let(:service) { described_class.new(project, payload) }
-    let(:payload) { nil }
-
-    context 'with a valid payload' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor#unfollow-1',
-          type: 'Undo',
-          actor: actor,
-          object: {
-            id: 'https://example.com/new-actor#follow-1',
-            type: 'Follow',
-            actor: actor,
-            object: 'https://localhost/our/project/-/releases'
-          }
-        }.with_indifferent_access
-      end
-
-      let(:actor) { 'https://example.com/new-actor' }
-
-      context 'when there is a subscription for this actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(existing_subscription)
-          allow(existing_subscription).to receive(:destroy).and_return(true)
-        end
-
-        it 'deletes the subscription' do
-          service.unfollow
-          expect(existing_subscription).to have_received(:destroy)
-        end
-
-        it 'returns true' do
-          expect(service.unfollow).to be_truthy
-        end
-      end
-
-      context 'when there is no subscription for this actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_subscriber_url).and_return(nil)
-        end
-
-        it 'does not delete anything' do
-          expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
-        end
-
-        it 'returns true' do
-          expect(service.unfollow).to be_truthy
-        end
-      end
-    end
-
-    shared_examples 'invalid unfollow request' do
-      it 'does not delete anything' do
-        expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
-      end
-
-      it 'sets an error' do
-        service.unfollow
-        expect(service.errors).not_to be_empty
-      end
-
-      it 'returns false' do
-        expect(service.unfollow).to be_falsey
-      end
-    end
-
-    context 'when actor is missing' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor#unfollow-1',
-          type: 'Undo',
-          object: {
-            id: 'https://example.com/new-actor#follow-1',
-            type: 'Follow',
-            object: 'https://localhost/our/project/-/releases'
-          }
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid unfollow request'
-    end
-
-    context 'when actor is an object with no id attribute' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor#unfollow-1',
-          actor: { type: 'Person' },
-          type: 'Undo',
-          object: {
-            id: 'https://example.com/new-actor#follow-1',
-            type: 'Follow',
-            actor: { type: 'Person' },
-            object: 'https://localhost/our/project/-/releases'
-          }
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid unfollow request'
-    end
-
-    context 'when actor is neither a string nor an object' do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/new-actor#unfollow-1',
-          actor: 27.13,
-          type: 'Undo',
-          object: {
-            id: 'https://example.com/new-actor#follow-1',
-            type: 'Follow',
-            actor: 27.13,
-            object: 'https://localhost/our/project/-/releases'
-          }
-        }.with_indifferent_access
-      end
-
-      it_behaves_like 'invalid unfollow request'
-    end
-
-    context "when actor tries to delete someone else's subscription" do
-      let(:payload) do
-        {
-          '@context': 'https://www.w3.org/ns/activitystreams',
-          id: 'https://example.com/actor#unfollow-1',
-          type: 'Undo',
-          actor: 'https://example.com/nasty-actor',
-          object: {
-            id: 'https://example.com/actor#follow-1',
-            type: 'Follow',
-            actor: existing_subscription.subscriber_url,
-            object: 'https://localhost/our/project/-/releases'
-          }
-        }.with_indifferent_access
-      end
-
-      it 'does not delete anything' do
-        expect { service.unfollow }.not_to change { ActivityPub::ReleasesSubscription.count }
-      end
-
-      it 'returns true' do
-        expect(service.unfollow).to be_truthy
-      end
-    end
-  end
-end
diff --git a/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb
new file mode 100644
index 00000000000000..7a803260e83018
--- /dev/null
+++ b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb
@@ -0,0 +1,158 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ActivityPub::Projects::ReleasesUnfollowService, feature_category: :release_orchestration do
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) }
+
+  describe '#execute' do
+    let(:service) { described_class.new(project, payload) }
+    let(:payload) { nil }
+
+    context 'with a valid payload' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          type: 'Undo',
+          actor: actor,
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: actor,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      let(:actor) { 'https://example.com/new-actor' }
+
+      context 'when there is a subscription for this actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber)
+            .and_return(existing_subscription)
+          allow(existing_subscription).to receive(:destroy).and_return(true)
+        end
+
+        it 'deletes the subscription' do
+          service.execute
+          expect(existing_subscription).to have_received(:destroy)
+        end
+
+        it 'returns true' do
+          expect(service.execute).to be_truthy
+        end
+      end
+
+      context 'when there is no subscription for this actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil)
+        end
+
+        it 'does not delete anything' do
+          expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count }
+        end
+
+        it 'returns true' do
+          expect(service.execute).to be_truthy
+        end
+      end
+    end
+
+    shared_examples 'invalid unfollow request' do
+      it 'does not delete anything' do
+        expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'sets an error' do
+        service.execute
+        expect(service.errors).not_to be_empty
+      end
+
+      it 'returns false' do
+        expect(service.execute).to be_falsey
+      end
+    end
+
+    context 'when actor is missing' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context 'when actor is an object with no id attribute' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          actor: { type: 'Person' },
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: { type: 'Person' },
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context 'when actor is neither a string nor an object' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/new-actor#unfollow-1',
+          actor: 27.13,
+          type: 'Undo',
+          object: {
+            id: 'https://example.com/new-actor#follow-1',
+            type: 'Follow',
+            actor: 27.13,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it_behaves_like 'invalid unfollow request'
+    end
+
+    context "when actor tries to delete someone else's subscription" do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'https://example.com/actor#unfollow-1',
+          type: 'Undo',
+          actor: 'https://example.com/nasty-actor',
+          object: {
+            id: 'https://example.com/actor#follow-1',
+            type: 'Follow',
+            actor: existing_subscription.subscriber_url,
+            object: 'https://localhost/our/project/-/releases'
+          }
+        }.with_indifferent_access
+      end
+
+      it 'does not delete anything' do
+        expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count }
+      end
+
+      it 'returns true' do
+        expect(service.execute).to be_truthy
+      end
+    end
+  end
+end
-- 
GitLab


From ffa03c1f61777faf990787f911376003734b6232 Mon Sep 17 00:00:00 2001
From: Patrick Cyiza <jpcyiza@gitlab.com>
Date: Mon, 6 Nov 2023 09:20:59 +0000
Subject: [PATCH 3/8] REFACTOR cosmetic change in #subscriber_url

---
 .../activity_pub/projects/releases_subscription_service.rb      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/activity_pub/projects/releases_subscription_service.rb b/app/services/activity_pub/projects/releases_subscription_service.rb
index df7b0f2f0d51f2..450eaee1b1bc85 100644
--- a/app/services/activity_pub/projects/releases_subscription_service.rb
+++ b/app/services/activity_pub/projects/releases_subscription_service.rb
@@ -18,7 +18,7 @@ def execute
       private
 
       def subscriber_url
-        return false unless payload['actor']
+        return unless payload['actor']
         return payload['actor'] if payload['actor'].is_a?(String)
         return unless payload['actor'].is_a?(Hash) && payload['actor']['id'].is_a?(String)
 
-- 
GitLab


From 26daa5d4adc93b24d99fa1d5c243bae7981373cc Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 14 Nov 2023 15:46:07 +0100
Subject: [PATCH 4/8] FIX remove bogus vestige specs

---
 .../projects/releases_follow_service_spec.rb  | 41 +++++--------------
 1 file changed, 10 insertions(+), 31 deletions(-)

diff --git a/spec/services/activity_pub/projects/releases_follow_service_spec.rb b/spec/services/activity_pub/projects/releases_follow_service_spec.rb
index fce92dcf044aa5..6d0d400b9c66a6 100644
--- a/spec/services/activity_pub/projects/releases_follow_service_spec.rb
+++ b/spec/services/activity_pub/projects/releases_follow_service_spec.rb
@@ -27,7 +27,11 @@
 
       let(:actor) { 'https://example.com/new-actor' }
 
-      shared_examples 'valid follow request' do
+      context 'when there is no subscription for that actor' do
+        before do
+          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil)
+        end
+
         it 'sets the subscriber url' do
           service.execute
           expect(ActivityPub::ReleasesSubscription.last.subscriber_url).to eq 'https://example.com/new-actor'
@@ -47,38 +51,13 @@
           expect { service.execute }.to change { ActivityPub::ReleasesSubscription.count }.by(1)
         end
 
-        it 'returns true' do
-          expect(service.execute).to be_truthy
-        end
-      end
-
-      context 'when there is no subscription for that actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil)
-        end
-
-        context 'when only the actor id is provided' do
-          it_behaves_like 'valid follow request'
-
-          it 'queues the subscription job' do
-            service.execute
-            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async)
-          end
+        it 'queues the subscription job' do
+          service.execute
+          expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async)
         end
 
-        context 'when the actor inbox is provided' do
-          let(:actor) { { id: 'https://example.com/new-actor', inbox: 'https://example.com/new-actor/inbox' } }
-
-          it_behaves_like 'valid follow request'
-
-          it 'sets the inbox url' do
-            service.execute
-            expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/new-actor/inbox'
-          end
-
-          it 'does not queue the subscription job' do
-            expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async)
-          end
+        it 'returns true' do
+          expect(service.execute).to be_truthy
         end
       end
 
-- 
GitLab


From 8454a49e77c6164f2e29549174f6309da1e24e8e Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 14 Nov 2023 15:49:21 +0100
Subject: [PATCH 5/8] REFACTOR unexpose internal attribute readers

---
 .../activity_pub/projects/releases_subscription_service.rb    | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/services/activity_pub/projects/releases_subscription_service.rb b/app/services/activity_pub/projects/releases_subscription_service.rb
index 450eaee1b1bc85..74e2df15a127b9 100644
--- a/app/services/activity_pub/projects/releases_subscription_service.rb
+++ b/app/services/activity_pub/projects/releases_subscription_service.rb
@@ -3,7 +3,7 @@
 module ActivityPub
   module Projects
     class ReleasesSubscriptionService
-      attr_reader :project, :payload, :errors, :subscription
+      attr_reader :errors
 
       def initialize(project, payload)
         @project = project
@@ -17,6 +17,8 @@ def execute
 
       private
 
+      attr_reader :project, :payload, :subscription
+
       def subscriber_url
         return unless payload['actor']
         return payload['actor'] if payload['actor'].is_a?(String)
-- 
GitLab


From 875a1d5870da9d0ead303f842d5c50d8192754dc Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 14 Nov 2023 15:56:07 +0100
Subject: [PATCH 6/8] REFACTOR remove attribute accessor used only once

---
 .../activity_pub/projects/releases_follow_service.rb        | 6 +++---
 .../activity_pub/projects/releases_subscription_service.rb  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/services/activity_pub/projects/releases_follow_service.rb b/app/services/activity_pub/projects/releases_follow_service.rb
index 626f2da7092bfb..3d877a1d08377b 100644
--- a/app/services/activity_pub/projects/releases_follow_service.rb
+++ b/app/services/activity_pub/projects/releases_follow_service.rb
@@ -11,7 +11,7 @@ def execute
 
         return true if previous_subscription.present?
 
-        @subscription = ReleasesSubscription.new(
+        subscription = ReleasesSubscription.new(
           subscriber_url: subscriber_url,
           subscriber_inbox_url: subscriber_inbox_url,
           payload: payload,
@@ -23,7 +23,7 @@ def execute
           return false
         end
 
-        enqueue_subscription
+        enqueue_subscription(subscription)
         true
       end
 
@@ -35,7 +35,7 @@ def subscriber_inbox_url
         payload['actor']['inbox']
       end
 
-      def enqueue_subscription
+      def enqueue_subscription(subscription)
         ReleasesSubscriptionWorker.perform_async(subscription.id)
       end
     end
diff --git a/app/services/activity_pub/projects/releases_subscription_service.rb b/app/services/activity_pub/projects/releases_subscription_service.rb
index 74e2df15a127b9..27d0e19a172770 100644
--- a/app/services/activity_pub/projects/releases_subscription_service.rb
+++ b/app/services/activity_pub/projects/releases_subscription_service.rb
@@ -17,7 +17,7 @@ def execute
 
       private
 
-      attr_reader :project, :payload, :subscription
+      attr_reader :project, :payload
 
       def subscriber_url
         return unless payload['actor']
-- 
GitLab


From 1ca24573f0a51817ba34361b6189fd3d28bb710c Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 14 Nov 2023 16:06:06 +0100
Subject: [PATCH 7/8] ADD more specs for
 AP::ReleaseSubscription.find_by_project_and_subscriber

---
 .../activity_pub/releases_subscription_spec.rb       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/spec/models/activity_pub/releases_subscription_spec.rb b/spec/models/activity_pub/releases_subscription_spec.rb
index 35c231a7c2dbb2..0633f293971b67 100644
--- a/spec/models/activity_pub/releases_subscription_spec.rb
+++ b/spec/models/activity_pub/releases_subscription_spec.rb
@@ -77,5 +77,17 @@
 
       expect(result).to be(nil)
     end
+
+    it 'returns nil if project does not match' do
+      result = described_class.find_by_project_and_subscriber(0, subscription.subscriber_url)
+
+      expect(result).to be(nil)
+    end
+
+    it 'returns nil if url does not match' do
+      result = described_class.find_by_project_and_subscriber(subscription.project_id, 'I really should not exist')
+
+      expect(result).to be(nil)
+    end
   end
 end
-- 
GitLab


From a52f7ffd6a260e91bd1548576f84287e675d6704 Mon Sep 17 00:00:00 2001
From: Olivier El Mekki <olivier@el-mekki.com>
Date: Tue, 14 Nov 2023 16:23:20 +0100
Subject: [PATCH 8/8] REFACTOR use full db transactions in unfollow specs

---
 .../projects/releases_unfollow_service_spec.rb         | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb
index 7a803260e83018..c732d82a2addc1 100644
--- a/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb
+++ b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb
@@ -26,18 +26,12 @@
         }.with_indifferent_access
       end
 
-      let(:actor) { 'https://example.com/new-actor' }
+      let(:actor) { existing_subscription.subscriber_url }
 
       context 'when there is a subscription for this actor' do
-        before do
-          allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber)
-            .and_return(existing_subscription)
-          allow(existing_subscription).to receive(:destroy).and_return(true)
-        end
-
         it 'deletes the subscription' do
           service.execute
-          expect(existing_subscription).to have_received(:destroy)
+          expect(ActivityPub::ReleasesSubscription.where(id: existing_subscription.id).first).to be_nil
         end
 
         it 'returns true' do
-- 
GitLab