From 2f8d8dca22eca9937cee14765ceadc2aee266616 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 30 Sep 2020 23:32:30 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee --- .../metrics/dashboard/annotations/delete_spec.rb | 18 +- spec/services/members/update_service_spec.rb | 32 +- .../todos/destroy/entity_leave_service_spec.rb | 328 ++++++++++++--------- 3 files changed, 220 insertions(+), 158 deletions(-) (limited to 'spec') diff --git a/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb b/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb index 7357f3e1e35..9a612c841a2 100644 --- a/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb +++ b/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb @@ -9,13 +9,9 @@ RSpec.describe Mutations::Metrics::Dashboard::Annotations::Delete do let_it_be(:project) { create(:project, :private, :repository) } let_it_be(:environment) { create(:environment, project: project) } let_it_be(:annotation) { create(:metrics_dashboard_annotation, environment: environment) } - let(:mutation) do - variables = { - id: GitlabSchema.id_from_object(annotation).to_s - } - graphql_mutation(:delete_annotation, variables) - end + let(:variables) { { id: GitlabSchema.id_from_object(annotation).to_s } } + let(:mutation) { graphql_mutation(:delete_annotation, variables) } def mutation_response graphql_mutation_response(:delete_annotation) @@ -37,15 +33,11 @@ RSpec.describe Mutations::Metrics::Dashboard::Annotations::Delete do end context 'with invalid params' do - let(:mutation) do - variables = { - id: 'invalid_id' - } + let(:variables) { { id: GitlabSchema.id_from_object(project).to_s } } - graphql_mutation(:delete_annotation, variables) + it_behaves_like 'a mutation that returns top-level errors' do + let(:match_errors) { eq(["#{variables[:id]} is not a valid ID for #{annotation.class}."]) } end - - it_behaves_like 'a mutation that returns top-level errors', errors: ['invalid_id is not a valid GitLab ID.'] end context 'when the delete fails' do diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index f510916558b..2d4457f3f62 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -31,17 +31,35 @@ RSpec.describe Members::UpdateService do end context 'when member is downgraded to guest' do - let(:params) do - { access_level: Gitlab::Access::GUEST } + shared_examples 'schedules to delete confidential todos' do + it do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end + + context 'with Gitlab::Access::GUEST level as a string' do + let(:params) { { access_level: Gitlab::Access::GUEST.to_s } } + + it_behaves_like 'schedules to delete confidential todos' end - it 'schedules to delete confidential todos' do - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + context 'with Gitlab::Access::GUEST level as an integer' do + let(:params) { { access_level: Gitlab::Access::GUEST } } + + it_behaves_like 'schedules to delete confidential todos' + end + end - updated_member = described_class.new(current_user, params).execute(member, permission: permission) + context 'when access_level is invalid' do + let(:params) { { access_level: 'invalid' } } - expect(updated_member).to be_valid - expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + it 'raises an error' do + expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"') end end end diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index ccafe3bb7a8..921037bd5db 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -3,77 +3,145 @@ require 'spec_helper' RSpec.describe Todos::Destroy::EntityLeaveService do + let_it_be(:user, reload: true) { create(:user) } + let_it_be(:user2, reload: true) { create(:user) } + let(:group) { create(:group, :private) } - let(:project) { create(:project, group: group) } - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:issue) { create(:issue, project: project, confidential: true) } - let(:mr) { create(:merge_request, source_project: project) } - - 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) } + let(:project) { create(:project, :private, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } + let!(:todo_group_user) { create(:todo, user: user, group: group) } + let!(:todo_group_user2) { create(:todo, user: user2, group: group) } + + let(:mr) { create(:merge_request, source_project: project) } + 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_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) } + let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) } + + shared_examples 'using different access permissions' do |access_table| + using RSpec::Parameterized::TableSyntax + + where(:group_access, :project_access, :c_todos, :mr_todos, :method, &access_table) + + with_them do + before do + set_access(project, user, project_access) if project_access + set_access(group, user, group_access) if group_access + end + + it "#{params[:method].to_s.humanize(capitalize: false)}" do + send(method) + end + end + end + + shared_examples 'does not remove any todos' do + it { does_not_remove_any_todos } + end + + shared_examples 'removes only confidential issues todos' do + it { removes_only_confidential_issues_todos } + end + + def does_not_remove_any_todos + expect { subject }.not_to change { Todo.count } + end + + def removes_only_confidential_issues_todos + expect { subject }.to change { Todo.count }.from(6).to(5) + end + + def removes_confidential_issues_and_merge_request_todos + expect { subject }.to change { Todo.count }.from(6).to(4) + expect(user.todos).to match_array([todo_issue_user, todo_group_user]) + end + + def set_access(object, user, access_name) + case access_name + when :developer + object.add_developer(user) + when :reporter + object.add_reporter(user) + when :guest + object.add_guest(user) + end + end describe '#execute' do - context 'when a user leaves a project' do + describe 'updating a Project' do subject { described_class.new(user.id, project.id, 'Project').execute } + # a private project in a private group is valid context 'when project is private' do - it 'removes project todos for the provided user' do - expect { subject }.to change { Todo.count }.from(5).to(3) - - expect(user.todos).to match_array([todo_group_user]) - expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) - end + context 'when user is not a member of the project' do + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(6).to(3) - context 'when the user is member of the project' do - before do - project.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) end end - context 'when the user is a project guest' do - before do - project.add_guest(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end + it_behaves_like 'using different access permissions', PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE end + end - context 'when the user is member of a parent group' do - before do - group.add_developer(user) - end + # a private project in an internal/public group is valid + context 'when project is private in an internal/public group' do + let(:group) { create(:group, :internal) } + + context 'when user is not a member of the project' do + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(6).to(3) - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) end end - context 'when the user is guest of a parent group' do - before do - project.add_guest(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end + it_behaves_like 'using different access permissions', PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE end end + # an internal project in an internal/public group is valid context 'when project is not private' do - before do - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - end + let(:group) { create(:group, :internal) } + let(:project) { create(:project, :internal, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } it 'enqueues the PrivateFeaturesWorker' do expect(TodosDestroyer::PrivateFeaturesWorker) @@ -84,50 +152,42 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'confidential issues' do context 'when a user is not an author of confidential issue' do - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end + it_behaves_like 'removes only confidential issues todos' end context 'when a user is an author of confidential issue' do before do - issue.update!(author: user) + issue_c.update!(author: user) end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end context 'when a user is an assignee of confidential issue' do before do - issue.assignees << user + issue_c.assignees << user end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end - context 'when a user is a project guest' do - before do - project.add_guest(user) - end - - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when a user is a project guest but group developer' do - before do - project.add_guest(user) - group.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration + + it_behaves_like 'using different access permissions', INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE end end @@ -138,42 +198,43 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'removes only users issue todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) + expect { subject }.to change { Todo.count }.from(6).to(5) end end end end end - context 'when a user leaves a group' do + describe 'updating a Group' do subject { described_class.new(user.id, group.id, 'Group').execute } context 'when group is private' do - 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, todo_group_user2]) - end - - context 'when the user is member of the group' do - before do - group.add_developer(user) - end + context 'when a user leaves a group' do + it 'removes group and subproject todos for the user' do + expect { subject }.to change { Todo.count }.from(6).to(2) - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + expect(user.todos).to be_empty + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) end end - context 'when the user is member of the group project but not the group' do - before do - project.add_developer(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'using different access permissions', PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE end context 'with nested groups' do @@ -191,12 +252,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when the user is not a member of any groups/projects' do it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(11).to(4) + expect { subject }.to change { Todo.count }.from(12).to(4) expect(user.todos).to be_empty expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -208,9 +269,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do parent_group.add_developer(user) end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end context 'when the user is member of a subgroup' do @@ -219,12 +278,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove group and subproject todos' do - expect { subject }.to change { Todo.count }.from(11).to(7) + expect { subject }.to change { Todo.count }.from(12).to(7) expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user]) expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -235,12 +294,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove subproject and group todos' do - expect { subject }.to change { Todo.count }.from(11).to(7) + expect { subject }.to change { Todo.count }.from(12).to(7) expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user]) expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -248,10 +307,10 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'when group is not private' do - before do - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - end + let(:group) { create(:group, :internal) } + let(:project) { create(:project, :internal, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } it 'enqueues the PrivateFeaturesWorker' do expect(TodosDestroyer::PrivateFeaturesWorker) @@ -260,31 +319,24 @@ RSpec.describe Todos::Destroy::EntityLeaveService do subject end - context 'when user is not member' do - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when user is a project guest' do - before do - project.add_guest(user) - end - - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when user is a project guest & group developer' do - before do - project.add_guest(user) - group.add_developer(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'using different access permissions', INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE end end end -- cgit v1.2.3