diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-02 21:26:24 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-02 21:26:24 +0000 |
commit | 0af96615b6330dcbccc85ecf67eb7578646ed75a (patch) | |
tree | 62bc292ce095a0836d8699f51ffff92f835dac7c /spec/requests | |
parent | 5f7307f6d99a45d64bb0ebbe74e47c4a93277011 (diff) | |
download | gitlab-ce-0af96615b6330dcbccc85ecf67eb7578646ed75a.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-0-stable-ee
Diffstat (limited to 'spec/requests')
6 files changed, 128 insertions, 30 deletions
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 |