From eff560cfb9a337623d25b912d9bb233fae25fbf1 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 27 Oct 2021 10:15:59 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-4-stable-ee --- app/models/concerns/resolvable_discussion.rb | 3 +- app/models/project_group_link.rb | 1 + app/policies/issuable_policy.rb | 6 +++ app/services/groups/transfer_service.rb | 8 ++++ config/gitlab.yml.example | 6 ++- config/initializers/1_settings.rb | 3 +- db/fixtures/production/002_admin.rb | 2 +- .../mutations/discussions/toggle_resolve_spec.rb | 22 ++++++++- spec/models/concerns/resolvable_discussion_spec.rb | 20 +++++++- spec/policies/issuable_policy_spec.rb | 14 +++++- spec/services/groups/transfer_service_spec.rb | 55 +++++++++++++++++++++- 11 files changed, 127 insertions(+), 13 deletions(-) diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 3e1e5faee54..60e1dde17b9 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -81,8 +81,7 @@ module ResolvableDiscussion return false unless current_user return false unless resolvable? - current_user == self.noteable.try(:author) || - current_user.can?(:resolve_note, self.project) + current_user.can?(:resolve_note, self.noteable) end def resolve!(current_user) diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb index d704f4c2c87..8394ebe1df4 100644 --- a/app/models/project_group_link.rb +++ b/app/models/project_group_link.rb @@ -15,6 +15,7 @@ class ProjectGroupLink < ApplicationRecord validate :different_group scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) } + scope :in_group, -> (group_ids) { where(group_id: group_ids) } alias_method :shared_with_group, :group diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 61263e47d7c..39ce26526e6 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -11,6 +11,8 @@ class IssuablePolicy < BasePolicy @user && @subject.assignee_or_author?(@user) end + condition(:is_author) { @subject&.author == @user } + rule { can?(:guest_access) & assignee_or_author }.policy do enable :read_issue enable :update_issue @@ -20,6 +22,10 @@ class IssuablePolicy < BasePolicy enable :reopen_merge_request end + rule { is_author }.policy do + enable :resolve_note + end + rule { locked & ~is_project_member }.policy do prevent :create_note prevent :admin_note diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index 774f81b0a5e..334083a859f 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -182,6 +182,14 @@ module Groups # schedule refreshing projects for all the members of the group @group.refresh_members_authorized_projects + + # When a group is transferred, it also affects who gets access to the projects shared to + # the subgroups within its hierarchy, so we also schedule jobs that refresh authorizations for all such shared projects. + project_group_shares_within_the_hierarchy = ProjectGroupLink.in_group(group.self_and_descendants.select(:id)) + + project_group_shares_within_the_hierarchy.find_each do |project_group_link| + AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project_group_link.project_id) + end end def raise_transfer_error(message) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 3d2acce9a69..bb69c215f8d 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -176,8 +176,10 @@ production: &base ## Application settings cache expiry in seconds (default: 60) # application_settings_cache_seconds: 60 - ## Print initial root password to stdout during initialization (default: true) - # display_initial_root_password: true + ## Print initial root password to stdout during initialization (default: false) + # WARNING: setting this to true means that the root password will be printed in + # plaintext. This can be a security risk. + # display_initial_root_password: false ## Reply by email # Allow users to comment on issues and merge requests by replying to notification emails. diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 0e4e6f5cc84..d6957491b16 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -218,8 +218,7 @@ Settings.gitlab['no_todos_messages'] ||= YAML.load_file(Rails.root.join('config' Settings.gitlab['impersonation_enabled'] ||= true if Settings.gitlab['impersonation_enabled'].nil? Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil? Settings.gitlab['max_request_duration_seconds'] ||= 57 - -Settings.gitlab['display_initial_root_password'] = true if Settings.gitlab['display_initial_root_password'].nil? +Settings.gitlab['display_initial_root_password'] = false if Settings.gitlab['display_initial_root_password'].nil? Gitlab.ee do Settings.gitlab['mirror_max_delay'] ||= 300 diff --git a/db/fixtures/production/002_admin.rb b/db/fixtures/production/002_admin.rb index b6a6da3a188..b4710bc3e97 100644 --- a/db/fixtures/production/002_admin.rb +++ b/db/fixtures/production/002_admin.rb @@ -26,7 +26,7 @@ if user.persisted? if ::Settings.gitlab['display_initial_root_password'] puts "password: #{user_args[:password]}".color(:green) else - puts "password: *** - You opted not to display initial root password to STDOUT." + puts "password: ******".color(:green) end else puts "password: You'll be prompted to create one on your first visit.".color(:green) diff --git a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb index b03c6cb094f..8c11279a80a 100644 --- a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb +++ b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Mutations::Discussions::ToggleResolve do described_class.new(object: nil, context: { current_user: user }, field: nil) end + let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } describe '#resolve' do @@ -136,20 +137,37 @@ RSpec.describe Mutations::Discussions::ToggleResolve do end end end + + context 'when user is the author and discussion is locked' do + let(:user) { author } + + before do + issuable.update!(discussion_locked: true) + end + + it 'raises an error' do + expect { mutation.resolve(id: id_arg, resolve: resolve_arg) }.to raise_error( + Gitlab::Graphql::Errors::ResourceNotAvailable, + "The resource that you are attempting to access does not exist or you don't have permission to perform this action" + ) + end + end end context 'when discussion is on a merge request' do - let_it_be(:noteable) { create(:merge_request, source_project: project) } + let_it_be(:noteable) { create(:merge_request, source_project: project, author: author) } let(:discussion) { create(:diff_note_on_merge_request, noteable: noteable, project: project).to_discussion } + let(:issuable) { noteable } it_behaves_like 'a working resolve method' end context 'when discussion is on a design' do - let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project)) } + let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project, author: author)) } let(:discussion) { create(:diff_note_on_design, noteable: noteable, project: project).to_discussion } + let(:issuable) { noteable.issue } it_behaves_like 'a working resolve method' end diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb index c0e5ddc23b1..fc154738f11 100644 --- a/spec/models/concerns/resolvable_discussion_spec.rb +++ b/spec/models/concerns/resolvable_discussion_spec.rb @@ -188,6 +188,16 @@ RSpec.describe Discussion, ResolvableDiscussion do it "returns true" do expect(subject.can_resolve?(current_user)).to be true end + + context 'when noteable is locked' do + before do + allow(subject.noteable).to receive(:discussion_locked?).and_return(true) + end + + it 'returns false' do + expect(subject.can_resolve?(current_user)).to be_falsey + end + end end context "when the signed in user can push to the project" do @@ -200,8 +210,11 @@ RSpec.describe Discussion, ResolvableDiscussion do end context "when the noteable has no author" do + before do + subject.noteable.author = nil + end + it "returns true" do - expect(noteable).to receive(:author).and_return(nil) expect(subject.can_resolve?(current_user)).to be true end end @@ -213,8 +226,11 @@ RSpec.describe Discussion, ResolvableDiscussion do end context "when the noteable has no author" do + before do + subject.noteable.author = nil + end + it "returns false" do - expect(noteable).to receive(:author).and_return(nil) expect(subject.can_resolve?(current_user)).to be false end end diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index 86b04ccda57..eeb298e853e 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -13,6 +13,10 @@ RSpec.describe IssuablePolicy, models: true do let(:merge_request) { create(:merge_request, source_project: project, author: user) } let(:policies) { described_class.new(user, merge_request) } + it 'allows resolving notes' do + expect(policies).to be_allowed(:resolve_note) + end + context 'when user is able to read project' do it 'enables user to read and update issuables' do expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request) @@ -43,16 +47,24 @@ RSpec.describe IssuablePolicy, models: true do it 'can not create a note nor award emojis' do expect(policies).to be_disallowed(:create_note, :award_emoji) end + + it 'does not allow resolving note' do + expect(policies).to be_disallowed(:resolve_note) + end end context 'when the user is a project member' do before do - project.add_guest(user) + project.add_developer(user) end it 'can create a note and award emojis' do expect(policies).to be_allowed(:create_note, :award_emoji) end + + it 'allows resolving notes' do + expect(policies).to be_allowed(:resolve_note) + end end end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index ee38c0fbb44..8b506d2bc2c 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::TransferService do +RSpec.describe Groups::TransferService, :sidekiq_inline do shared_examples 'project namespace path is in sync with project path' do it 'keeps project and project namespace attributes in sync' do projects_with_project_namespace.each do |project| @@ -653,6 +653,59 @@ RSpec.describe Groups::TransferService do transfer_service.execute(new_parent_group) end end + + context 'transferring groups with shared_projects' do + let_it_be_with_reload(:shared_project) { create(:project, :public) } + + shared_examples_for 'drops the authorizations of ancestor members from the old hierarchy' do + it 'drops the authorizations of ancestor members from the old hierarchy' do + expect { transfer_service.execute(new_parent_group) }.to change { + ProjectAuthorization.where(project: shared_project, user: old_group_member).size + }.from(1).to(0) + end + end + + context 'when the group that has existing project share is transferred' do + before do + create(:project_group_link, :maintainer, project: shared_project, group: group) + end + + it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy' + end + + context 'when the group whose subgroup has an existing project share is transferred' do + let_it_be_with_reload(:subgroup) { create(:group, :private, parent: group) } + + before do + create(:project_group_link, :maintainer, project: shared_project, group: subgroup) + end + + it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy' + end + end + + context 'when a group that has existing group share is transferred' do + let(:shared_with_group) { group } + + let_it_be(:member_of_shared_with_group) { create(:user) } + let_it_be(:shared_group) { create(:group, :private) } + let_it_be(:project_in_shared_group) { create(:project, namespace: shared_group) } + + before do + shared_with_group.add_developer(member_of_shared_with_group) + create(:group_group_link, :maintainer, shared_group: shared_group, shared_with_group: shared_with_group) + shared_with_group.refresh_members_authorized_projects + end + + it 'retains the authorizations of direct members' do + expect { transfer_service.execute(new_parent_group) }.not_to change { + ProjectAuthorization.where( + project: project_in_shared_group, + user: member_of_shared_with_group, + access_level: Gitlab::Access::DEVELOPER).size + }.from(1) + end + end end end -- cgit v1.2.3