Commit a35aef44 authored by GitLab Bot's avatar GitLab Bot
Browse files

Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee

parent f22240f86932
......@@ -9,7 +9,7 @@ class Base < BaseMutation
# This method is defined here in order to be used by `authorized_find!` in the subclasses.
def find_object(id:)
GitlabSchema.object_from_id(id)
GitlabSchema.object_from_id(id, expected_type: ::Metrics::Dashboard::Annotation)
end
end
end
......
......@@ -7,6 +7,11 @@ class BaseService < ::BaseService
def initialize(current_user = nil, params = {})
@current_user = current_user
@params = params
# could be a string, force to an integer, part of fix
# https://gitlab.com/gitlab-org/gitlab/-/issues/219496
# Allow the ArgumentError to be raised if it can't be converted to an integer.
@params[:access_level] = Integer(@params[:access_level]) if @params[:access_level]
end
def after_execute(args)
......
......@@ -52,7 +52,14 @@ def remove_confidential_issue_todos
# rubocop: disable CodeReuse/ActiveRecord
def remove_project_todos
Todo.where(project_id: non_authorized_projects, user_id: user.id).delete_all
# Issues are viewable by guests (even in private projects), so remove those todos
# from projects without guest access
Todo.where(project_id: non_authorized_guest_projects, user_id: user.id)
.delete_all
# MRs require reporter access, so remove those todos that are not authorized
Todo.where(project_id: non_authorized_reporter_projects, target_type: MergeRequest.name, user_id: user.id)
.delete_all
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -68,7 +75,7 @@ def projects
when Project
{ id: entity.id }
when Namespace
{ namespace_id: non_member_groups }
{ namespace_id: non_authorized_reporter_groups }
end
Project.where(condition)
......@@ -76,8 +83,32 @@ def projects
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def non_authorized_projects
projects.where('id NOT IN (?)', user.authorized_projects.select(:id))
def authorized_reporter_projects
user.authorized_projects(Gitlab::Access::REPORTER).select(:id)
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def authorized_guest_projects
user.authorized_projects(Gitlab::Access::GUEST).select(:id)
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def non_authorized_reporter_projects
projects.where('id NOT IN (?)', authorized_reporter_projects)
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def non_authorized_guest_projects
projects.where('id NOT IN (?)', authorized_guest_projects)
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def authorized_reporter_groups
GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id)
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -91,9 +122,9 @@ def non_authorized_groups
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def non_member_groups
def non_authorized_reporter_groups
entity.self_and_descendants.select(:id)
.where('id NOT IN (?)', user.membership_groups.select(:id))
.where('id NOT IN (?)', authorized_reporter_groups)
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -106,8 +137,6 @@ def user_has_reporter_access?
# rubocop: disable CodeReuse/ActiveRecord
def confidential_issues
assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user.id)
authorized_reporter_projects = user
.authorized_projects(Gitlab::Access::REPORTER).select(:id)
Issue.where(project_id: projects, confidential: true)
.where('project_id NOT IN(?)', authorized_reporter_projects)
......
---
title: Ensure global ID is of Annotation type in GraphQL destroy mutation
merge_request:
author:
type: security
---
title: Fix redaction of confidential Todos
merge_request:
author:
type: security
......@@ -9,13 +9,9 @@
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 @@ def mutation_response
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
......
......@@ -31,17 +31,35 @@
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
......
......@@ -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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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]
)