From c0b4a99fc2b5c499683740c4259e72bb227d5964 Mon Sep 17 00:00:00 2001 From: Vincent Pochet Date: Fri, 4 Oct 2024 09:58:02 +0200 Subject: [PATCH] fix(membership): Ensure membership is in current organization when revoking --- app/graphql/mutations/memberships/revoke.rb | 4 ++- app/services/memberships/revoke_service.rb | 16 ++++++++-- .../mutations/memberships/revoke_spec.rb | 6 +++- .../memberships/revoke_service_spec.rb | 31 ++++++++++++------- 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/app/graphql/mutations/memberships/revoke.rb b/app/graphql/mutations/memberships/revoke.rb index dc378a922df..46e7cc68b34 100644 --- a/app/graphql/mutations/memberships/revoke.rb +++ b/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 diff --git a/app/services/memberships/revoke_service.rb b/app/services/memberships/revoke_service.rb index 9ca05778523..bd483b7b258 100644 --- a/app/services/memberships/revoke_service.rb +++ b/app/services/memberships/revoke_service.rb @@ -2,10 +2,16 @@ 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! @@ -13,5 +19,9 @@ def call(id) result.membership = membership result end + + private + + attr_reader :user, :membership end end diff --git a/spec/graphql/mutations/memberships/revoke_spec.rb b/spec/graphql/mutations/memberships/revoke_spec.rb index d6d0f04fec2..c56508daa13 100644 --- a/spec/graphql/mutations/memberships/revoke_spec.rb +++ b/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, diff --git a/spec/services/memberships/revoke_service_spec.rb b/spec/services/memberships/revoke_service_spec.rb index 4d9271bf4c8..ce65a5af77c 100644 --- a/spec/services/memberships/revoke_service_spec.rb +++ b/spec/services/memberships/revoke_service_spec.rb @@ -3,14 +3,23 @@ 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') @@ -18,8 +27,10 @@ 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') @@ -27,14 +38,12 @@ 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 @@ -42,11 +51,11 @@ 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')