fix(membership): Ensure membership is in current organization when re…

…voking (#2658)

## Context

Today it is possible the revoke a membership from any organization as
long as you know the ID.
The `Mutations::Memberships::Revoke` is delegating the logic to
`Memberships::RevokeService`, but this service is not checking that the
membership is part of any organization (`Membership.find_by`)

## Description

This PR refactors the service to use the common `call` pattern.
It also update the mutation to require an Organization and lookup for
the membership with a scope to the current organization.

app/graphql/mutations/memberships/revoke.rb

@@ -4,6 +4,7 @@ module Mutations
   module Memberships
     class Revoke < BaseMutation
       include AuthenticableApiUser
+      include RequiredOrganization
 
       REQUIRED_PERMISSION = 'organization:members:update'
 
@@ -15,7 +16,8 @@ class Revoke < BaseMutation
       type Types::MembershipType
 
       def resolve(id:)
-        result = ::Memberships::RevokeService.new(context[:current_user]).call(id)
+        membership = current_organization.memberships.find_by(id: id)
+        result = ::Memberships::RevokeService.call(user: context[:current_user], membership:)
 
         result.success? ? result.membership : result_error(result)
       end

app/services/memberships/revoke_service.rb

@@ -2,16 +2,26 @@
 
 module Memberships
   class RevokeService < BaseService
-    def call(id)
-      membership = Membership.find_by(id:)
+    def initialize(user:, membership:)
+      @user = user
+      @membership = membership
+
+      super
+    end
+
+    def call
       return result.not_found_failure!(resource: 'membership') unless membership
-      return result.not_allowed_failure!(code: 'cannot_revoke_own_membership') if result.user.id == membership.user.id
+      return result.not_allowed_failure!(code: 'cannot_revoke_own_membership') if user.id == membership.user.id
       return result.not_allowed_failure!(code: 'last_admin') if membership.organization.memberships.admin.count == 1 && membership.admin?
 
       membership.mark_as_revoked!
 
       result.membership = membership
       result
     end
+
+    private
+
+    attr_reader :user, :membership
   end
 end

spec/graphql/mutations/memberships/revoke_spec.rb

@@ -19,13 +19,15 @@
   end
 
   it_behaves_like 'requires current user'
+  it_behaves_like 'requires current organization'
   it_behaves_like 'requires permission', 'organization:members:update'
 
   it 'Revokes a membership' do
     user = create(:user)
-    create(:membership, organization: organization, role: :admin)
+    create(:membership, organization:, role: :admin, user:)
 
     result = execute_graphql(
+      current_organization: organization,
       current_user: user,
       permissions: required_permission,
       query: mutation,
@@ -42,6 +44,7 @@
 
   it 'Cannot Revoke my own membership' do
     result = execute_graphql(
+      current_organization: organization,
       current_user: membership.user,
       permissions: required_permission,
       query: mutation,
@@ -63,6 +66,7 @@
     other_user = create(:membership, organization: organization, role: :finance)
 
     result = execute_graphql(
+      current_organization: organization,
       current_user: other_user.user,
       permissions: required_permission,
       query: mutation,

spec/services/memberships/revoke_service_spec.rb

@@ -3,50 +3,59 @@
 require 'rails_helper'
 
 RSpec.describe Memberships::RevokeService, type: :service do
-  subject(:revoke_service) { described_class.new(membership.user) }
+  subject(:revoke_service) { described_class.new(user:, membership:) }
 
-  let(:membership) { create(:membership) }
+  let(:organization) { create(:organization) }
+
+  let(:user) { create(:user) }
+  let(:membership) { create(:membership, organization:) }
+  let(:other_membership) { create(:membership, user:, organization:, role: :admin) }
+
+  before { other_membership }
 
   describe '#call' do
     context 'when revoking my own membership' do
+      let(:membership) { create(:membership, user:, organization:) }
+      let(:other_membership) { create(:membership, organization:, role: :admin) }
+
       it 'returns an error' do
-        result = revoke_service.call(membership.id)
+        result = revoke_service.call
 
         expect(result).not_to be_success
         expect(result.error.code).to eq('cannot_revoke_own_membership')
       end
     end
 
     context 'when membership is not found' do
+      let(:membership) { nil }
+
       it 'returns an error' do
-        result = revoke_service.call(nil)
+        result = revoke_service.call
 
         expect(result).not_to be_success
         expect(result.error.error_code).to eq('membership_not_found')
       end
     end
 
     context 'when revoking another membership' do
-      let(:another_membership) { create(:membership, organization: membership.organization) }
-
       it 'revokes the membership' do
         freeze_time do
-          result = revoke_service.call(another_membership.id)
+          result = revoke_service.call
 
           expect(result).to be_success
-          expect(result.membership.id).to eq(another_membership.id)
+          expect(result.membership.id).to eq(membership.id)
           expect(result.membership.status).to eq('revoked')
           expect(result.membership.revoked_at).to eq(Time.current)
         end
       end
     end
 
     context 'when removing the last admin' do
-      let(:membership) { create(:membership, role: :finance) }
-      let(:admin_membership) { create(:membership, organization: membership.organization, role: :admin) }
+      let(:membership) { create(:membership, organization:, role: :admin) }
+      let(:other_membership) { create(:membership, user:, organization:, role: :finance) }
 
       it 'returns an error' do
-        result = revoke_service.call(admin_membership.id)
+        result = revoke_service.call
 
         expect(result).not_to be_success
         expect(result.error.code).to eq('last_admin')