From 10d4625ed3b73f73bc67bf7d35347dcd1912cf7b Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 3 Oct 2022 09:09:43 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .gitpod.yml | 16 +-- .../autocomplete_sources_controller.rb | 5 +- app/graphql/resolvers/base_issues_resolver.rb | 1 + app/models/award_emoji.rb | 4 + app/models/concerns/issuable.rb | 10 +- app/models/concerns/participable.rb | 12 +- app/models/issue.rb | 4 + app/models/merge_request.rb | 2 +- app/models/note.rb | 4 + app/policies/issuable_policy.rb | 6 + app/policies/note_policy.rb | 3 +- .../concerns/users/participable_service.rb | 2 + app/services/projects/autocomplete_service.rb | 2 +- ...r_registry_tags_list_default_page_size.yml | 8 -- doc/api/issues.md | 2 +- doc/api/version.md | 11 +- lib/api/access_requests.rb | 6 +- lib/api/api.rb | 1 - lib/api/ci/resource_groups.rb | 19 +++ lib/api/metadata.rb | 26 +++- lib/api/version.rb | 34 ----- lib/container_registry/client.rb | 2 +- .../autocomplete_sources_controller_spec.rb | 130 +++++++++++++++--- .../users/participants_resolver_spec.rb | 83 ++++++++--- spec/lib/container_registry/client_spec.rb | 12 -- spec/models/award_emoji_spec.rb | 9 ++ spec/models/concerns/participable_spec.rb | 3 + spec/policies/issuable_policy_spec.rb | 20 ++- spec/requests/api/ci/resource_groups_spec.rb | 42 ++++++ .../api/graphql/project/issues_spec.rb | 24 ++++ .../graphql/project/merge_requests_spec.rb | 21 +++ spec/requests/api/metadata_spec.rb | 26 ++-- spec/requests/api/version_spec.rb | 93 ------------- .../graphql/n_plus_one_query_examples.rb | 5 +- .../concerns/participable_shared_examples.rb | 42 ++++++ 35 files changed, 460 insertions(+), 230 deletions(-) delete mode 100644 config/feature_flags/development/container_registry_tags_list_default_page_size.yml delete mode 100644 lib/api/version.rb delete mode 100644 spec/requests/api/version_spec.rb create mode 100644 spec/support/shared_examples/models/concerns/participable_shared_examples.rb diff --git a/.gitpod.yml b/.gitpod.yml index 3fb623b9fb6..535c60b42c8 100644 --- a/.gitpod.yml +++ b/.gitpod.yml @@ -86,7 +86,7 @@ tasks: printf "$(date) – GitLab is up (took ~%.1f minutes)\n" "$((10*$SECONDS/60))e-1" | tee -a /workspace/startup.log gp preview $(gp url 3000) || true PREBUILD_LOG=(/workspace/.gitpod/prebuild-log-*) - [[ -f /workspace/gitpod_start_time.sh ]] && printf "Took %.1f minutes from https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitpod.yml being executed through to completion %s\n" "$((10*(($(date +%s)-${START_TIME_IN_SECONDS}))/60))e-1" "$([[ -f "$PREBUILD_LOG" ]] && echo "With Prebuilds")" + [[ -f /workspace/gitpod_start_time.sh ]] && printf "Took %.1f minutes from https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitpod.yml being executed through to completion %s\n" "$((10*(($(date +%s)-${START_TIME_IN_SECONDS}))/60))e-1" "$([[ -f "$PREBUILD_LOG" ]] && echo "With Prebuilds")" ) ports: @@ -111,10 +111,10 @@ ports: vscode: extensions: - - rebornix.ruby@0.28.0 - - wingrunr21.vscode-ruby@0.27.0 - - karunamurti.haml@1.3.1 - - octref.vetur@0.34.1 - - dbaeumer.vscode-eslint@2.1.8 - - gitlab.gitlab-workflow@3.24.0 - - DavidAnson.vscode-markdownlint@0.44.4 + - rebornix.ruby@0.28.1 + - wingrunr21.vscode-ruby@0.28.0 + - karunamurti.haml@1.4.1 + - octref.vetur@0.36.0 + - dbaeumer.vscode-eslint@2.2.6 + - GitLab.gitlab-workflow@3.48.1 + - DavidAnson.vscode-markdownlint@0.47.0 diff --git a/app/controllers/projects/autocomplete_sources_controller.rb b/app/controllers/projects/autocomplete_sources_controller.rb index 9dbf989ca3f..6ef84f1f5b9 100644 --- a/app/controllers/projects/autocomplete_sources_controller.rb +++ b/app/controllers/projects/autocomplete_sources_controller.rb @@ -51,9 +51,12 @@ class Projects::AutocompleteSourcesController < Projects::ApplicationController end def target + # type_id is not required in general + target_type = params.require(:type) + QuickActions::TargetService .new(project, current_user) - .execute(params[:type], params[:type_id]) + .execute(target_type, params[:type_id]) end def authorize_read_crm_contact! diff --git a/app/graphql/resolvers/base_issues_resolver.rb b/app/graphql/resolvers/base_issues_resolver.rb index ec47a8996eb..479b1df6876 100644 --- a/app/graphql/resolvers/base_issues_resolver.rb +++ b/app/graphql/resolvers/base_issues_resolver.rb @@ -49,6 +49,7 @@ module Resolvers alert_management_alert: [:alert_management_alert], labels: [:labels], assignees: [:assignees], + participants: Issue.participant_includes, timelogs: [:timelogs], customer_relations_contacts: { customer_relations_contacts: [:group] }, escalation_status: [:incident_management_issuable_escalation_status] diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index 5430575ace7..e9530a80d9f 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -73,4 +73,8 @@ class AwardEmoji < ApplicationRecord awardable.expire_etag_cache if awardable.is_a?(Note) awardable.try(:update_upvotes_count) if upvote? end + + def to_ability_name + 'emoji' + end end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 67afb81ee30..3a362f75c9c 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -231,7 +231,7 @@ module Issuable class_methods do def participant_includes - [:assignees, :author, { notes: [:author, :award_emoji] }] + [:author, :award_emoji, { notes: [:author, :award_emoji, :system_note_metadata] }] end # Searches for records with a matching title. @@ -641,6 +641,14 @@ module Issuable def draftless_title_changed(old_title) old_title != title end + + def read_ability_for(participable_source) + return super if participable_source == self + + name = participable_source.try(:issuable_ability_name) || :read_issuable_participables + + { name: name, subject: self } + end end Issuable.prepend_mod_with('Issuable') diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 8130adf05f1..6035cb87c9b 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -152,7 +152,9 @@ module Participable end def source_visible_to_user?(source, user) - Ability.allowed?(user, "read_#{source.model_name.element}".to_sym, source) + ability = read_ability_for(source) + + Ability.allowed?(user, ability[:name], ability[:subject]) end def filter_by_ability(participants) @@ -172,6 +174,14 @@ module Participable participant.can?(:read_project, project) end end + + # Returns Hash containing ability name and subject needed to read a specific participable. + # Should be overridden if a different ability is required. + def read_ability_for(participable_source) + name = participable_source.try(:to_ability_name) || participable_source.model_name.element + + { name: "read_#{name}".to_sym, subject: participable_source } + end end Participable.prepend_mod_with('Participable') diff --git a/app/models/issue.rb b/app/models/issue.rb index 9437a1a3363..470b7a3ba96 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -276,6 +276,10 @@ class Issue < ApplicationRecord end end + def self.participant_includes + [:assignees] + super + end + def next_object_by_relative_position(ignoring: nil, order: :asc) array_mapping_scope = -> (id_expression) do relation = Issue.where(Issue.arel_table[:project_id].eq(id_expression)) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7f8e0ebf935..067e9dc091d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -610,7 +610,7 @@ class MergeRequest < ApplicationRecord end def self.participant_includes - [:reviewers, :award_emoji] + super + [:assignees, :reviewers] + super end def committers diff --git a/app/models/note.rb b/app/models/note.rb index b3c5a7a0ef7..6375d3f777f 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -706,6 +706,10 @@ class Note < ApplicationRecord super.sub!('task', 'checklist item') end + def issuable_ability_name + confidential? ? :read_internal_note : :read_note + end + private def system_note_viewable_by?(user) diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index e864ce8752a..a412c97b219 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -58,6 +58,12 @@ class IssuablePolicy < BasePolicy rule { can_read_issuable }.policy do enable :read_issuable + enable :read_issuable_participables + end + + # This rule replicates permissions in NotePolicy#can_read_confidential + rule { can?(:reporter_access) | assignee_or_author | admin }.policy do + enable :read_internal_note end end diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 1bffcc5aea2..dbfc63a0069 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -20,7 +20,8 @@ class NotePolicy < BasePolicy condition(:confidential, scope: :subject) { @subject.confidential? } - # If this condition changes IssuablePolicy#read_confidential_notes should be updated too + # Should be matched with IssuablePolicy#read_internal_note + # and EpicPolicy#read_internal_note condition(:can_read_confidential) do access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user) || admin? end diff --git a/app/services/concerns/users/participable_service.rb b/app/services/concerns/users/participable_service.rb index c1c93aa604e..281b2508090 100644 --- a/app/services/concerns/users/participable_service.rb +++ b/app/services/concerns/users/participable_service.rb @@ -32,6 +32,8 @@ module Users end def groups + return [] unless current_user + current_user.authorized_groups.with_route.sort_by(&:path) end diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb index e6b1b33a82a..4b4981fde99 100644 --- a/app/services/projects/autocomplete_service.rb +++ b/app/services/projects/autocomplete_service.rb @@ -24,7 +24,7 @@ module Projects end def commands(noteable, type) - return [] unless noteable + return [] unless noteable && current_user QuickActions::InterpretService.new(project, current_user).available_commands(noteable) end diff --git a/config/feature_flags/development/container_registry_tags_list_default_page_size.yml b/config/feature_flags/development/container_registry_tags_list_default_page_size.yml deleted file mode 100644 index 9c656d6322d..00000000000 --- a/config/feature_flags/development/container_registry_tags_list_default_page_size.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: container_registry_tags_list_default_page_size -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98215 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/374073 -milestone: '15.5' -type: development -group: group::package -default_enabled: false diff --git a/doc/api/issues.md b/doc/api/issues.md index 4de2269493d..a0eb518f23e 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -65,7 +65,7 @@ GET /issues?state=opened | `created_before` | datetime | no | Return issues created on or before the given time. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`) | | `due_date` | string | no | Return issues that have no due date, are overdue, or whose due date is this week, this month, or between two weeks ago and next month. Accepts: `0` (no due date), `any`, `today`, `tomorrow`, `overdue`, `week`, `month`, `next_month_and_previous_two_weeks`. | | `epic_id` **(PREMIUM)** | integer | no | Return issues associated with the given epic ID. `None` returns issues that are not associated with an epic. `Any` returns issues that are associated with an epic. _([Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46887) in GitLab 13.6)_ -| `health_status` **(ULTIMATE)** | string | no | Return issues with the specified `health_status`. _([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370721) in GitLab 15.4)_ +| `health_status` **(ULTIMATE)** | string | no | Return issues with the specified `health_status`. _([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370721) in GitLab 15.4)._ In [GitLab 15.5 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/370721), `None` returns issues with no health status assigned, and `Any` returns issues with a health status assigned. | `iids[]` | integer array | no | Return only the issues having the given `iid` | | `in` | string | no | Modify the scope of the `search` attribute. `title`, `description`, or a string joining them with comma. Default is `title,description` | | `issue_type` | string | no | Filter to a given type of issue. One of `issue`, `incident`, or `test_case`. _([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/260375) in GitLab 13.12)_ | diff --git a/doc/api/version.md b/doc/api/version.md index 6de741fcb4f..0ceab92b9d6 100644 --- a/doc/api/version.md +++ b/doc/api/version.md @@ -9,8 +9,9 @@ info: To determine the technical writer assigned to the Stage/Group associated w NOTE: We recommend you use the [Metadata API](metadata.md) instead of the Version API. It contains additional information and is aligned with the GraphQL metadata endpoint. +As of GitLab 15.5, the Version API is a mirror of the Metadata API. -Retrieve version information for this GitLab instance. Responds `200 OK` for +Retrieves version information for the GitLab instance. Responds with `200 OK` for authenticated users. ```plaintext @@ -21,7 +22,13 @@ GET /version curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/version" ``` -Example response: +## Example responses + +### GitLab 15.5 and later + +See [Metadata API](metadata.md) for the response. + +### GitLab 15.4 and earlier ```json { diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index e6ce62a1c6e..74f6515f07f 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -12,7 +12,8 @@ module API %w[group project].each do |source_type| params do - requires :id, type: String, desc: "The #{source_type} ID" + requires :id, type: String, + desc: "The ID or URL-encoded path of the #{source_type} owned by the authenticated user" end resource source_type.pluralize, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do desc "Gets a list of access requests for a #{source_type}." do @@ -54,7 +55,8 @@ module API end params do requires :user_id, type: Integer, desc: 'The user ID of the access requester' - optional :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)' + optional :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, the Developer role)', + default: 30 end # rubocop: disable CodeReuse/ActiveRecord put ':id/access_requests/:user_id/approve' do diff --git a/lib/api/api.rb b/lib/api/api.rb index 58a5b4ea9ef..28d9bddf81c 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -316,7 +316,6 @@ module API mount ::API::UsageDataQueries mount ::API::UserCounts mount ::API::Users - mount ::API::Version mount ::API::Wikis mount ::API::Ml::Mlflow end diff --git a/lib/api/ci/resource_groups.rb b/lib/api/ci/resource_groups.rb index e3fd887475a..ea6d3cc8fd4 100644 --- a/lib/api/ci/resource_groups.rb +++ b/lib/api/ci/resource_groups.rb @@ -38,6 +38,25 @@ module API present resource_group, with: Entities::Ci::ResourceGroup end + desc 'List upcoming jobs of a resource group' do + success Entities::Ci::JobBasic + end + params do + requires :key, type: String, desc: 'The key of the resource group' + + use :pagination + end + get ':id/resource_groups/:key/upcoming_jobs' do + authorize! :read_resource_group, resource_group + authorize! :read_build, user_project + + upcoming_processables = resource_group + .upcoming_processables + .preload(:user, pipeline: :project) # rubocop:disable CodeReuse/ActiveRecord + + present paginate(upcoming_processables), with: Entities::Ci::JobBasic + end + desc 'Edit a resource group' do success Entities::Ci::ResourceGroup end diff --git a/lib/api/metadata.rb b/lib/api/metadata.rb index c4984f0e7f0..747082430a9 100644 --- a/lib/api/metadata.rb +++ b/lib/api/metadata.rb @@ -25,15 +25,31 @@ module API } EOF + helpers do + def run_metadata_query + run_graphql!( + query: METADATA_QUERY, + context: { current_user: current_user }, + transform: ->(result) { result.dig('data', 'metadata') } + ) + end + end + desc 'Get the metadata information of the GitLab instance.' do detail 'This feature was introduced in GitLab 15.2.' end get '/metadata' do - run_graphql!( - query: METADATA_QUERY, - context: { current_user: current_user }, - transform: ->(result) { result.dig('data', 'metadata') } - ) + run_metadata_query + end + + # Support the deprecated `/version` route. + # See https://gitlab.com/gitlab-org/gitlab/-/issues/366287 + desc 'Get the version information of the GitLab instance.' do + detail 'This feature was introduced in GitLab 8.13 and deprecated in 15.5. ' \ + 'We recommend you instead use the Metadata API.' + end + get '/version' do + run_metadata_query end end end diff --git a/lib/api/version.rb b/lib/api/version.rb deleted file mode 100644 index bdce88ab827..00000000000 --- a/lib/api/version.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -module API - class Version < ::API::Base - helpers ::API::Helpers::GraphqlHelpers - include APIGuard - - allow_access_with_scope :read_user, if: -> (request) { request.get? || request.head? } - - before { authenticate! } - - feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned - - METADATA_QUERY = <<~EOF - { - metadata { - version - revision - } - } - EOF - - desc 'Get the version information of the GitLab instance.' do - detail 'This feature was introduced in GitLab 8.13.' - end - get '/version' do - run_graphql!( - query: METADATA_QUERY, - context: { current_user: current_user }, - transform: ->(result) { result.dig('data', 'metadata') } - ) - end - end -end diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb index 435029cd07e..723935f8aaf 100644 --- a/lib/container_registry/client.rb +++ b/lib/container_registry/client.rb @@ -56,7 +56,7 @@ module ContainerRegistry def repository_tags(name, page_size: DEFAULT_TAGS_PAGE_SIZE) response = faraday.get("/v2/#{name}/tags/list") do |req| - req.params['n'] = page_size if Feature.enabled?(:container_registry_tags_list_default_page_size) + req.params['n'] = page_size end response_body(response) end diff --git a/spec/controllers/projects/autocomplete_sources_controller_spec.rb b/spec/controllers/projects/autocomplete_sources_controller_spec.rb index a5274b6543e..01eef508d12 100644 --- a/spec/controllers/projects/autocomplete_sources_controller_spec.rb +++ b/spec/controllers/projects/autocomplete_sources_controller_spec.rb @@ -5,37 +5,133 @@ require 'spec_helper' RSpec.describe Projects::AutocompleteSourcesController do let_it_be(:group, reload: true) { create(:group) } let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:public_project) { create(:project, :public, group: group) } + let_it_be(:development) { create(:label, project: project, name: 'Development') } + let_it_be(:issue) { create(:labeled_issue, project: project, labels: [development]) } let_it_be(:user) { create(:user) } def members_by_username(username) json_response.find { |member| member['username'] == username } end - describe 'GET members' do + describe 'GET commands' do + before do + group.add_owner(user) + end + + context 'with a public project' do + shared_examples 'issuable commands' do + it 'returns empty array when no user logged in' do + get :commands, format: :json, params: { namespace_id: group.path, project_id: public_project.path, type: issuable_type, type_id: issuable_iid } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq([]) + end + + it 'raises an error when no target type specified' do + sign_in(user) + + expect { get :commands, format: :json, params: { namespace_id: group.path, project_id: project.path } } + .to raise_error(ActionController::ParameterMissing) + end + + it 'returns an array of commands' do + sign_in(user) + + get :commands, format: :json, params: { namespace_id: group.path, project_id: public_project.path, type: issuable_type, type_id: issuable_iid } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_present + end + end + + context 'with an issue' do + let(:issuable_type) { issue.class.name } + let(:issuable_iid) { issue.iid } + + it_behaves_like 'issuable commands' + end + + context 'with merge request' do + let(:merge_request) { create(:merge_request, target_project: public_project, source_project: public_project) } + let(:issuable_type) { merge_request.class.name } + let(:issuable_iid) { merge_request.iid } + + it_behaves_like 'issuable commands' + end + end + end + + describe 'GET labels' do before do group.add_owner(user) sign_in(user) end - it 'returns an array of member object' do - get :members, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } + it 'raises an error when no target type specified' do + expect { get :labels, format: :json, params: { namespace_id: group.path, project_id: project.path } } + .to raise_error(ActionController::ParameterMissing) + end - expect(members_by_username('all').symbolize_keys).to include( - username: 'all', - name: 'All Project and Group Members', - count: 1) + it 'returns an array of labels' do + get :labels, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } - expect(members_by_username(group.full_path).symbolize_keys).to include( - type: group.class.name, - name: group.full_name, - avatar_url: group.avatar_url, - count: 1) + expect(json_response).to be_a(Array) + expect(json_response.count).to eq(1) + expect(json_response[0]['title']).to eq('Development') + end + end - expect(members_by_username(user.username).symbolize_keys).to include( - type: user.class.name, - name: user.name, - avatar_url: user.avatar_url) + describe 'GET members' do + context 'when logged in' do + before do + group.add_owner(user) + sign_in(user) + end + + it 'returns 400 when no target type specified' do + expect { get :members, format: :json, params: { namespace_id: group.path, project_id: project.path } } + .to raise_error(ActionController::ParameterMissing) + end + + it 'returns an array of member object' do + get :members, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } + + expect(members_by_username('all').symbolize_keys).to include( + username: 'all', + name: 'All Project and Group Members', + count: 1) + + expect(members_by_username(group.full_path).symbolize_keys).to include( + type: group.class.name, + name: group.full_name, + avatar_url: group.avatar_url, + count: 1) + + expect(members_by_username(user.username).symbolize_keys).to include( + type: user.class.name, + name: user.name, + avatar_url: user.avatar_url) + end + end + + context 'when anonymous' do + it 'redirects to login page' do + get :members, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } + + expect(response).to redirect_to new_user_session_path + end + + context 'with public project' do + it 'returns no members' do + get :members, format: :json, params: { namespace_id: group.path, project_id: public_project.path, type: issue.class.name, type_id: issue.id } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_a(Array) + expect(json_response.count).to eq(1) + expect(json_response.first['count']).to eq(0) + end + end end end diff --git a/spec/graphql/resolvers/users/participants_resolver_spec.rb b/spec/graphql/resolvers/users/participants_resolver_spec.rb index 3f04d157410..eb2418b63f4 100644 --- a/spec/graphql/resolvers/users/participants_resolver_spec.rb +++ b/spec/graphql/resolvers/users/participants_resolver_spec.rb @@ -10,18 +10,31 @@ RSpec.describe Resolvers::Users::ParticipantsResolver do let_it_be(:guest) { create(:user) } let_it_be(:project) { create(:project, :public) } let_it_be(:issue) { create(:issue, project: project) } - let_it_be(:note) do - create( - :note, - :system, - :confidential, - project: project, - noteable: issue, - author: create(:user) - ) - end - let_it_be(:note_metadata) { create(:system_note_metadata, note: note) } + let_it_be(:public_note_author) { create(:user) } + let_it_be(:public_reply_author) { create(:user) } + let_it_be(:internal_note_author) { create(:user) } + let_it_be(:internal_reply_author) { create(:user) } + + let_it_be(:public_note) { create(:note, project: project, noteable: issue, author: public_note_author) } + let_it_be(:internal_note) { create(:note, :confidential, project: project, noteable: issue, author: internal_note_author) } + + let_it_be(:public_reply) { create(:note, noteable: issue, in_reply_to: public_note, project: project, author: public_reply_author) } + let_it_be(:internal_reply) { create(:note, :confidential, noteable: issue, in_reply_to: internal_note, project: project, author: internal_reply_author) } + + let_it_be(:note_metadata2) { create(:system_note_metadata, note: public_note) } + + let_it_be(:issue_emoji) { create(:award_emoji, name: 'thumbsup', awardable: issue) } + let_it_be(:note_emoji1) { create(:award_emoji, name: 'thumbsup', awardable: public_note) } + let_it_be(:note_emoji2) { create(:award_emoji, name: 'thumbsup', awardable: internal_note) } + let_it_be(:note_emoji3) { create(:award_emoji, name: 'thumbsup', awardable: public_reply) } + let_it_be(:note_emoji4) { create(:award_emoji, name: 'thumbsup', awardable: internal_reply) } + + let_it_be(:issue_emoji_author) { issue_emoji.user } + let_it_be(:public_note_emoji_author) { note_emoji1.user } + let_it_be(:internal_note_emoji_author) { note_emoji2.user } + let_it_be(:public_reply_emoji_author) { note_emoji3.user } + let_it_be(:internal_reply_emoji_author) { note_emoji4.user } subject(:resolved_items) { resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items } @@ -34,7 +47,16 @@ RSpec.describe Resolvers::Users::ParticipantsResolver do let(:current_user) { nil } it 'returns only publicly visible participants for this user' do - is_expected.to match_array([issue.author]) + is_expected.to match_array( + [ + issue.author, + issue_emoji_author, + public_note_author, + public_note_emoji_author, + public_reply_author, + public_reply_emoji_author + ] + ) end end @@ -42,15 +64,37 @@ RSpec.describe Resolvers::Users::ParticipantsResolver do let(:current_user) { guest } it 'returns only publicly visible participants for this user' do - is_expected.to match_array([issue.author]) + is_expected.to match_array( + [ + issue.author, + issue_emoji_author, + public_note_author, + public_note_emoji_author, + public_reply_author, + public_reply_emoji_author + ] + ) end end - context 'when current user has access to confidential notes' do + context 'when current user has access to internal notes' do let(:current_user) { user } it 'returns all participants for this user' do - is_expected.to match_array([issue.author, note.author]) + is_expected.to match_array( + [ + issue.author, + issue_emoji_author, + public_note_author, + public_note_emoji_author, + public_reply_author, + internal_note_author, + internal_note_emoji_author, + internal_reply_author, + public_reply_emoji_author, + internal_reply_emoji_author + ] + ) end context 'N+1 queries' do @@ -64,9 +108,14 @@ RSpec.describe Resolvers::Users::ParticipantsResolver do it 'does not execute N+1 for project relation' do control_count = ActiveRecord::QueryRecorder.new { query.call } - create(:note, :confidential, project: project, noteable: issue, author: create(:user)) + create(:award_emoji, :upvote, awardable: issue) + internal_note = create(:note, :confidential, project: project, noteable: issue, author: create(:user)) + create(:award_emoji, name: 'thumbsup', awardable: internal_note) + public_note = create(:note, project: project, noteable: issue, author: create(:user)) + create(:award_emoji, name: 'thumbsup', awardable: public_note) - expect { query.call }.not_to exceed_query_limit(control_count) + # 1 extra query per source (3 emojis + 2 notes) to fetch participables collection + expect { query.call }.not_to exceed_query_limit(control_count).with_threshold(5) end it 'does not execute N+1 for system note metadata relation' do diff --git a/spec/lib/container_registry/client_spec.rb b/spec/lib/container_registry/client_spec.rb index 61d8d56d08b..cb2da24b712 100644 --- a/spec/lib/container_registry/client_spec.rb +++ b/spec/lib/container_registry/client_spec.rb @@ -437,18 +437,6 @@ RSpec.describe ContainerRegistry::Client do expect(subject).to eq('tags' => %w[t1 t2]) end - - context 'with container_registry_tags_list_default_page_size disabled' do - before do - stub_feature_flags(container_registry_tags_list_default_page_size: false) - end - - it 'returns a successful response' do - stub_registry_tags_list(tags: %w[t1 t2]) - - expect(subject).to eq('tags' => %w[t1 t2]) - end - end end describe '.registry_info' do diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index 4da19267b1c..2593c9b3595 100644 --- a/spec/models/award_emoji_spec.rb +++ b/spec/models/award_emoji_spec.rb @@ -290,4 +290,13 @@ RSpec.describe AwardEmoji do end end end + + describe '#to_ability_name' do + let(:merge_request) { create(:merge_request) } + let(:award_emoji) { build(:award_emoji, user: build(:user), awardable: merge_request) } + + it 'returns correct ability name' do + expect(award_emoji.to_ability_name).to be('emoji') + end + end end diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb index f7f68cb38d8..58a44fec3aa 100644 --- a/spec/models/concerns/participable_spec.rb +++ b/spec/models/concerns/participable_spec.rb @@ -186,6 +186,9 @@ RSpec.describe Participable do expect(instance.visible_participants(user1)).to match_array [user1, user2] end end + + it_behaves_like 'visible participants for issuable with read ability', :issue + it_behaves_like 'visible participants for issuable with read ability', :merge_request end describe '#participant?' do diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index c02294571ff..9a862db992f 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -31,8 +31,8 @@ RSpec.describe IssuablePolicy, models: true do expect(policies).to be_allowed(:resolve_note) end - it 'allows reading confidential notes' do - expect(policies).to be_allowed(:read_confidential_notes) + it 'allows reading internal notes' do + expect(policies).to be_allowed(:read_internal_note) end context 'when user is able to read project' do @@ -94,8 +94,8 @@ RSpec.describe IssuablePolicy, models: true do let(:issue) { create(:issue, project: project, assignees: [user]) } let(:policies) { described_class.new(user, issue) } - it 'allows reading confidential notes' do - expect(policies).to be_allowed(:read_confidential_notes) + it 'allows reading internal notes' do + expect(policies).to be_allowed(:read_internal_note) end end @@ -145,6 +145,10 @@ RSpec.describe IssuablePolicy, models: true do it 'does not allow timelogs creation' do expect(policies).to be_disallowed(:create_timelog) end + + it 'does not allow reading internal notes' do + expect(permissions(guest, issue)).to be_disallowed(:read_internal_note) + end end context 'when user is a guest member of the project' do @@ -170,8 +174,8 @@ RSpec.describe IssuablePolicy, models: true do expect(permissions(reporter, issue)).to be_allowed(:create_timelog) end - it 'allows reading confidential notes' do - expect(permissions(reporter, issue)).to be_allowed(:read_confidential_notes) + it 'allows reading internal notes' do + expect(permissions(reporter, issue)).to be_allowed(:read_internal_note) end end @@ -188,6 +192,7 @@ RSpec.describe IssuablePolicy, models: true do it 'does not allow :read_issuable' do expect(policy).not_to be_allowed(:read_issuable) + expect(policy).not_to be_allowed(:read_issuable_participables) end end @@ -196,6 +201,7 @@ RSpec.describe IssuablePolicy, models: true do it 'allows :read_issuable' do expect(policy).to be_allowed(:read_issuable) + expect(policy).to be_allowed(:read_issuable_participables) end end end @@ -213,6 +219,7 @@ RSpec.describe IssuablePolicy, models: true do it 'does not allow :read_issuable' do expect(policy).not_to be_allowed(:read_issuable) + expect(policy).not_to be_allowed(:read_issuable_participables) end end @@ -221,6 +228,7 @@ RSpec.describe IssuablePolicy, models: true do it 'allows :read_issuable' do expect(policy).to be_allowed(:read_issuable) + expect(policy).to be_allowed(:read_issuable_participables) end end end diff --git a/spec/requests/api/ci/resource_groups_spec.rb b/spec/requests/api/ci/resource_groups_spec.rb index 864c363e6d3..87df71f6096 100644 --- a/spec/requests/api/ci/resource_groups_spec.rb +++ b/spec/requests/api/ci/resource_groups_spec.rb @@ -77,6 +77,48 @@ RSpec.describe API::Ci::ResourceGroups do end end + describe 'GET /projects/:id/resource_groups/:key/upcoming_jobs' do + subject { get api("/projects/#{project.id}/resource_groups/#{key}/upcoming_jobs", user) } + + let_it_be(:resource_group) { create(:ci_resource_group, project: project) } + let_it_be(:processable) { create(:ci_processable, resource_group: resource_group) } + let_it_be(:upcoming_processable) { create(:ci_processable, :waiting_for_resource, resource_group: resource_group) } + + let(:key) { resource_group.key } + + it 'returns upcoming jobs of resource group', :aggregate_failures do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.length).to eq(1) + expect(json_response[0]['id']).to eq(upcoming_processable.id) + expect(json_response[0]['name']).to eq(upcoming_processable.name) + expect(json_response[0]['ref']).to eq(upcoming_processable.ref) + expect(json_response[0]['stage']).to eq(upcoming_processable.stage) + expect(json_response[0]['status']).to eq(upcoming_processable.status) + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns forbidden' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'when there is no corresponding resource group' do + let(:key) { 'unknown' } + + it 'returns not found' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + describe 'PUT /projects/:id/resource_groups/:key' do subject { put api("/projects/#{project.id}/resource_groups/#{key}", user), params: params } diff --git a/spec/requests/api/graphql/project/issues_spec.rb b/spec/requests/api/graphql/project/issues_spec.rb index fe885c3cea7..163f9d02a3b 100644 --- a/spec/requests/api/graphql/project/issues_spec.rb +++ b/spec/requests/api/graphql/project/issues_spec.rb @@ -686,6 +686,30 @@ RSpec.describe 'getting an issue list for a project' do include_examples 'N+1 query check' end + + context 'when requesting participants' do + let_it_be(:issue_c) { create(:issue, project: project) } + + let(:search_params) { { iids: [issue_a.iid.to_s, issue_c.iid.to_s] } } + let(:requested_fields) { 'participants { nodes { name } }' } + + before do + create(:award_emoji, :upvote, awardable: issue_a) + create(:award_emoji, :upvote, awardable: issue_b) + create(:award_emoji, :upvote, awardable: issue_c) + + note_with_emoji_a = create(:note_on_issue, noteable: issue_a, project: project) + note_with_emoji_b = create(:note_on_issue, noteable: issue_b, project: project) + note_with_emoji_c = create(:note_on_issue, noteable: issue_c, project: project) + + create(:award_emoji, :upvote, awardable: note_with_emoji_a) + create(:award_emoji, :upvote, awardable: note_with_emoji_b) + create(:award_emoji, :upvote, awardable: note_with_emoji_c) + end + + # Executes 3 extra queries to fetch participant_attrs + include_examples 'N+1 query check', 3 + end end def issues_ids diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index 76660d510b4..ef83f9be8ac 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -338,6 +338,27 @@ RSpec.describe 'getting merge request listings nested in a project' do include_examples 'N+1 query check' end + + context 'when requesting participants' do + let(:requested_fields) { 'participants { nodes { name } }' } + + before do + create(:award_emoji, :upvote, awardable: merge_request_a) + create(:award_emoji, :upvote, awardable: merge_request_b) + create(:award_emoji, :upvote, awardable: merge_request_c) + + note_with_emoji_a = create(:note_on_merge_request, noteable: merge_request_a, project: project) + note_with_emoji_b = create(:note_on_merge_request, noteable: merge_request_b, project: project) + note_with_emoji_c = create(:note_on_merge_request, noteable: merge_request_c, project: project) + + create(:award_emoji, :upvote, awardable: note_with_emoji_a) + create(:award_emoji, :upvote, awardable: note_with_emoji_b) + create(:award_emoji, :upvote, awardable: note_with_emoji_c) + end + + # Executes 3 extra queries to fetch participant_attrs + include_examples 'N+1 query check', 3 + end end describe 'performance' do diff --git a/spec/requests/api/metadata_spec.rb b/spec/requests/api/metadata_spec.rb index dbca06b7f3e..5b6407c689b 100644 --- a/spec/requests/api/metadata_spec.rb +++ b/spec/requests/api/metadata_spec.rb @@ -6,7 +6,7 @@ RSpec.describe API::Metadata do shared_examples_for 'GET /metadata' do context 'when unauthenticated' do it 'returns authentication error' do - get api('/metadata') + get api(endpoint) expect(response).to have_gitlab_http_status(:unauthorized) end @@ -16,7 +16,7 @@ RSpec.describe API::Metadata do let(:user) { create(:user) } it 'returns the metadata information' do - get api('/metadata', user) + get api(endpoint, user) expect_metadata end @@ -29,13 +29,13 @@ RSpec.describe API::Metadata do let(:scopes) { %i(api) } it 'returns the metadata information' do - get api('/metadata', personal_access_token: personal_access_token) + get api(endpoint, personal_access_token: personal_access_token) expect_metadata end it 'returns "200" response on head requests' do - head api('/metadata', personal_access_token: personal_access_token) + head api(endpoint, personal_access_token: personal_access_token) expect(response).to have_gitlab_http_status(:ok) end @@ -45,13 +45,13 @@ RSpec.describe API::Metadata do let(:scopes) { %i(read_user) } it 'returns the metadata information' do - get api('/metadata', personal_access_token: personal_access_token) + get api(endpoint, personal_access_token: personal_access_token) expect_metadata end it 'returns "200" response on head requests' do - head api('/metadata', personal_access_token: personal_access_token) + head api(endpoint, personal_access_token: personal_access_token) expect(response).to have_gitlab_http_status(:ok) end @@ -61,7 +61,7 @@ RSpec.describe API::Metadata do let(:scopes) { %i(read_repository) } it 'returns authorization error' do - get api('/metadata', personal_access_token: personal_access_token) + get api(endpoint, personal_access_token: personal_access_token) expect(response).to have_gitlab_http_status(:forbidden) end @@ -76,18 +76,14 @@ RSpec.describe API::Metadata do end end - context 'with graphql enabled' do - before do - stub_feature_flags(graphql: true) - end + describe 'GET /metadata' do + let(:endpoint) { '/metadata' } include_examples 'GET /metadata' end - context 'with graphql disabled' do - before do - stub_feature_flags(graphql: false) - end + describe 'GET /version' do + let(:endpoint) { '/version' } include_examples 'GET /metadata' end diff --git a/spec/requests/api/version_spec.rb b/spec/requests/api/version_spec.rb deleted file mode 100644 index 7abbaf4f9ec..00000000000 --- a/spec/requests/api/version_spec.rb +++ /dev/null @@ -1,93 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe API::Version do - shared_examples_for 'GET /version' do - context 'when unauthenticated' do - it 'returns authentication error' do - get api('/version') - - expect(response).to have_gitlab_http_status(:unauthorized) - end - end - - context 'when authenticated as user' do - let(:user) { create(:user) } - - it 'returns the version information' do - get api('/version', user) - - expect_version - end - end - - context 'when authenticated with token' do - let(:personal_access_token) { create(:personal_access_token, scopes: scopes) } - - context 'with api scope' do - let(:scopes) { %i(api) } - - it 'returns the version information' do - get api('/version', personal_access_token: personal_access_token) - - expect_version - end - - it 'returns "200" response on head requests' do - head api('/version', personal_access_token: personal_access_token) - - expect(response).to have_gitlab_http_status(:ok) - end - end - - context 'with read_user scope' do - let(:scopes) { %i(read_user) } - - it 'returns the version information' do - get api('/version', personal_access_token: personal_access_token) - - expect_version - end - - it 'returns "200" response on head requests' do - head api('/version', personal_access_token: personal_access_token) - - expect(response).to have_gitlab_http_status(:ok) - end - end - - context 'with neither api nor read_user scope' do - let(:scopes) { %i(read_repository) } - - it 'returns authorization error' do - get api('/version', personal_access_token: personal_access_token) - - expect(response).to have_gitlab_http_status(:forbidden) - end - end - end - - def expect_version - expect(response).to have_gitlab_http_status(:ok) - expect(json_response['version']).to eq(Gitlab::VERSION) - expect(json_response['revision']).to eq(Gitlab.revision) - end - end - - context 'with graphql enabled' do - before do - stub_feature_flags(graphql: true) - end - - include_examples 'GET /version' - end - - context 'with graphql disabled' do - before do - stub_feature_flags(graphql: false) - end - - include_examples 'GET /version' - end -end diff --git a/spec/support/shared_examples/graphql/n_plus_one_query_examples.rb b/spec/support/shared_examples/graphql/n_plus_one_query_examples.rb index faf1bb204c9..ea14c465872 100644 --- a/spec/support/shared_examples/graphql/n_plus_one_query_examples.rb +++ b/spec/support/shared_examples/graphql/n_plus_one_query_examples.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -RSpec.shared_examples 'N+1 query check' do +RSpec.shared_examples 'N+1 query check' do |threshold = 0| it 'prevents N+1 queries' do execute_query # "warm up" to prevent undeterministic counts expect(graphql_errors).to be_blank # Sanity check - ex falso quodlibet! @@ -8,6 +8,7 @@ RSpec.shared_examples 'N+1 query check' do expect(control.count).to be > 0 search_params[:iids] << extra_iid_for_second_query - expect { execute_query }.not_to exceed_query_limit(control) + + expect { execute_query }.not_to exceed_query_limit(control).with_threshold(threshold) end end diff --git a/spec/support/shared_examples/models/concerns/participable_shared_examples.rb b/spec/support/shared_examples/models/concerns/participable_shared_examples.rb new file mode 100644 index 00000000000..ec7a9105bb2 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/participable_shared_examples.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'visible participants for issuable with read ability' do |model_class| + let_it_be(:user1) { create(:user) } + + let(:model) { model_class.to_s.classify.constantize } + + before do + allow(Ability).to receive(:allowed?).with(anything, :"read_#{model_class}", anything).and_return(true) + allow(model).to receive(:participant_attrs).and_return([:bar]) + end + + shared_examples 'check for participables read ability' do |ability_name| + it 'receives expected ability' do + instance = model.new + + allow(instance).to receive(:bar).and_return(participable_source) + + expect(Ability).to receive(:allowed?).with(anything, ability_name, instance) + + expect(instance.visible_participants(user1)).to be_empty + end + end + + context 'when source is an award emoji' do + let(:participable_source) { build(:award_emoji, :upvote) } + + it_behaves_like 'check for participables read ability', :read_issuable_participables + end + + context 'when source is a note' do + let(:participable_source) { build(:note) } + + it_behaves_like 'check for participables read ability', :read_note + end + + context 'when source is an internal note' do + let(:participable_source) { build(:note, :confidential) } + + it_behaves_like 'check for participables read ability', :read_internal_note + end +end