diff options
author | Jarka Kadlecová <jarka@gitlab.com> | 2018-08-02 17:16:58 +0300 |
---|---|---|
committer | Jarka Kadlecová <jarka@gitlab.com> | 2018-08-02 17:16:58 +0300 |
commit | 4d4b8f8bbedbfadb49e12df2b123b3528cda4c08 (patch) | |
tree | e4701b090c540811e41c79ab9fd5c2e378c584a3 | |
parent | e60ec75303475083746e2d09d2a99cc5c6ea0221 (diff) |
Remove group todos when a users looses access
-rw-r--r-- | app/services/groups/update_service.rb | 11 | ||||
-rw-r--r-- | app/services/todos/destroy/entity_leave_service.rb | 13 | ||||
-rw-r--r-- | app/services/todos/destroy/group_private_service.rb | 30 | ||||
-rw-r--r-- | app/workers/todos_destroyer/group_private_worker.rb | 10 | ||||
-rw-r--r-- | spec/services/groups/update_service_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/todos/destroy/entity_leave_service_spec.rb | 32 | ||||
-rw-r--r-- | spec/services/todos/destroy/group_private_service_spec.rb | 38 | ||||
-rw-r--r-- | spec/workers/todos_destroyer/group_private_worker_spec.rb | 12 |
8 files changed, 156 insertions, 18 deletions
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 436a6b18cb1..fe47aa2f140 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -14,7 +14,9 @@ module Groups group.assign_attributes(params) begin - group.save + after_update if group.save + + true rescue Gitlab::UpdatePathError => e group.errors.add(:base, e.message) @@ -24,6 +26,13 @@ module Groups private + def after_update + if group.previous_changes.include?(:visibility_level) && group.private? + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + end + end + def reject_parent_id! params.except!(:parent_id) end diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 2ff9f94b718..b1c4eb95e87 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -19,7 +19,7 @@ module Todos override :todos def todos if entity.private? - Todo.where(project_id: project_ids, user_id: user_id) + Todo.where('(project_id IN (?) OR group_id IN (?)) AND user_id = ?', project_ids, group_ids, user_id) else project_ids.each do |project_id| TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id) @@ -37,7 +37,16 @@ module Todos when Project [entity.id] when Namespace - Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id)) + Project.select(:id).where(namespace_id: group_ids) + end + end + + def group_ids + case entity + when Project + [] + when Namespace + entity.self_and_descendants.select(:id) end end diff --git a/app/services/todos/destroy/group_private_service.rb b/app/services/todos/destroy/group_private_service.rb new file mode 100644 index 00000000000..1b00bbe91e1 --- /dev/null +++ b/app/services/todos/destroy/group_private_service.rb @@ -0,0 +1,30 @@ +module Todos + module Destroy + class GroupPrivateService < ::Todos::Destroy::BaseService + extend ::Gitlab::Utils::Override + + attr_reader :group + + def initialize(group_id) + @group = Group.find_by(id: group_id) + end + + private + + override :todos + def todos + Todo.where(group_id: group.id) + end + + override :authorized_users + def authorized_users + GroupMember.select(:user_id).where(source: group.id) + end + + override :todos_to_remove? + def todos_to_remove? + group&.private? + end + end + end +end diff --git a/app/workers/todos_destroyer/group_private_worker.rb b/app/workers/todos_destroyer/group_private_worker.rb new file mode 100644 index 00000000000..3e47eec7461 --- /dev/null +++ b/app/workers/todos_destroyer/group_private_worker.rb @@ -0,0 +1,10 @@ +module TodosDestroyer + class GroupPrivateWorker + include ApplicationWorker + include TodosDestroyerQueue + + def perform(group_id) + ::Todos::Destroy::GroupPrivateService.new(group_id).execute + end + end +end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 48d689e11d4..7c5c7409cc1 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -12,13 +12,17 @@ describe Groups::UpdateService do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } before do - public_group.add_user(user, Gitlab::Access::MAINTAINER) + public_group.add_user(user, Gitlab::Access::OWNER) create(:project, :public, group: public_group) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end it "does not change permission level" do service.execute expect(public_group.errors.count).to eq(1) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end end @@ -26,8 +30,10 @@ describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } before do - internal_group.add_user(user, Gitlab::Access::MAINTAINER) + internal_group.add_user(user, Gitlab::Access::OWNER) create(:project, :internal, group: internal_group) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end it "does not change permission level" do @@ -35,6 +41,24 @@ describe Groups::UpdateService do expect(internal_group.errors.count).to eq(1) end end + + context "internal group with private project" do + let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } + + before do + internal_group.add_user(user, Gitlab::Access::OWNER) + create(:project, :private, group: internal_group) + + expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) + .with(1.hour, internal_group.id) + end + + it "changes permission level to private" do + service.execute + expect(internal_group.visibility_level) + .to eq(Gitlab::VisibilityLevel::PRIVATE) + end + end end context "with parent_id user doesn't have permissions for" do diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index bad408a314e..c554db5e024 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -10,18 +10,20 @@ describe Todos::Destroy::EntityLeaveService do let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) } let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) } + let!(:todo_group_user) { create(:todo, user: user, group: group) } let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) } + let!(:todo_group_user2) { create(:todo, user: user2, group: group) } describe '#execute' do context 'when a user leaves a project' do subject { described_class.new(user.id, project.id, 'Project').execute } context 'when project is private' do - it 'removes todos for the provided user' do - expect { subject }.to change { Todo.count }.from(3).to(1) + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(5).to(3) - expect(user.todos).to be_empty - expect(user2.todos).to match_array([todo_issue_user2]) + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) end end @@ -37,7 +39,7 @@ describe Todos::Destroy::EntityLeaveService do end it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + expect { subject }.to change { Todo.count }.from(5).to(4) end end @@ -69,7 +71,7 @@ describe Todos::Destroy::EntityLeaveService do end it 'removes only users issue todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + expect { subject }.to change { Todo.count }.from(5).to(4) end end end @@ -80,26 +82,30 @@ describe Todos::Destroy::EntityLeaveService do subject { described_class.new(user.id, group.id, 'Group').execute } context 'when group is private' do - it 'removes todos for the user' do - expect { subject }.to change { Todo.count }.from(3).to(1) + it 'removes group and subproject todos for the user' do + expect { subject }.to change { Todo.count }.from(5).to(2) expect(user.todos).to be_empty - expect(user2.todos).to match_array([todo_issue_user2]) + expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) end context 'with nested groups', :nested_groups do let(:subgroup) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } - let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) } + let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) } + let!(:todo_subgroup_user) { create(:todo, user: user, group: subgroup) } let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } + let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(5).to(2) + expect { subject }.to change { Todo.count }.from(9).to(4) expect(user.todos).to be_empty expect(user2.todos) - .to match_array([todo_issue_user2, todo_subproject_user2]) + .to match_array( + [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + ) end end end @@ -113,7 +119,7 @@ describe Todos::Destroy::EntityLeaveService do end it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + expect { subject }.to change { Todo.count }.from(5).to(4) end end end diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb new file mode 100644 index 00000000000..48e5a5a91eb --- /dev/null +++ b/spec/services/todos/destroy/group_private_service_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe Todos::Destroy::GroupPrivateService do + let(:group) { create(:group, :public) } + let(:user) { create(:user) } + let(:group_member) { create(:user) } + + let!(:todo_non_member) { create(:todo, user: user, group: group) } + let!(:todo_member) { create(:todo, user: group_member, group: group) } + let!(:todo_another_non_member) { create(:todo, user: user, group: group) } + + describe '#execute' do + before do + group.add_developer(group_member) + end + + subject { described_class.new(group.id).execute } + + context 'when a project set to private' do + before do + group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'removes todos for a user who is not a member' do + expect { subject }.to change { Todo.count }.from(3).to(1) + + expect(user.todos).to be_empty + expect(group_member.todos).to match_array([todo_member]) + end + end + + context 'when group is not private' do + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + end +end diff --git a/spec/workers/todos_destroyer/group_private_worker_spec.rb b/spec/workers/todos_destroyer/group_private_worker_spec.rb new file mode 100644 index 00000000000..fcc38989ced --- /dev/null +++ b/spec/workers/todos_destroyer/group_private_worker_spec.rb @@ -0,0 +1,12 @@ +require 'spec_helper' + +describe TodosDestroyer::GroupPrivateWorker do + it "calls the Todos::Destroy::GroupPrivateService with the params it was given" do + service = double + + expect(::Todos::Destroy::GroupPrivateService).to receive(:new).with(100).and_return(service) + expect(service).to receive(:execute) + + described_class.new.perform(100) + end +end |