diff options
36 files changed, 773 insertions, 138 deletions
diff --git a/app/assets/javascripts/frequent_items/components/app.vue b/app/assets/javascripts/frequent_items/components/app.vue index 8cf939254c1..2ffecce0a56 100644 --- a/app/assets/javascripts/frequent_items/components/app.vue +++ b/app/assets/javascripts/frequent_items/components/app.vue @@ -5,7 +5,7 @@ import AccessorUtilities from '~/lib/utils/accessor'; import eventHub from '../event_hub'; import store from '../store/'; import { FREQUENT_ITEMS, STORAGE_KEY } from '../constants'; -import { isMobile, updateExistingFrequentItem } from '../utils'; +import { isMobile, updateExistingFrequentItem, sanitizeItem } from '../utils'; import FrequentItemsSearchInput from './frequent_items_search_input.vue'; import FrequentItemsList from './frequent_items_list.vue'; import frequentItemsMixin from './frequent_items_mixin'; @@ -64,7 +64,9 @@ export default { this.fetchFrequentItems(); } }, - logItemAccess(storageKey, item) { + logItemAccess(storageKey, unsanitizedItem) { + const item = sanitizeItem(unsanitizedItem); + if (!AccessorUtilities.isLocalStorageAccessSafe()) { return false; } diff --git a/app/assets/javascripts/frequent_items/components/frequent_items_list.vue b/app/assets/javascripts/frequent_items/components/frequent_items_list.vue index 67ffa97a046..0ece64692ae 100644 --- a/app/assets/javascripts/frequent_items/components/frequent_items_list.vue +++ b/app/assets/javascripts/frequent_items/components/frequent_items_list.vue @@ -1,6 +1,7 @@ <script> import FrequentItemsListItem from './frequent_items_list_item.vue'; import frequentItemsMixin from './frequent_items_mixin'; +import { sanitizeItem } from '../utils'; export default { components: { @@ -48,6 +49,9 @@ export default { ? this.translations.itemListErrorMessage : this.translations.itemListEmptyMessage; }, + sanitizedItems() { + return this.items.map(sanitizeItem); + }, }, }; </script> @@ -59,7 +63,7 @@ export default { {{ listEmptyMessage }} </li> <frequent-items-list-item - v-for="item in items" + v-for="item in sanitizedItems" v-else :key="item.id" :item-id="item.id" diff --git a/app/assets/javascripts/frequent_items/utils.js b/app/assets/javascripts/frequent_items/utils.js index cc1668b1a0d..5188d6118ac 100644 --- a/app/assets/javascripts/frequent_items/utils.js +++ b/app/assets/javascripts/frequent_items/utils.js @@ -1,5 +1,6 @@ import _ from 'underscore'; import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils'; +import sanitize from 'sanitize-html'; import { FREQUENT_ITEMS, HOUR_IN_MS } from './constants'; export const isMobile = () => ['md', 'sm', 'xs'].includes(bp.getBreakpointSize()); @@ -43,3 +44,9 @@ export const updateExistingFrequentItem = (frequentItem, item) => { lastAccessedOn: accessedOverHourAgo ? Date.now() : frequentItem.lastAccessedOn, }; }; + +export const sanitizeItem = item => ({ + ...item, + name: sanitize(item.name.toString(), { allowedTags: [] }), + namespace: sanitize(item.namespace.toString(), { allowedTags: [] }), +}); diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 1a97b39d3ae..1668cf004f8 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -19,7 +19,7 @@ class DashboardController < Dashboard::ApplicationController format.json do load_events - pager_json("events/_events", @events.count) + pager_json('events/_events', @events.count { |event| event.visible_to_user?(current_user) }) end end end @@ -37,6 +37,7 @@ class DashboardController < Dashboard::ApplicationController @events = EventCollection .new(projects, offset: params[:offset].to_i, filter: event_filter) .to_a + .map(&:present) Events::RenderService.new(current_user).execute(@events) end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 0953ca96317..958dc27984f 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -91,7 +91,7 @@ class GroupsController < Groups::ApplicationController format.json do load_events - pager_json("events/_events", @events.count) + pager_json("events/_events", @events.count { |event| event.visible_to_user?(current_user) }) end end end @@ -209,8 +209,9 @@ class GroupsController < Groups::ApplicationController .includes(:namespace) @events = EventCollection - .new(projects, offset: params[:offset].to_i, filter: event_filter, groups: groups) - .to_a + .new(projects, offset: params[:offset].to_i, filter: event_filter, groups: groups) + .to_a + .map(&:present) Events::RenderService .new(current_user) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index bf05defbc2e..9d0901881d3 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -118,7 +118,7 @@ class ProjectsController < Projects::ApplicationController format.html format.json do load_events - pager_json('events/_events', @events.count) + pager_json('events/_events', @events.count { |event| event.visible_to_user?(current_user) }) end end end @@ -343,6 +343,7 @@ class ProjectsController < Projects::ApplicationController @events = EventCollection .new(projects, offset: params[:offset].to_i, filter: event_filter) .to_a + .map(&:present) Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?) end diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb index 8a768b3a2c0..6c8bfc35334 100644 --- a/app/models/generic_commit_status.rb +++ b/app/models/generic_commit_status.rb @@ -1,11 +1,14 @@ # frozen_string_literal: true class GenericCommitStatus < CommitStatus + EXTERNAL_STAGE_IDX = 1_000_000 + before_validation :set_default_values validates :target_url, addressable_url: true, length: { maximum: 255 }, allow_nil: true + validate :name_uniqueness_across_types, unless: :importing? # GitHub compatible API alias_attribute :context, :name @@ -13,7 +16,7 @@ class GenericCommitStatus < CommitStatus def set_default_values self.context ||= 'default' self.stage ||= 'external' - self.stage_idx ||= 1000000 + self.stage_idx ||= EXTERNAL_STAGE_IDX end def tags @@ -25,4 +28,14 @@ class GenericCommitStatus < CommitStatus .new(self, current_user) .fabricate! end + + private + + def name_uniqueness_across_types + return if !pipeline || name.blank? + + if pipeline.statuses.by_name(name).where.not(type: type).exists? + errors.add(:name, :taken) + end + end end diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 3a16f7dc239..c93a19bdc3d 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -21,6 +21,14 @@ class BasePolicy < DeclarativePolicy::Base with_options scope: :user, score: 0 condition(:deactivated) { @user&.deactivated? } + desc "User email is unconfirmed or user account is locked" + with_options scope: :user, score: 0 + condition(:inactive) do + Feature.enabled?(:inactive_policy_condition, default_enabled: true) && + @user && + !@user&.active_for_authentication? + end + with_options scope: :user, score: 0 condition(:external_user) { @user.nil? || @user.external? } diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index f212bb06bc9..2187c703760 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -36,6 +36,13 @@ class GlobalPolicy < BasePolicy enable :use_slash_commands end + rule { inactive }.policy do + prevent :log_in + prevent :access_api + prevent :access_git + prevent :use_slash_commands + end + rule { blocked | internal }.policy do prevent :log_in prevent :access_api diff --git a/app/presenters/event_presenter.rb b/app/presenters/event_presenter.rb index f31d362d5fa..5657e0b96bc 100644 --- a/app/presenters/event_presenter.rb +++ b/app/presenters/event_presenter.rb @@ -3,6 +3,18 @@ class EventPresenter < Gitlab::View::Presenter::Delegated presents :event + def initialize(subject, **attributes) + super + + @visible_to_user_cache = ActiveSupport::Cache::MemoryStore.new + end + + # Caching `visible_to_user?` method in the presenter beause it might be called multiple times. + def visible_to_user?(user = nil) + @visible_to_user_cache.fetch(user&.id) { super(user) } + end + + # implement cache here def resource_parent_name resource_parent&.full_name || '' end diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 3f0aedfbfb2..569b91de73e 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -18,7 +18,7 @@ class CompareService return unless raw_compare && raw_compare.base && raw_compare.head Compare.new(raw_compare, - target_project, + start_project, base_sha: base_sha, straight: straight) end diff --git a/app/services/projects/group_links/destroy_service.rb b/app/services/projects/group_links/destroy_service.rb index c96dcaae8d5..ea7d05551fd 100644 --- a/app/services/projects/group_links/destroy_service.rb +++ b/app/services/projects/group_links/destroy_service.rb @@ -6,6 +6,12 @@ module Projects def execute(group_link) return false unless group_link + if group_link.project.private? + TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) + else + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, nil, project.id) + end + group_link.destroy end end diff --git a/changelogs/unreleased/security-35235-todos-cleanup.yml b/changelogs/unreleased/security-35235-todos-cleanup.yml new file mode 100644 index 00000000000..119220fbc73 --- /dev/null +++ b/changelogs/unreleased/security-35235-todos-cleanup.yml @@ -0,0 +1,5 @@ +--- +title: Cleanup todos for users from a removed linked group +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-commits-api-last-pipeline-status.yml b/changelogs/unreleased/security-commits-api-last-pipeline-status.yml new file mode 100644 index 00000000000..a68151f9732 --- /dev/null +++ b/changelogs/unreleased/security-commits-api-last-pipeline-status.yml @@ -0,0 +1,5 @@ +--- +title: Disable access to last_pipeline in commits API for users without read permissions +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-email-confirmation-bypass-via-api-ee.yml b/changelogs/unreleased/security-email-confirmation-bypass-via-api-ee.yml new file mode 100644 index 00000000000..8bd2b7a452f --- /dev/null +++ b/changelogs/unreleased/security-email-confirmation-bypass-via-api-ee.yml @@ -0,0 +1,5 @@ +--- +title: Prevent API access for unconfirmed users +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml b/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml new file mode 100644 index 00000000000..7d74d6108f8 --- /dev/null +++ b/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml @@ -0,0 +1,5 @@ +--- +title: Enforce permission check when counting activity events +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-xss-on-frequent-groups-dropdown.yml b/changelogs/unreleased/security-fix-xss-on-frequent-groups-dropdown.yml new file mode 100644 index 00000000000..9381efff0c8 --- /dev/null +++ b/changelogs/unreleased/security-fix-xss-on-frequent-groups-dropdown.yml @@ -0,0 +1,5 @@ +--- +title: Fix xss on frequent groups dropdown +merge_request: 50 +author: +type: security diff --git a/changelogs/unreleased/security-proctect-internal-builds-from-external-overrides.yml b/changelogs/unreleased/security-proctect-internal-builds-from-external-overrides.yml new file mode 100644 index 00000000000..b540172d95c --- /dev/null +++ b/changelogs/unreleased/security-proctect-internal-builds-from-external-overrides.yml @@ -0,0 +1,5 @@ +--- +title: Protect internal CI builds from external overrides +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-reverse-polarity-of-branch-compare.yml b/changelogs/unreleased/security-reverse-polarity-of-branch-compare.yml new file mode 100644 index 00000000000..db6a4f064a4 --- /dev/null +++ b/changelogs/unreleased/security-reverse-polarity-of-branch-compare.yml @@ -0,0 +1,5 @@ +--- +title: Make cross-repository comparisons happen in the source repository +merge_request: +author: +type: security diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 6e26ee309f0..569c4e04dc5 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -85,6 +85,8 @@ module API protected: user_project.protected_for?(ref)) end + authorize! :update_pipeline, pipeline + status = GenericCommitStatus.running_or_pending.find_or_initialize_by( project: user_project, pipeline: pipeline, diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 63a7fdfa3ab..9dcf9b015aa 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -154,7 +154,7 @@ module API not_found! 'Commit' unless commit - present commit, with: Entities::CommitDetail, stats: params[:stats] + present commit, with: Entities::CommitDetail, stats: params[:stats], current_user: current_user end desc 'Get the diff for a specific commit of a project' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index dfd0e676586..8c4d986eb34 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -491,8 +491,18 @@ module API class CommitDetail < Commit expose :stats, using: Entities::CommitStats, if: :stats expose :status - expose :last_pipeline, using: 'API::Entities::PipelineBasic' expose :project_id + + expose :last_pipeline do |commit, options| + pipeline = commit.last_pipeline if can_read_pipeline? + ::API::Entities::PipelineBasic.represent(pipeline, options) + end + + private + + def can_read_pipeline? + Ability.allowed?(options[:current_user], :read_pipeline, object.last_pipeline) + end end class CommitSignature < Grape::Entity diff --git a/lib/gitlab/git/cross_repo_comparer.rb b/lib/gitlab/git/cross_repo_comparer.rb new file mode 100644 index 00000000000..3958373f7cb --- /dev/null +++ b/lib/gitlab/git/cross_repo_comparer.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Gitlab + module Git + class CrossRepoComparer + attr_reader :source_repo, :target_repo + + def initialize(source_repo, target_repo) + @source_repo = source_repo + @target_repo = target_repo + end + + def compare(source_ref, target_ref, straight:) + ensuring_ref_in_source(target_ref) do |target_commit_id| + Gitlab::Git::Compare.new( + source_repo, + target_commit_id, + source_ref, + straight: straight + ) + end + end + + private + + def ensuring_ref_in_source(ref, &blk) + return yield ref if source_repo == target_repo + + # If the commit doesn't exist in the target, there's nothing we can do + commit_id = target_repo.commit(ref)&.sha + return unless commit_id + + # The commit pointed to by ref may exist in the source even when they + # are different repositories. This is particularly true of close forks, + # but may also be the case if a temporary ref for this comparison has + # already been created in the past, and the result hasn't been GC'd yet. + return yield commit_id if source_repo.commit(commit_id) + + # Worst case: the commit is not in the source repo so we need to fetch + # it. Use a temporary ref and clean up afterwards + with_commit_in_source_tmp(commit_id, &blk) + end + + # Fetch the ref into source_repo from target_repo, using a temporary ref + # name that will be deleted once the method completes. This is a no-op if + # fetching the source branch fails + def with_commit_in_source_tmp(commit_id, &blk) + tmp_ref = "refs/tmp/#{SecureRandom.hex}" + + yield commit_id if source_repo.fetch_source_branch!(target_repo, commit_id, tmp_ref) + ensure + source_repo.delete_refs(tmp_ref) # best-effort + end + end + end +end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index ed3e7a1e39c..0120e3be14c 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -746,29 +746,9 @@ module Gitlab end def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:) - reachable_ref = - if source_repository == self - source_branch_name - else - # If a tmp ref was created before for a separate repo comparison (forks), - # we're able to short-circuit the tmp ref re-creation: - # 1. Take the SHA from the source repo - # 2. Read that in the current "target" repo - # 3. If that SHA is still known (readable), it means GC hasn't - # cleaned it up yet, so we can use it instead re-writing the tmp ref. - source_commit_id = source_repository.commit(source_branch_name)&.sha - commit(source_commit_id)&.sha if source_commit_id - end - - return compare(target_branch_name, reachable_ref, straight: straight) if reachable_ref - - tmp_ref = "refs/tmp/#{SecureRandom.hex}" - - return unless fetch_source_branch!(source_repository, source_branch_name, tmp_ref) - - compare(target_branch_name, tmp_ref, straight: straight) - ensure - delete_refs(tmp_ref) if tmp_ref + CrossRepoComparer + .new(source_repository, self) + .compare(source_branch_name, target_branch_name, straight: straight) end def write_ref(ref_path, ref, old_ref: nil) @@ -1046,13 +1026,6 @@ module Gitlab private - def compare(base_ref, head_ref, straight:) - Gitlab::Git::Compare.new(self, - base_ref, - head_ref, - straight: straight) - end - def empty_diff_stats Gitlab::Git::DiffStatsCollection.new([]) end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index a733c3ecaa1..305419efe96 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -23,6 +23,47 @@ describe DashboardController do end end + describe "GET activity as JSON" do + render_views + + let(:user) { create(:user) } + let(:project) { create(:project, :public, issues_access_level: ProjectFeature::PRIVATE) } + + before do + create(:event, :created, project: project, target: create(:issue)) + + sign_in(user) + + request.cookies[:event_filter] = 'all' + end + + context 'when user has permission to see the event' do + before do + project.add_developer(user) + end + + it 'returns count' do + get :activity, params: { format: :json } + + expect(json_response['count']).to eq(1) + end + end + + context 'when user has no permission to see the event' do + it 'filters out invisible event' do + get :activity, params: { format: :json } + + expect(json_response['html']).to include(_('No activities found')) + end + + it 'filters out invisible event when calculating the count' do + get :activity, params: { format: :json } + + expect(json_response['count']).to eq(0) + end + end + end + it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 2ed2b319298..ddfd2b424e7 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -47,7 +47,7 @@ describe GroupsController do it 'assigns events for all the projects in the group', :sidekiq_might_not_need_inline do subject - expect(assigns(:events)).to contain_exactly(event) + expect(assigns(:events).map(&:id)).to contain_exactly(event.id) end end end @@ -119,12 +119,12 @@ describe GroupsController do describe 'GET #activity' do render_views - before do - sign_in(user) - project - end - context 'as json' do + before do + sign_in(user) + project + end + it 'includes events from all projects in group and subgroups', :sidekiq_might_not_need_inline do 2.times do project = create(:project, group: group) @@ -141,6 +141,31 @@ describe GroupsController do expect(assigns(:projects).limit_value).to be_nil end end + + context 'when user has no permission to see the event' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + + let(:project_with_restricted_access) do + create(:project, :public, issues_access_level: ProjectFeature::PRIVATE, group: group) + end + + before do + create(:event, project: project) + create(:event, :created, project: project_with_restricted_access, target: create(:issue)) + + group.add_guest(user) + + sign_in(user) + end + + it 'filters out invisible event' do + get :activity, params: { id: group.to_param }, format: :json + + expect(json_response['count']).to eq(1) + end + end end describe 'POST #create' do diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 9ae1277de26..3870ef5d947 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -64,6 +64,46 @@ describe ProjectsController do end end + describe "GET #activity as JSON" do + render_views + + let(:project) { create(:project, :public, issues_access_level: ProjectFeature::PRIVATE) } + + before do + create(:event, :created, project: project, target: create(:issue)) + + sign_in(user) + + request.cookies[:event_filter] = 'all' + end + + context 'when user has permission to see the event' do + before do + project.add_developer(user) + end + + it 'returns count' do + get :activity, params: { namespace_id: project.namespace, id: project, format: :json } + + expect(json_response['count']).to eq(1) + end + end + + context 'when user has no permission to see the event' do + it 'filters out invisible event' do + get :activity, params: { namespace_id: project.namespace, id: project, format: :json } + + expect(json_response['html']).to eq("\n") + end + + it 'filters out invisible event when calculating the count' do + get :activity, params: { namespace_id: project.namespace, id: project, format: :json } + + expect(json_response['count']).to eq(0) + end + end + end + describe "GET show" do context "user not project member" do before do diff --git a/spec/javascripts/frequent_items/utils_spec.js b/spec/javascripts/frequent_items/utils_spec.js index 2480af5b31d..fd5bd002428 100644 --- a/spec/javascripts/frequent_items/utils_spec.js +++ b/spec/javascripts/frequent_items/utils_spec.js @@ -1,5 +1,10 @@ import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils'; -import { isMobile, getTopFrequentItems, updateExistingFrequentItem } from '~/frequent_items/utils'; +import { + isMobile, + getTopFrequentItems, + updateExistingFrequentItem, + sanitizeItem, +} from '~/frequent_items/utils'; import { HOUR_IN_MS, FREQUENT_ITEMS } from '~/frequent_items/constants'; import { mockProject, unsortedFrequentItems, sortedFrequentItems } from './mock_data'; @@ -92,4 +97,16 @@ describe('Frequent Items utils spec', () => { expect(result.frequency).toBe(mockedProject.frequency); }); }); + + describe('sanitizeItem', () => { + it('strips HTML tags for name and namespace', () => { + const input = { + name: '<br><b>test</b>', + namespace: '<br>test', + id: 1, + }; + + expect(sanitizeItem(input)).toEqual({ name: 'test', namespace: 'test', id: 1 }); + }); + }); }); diff --git a/spec/lib/gitlab/git/cross_repo_comparer_spec.rb b/spec/lib/gitlab/git/cross_repo_comparer_spec.rb new file mode 100644 index 00000000000..8b37b6d1667 --- /dev/null +++ b/spec/lib/gitlab/git/cross_repo_comparer_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Git::CrossRepoComparer do + let(:source_project) { create(:project, :repository) } + let(:target_project) { create(:project, :repository) } + + let(:source_repo) { source_project.repository.raw_repository } + let(:target_repo) { target_project.repository.raw_repository } + + let(:source_branch) { 'feature' } + let(:target_branch) { 'master' } + let(:straight) { false } + + let(:source_commit) { source_repo.commit(source_branch) } + let(:target_commit) { source_repo.commit(target_branch) } + + subject(:result) { described_class.new(source_repo, target_repo).compare(source_branch, target_branch, straight: straight) } + + describe '#compare' do + context 'within a single repository' do + let(:target_project) { source_project } + + context 'a non-straight comparison' do + it 'compares without fetching from another repo' do + expect(source_repo).not_to receive(:fetch_source_branch!) + + expect_compare(result, from: source_commit, to: target_commit) + expect(result.straight).to eq(false) + end + end + + context 'a straight comparison' do + let(:straight) { true } + + it 'compares without fetching from another repo' do + expect(source_repo).not_to receive(:fetch_source_branch!) + + expect_compare(result, from: source_commit, to: target_commit) + expect(result.straight).to eq(true) + end + end + end + + context 'across two repositories' do + context 'target ref exists in source repo' do + it 'compares without fetching from another repo' do + expect(source_repo).not_to receive(:fetch_source_branch!) + expect(source_repo).not_to receive(:delete_refs) + + expect_compare(result, from: source_commit, to: target_commit) + end + end + + context 'target ref does not exist in source repo' do + it 'compares in the source repo by fetching from the target to a temporary ref' do + new_commit_id = create_commit(target_project.owner, target_repo, target_branch) + new_commit = target_repo.commit(new_commit_id) + + # This is how the temporary ref is generated + expect(SecureRandom).to receive(:hex).at_least(:once).and_return('foo') + + expect(source_repo) + .to receive(:fetch_source_branch!) + .with(target_repo, new_commit_id, 'refs/tmp/foo') + .and_call_original + + expect(source_repo).to receive(:delete_refs).with('refs/tmp/foo').and_call_original + + expect_compare(result, from: source_commit, to: new_commit) + end + end + + context 'source ref does not exist in source repo' do + let(:source_branch) { 'does-not-exist' } + + it 'returns an empty comparison' do + expect(source_repo).not_to receive(:fetch_source_branch!) + expect(source_repo).not_to receive(:delete_refs) + + expect(result).to be_a(::Gitlab::Git::Compare) + expect(result.commits.size).to eq(0) + end + end + + context 'target ref does not exist in target repo' do + let(:target_branch) { 'does-not-exist' } + + it 'returns nil' do + expect(source_repo).not_to receive(:fetch_source_branch!) + expect(source_repo).not_to receive(:delete_refs) + + is_expected.to be_nil + end + end + end + end + + def expect_compare(of, from:, to:) + expect(of).to be_a(::Gitlab::Git::Compare) + expect(from).to be_a(::Gitlab::Git::Commit) + expect(to).to be_a(::Gitlab::Git::Commit) + + expect(of.commits).not_to be_empty + expect(of.head).to eq(from) + expect(of.base).to eq(to) + end + + def create_commit(user, repo, branch) + action = { action: :create, file_path: '/FILE', content: 'content' } + + result = repo.multi_action(user, branch_name: branch, message: 'Commit', actions: [action]) + + result.newrev + end +end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 6854d514dcc..07fef203691 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1962,66 +1962,15 @@ describe Gitlab::Git::Repository, :seed_helper do end describe '#compare_source_branch' do - let(:repository) { Gitlab::Git::Repository.new('default', TEST_GITATTRIBUTES_REPO_PATH, '', 'group/project') } - - context 'within same repository' do - it 'does not create a temp ref' do - expect(repository).not_to receive(:fetch_source_branch!) - expect(repository).not_to receive(:delete_refs) - - compare = repository.compare_source_branch('master', repository, 'feature', straight: false) - expect(compare).to be_a(Gitlab::Git::Compare) - expect(compare.commits.count).to be > 0 - end - - it 'returns empty commits when source ref does not exist' do - compare = repository.compare_source_branch('master', repository, 'non-existent-branch', straight: false) + it 'delegates to Gitlab::Git::CrossRepoComparer' do + expect_next_instance_of(::Gitlab::Git::CrossRepoComparer) do |instance| + expect(instance.source_repo).to eq(:source_repository) + expect(instance.target_repo).to eq(repository) - expect(compare.commits).to be_empty + expect(instance).to receive(:compare).with('feature', 'master', straight: :straight) end - end - context 'with different repositories' do - context 'when ref is known by source repo, but not by target' do - before do - mutable_repository.write_ref('another-branch', 'feature') - end - - it 'creates temp ref' do - expect(repository).not_to receive(:fetch_source_branch!) - expect(repository).not_to receive(:delete_refs) - - compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) - expect(compare).to be_a(Gitlab::Git::Compare) - expect(compare.commits.count).to be > 0 - end - end - - context 'when ref is known by source and target repos' do - before do - mutable_repository.write_ref('another-branch', 'feature') - repository.write_ref('another-branch', 'feature') - end - - it 'does not create a temp ref' do - expect(repository).not_to receive(:fetch_source_branch!) - expect(repository).not_to receive(:delete_refs) - - compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) - expect(compare).to be_a(Gitlab::Git::Compare) - expect(compare.commits.count).to be > 0 - end - end - - context 'when ref is unknown by source repo' do - it 'returns nil when source ref does not exist' do - expect(repository).to receive(:fetch_source_branch!).and_call_original - expect(repository).to receive(:delete_refs).and_call_original - - compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false) - expect(compare).to be_nil - end - end + repository.compare_source_branch('master', :source_repository, 'feature', straight: :straight) end end diff --git a/spec/models/generic_commit_status_spec.rb b/spec/models/generic_commit_status_spec.rb index c851810ffb3..c8ed898122b 100644 --- a/spec/models/generic_commit_status_spec.rb +++ b/spec/models/generic_commit_status_spec.rb @@ -19,6 +19,74 @@ describe GenericCommitStatus do it { is_expected.not_to allow_value('javascript:alert(1)').for(:target_url) } end + describe '#name_uniqueness_across_types' do + let(:attributes) { {} } + let(:commit_status) { described_class.new(attributes) } + let(:status_name) { 'test-job' } + + subject(:errors) { commit_status.errors[:name] } + + shared_examples 'it does not have uniqueness errors' do + it 'does not return errors' do + commit_status.valid? + + is_expected.to be_empty + end + end + + context 'without attributes' do + it_behaves_like 'it does not have uniqueness errors' + end + + context 'with only a pipeline' do + let(:attributes) { { pipeline: pipeline } } + + context 'without name' do + it_behaves_like 'it does not have uniqueness errors' + end + end + + context 'with only a name' do + let(:attributes) { { name: status_name } } + + context 'without pipeline' do + it_behaves_like 'it does not have uniqueness errors' + end + end + + context 'with pipeline and name' do + let(:attributes) do + { + pipeline: pipeline, + name: status_name + } + end + + context 'without other statuses' do + it_behaves_like 'it does not have uniqueness errors' + end + + context 'with generic statuses' do + before do + create(:generic_commit_status, pipeline: pipeline, name: status_name) + end + + it_behaves_like 'it does not have uniqueness errors' + end + + context 'with ci_build statuses' do + before do + create(:ci_build, pipeline: pipeline, name: status_name) + end + + it 'returns name error' do + expect(commit_status).to be_invalid + is_expected.to include('has already been taken') + end + end + end + end + describe '#context' do subject { generic_commit_status.context } @@ -79,6 +147,12 @@ describe GenericCommitStatus do it { is_expected.not_to be_nil } end + + describe '#stage_idx' do + subject { generic_commit_status.stage_idx } + + it { is_expected.not_to be_nil } + end end describe '#present' do diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index f715ecae347..d227c018694 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -141,6 +141,34 @@ describe GlobalPolicy do it { is_expected.to be_allowed(:access_api) } end end + + context 'inactive user' do + before do + current_user.update!(confirmed_at: nil, confirmation_sent_at: 5.days.ago) + end + + context 'when within the confirmation grace period' do + before do + allow(User).to receive(:allow_unconfirmed_access_for).and_return(10.days) + end + + it { is_expected.to be_allowed(:access_api) } + end + + context 'when confirmation grace period is expired' do + before do + allow(User).to receive(:allow_unconfirmed_access_for).and_return(2.days) + end + + it { is_expected.not_to be_allowed(:access_api) } + end + + it 'when `inactive_policy_condition` feature flag is turned off' do + stub_feature_flags(inactive_policy_condition: false) + + is_expected.to be_allowed(:access_api) + end + end end describe 'receive notifications' do @@ -202,6 +230,20 @@ describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_git) } end + describe 'inactive user' do + before do + current_user.update!(confirmed_at: nil) + end + + it { is_expected.not_to be_allowed(:access_git) } + + it 'when `inactive_policy_condition` feature flag is turned off' do + stub_feature_flags(inactive_policy_condition: false) + + is_expected.to be_allowed(:access_git) + end + end + context 'when terms are enforced' do before do enforce_terms @@ -298,6 +340,20 @@ describe GlobalPolicy do it { is_expected.not_to be_allowed(:use_slash_commands) } end + describe 'inactive user' do + before do + current_user.update!(confirmed_at: nil) + end + + it { is_expected.not_to be_allowed(:use_slash_commands) } + + it 'when `inactive_policy_condition` feature flag is turned off' do + stub_feature_flags(inactive_policy_condition: false) + + is_expected.to be_allowed(:use_slash_commands) + end + end + context 'when access locked' do before do current_user.lock_access! diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 639b8e96343..24ed836996e 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -164,6 +164,7 @@ describe API::CommitStatuses do expect(response).to have_gitlab_http_status(201) expect(job.status).to eq('pending') + expect(job.stage_idx).to eq(GenericCommitStatus::EXTERNAL_STAGE_IDX) end end @@ -331,6 +332,29 @@ describe API::CommitStatuses do end end + context 'when updating a protected ref' do + before do + create(:protected_branch, project: project, name: 'master') + post api(post_url, user), params: { state: 'running', ref: 'master' } + end + + context 'with user as developer' do + let(:user) { developer } + + it 'does not create commit status' do + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with user as maintainer' do + let(:user) { create_user(:maintainer) } + + it 'creates commit status' do + expect(response).to have_gitlab_http_status(201) + end + end + end + context 'when commit SHA is invalid' do let(:sha) { 'invalid_sha' } @@ -372,6 +396,22 @@ describe API::CommitStatuses do .to include 'is blocked: Only allowed schemes are http, https' end end + + context 'when trying to update a status of a different type' do + let!(:pipeline) { create(:ci_pipeline, project: project, sha: sha, ref: 'ref') } + let!(:ci_build) { create(:ci_build, pipeline: pipeline, name: 'test-job') } + let(:params) { { state: 'pending', name: 'test-job' } } + + before do + post api(post_url, developer), params: params + end + + it 'responds with bad request status and validation errors' do + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['name']) + .to include 'has already been taken' + end + end end context 'reporter user' do diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index d8da1c001b0..e390f3945a9 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -8,6 +8,7 @@ describe API::Commits do let(:user) { create(:user) } let(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let(:developer) { create(:user).tap { |u| project.add_developer(u) } } let(:project) { create(:project, :repository, creator: user, path: 'my.project') } let(:branch_with_dot) { project.repository.find_branch('ends-with.json') } let(:branch_with_slash) { project.repository.find_branch('improve/awesome') } @@ -964,6 +965,56 @@ describe API::Commits do end end + shared_examples_for 'ref with pipeline' do + let!(:pipeline) do + project + .ci_pipelines + .create!(source: :push, ref: 'master', sha: commit.sha, protected: false) + end + + it 'includes status as "created" and a last_pipeline object' do + get api(route, current_user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to match_response_schema('public_api/v4/commit/detail') + expect(json_response['status']).to eq('created') + expect(json_response['last_pipeline']['id']).to eq(pipeline.id) + expect(json_response['last_pipeline']['ref']).to eq(pipeline.ref) + expect(json_response['last_pipeline']['sha']).to eq(pipeline.sha) + expect(json_response['last_pipeline']['status']).to eq(pipeline.status) + end + + context 'when pipeline succeeds' do + before do + pipeline.update!(status: 'success') + end + + it 'includes a "success" status' do + get api(route, current_user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to match_response_schema('public_api/v4/commit/detail') + expect(json_response['status']).to eq('success') + end + end + end + + shared_examples_for 'ref with unaccessible pipeline' do + let!(:pipeline) do + project + .ci_pipelines + .create!(source: :push, ref: 'master', sha: commit.sha, protected: false) + end + + it 'does not include last_pipeline' do + get api(route, current_user) + + expect(response).to match_response_schema('public_api/v4/commit/detail') + expect(response).to have_gitlab_http_status(200) + expect(json_response['last_pipeline']).to be_nil + end + end + context 'when stat param' do let(:route) { "/projects/#{project_id}/repository/commits/#{commit_id}" } @@ -993,6 +1044,15 @@ describe API::Commits do let(:project) { create(:project, :public, :repository) } it_behaves_like 'ref commit' + it_behaves_like 'ref with pipeline' + + context 'with private builds' do + before do + project.project_feature.update!(builds_access_level: ProjectFeature::PRIVATE) + end + + it_behaves_like 'ref with unaccessible pipeline' + end end context 'when unauthenticated', 'and project is private' do @@ -1006,6 +1066,17 @@ describe API::Commits do let(:current_user) { user } it_behaves_like 'ref commit' + it_behaves_like 'ref with pipeline' + + context 'when builds are disabled' do + before do + project + .project_feature + .update!(builds_access_level: ProjectFeature::DISABLED) + end + + it_behaves_like 'ref with unaccessible pipeline' + end context 'when branch contains a dot' do let(:commit) { project.repository.commit(branch_with_dot.name) } @@ -1041,35 +1112,53 @@ describe API::Commits do it_behaves_like 'ref commit' end end + end - context 'when the ref has a pipeline' do - let!(:pipeline) { project.ci_pipelines.create(source: :push, ref: 'master', sha: commit.sha, protected: false) } + context 'when authenticated', 'as a developer' do + let(:current_user) { developer } - it 'includes a "created" status' do - get api(route, current_user) + it_behaves_like 'ref commit' + it_behaves_like 'ref with pipeline' - expect(response).to have_gitlab_http_status(200) - expect(response).to match_response_schema('public_api/v4/commit/detail') - expect(json_response['status']).to eq('created') - expect(json_response['last_pipeline']['id']).to eq(pipeline.id) - expect(json_response['last_pipeline']['ref']).to eq(pipeline.ref) - expect(json_response['last_pipeline']['sha']).to eq(pipeline.sha) - expect(json_response['last_pipeline']['status']).to eq(pipeline.status) + context 'with private builds' do + before do + project.project_feature.update!(builds_access_level: ProjectFeature::PRIVATE) end - context 'when pipeline succeeds' do - before do - pipeline.update(status: 'success') - end + it_behaves_like 'ref with pipeline' + end + end - it 'includes a "success" status' do - get api(route, current_user) + context 'when authenticated', 'as a guest' do + let(:current_user) { guest } - expect(response).to have_gitlab_http_status(200) - expect(response).to match_response_schema('public_api/v4/commit/detail') - expect(json_response['status']).to eq('success') - end + it_behaves_like '403 response' do + let(:request) { get api(route, guest) } + let(:message) { '403 Forbidden' } + end + end + + context 'when authenticated', 'as a non member' do + let(:current_user) { create(:user) } + + it_behaves_like '403 response' do + let(:request) { get api(route, guest) } + let(:message) { '403 Forbidden' } + end + end + + context 'when authenticated', 'as non_member and project is public' do + let(:current_user) { create(:user) } + let(:project) { create(:project, :public, :repository) } + + it_behaves_like 'ref with pipeline' + + context 'with private builds' do + before do + project.project_feature.update!(builds_access_level: ProjectFeature::PRIVATE) end + + it_behaves_like 'ref with unaccessible pipeline' end end end diff --git a/spec/requests/api/oauth_tokens_spec.rb b/spec/requests/api/oauth_tokens_spec.rb index 8d7b3fa3c09..ce03756a19a 100644 --- a/spec/requests/api/oauth_tokens_spec.rb +++ b/spec/requests/api/oauth_tokens_spec.rb @@ -30,26 +30,40 @@ describe 'OAuth tokens' do end end - context "when user is blocked" do - it "does not create an access token" do - user = create(:user) + shared_examples 'does not create an access token' do + let(:user) { create(:user) } + + it { expect(response).to have_gitlab_http_status(401) } + end + + context 'when user is blocked' do + before do user.block request_oauth_token(user) - - expect(response).to have_gitlab_http_status(401) end + + include_examples 'does not create an access token' end - context "when user is ldap_blocked" do - it "does not create an access token" do - user = create(:user) + context 'when user is ldap_blocked' do + before do user.ldap_block request_oauth_token(user) + end - expect(response).to have_gitlab_http_status(401) + include_examples 'does not create an access token' + end + + context 'when user account is not confirmed' do + before do + user.update!(confirmed_at: nil) + + request_oauth_token(user) end + + include_examples 'does not create an access token' end end end diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index d78ab78c3d8..0fd1fcfe1a5 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' describe Projects::GroupLinks::DestroyService, '#execute' do - let(:group_link) { create :project_group_link } - let(:project) { group_link.project } + let(:project) { create(:project, :private) } + let!(:group_link) { create(:project_group_link, project: project) } let(:user) { create :user } let(:subject) { described_class.new(project, user) } @@ -15,4 +15,39 @@ describe Projects::GroupLinks::DestroyService, '#execute' do it 'returns false if group_link is blank' do expect { subject.execute(nil) }.not_to change { project.project_group_links.count } end + + describe 'todos cleanup' do + context 'when project is private' do + it 'triggers todos cleanup' do + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) + expect(project.private?).to be true + + subject.execute(group_link) + end + end + + context 'when project is public or internal' do + shared_examples_for 'removes confidential todos' do + it 'does not trigger todos cleanup' do + expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id) + expect(project.private?).to be false + + subject.execute(group_link) + end + end + + context 'when project is public' do + let(:project) { create(:project, :public) } + + it_behaves_like 'removes confidential todos' + end + + context 'when project is internal' do + let(:project) { create(:project, :public) } + + it_behaves_like 'removes confidential todos' + end + end + end end |