diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-03 00:26:24 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-03 00:26:24 +0300 |
commit | 0af96615b6330dcbccc85ecf67eb7578646ed75a (patch) | |
tree | 62bc292ce095a0836d8699f51ffff92f835dac7c /spec | |
parent | 5f7307f6d99a45d64bb0ebbe74e47c4a93277011 (diff) |
Add latest changes from gitlab-org/security/gitlab@14-0-stable-ee
Diffstat (limited to 'spec')
23 files changed, 461 insertions, 74 deletions
diff --git a/spec/features/dashboard/todos/target_state_spec.rb b/spec/features/dashboard/todos/target_state_spec.rb index 4c43948201c..b0aafdda59a 100644 --- a/spec/features/dashboard/todos/target_state_spec.rb +++ b/spec/features/dashboard/todos/target_state_spec.rb @@ -3,16 +3,20 @@ require 'spec_helper' RSpec.describe 'Dashboard > Todo target states' do - let(:user) { create(:user) } - let(:author) { create(:user) } - let(:project) { create(:project, :public) } + let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user) } + let_it_be(:project) { create(:project, :public) } + + before_all do + project.add_developer(user) + end before do sign_in(user) end it 'on a closed issue todo has closed label' do - issue_closed = create(:issue, state: 'closed') + issue_closed = create(:issue, state: 'closed', project: project) create_todo issue_closed visit dashboard_todos_path @@ -22,7 +26,7 @@ RSpec.describe 'Dashboard > Todo target states' do end it 'on an open issue todo does not have an open label' do - issue_open = create(:issue) + issue_open = create(:issue, project: project) create_todo issue_open visit dashboard_todos_path @@ -32,7 +36,7 @@ RSpec.describe 'Dashboard > Todo target states' do end it 'on a merged merge request todo has merged label' do - mr_merged = create(:merge_request, :simple, :merged, author: user) + mr_merged = create(:merge_request, :simple, :merged, author: user, source_project: project) create_todo mr_merged visit dashboard_todos_path @@ -42,7 +46,7 @@ RSpec.describe 'Dashboard > Todo target states' do end it 'on a closed merge request todo has closed label' do - mr_closed = create(:merge_request, :simple, :closed, author: user) + mr_closed = create(:merge_request, :simple, :closed, author: user, source_project: project) create_todo mr_closed visit dashboard_todos_path @@ -52,7 +56,7 @@ RSpec.describe 'Dashboard > Todo target states' do end it 'on an open merge request todo does not have an open label' do - mr_open = create(:merge_request, :simple, author: user) + mr_open = create(:merge_request, :simple, author: user, source_project: project) create_todo mr_open visit dashboard_todos_path diff --git a/spec/features/dashboard/todos/todos_filtering_spec.rb b/spec/features/dashboard/todos/todos_filtering_spec.rb index b1464af4194..53209db3107 100644 --- a/spec/features/dashboard/todos/todos_filtering_spec.rb +++ b/spec/features/dashboard/todos/todos_filtering_spec.rb @@ -128,7 +128,7 @@ RSpec.describe 'Dashboard > User filters todos', :js do describe 'filter by action' do before do - create(:todo, :build_failed, user: user_1, author: user_2, project: project_1) + create(:todo, :build_failed, user: user_1, author: user_2, project: project_1, target: merge_request) create(:todo, :marked, user: user_1, author: user_2, project: project_1, target: issue1) create(:todo, :review_requested, user: user_1, author: user_2, project: project_1, target: issue1) end diff --git a/spec/features/dashboard/todos/todos_spec.rb b/spec/features/dashboard/todos/todos_spec.rb index 0bc6cc9c017..7345bfa19e2 100644 --- a/spec/features/dashboard/todos/todos_spec.rb +++ b/spec/features/dashboard/todos/todos_spec.rb @@ -3,10 +3,16 @@ require 'spec_helper' RSpec.describe 'Dashboard Todos' do + include DesignManagementTestHelpers + let_it_be(:user) { create(:user, username: 'john') } let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :public) } - let_it_be(:issue) { create(:issue, due_date: Date.today, title: "Fix bug") } + let_it_be(:issue) { create(:issue, project: project, due_date: Date.today, title: "Fix bug") } + + before_all do + project.add_developer(user) + end context 'User does not have todos' do before do @@ -21,8 +27,8 @@ RSpec.describe 'Dashboard Todos' do context 'when the todo references a merge request' do let(:referenced_mr) { create(:merge_request, source_project: project) } - let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}") } - let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note) } + let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}", noteable: create(:issue, project: project)) } + let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note, target: note.noteable) } before do sign_in(user) @@ -39,9 +45,26 @@ RSpec.describe 'Dashboard Todos' do end end - context 'User has a todo', :js do + context 'user has an unauthorized todo' do before do + sign_in(user) + end + + it 'does not render the todo' do + unauthorized_issue = create(:issue) + create(:todo, :mentioned, user: user, project: unauthorized_issue.project, target: unauthorized_issue, author: author) create(:todo, :mentioned, user: user, project: project, target: issue, author: author) + + visit dashboard_todos_path + + expect(page).to have_selector('.todos-list .todo', count: 1) + end + end + + context 'User has a todo', :js do + let_it_be(:user_todo) { create(:todo, :mentioned, user: user, project: project, target: issue, author: author) } + + before do sign_in(user) visit dashboard_todos_path @@ -183,7 +206,7 @@ RSpec.describe 'Dashboard Todos' do end context 'approval todo' do - let(:merge_request) { create(:merge_request, title: "Fixes issue") } + let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) } before do create(:todo, :approval_required, user: user, project: project, target: merge_request, author: user) @@ -199,7 +222,7 @@ RSpec.describe 'Dashboard Todos' do end context 'review request todo' do - let(:merge_request) { create(:merge_request, title: "Fixes issue") } + let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) } before do create(:todo, :review_requested, user: user, project: project, target: merge_request, author: user) @@ -355,7 +378,7 @@ RSpec.describe 'Dashboard Todos' do end context 'User has a Build Failed todo' do - let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author) } + let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author, target: create(:merge_request, source_project: project)) } before do sign_in(user) @@ -386,6 +409,7 @@ RSpec.describe 'Dashboard Todos' do end before do + enable_design_management project.add_developer(user) sign_in(user) diff --git a/spec/features/markdown/mermaid_spec.rb b/spec/features/markdown/mermaid_spec.rb index c4994838d26..e080c7ffb3f 100644 --- a/spec/features/markdown/mermaid_spec.rb +++ b/spec/features/markdown/mermaid_spec.rb @@ -260,8 +260,6 @@ RSpec.describe 'Mermaid rendering', :js do description *= 51 - project = create(:project, :public) - wiki_page = build(:wiki_page, { container: project, content: description }) wiki_page.create message: 'mermaid test commit' # rubocop:disable Rails/SaveBang wiki_page = project.wiki.find_page(wiki_page.slug) @@ -277,6 +275,27 @@ RSpec.describe 'Mermaid rendering', :js do expect(page).not_to have_selector('.js-lazy-render-mermaid-container') end end + + it 'does not allow HTML injection' do + description = <<~MERMAID + ```mermaid + %%{init: {"flowchart": {"htmlLabels": "false"}} }%% + flowchart + A["<iframe></iframe>"] + ``` + MERMAID + + issue = create(:issue, project: project, description: description) + + visit project_issue_path(project, issue) + + wait_for_requests + wait_for_mermaid + + page.within('.description') do + expect(page).not_to have_xpath("//iframe") + end + end end def wait_for_mermaid diff --git a/spec/graphql/mutations/todos/mark_done_spec.rb b/spec/graphql/mutations/todos/mark_done_spec.rb index b5f2ff5d044..9723ac8af42 100644 --- a/spec/graphql/mutations/todos/mark_done_spec.rb +++ b/spec/graphql/mutations/todos/mark_done_spec.rb @@ -5,17 +5,23 @@ require 'spec_helper' RSpec.describe Mutations::Todos::MarkDone do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) } let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } + before_all do + project.add_developer(current_user) + end + specify { expect(described_class).to require_graphql_authorizations(:update_todo) } describe '#resolve' do diff --git a/spec/graphql/mutations/todos/restore_spec.rb b/spec/graphql/mutations/todos/restore_spec.rb index 22fb1bba7a8..954bb3db668 100644 --- a/spec/graphql/mutations/todos/restore_spec.rb +++ b/spec/graphql/mutations/todos/restore_spec.rb @@ -5,17 +5,23 @@ require 'spec_helper' RSpec.describe Mutations::Todos::Restore do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) } let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } + before_all do + project.add_developer(current_user) + end + specify { expect(described_class).to require_graphql_authorizations(:update_todo) } describe '#resolve' do diff --git a/spec/graphql/resolvers/todo_resolver_spec.rb b/spec/graphql/resolvers/todo_resolver_spec.rb index ac14852b365..0760935a2fe 100644 --- a/spec/graphql/resolvers/todo_resolver_spec.rb +++ b/spec/graphql/resolvers/todo_resolver_spec.rb @@ -4,19 +4,28 @@ require 'spec_helper' RSpec.describe Resolvers::TodoResolver do include GraphqlHelpers + include DesignManagementTestHelpers specify do expect(described_class).to have_nullable_graphql_type(Types::TodoType.connection_type) end describe '#resolve' do + let_it_be(:project) { create(:project) } let_it_be(:current_user) { create(:user) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:author1) { create(:user) } let_it_be(:author2) { create(:user) } - let_it_be(:merge_request_todo_pending) { create(:todo, user: current_user, target_type: 'MergeRequest', state: :pending, action: Todo::MENTIONED, author: author1) } - let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2) } - let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) } + let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2, target: issue) } + let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue) } + + let(:merge_request) { create(:merge_request, source_project: project) } + let!(:merge_request_todo_pending) { create(:todo, user: current_user, target: merge_request, state: :pending, action: Todo::MENTIONED, author: author1) } + + before_all do + project.add_developer(current_user) + end it 'calls TodosFinder' do expect_next_instance_of(TodosFinder) do |finder| @@ -40,7 +49,9 @@ RSpec.describe Resolvers::TodoResolver do end it 'returns the todos for multiple filters' do - design_todo_pending = create(:todo, target_type: 'DesignManagement::Design', user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) + enable_design_management + design = create(:design, issue: issue) + design_todo_pending = create(:todo, target: design, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) todos = resolve_todos(type: ['MergeRequest', 'DesignManagement::Design']) @@ -59,11 +70,15 @@ RSpec.describe Resolvers::TodoResolver do group3 = create(:group) group1.add_developer(current_user) + issue1 = create(:issue, project: create(:project, group: group1)) group2.add_developer(current_user) + issue2 = create(:issue, project: create(:project, group: group2)) + group3.add_developer(current_user) + issue3 = create(:issue, project: create(:project, group: group3)) - todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) - todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) - create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) + todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue1) + todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue2) + create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue3) todos = resolve_todos(group_id: [group2.id, group1.id]) @@ -93,9 +108,13 @@ RSpec.describe Resolvers::TodoResolver do project2 = create(:project) project3 = create(:project) - todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) - todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) - create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) + project1.add_developer(current_user) + project2.add_developer(current_user) + project3.add_developer(current_user) + + todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project1)) + todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project2)) + create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project3)) todos = resolve_todos(project_id: [project2.id, project1.id]) diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index 6019318a401..7771289abe6 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -136,7 +136,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do let(:command) do Gitlab::Ci::Pipeline::Chain::Command.new( source: :push, - origin_ref: 'mytag', + origin_ref: origin_ref, checkout_sha: project.commit.id, after_sha: nil, before_sha: nil, @@ -147,6 +147,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do current_user: user) end + let(:origin_ref) { 'mytag' } + before do allow_any_instance_of(Repository).to receive(:tag_exists?).with('mytag').and_return(true) @@ -156,6 +158,14 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do it 'correctly indicated that this is a tagged pipeline' do expect(pipeline).to be_tag end + + context 'when origin_ref is branch but tag ref with the same name exists' do + let(:origin_ref) { 'refs/heads/mytag' } + + it 'correctly indicated that a pipeline is not tagged' do + expect(pipeline).not_to be_tag + end + end end context 'when pipeline is running for a merge request' do diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 900dfec38e2..2e73043e309 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -27,6 +27,18 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(true) } end + context 'for fully described tag ref' do + let(:origin_ref) { 'refs/tags/master' } + + it { is_expected.to eq(false) } + end + + context 'for fully described branch ref' do + let(:origin_ref) { 'refs/heads/master' } + + it { is_expected.to eq(true) } + end + context 'for invalid branch' do let(:origin_ref) { 'something' } @@ -43,6 +55,18 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(true) } end + context 'for fully described tag ref' do + let(:origin_ref) { 'refs/tags/v1.0.0' } + + it { is_expected.to eq(true) } + end + + context 'for fully described branch ref' do + let(:origin_ref) { 'refs/heads/v1.0.0' } + + it { is_expected.to eq(false) } + end + context 'for invalid ref' do let(:origin_ref) { 'something' } diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 2731eadecc0..11652d9841b 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -552,4 +552,10 @@ RSpec.describe DiffNote do expect(subject.on_image?).to be_truthy end end + + describe '#to_ability_name' do + subject { described_class.new.to_ability_name } + + it { is_expected.to eq('note') } + end end diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb new file mode 100644 index 00000000000..6e1b39cc438 --- /dev/null +++ b/spec/models/discussion_note_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe DiscussionNote do + describe '#to_ability_name' do + subject { described_class.new.to_ability_name } + + it { is_expected.to eq('note') } + end +end diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb new file mode 100644 index 00000000000..ee3bbf186b9 --- /dev/null +++ b/spec/models/legacy_diff_note_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LegacyDiffNote do + describe '#to_ability_name' do + subject { described_class.new.to_ability_name } + + it { is_expected.to eq('note') } + end +end diff --git a/spec/models/synthetic_note_spec.rb b/spec/models/synthetic_note_spec.rb new file mode 100644 index 00000000000..55fa4f7c33d --- /dev/null +++ b/spec/models/synthetic_note_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SyntheticNote do + describe '#to_ability_name' do + subject { described_class.new.to_ability_name } + + it { is_expected.to eq('note') } + end +end diff --git a/spec/policies/todo_policy_spec.rb b/spec/policies/todo_policy_spec.rb index b4876baa504..16435b21666 100644 --- a/spec/policies/todo_policy_spec.rb +++ b/spec/policies/todo_policy_spec.rb @@ -9,22 +9,28 @@ RSpec.describe TodoPolicy do let_it_be(:user2) { create(:user) } let_it_be(:user3) { create(:user) } - let_it_be(:todo1) { create(:todo, author: author, user: user1) } - let_it_be(:todo2) { create(:todo, author: author, user: user2) } + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } + + let_it_be(:todo1) { create(:todo, author: author, user: user1, issue: issue) } + let_it_be(:todo2) { create(:todo, author: author, user: user2, issue: issue) } let_it_be(:todo3) { create(:todo, author: author, user: user2) } - let_it_be(:todo4) { create(:todo, author: author, user: user3) } + let_it_be(:todo4) { create(:todo, author: author, user: user3, issue: issue) } def permissions(user, todo) described_class.new(user, todo) end + before_all do + project.add_developer(user1) + project.add_developer(user2) + end + describe 'own_todo' do - it 'allows owners to access their own todos' do + it 'allows owners to access their own todos if they can read todo target' do [ [user1, todo1], - [user2, todo2], - [user2, todo3], - [user3, todo4] + [user2, todo2] ].each do |user, todo| expect(permissions(user, todo)).to be_allowed(:read_todo) end @@ -38,7 +44,9 @@ RSpec.describe TodoPolicy do [user2, todo4], [user3, todo1], [user3, todo2], - [user3, todo3] + [user3, todo3], + [user2, todo3], + [user3, todo4] ].each do |user, todo| expect(permissions(user, todo)).to be_disallowed(:read_todo) end diff --git a/spec/requests/api/graphql/current_user/todos_query_spec.rb b/spec/requests/api/graphql/current_user/todos_query_spec.rb index e298de0df01..981b10a7467 100644 --- a/spec/requests/api/graphql/current_user/todos_query_spec.rb +++ b/spec/requests/api/graphql/current_user/todos_query_spec.rb @@ -4,12 +4,17 @@ require 'spec_helper' RSpec.describe 'Query current user todos' do include GraphqlHelpers + include DesignManagementTestHelpers let_it_be(:current_user) { create(:user) } - let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: create(:project, :repository)) } - let_it_be(:issue_todo) { create(:todo, user: current_user, target: create(:issue)) } - let_it_be(:merge_request_todo) { create(:todo, user: current_user, target: create(:merge_request)) } - let_it_be(:design_todo) { create(:todo, user: current_user, target: create(:design)) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:unauthorize_project) { create(:project) } + let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: project) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:issue_todo) { create(:todo, project: project, user: current_user, target: issue) } + let_it_be(:merge_request_todo) { create(:todo, project: project, user: current_user, target: create(:merge_request, source_project: project)) } + let_it_be(:design_todo) { create(:todo, project: project, user: current_user, target: create(:design, issue: issue)) } + let_it_be(:unauthorized_todo) { create(:todo, user: current_user, project: unauthorize_project, target: create(:issue, project: unauthorize_project)) } let(:fields) do <<~QUERY @@ -23,16 +28,22 @@ RSpec.describe 'Query current user todos' do graphql_query_for('currentUser', {}, query_graphql_field('todos', {}, fields)) end + before_all do + project.add_developer(current_user) + end + subject { graphql_data.dig('currentUser', 'todos', 'nodes') } before do + enable_design_management + post_graphql(query, current_user: current_user) end it_behaves_like 'a working graphql query' it 'contains the expected ids' do - is_expected.to include( + is_expected.to contain_exactly( a_hash_including('id' => commit_todo.to_global_id.to_s), a_hash_including('id' => issue_todo.to_global_id.to_s), a_hash_including('id' => merge_request_todo.to_global_id.to_s), @@ -41,11 +52,33 @@ RSpec.describe 'Query current user todos' do end it 'returns Todos for all target types' do - is_expected.to include( + is_expected.to contain_exactly( a_hash_including('targetType' => 'COMMIT'), a_hash_including('targetType' => 'ISSUE'), a_hash_including('targetType' => 'MERGEREQUEST'), a_hash_including('targetType' => 'DESIGN') ) end + + context 'when requesting a single field' do + let(:fields) do + <<~QUERY + nodes { + id + } + QUERY + end + + it 'avoids N+1 queries', :request_store do + control = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) } + + project2 = create(:project) + project2.add_developer(current_user) + issue2 = create(:issue, project: project2) + create(:todo, user: current_user, target: issue2, project: project2) + + # An additional query is made for each different group that owns a todo through a project + expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(control).with_threshold(2) + end + end end diff --git a/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb b/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb index 8f92105dc9c..9ac98db91e2 100644 --- a/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb +++ b/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb @@ -5,14 +5,16 @@ require 'spec_helper' RSpec.describe 'Marking all todos done' do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } let_it_be(:other_user2) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) } - let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) } + let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) } @@ -28,6 +30,10 @@ RSpec.describe 'Marking all todos done' do ) end + before_all do + project.add_developer(current_user) + end + def mutation_response graphql_mutation_response(:todos_mark_all_done) end diff --git a/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb b/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb index 8a9a0b9e845..7f5ea71c760 100644 --- a/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb +++ b/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe 'Marking todos done' do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) } @@ -29,6 +31,10 @@ RSpec.describe 'Marking todos done' do ) end + before_all do + project.add_developer(current_user) + end + def mutation_response graphql_mutation_response(:todo_mark_done) end diff --git a/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb b/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb index e71a232ff7c..70e3cc7f5cd 100644 --- a/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb +++ b/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe 'Restoring many Todos' do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) } @@ -30,6 +32,10 @@ RSpec.describe 'Restoring many Todos' do ) end + before_all do + project.add_developer(current_user) + end + def mutation_response graphql_mutation_response(:todo_restore_many) end diff --git a/spec/requests/api/graphql/mutations/todos/restore_spec.rb b/spec/requests/api/graphql/mutations/todos/restore_spec.rb index a58c7fc69fc..d995191c97e 100644 --- a/spec/requests/api/graphql/mutations/todos/restore_spec.rb +++ b/spec/requests/api/graphql/mutations/todos/restore_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe 'Restoring Todos' do include GraphqlHelpers + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:other_user) { create(:user) } - let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) } - let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) } + let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) } + let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) } let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) } @@ -29,6 +31,10 @@ RSpec.describe 'Restoring Todos' do ) end + before_all do + project.add_developer(current_user) + end + def mutation_response graphql_mutation_response(:todo_restore) end diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 00de1ef5964..d31f571e636 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -3,18 +3,22 @@ require 'spec_helper' RSpec.describe API::Todos do + include DesignManagementTestHelpers + let_it_be(:group) { create(:group) } let_it_be(:project_1) { create(:project, :repository, group: group) } let_it_be(:project_2) { create(:project) } let_it_be(:author_1) { create(:user) } let_it_be(:author_2) { create(:user) } let_it_be(:john_doe) { create(:user, username: 'john_doe') } + let_it_be(:issue) { create(:issue, project: project_1) } let_it_be(:merge_request) { create(:merge_request, source_project: project_1) } let_it_be(:merge_request_todo) { create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request) } - let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) } - let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe) } + let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: issue) } + let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe, target: issue) } let_it_be(:pending_3) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe) } - let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe) } + let_it_be(:pending_4) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe, commit_id: 'invalid_id') } + let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe, target: issue) } let_it_be(:award_emoji_1) { create(:award_emoji, awardable: merge_request, user: author_1, name: 'thumbsup') } let_it_be(:award_emoji_2) { create(:award_emoji, awardable: pending_1.target, user: author_1, name: 'thumbsup') } let_it_be(:award_emoji_3) { create(:award_emoji, awardable: pending_2.target, user: author_2, name: 'thumbsdown') } @@ -77,13 +81,13 @@ RSpec.describe API::Todos do expect(json_response[0]['target_type']).to eq('Commit') expect(json_response[1]['target_type']).to eq('Issue') - expect(json_response[1]['target']['upvotes']).to eq(0) + expect(json_response[1]['target']['upvotes']).to eq(1) expect(json_response[1]['target']['downvotes']).to eq(1) expect(json_response[1]['target']['merge_requests_count']).to eq(0) expect(json_response[2]['target_type']).to eq('Issue') expect(json_response[2]['target']['upvotes']).to eq(1) - expect(json_response[2]['target']['downvotes']).to eq(0) + expect(json_response[2]['target']['downvotes']).to eq(1) expect(json_response[2]['target']['merge_requests_count']).to eq(0) expect(json_response[3]['target_type']).to eq('MergeRequest') @@ -93,6 +97,19 @@ RSpec.describe API::Todos do expect(json_response[3]['target']['downvotes']).to eq(0) end + context "when current user does not have access to one of the TODO's target" do + it 'filters out unauthorized todos' do + no_access_project = create(:project, :repository, group: group) + no_access_merge_request = create(:merge_request, source_project: no_access_project) + no_access_todo = create(:todo, project: no_access_project, author: author_2, user: john_doe, target: no_access_merge_request) + + get api('/todos', john_doe) + + expect(json_response.count).to eq(4) + expect(json_response.map { |t| t['id'] }).not_to include(no_access_todo.id, pending_4.id) + end + end + context 'and using the author filter' do it 'filters based on author_id param' do get api('/todos', john_doe), params: { author_id: author_2.id } @@ -163,23 +180,31 @@ RSpec.describe API::Todos do end it 'avoids N+1 queries', :request_store do + create_issue_todo_for(john_doe) create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request) get api('/todos', john_doe) - control = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) } + control1 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) } + + create_issue_todo_for(john_doe) + create_mr_todo_for(john_doe, project_2) + create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: merge_request) + new_todo = create_mr_todo_for(john_doe) + merge_request_3 = create(:merge_request, :jira_branch, source_project: new_todo.project) + create(:on_commit_todo, project: new_todo.project, author: author_1, user: john_doe, target: merge_request_3) + create(:todo, project: new_todo.project, author: author_2, user: john_doe, target: merge_request_3) - merge_request_2 = create(:merge_request, source_project: project_2) - create(:todo, project: project_2, author: author_2, user: john_doe, target: merge_request_2) + expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control1).with_threshold(4) + control2 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) } - project_3 = create(:project, :repository) - project_3.add_developer(john_doe) - merge_request_3 = create(:merge_request, source_project: project_3) - create(:todo, project: project_3, author: author_2, user: john_doe, target: merge_request_3) - create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) - create(:on_commit_todo, project: project_3, author: author_1, user: john_doe) + create_issue_todo_for(john_doe) + create_issue_todo_for(john_doe, project_1) + create_issue_todo_for(john_doe, project_1) + + # Additional query only when target belongs to project from different group + expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control2).with_threshold(1) - expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control) expect(response).to have_gitlab_http_status(:ok) end @@ -201,6 +226,8 @@ RSpec.describe API::Todos do end before do + enable_design_management + api_request end @@ -222,6 +249,20 @@ RSpec.describe API::Todos do ) end end + + def create_mr_todo_for(user, project = nil) + new_project = project || create(:project, group: create(:group)) + new_project.add_developer(user) if project.blank? + new_merge_request = create(:merge_request, source_project: new_project) + create(:todo, project: new_project, author: user, user: user, target: new_merge_request) + end + + def create_issue_todo_for(user, project = nil) + new_project = project || create(:project, group: create(:group)) + new_project.group.add_developer(user) if project.blank? + issue = create(:issue, project: new_project) + create(:todo, project: new_project, target: issue, author: user, user: user) + end end describe 'POST /todos/:id/mark_as_done' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 3316f8c3d9b..8b2e135b190 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1156,6 +1156,73 @@ RSpec.describe Ci::CreatePipelineService do end end + context 'when pipeline is running for a nonexistant-branch' do + let(:gitlab_ci_yaml) { YAML.dump(test: { script: 'test' }) } + + let(:ref_name) { 'refs/heads/nonexistant-branch' } + + let(:pipeline) { execute_service } + + it 'does not create the pipeline' do + expect(pipeline).not_to be_created_successfully + expect(pipeline.errors[:base]).to eq(['Reference not found']) + end + + context 'when there is a tag with that nonexistant-branch' do + # v1.0.0 is on the test repo as a tag + let(:ref_name) { 'refs/heads/v1.0.0' } + + it 'does not create the pipeline' do + expect(pipeline).not_to be_created_successfully + expect(pipeline.errors[:base]).to eq(['Reference not found']) + end + end + end + + context 'when pipeline is running for a branch with the name of both a branch and a tag' do + let(:gitlab_ci_yaml) { YAML.dump(test: { script: 'test' }) } + + # v1.1.0 is on the test repo as branch and tag + let(:ref_name) { 'refs/heads/v1.1.0' } + + let(:pipeline) { execute_service } + + it 'creates the pipeline for the branch' do + expect(pipeline).to be_created_successfully + expect(pipeline.branch?).to be true + expect(pipeline.tag?).to be false + end + end + + context 'when pipeline is running for a tag with the name of both a branch and a tag' do + let(:gitlab_ci_yaml) { YAML.dump(test: { script: 'test' }) } + + # v1.1.0 is on the test repo as branch and tag + let(:ref_name) { 'refs/tags/v1.1.0' } + + let(:pipeline) { execute_service } + + it 'creates the pipeline for the tag' do + expect(pipeline).to be_created_successfully + expect(pipeline.branch?).to be false + expect(pipeline.tag?).to be true + end + end + + context 'when pipeline is running for an ambiguous ref' do + let(:gitlab_ci_yaml) { YAML.dump(test: { script: 'test' }) } + + # v1.1.0 is on the test repo as branch and tag + let(:ref_name) { 'v1.1.0' } + + let(:pipeline) { execute_service } + + it 'does not create the pipeline' do + expect(pipeline).not_to be_created_successfully + expect(pipeline.errors[:base]).to eq(['Ref is ambiguous']) + end + end + context 'when pipeline variables are specified' do let(:variables_attributes) do [{ key: 'first', secret_value: 'world' }, diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index 087f4ba372b..ac9bac4e6ad 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -109,9 +109,13 @@ RSpec.describe Git::ProcessRefChangesService do .to receive(:commit) .and_return(project.commit) - allow_any_instance_of(Repository) - .to receive(:branch_exists?) - .and_return(true) + if changes_method == :branch_changes + allow_any_instance_of(Repository).to receive(:branch_exists?) { true } + end + + if changes_method == :tag_changes + allow_any_instance_of(Repository).to receive(:tag_exists?) { true } + end end context 'when git_push_create_all_pipelines is disabled' do diff --git a/spec/services/todos/allowed_target_filter_service_spec.rb b/spec/services/todos/allowed_target_filter_service_spec.rb new file mode 100644 index 00000000000..707df8e8514 --- /dev/null +++ b/spec/services/todos/allowed_target_filter_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Todos::AllowedTargetFilterService do + include DesignManagementTestHelpers + + let_it_be(:authorized_group) { create(:group, :private) } + let_it_be(:authorized_project) { create(:project, group: authorized_group) } + let_it_be(:unauthorized_group) { create(:group, :private) } + let_it_be(:unauthorized_project) { create(:project, group: unauthorized_group) } + let_it_be(:user) { create(:user) } + let_it_be(:authorized_issue) { create(:issue, project: authorized_project) } + let_it_be(:authorized_issue_todo) { create(:todo, project: authorized_project, target: authorized_issue, user: user) } + let_it_be(:unauthorized_issue) { create(:issue, project: unauthorized_project) } + let_it_be(:unauthorized_issue_todo) { create(:todo, project: unauthorized_project, target: unauthorized_issue, user: user) } + let_it_be(:authorized_design) { create(:design, issue: authorized_issue) } + let_it_be(:authorized_design_todo) { create(:todo, project: authorized_project, target: authorized_design, user: user) } + let_it_be(:unauthorized_design) { create(:design, issue: unauthorized_issue) } + let_it_be(:unauthorized_design_todo) { create(:todo, project: unauthorized_project, target: unauthorized_design, user: user) } + + # Cannot use let_it_be with MRs + let(:authorized_mr) { create(:merge_request, source_project: authorized_project) } + let(:authorized_mr_todo) { create(:todo, project: authorized_project, user: user, target: authorized_mr) } + let(:unauthorized_mr) { create(:merge_request, source_project: unauthorized_project) } + let(:unauthorized_mr_todo) { create(:todo, project: unauthorized_project, user: user, target: unauthorized_mr) } + + before_all do + authorized_group.add_developer(user) + end + + describe '#execute' do + subject(:execute_service) { described_class.new(all_todos, user).execute } + + let!(:all_todos) { authorized_todos + unauthorized_todos } + + let(:authorized_todos) do + [ + authorized_mr_todo, + authorized_issue_todo, + authorized_design_todo + ] + end + + let(:unauthorized_todos) do + [ + unauthorized_mr_todo, + unauthorized_issue_todo, + unauthorized_design_todo + ] + end + + before do + enable_design_management + end + + it { is_expected.to match_array(authorized_todos) } + end +end |