diff --git a/doc/user/group/epics/manage_epics.md b/doc/user/group/epics/manage_epics.md index 61aa5c5fe0266421d1a17eccd7dd77eca14eb04d..8e7b6fd82ad5b47433a569e69d67ccaec3c35657 100644 --- a/doc/user/group/epics/manage_epics.md +++ b/doc/user/group/epics/manage_epics.md @@ -494,7 +494,8 @@ The maximum number of direct child epics is 100. ### Child epics from other groups -> [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/8502) in GitLab 15.6 [with a flag](../../../administration/feature_flags.md) named `child_epics_from_different_hierarchies`. Disabled by default. +> - [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/8502) in GitLab 15.6 [with a flag](../../../administration/feature_flags.md) named `child_epics_from_different_hierarchies`. Disabled by default. +> - Minimum required role for the group [changed](https://gitlab.com/gitlab-org/gitlab/-/issues/382503) from Reporter to Guest in GitLab 15.7. FLAG: On self-managed GitLab, by default this feature is not available. To make it available per group, ask an administrator to [enable the feature flag](../../../administration/feature_flags.md) named `child_epics_from_different_hierarchies`. @@ -504,16 +505,18 @@ You can add a child epic that belongs to a group that is different from the pare Prerequisites: -- You must have at least the Reporter role for both the child and parent epics' groups. +- You must have at least the Guest role for both the child and parent epics' groups. - Multi-level child epics must be available for both the child and parent epics' groups. To add a child epic from another group, paste the epic's URL when [adding an existing epic](#add-a-child-epic-to-an-epic). ### Add a child epic to an epic +> Minimum required role for the group [changed](https://gitlab.com/gitlab-org/gitlab/-/issues/382503) from Reporter to Guest in GitLab 15.7. + Prerequisites: -- You must have at least the Reporter role for the parent epic's group. +- You must have at least the Guest role for the parent epic's group. To add a new epic as child epic: @@ -534,7 +537,8 @@ To add an existing epic as child epic: ### Move child epics between epics -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/33039) in GitLab 13.0. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/33039) in GitLab 13.0. +> - Minimum required role for the group [changed](https://gitlab.com/gitlab-org/gitlab/-/issues/382503) from Reporter to Guest in GitLab 15.7. New child epics appear at the top of the list in the **Epics and Issues** tab. You can move child epics from one epic to another. @@ -543,7 +547,7 @@ Issues and child epics cannot be intermingled. Prerequisites: -- You must have at least the Reporter role for the parent epic's group. +- You must have at least the Guest role for the parent epic's group. To move child epics to another epic: @@ -552,14 +556,15 @@ To move child epics to another epic: ### Reorder child epics assigned to an epic -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/9367) in GitLab 12.5. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/9367) in GitLab 12.5. +> - Minimum required role for the group [changed](https://gitlab.com/gitlab-org/gitlab/-/issues/382503) from Reporter to Guest in GitLab 15.7. New child epics appear at the top of the list in the **Epics and Issues** tab. You can reorder the list of child epics. Prerequisites: -- You must have at least the Reporter role for the parent epic's group. +- You must have at least the Guest role for the parent epic's group. To reorder child epics assigned to an epic: @@ -568,9 +573,11 @@ To reorder child epics assigned to an epic: ### Remove a child epic from a parent epic +> Minimum required role for the group [changed](https://gitlab.com/gitlab-org/gitlab/-/issues/382503) from Reporter to Guest in GitLab 15.7. + Prerequisites: -- You must have at least the Reporter role for the parent epic's group. +- You must have at least the Guest role for the parent epic's group. To remove a child epic from a parent epic: diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 5f56be66d5416c0a5f7485efade5c775451f5fd6..f75c2e9dc7299ab2e8dc112ae2b914eaddffeeb7 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -358,6 +358,7 @@ The following table lists group permissions available for each role: | Action | Guest | Reporter | Developer | Maintainer | Owner | |-----------------------------------------------------------------------------------------|-------|----------|-----------|------------|-------| +| Add/remove [child epics](group/epics/manage_epics.md#multi-level-child-epics) | ✓ (8) | ✓ | ✓ | ✓ | ✓ | | Add an issue to an [epic](group/epics/index.md) | ✓ (7) | ✓ (7) | ✓ (7) | ✓ (7) | ✓ (7) | | Browse group | ✓ | ✓ | ✓ | ✓ | ✓ | | Pull a container image using the dependency proxy | ✓ | ✓ | ✓ | ✓ | ✓ | @@ -427,6 +428,7 @@ The following table lists group permissions available for each role: 5. In addition, if your group is public or internal, all users who can see the group can also see group wiki pages. 6. Users can only view events based on their individual actions. 7. You must have permission to [view the epic](group/epics/manage_epics.md#who-can-view-an-epic) and edit the issue. +8. You must have permission to [view](group/epics/manage_epics.md#who-can-view-an-epic) the parent and child epics. diff --git a/ee/app/controllers/groups/epics/epic_links_controller.rb b/ee/app/controllers/groups/epics/epic_links_controller.rb index f2a0becac46bda7be110c5c073e717dc5241ad66..66bb41f4797b6a1ca91c4557ebb1fe4be47a1e5e 100644 --- a/ee/app/controllers/groups/epics/epic_links_controller.rb +++ b/ee/app/controllers/groups/epics/epic_links_controller.rb @@ -28,7 +28,7 @@ def destroy def authorize_admin! return super unless action_name == 'destroy' - render_403 unless can?(current_user, 'destroy_epic_link', epic) + render_403 unless can?(current_user, 'admin_epic_relation', epic) end def create_service @@ -44,7 +44,7 @@ def child_epic end def authorized_object - 'epic_link' + 'epic_tree_relation' end end end diff --git a/ee/app/graphql/mutations/epic_tree/reorder.rb b/ee/app/graphql/mutations/epic_tree/reorder.rb index a7cd5507db161a6a06dd6b7f92941f20bce5905f..ff42a3b935404bf50e6c9506880c27877b16a293 100644 --- a/ee/app/graphql/mutations/epic_tree/reorder.rb +++ b/ee/app/graphql/mutations/epic_tree/reorder.rb @@ -5,7 +5,7 @@ module EpicTree class Reorder < ::Mutations::BaseMutation graphql_name "EpicTreeReorder" - authorize :admin_epic + authorize :admin_epic_relation argument :base_epic_id, ::Types::GlobalIDType[::Epic], diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index dd2d5f9648f70054125cad7e5aec3d49b3910b81..6645fa0bbe22fe50572d9b4d170935a25bdf667d 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -323,15 +323,10 @@ module GroupPolicy enable :admin_epic enable :update_epic enable :read_confidential_epic - enable :destroy_epic_link enable :admin_epic_board enable :admin_epic_board_list end - rule { reporter & subepics_available }.policy do - enable :admin_epic_link - end - rule { owner & epics_available }.enable :destroy_epic rule { ~can?(:read_cross_project) }.policy do diff --git a/ee/app/policies/epic_policy.rb b/ee/app/policies/epic_policy.rb index 8e532b6466e14e0bb82d68dbf389cd7acb93f515..e1993b730e41fa63ed091f4a685889e464052c22 100644 --- a/ee/app/policies/epic_policy.rb +++ b/ee/app/policies/epic_policy.rb @@ -18,6 +18,10 @@ class EpicPolicy < BasePolicy @subject.group.licensed_feature_available?(:related_epics) end + condition(:subepics_available, scope: :subject) do + @subject.group.licensed_feature_available?(:subepics) + end + rule { can?(:read_epic) }.policy do enable :read_epic_iid enable :read_note @@ -43,6 +47,25 @@ class EpicPolicy < BasePolicy prevent :read_note end + # Checking for guest access is important as in public groups non-members can read epics, + # but should not be able to alter epic tree or related epics relationship. User has to + # have at least a Guest role for that. + rule { can?(:guest_access) & can?(:read_epic) }.policy do + # This generic permission means that a user is able to create, read, destroy an epic + # relationship, but it needs some extra checks depending on the feature: + # 1. To add an issue to the epic tree this permission is enough + # 2. To add a sub-epic or link a parent epic we also need to check that sub-epics feature + # feature is available, i.e. `subepics_available` + # 3. To add a related epic we also need to check that related epics feature is available, + # i.e. `related_epics_available` + enable :admin_epic_relation + end + + # This needs to be checked on both epics involved in the epic tree relationship. + rule { can?(:admin_epic_relation) & subepics_available }.policy do + enable :admin_epic_tree_relation + end + # Special case to not prevent support bot # assiging issues to confidential epics using quick actions # when group has projects with service desk enabled. diff --git a/ee/app/serializers/linked_epic_entity.rb b/ee/app/serializers/linked_epic_entity.rb index d36b2069586129c8efaddbddd7dc9950871f7647..23cb75c56fe250a048144617a6228c58ea35bdf6 100644 --- a/ee/app/serializers/linked_epic_entity.rb +++ b/ee/app/serializers/linked_epic_entity.rb @@ -30,6 +30,6 @@ class LinkedEpicEntity < Grape::Entity end expose :can_admin do |epic| - can?(request.current_user, :admin_epic_link, epic) + can?(request.current_user, :admin_epic_tree_relation, epic) end end diff --git a/ee/app/services/epics/epic_links/create_service.rb b/ee/app/services/epics/epic_links/create_service.rb index e059e5bef2250c5d39e97607c4521b791b1f5897..5c1f0a1c6ddf4eaada4c8b9c24b23475a1bb018b 100644 --- a/ee/app/services/epics/epic_links/create_service.rb +++ b/ee/app/services/epics/epic_links/create_service.rb @@ -4,7 +4,7 @@ module Epics module EpicLinks class CreateService < IssuableLinks::CreateService def execute - unless can?(current_user, :admin_epic_link, issuable.group) + unless can?(current_user, :admin_epic_tree_relation, issuable) return error(issuables_not_found_message, 404) end @@ -110,7 +110,7 @@ def can_link_epic?(epic) ::Feature.disabled?(:child_epics_from_different_hierarchies, group) end - return true if cross_group_children_disabled || can?(current_user, :admin_epic_link, epic) + return true if cross_group_children_disabled || can?(current_user, :admin_epic_tree_relation, epic) epic.errors.add(:parent, _("This epic cannot be added. You don't have access to perform this action.")) diff --git a/ee/app/services/epics/epic_links/destroy_service.rb b/ee/app/services/epics/epic_links/destroy_service.rb index ce0f3f60726512f3f914df025985073c1cecdd81..96fc61998784d31b66a62704d0c34b96904feae1 100644 --- a/ee/app/services/epics/epic_links/destroy_service.rb +++ b/ee/app/services/epics/epic_links/destroy_service.rb @@ -27,8 +27,8 @@ def create_notes def permission_to_remove_relation? child_epic.present? && parent_epic.present? && - can?(current_user, :admin_epic, parent_epic) && - can?(current_user, :admin_epic, child_epic) + can?(current_user, :admin_epic_relation, parent_epic) && + can?(current_user, :admin_epic_relation, child_epic) end def not_found_message diff --git a/ee/app/services/epics/epic_links/update_service.rb b/ee/app/services/epics/epic_links/update_service.rb index b00b167404444a2851e44cf2f615dd8592767a34..0a9068c20193188b006d435f0363804548cd1398 100644 --- a/ee/app/services/epics/epic_links/update_service.rb +++ b/ee/app/services/epics/epic_links/update_service.rb @@ -13,7 +13,7 @@ def initialize(epic, user, params) end def execute - unless can?(current_user, :admin_epic_link, epic.group) + unless can?(current_user, :admin_epic_tree_relation, epic) return error('Epic not found for given params', 404) end diff --git a/ee/app/services/epics/tree_reorder_service.rb b/ee/app/services/epics/tree_reorder_service.rb index 3cd5df82924380b7900fa16989a670f119407a11..dd4689012558e4af8a269dacd60271eb9a43dfb9 100644 --- a/ee/app/services/epics/tree_reorder_service.rb +++ b/ee/app/services/epics/tree_reorder_service.rb @@ -109,30 +109,38 @@ def supported_type?(object) end def authorized? - return false unless can?(current_user, :admin_epic, base_epic.group) + return false unless can_admin?(base_epic) if adjacent_reference - return false unless can?(current_user, :admin_epic, adjacent_reference_group) + return false unless can_admin?(adjacent_reference_epic) end if new_parent - return false unless can?(current_user, :admin_epic, new_parent.group) - return false unless moving_object.parent && can?(current_user, :admin_epic, moving_object.parent.group) + return false unless can_admin?(new_parent) && can_admin?(moving_object.parent) + end + + true + end + def can_admin?(epic) + return false unless epic + + ability, subject = if moving_object.is_a?(Epic) - return false unless can?(current_user, :admin_epic_link, moving_object.parent.group) + [:admin_epic_tree_relation, epic] + else + [:admin_epic, epic.group] end - end - true + can?(current_user, ability, subject) end - def adjacent_reference_group + def adjacent_reference_epic case adjacent_reference when EpicIssue - adjacent_reference&.epic&.group + adjacent_reference&.epic when Epic - adjacent_reference&.group + adjacent_reference else nil end diff --git a/ee/lib/api/epic_links.rb b/ee/lib/api/epic_links.rb index b625bf8a88c276a997b641a2a4bf6a43fc4e5f40..ae372c31b38817847d9101cce5765fbb6166af13 100644 --- a/ee/lib/api/epic_links.rb +++ b/ee/lib/api/epic_links.rb @@ -89,8 +89,7 @@ def cross_group_child_epics_enabled? use :child_epic_id end post ':id/(-/)epics/:epic_iid/epics/:child_epic_id' do - authorize_subepics_feature! - authorize_can_admin_epic_link! + authorize_admin_epic_tree_relation! target_child_epic = Epic.find_by_id(declared_params[:child_epic_id]) @@ -122,8 +121,7 @@ def cross_group_child_epics_enabled? documentation: { example: true } end post ':id/(-/)epics/:epic_iid/epics' do - authorize_subepics_feature! - authorize_can_admin_epic_link! + authorize_admin_epic_tree_relation! confidential = params[:confidential].nil? ? epic.confidential : params[:confidential] create_params = { parent_id: epic.id, title: params[:title], confidential: confidential } @@ -148,7 +146,7 @@ def cross_group_child_epics_enabled? use :child_epic_id end delete ':id/(-/)epics/:epic_iid/epics/:child_epic_id' do - authorize_can_destroy_epic_link! + authorize!(:admin_epic_relation, epic) result = ::Epics::EpicLinks::DestroyService.new(child_epic, current_user).execute @@ -178,8 +176,7 @@ def cross_group_child_epics_enabled? documentation: { example: 1 } end put ':id/(-/)epics/:epic_iid/epics/:child_epic_id' do - authorize_subepics_feature! - authorize_can_admin_epic_link! + authorize_admin_epic_tree_relation! update_params = params.slice(:move_before_id, :move_after_id) diff --git a/ee/lib/api/helpers/epics_helpers.rb b/ee/lib/api/helpers/epics_helpers.rb index 5160290c240bc197dc3483a4d149e900dac9fece..cb7c41304cd1f497660b12741492432ea9cc7ae6 100644 --- a/ee/lib/api/helpers/epics_helpers.rb +++ b/ee/lib/api/helpers/epics_helpers.rb @@ -7,10 +7,6 @@ def authorize_epics_feature! forbidden! unless user_group.licensed_feature_available?(:epics) end - def authorize_subepics_feature! - forbidden! unless user_group.licensed_feature_available?(:subepics) - end - def authorize_related_epics_feature! forbidden! unless user_group.licensed_feature_available?(:related_epics) end @@ -19,16 +15,12 @@ def authorize_can_read! authorize!(:read_epic, epic) end - def authorize_can_admin_epic! - authorize!(:admin_epic, epic) - end - - def authorize_can_admin_epic_link! - authorize!(:admin_epic_link, epic) + def authorize_admin_epic_tree_relation! + authorize!(:admin_epic_tree_relation, epic) end - def authorize_can_destroy_epic_link! - authorize!(:destroy_epic_link, epic) + def authorize_can_admin_epic! + authorize!(:admin_epic, epic) end def authorize_can_create! diff --git a/ee/spec/policies/epic_policy_spec.rb b/ee/spec/policies/epic_policy_spec.rb index f0ff4a4c07cee08074dd041bb54fe96040c1ade5..97649aec8b1cb344a10c816fb15ee1010c086e29 100644 --- a/ee/spec/policies/epic_policy_spec.rb +++ b/ee/spec/policies/epic_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe EpicPolicy do +RSpec.describe EpicPolicy, feature_category: :portfolio_management do include ExternalAuthorizationServiceHelpers let(:user) { create(:user) } @@ -57,7 +57,8 @@ :read_issuable_participables, :create_todo, :read_related_epic_link, :admin_related_epic_link, :set_epic_metadata, - :set_confidentiality) + :set_confidentiality, :admin_epic_relation, + :admin_epic_tree_relation) end end @@ -69,7 +70,7 @@ :read_issuable_participables, :read_internal_note, :read_related_epic_link, :admin_related_epic_link, :set_epic_metadata, :set_confidentiality, - :mark_note_as_confidential) + :admin_epic_relation, :admin_epic_tree_relation) end end @@ -137,9 +138,9 @@ it_behaves_like 'all epic permissions disabled' end - context 'when epics feature is enabled' do + context 'when epics features are enabled' do before do - stub_licensed_features(epics: true, related_epics: true) + stub_licensed_features(epics: true, related_epics: true, subepics: true) end context 'when an epic is in a private group' do @@ -181,8 +182,13 @@ context 'anonymous user' do let(:user) { nil } - it { is_expected.to be_allowed(:read_epic, :read_epic_iid, :read_note, :read_issuable_participables) } - it { is_expected.to be_disallowed(:create_todo, :read_internal_note) } + it 'matches expected permissions' do + is_expected.to be_allowed(:read_epic, :read_epic_iid, :read_note, + :read_issuable_participables) + + is_expected.to be_disallowed(:create_todo, :read_internal_note, + :admin_epic_tree_relation) + end it_behaves_like 'cannot comment on epics' end @@ -291,7 +297,7 @@ let(:group) { create(:group) } before do - stub_licensed_features(epics: true) + stub_licensed_features(epics: true, subepics: true) group.add_maintainer(user) end @@ -299,10 +305,28 @@ is_expected.to be_allowed(:read_epic, :read_epic_iid, :update_epic, :admin_epic, :create_epic, :create_note, :award_emoji, :read_note, :create_todo, - :read_issuable_participables) + :read_issuable_participables, :admin_epic_relation, + :admin_epic_tree_relation) is_expected.to be_disallowed(:read_related_epic_link, :admin_related_epic_link) end end + + context 'when subepics feature is not available' do + let(:group) { create(:group) } + + before do + stub_licensed_features(epics: true, related_epics: true) + group.add_maintainer(user) + end + + it 'matches expected permissions' do + is_expected.to be_allowed(:read_epic, :read_epic_iid, :update_epic, + :admin_epic, :create_epic, :create_note, + :award_emoji, :read_note, :create_todo, + :read_issuable_participables, :admin_epic_relation) + is_expected.to be_disallowed(:admin_epic_tree_relation) + end + end end end diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 6b5c32570a0740223087a07efa71f317946b570f..498688890b4979657698511e1d16d88a19fb368e 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -12,7 +12,7 @@ let(:epic_rules) do %i(read_epic create_epic admin_epic destroy_epic read_confidential_epic - destroy_epic_link read_epic_board read_epic_board_list admin_epic_board + read_epic_board read_epic_board_list admin_epic_board admin_epic_board_list) end diff --git a/ee/spec/requests/api/epic_links_spec.rb b/ee/spec/requests/api/epic_links_spec.rb index 48409d00fdc55bdc5136661d59fb86554921e42a..0b5d502cbfffa67acb1eb14a8607736d2b36cba5 100644 --- a/ee/spec/requests/api/epic_links_spec.rb +++ b/ee/spec/requests/api/epic_links_spec.rb @@ -139,19 +139,17 @@ def get_epics stub_licensed_features(epics: true, subepics: true) end - context 'when user is guest' do + context 'when user is not a member' do it 'returns 403' do - group.add_guest(user) - subject expect(response).to have_gitlab_http_status(:forbidden) end end - context 'when user is developer' do + context 'when user is guest' do it 'returns 201 status' do - group.add_developer(user) + group.add_guest(user) subject @@ -166,7 +164,7 @@ def get_epics let_it_be(:child_epic) { create(:epic, group: other_group) } it 'returns 404 status' do - group.add_developer(user) + group.add_guest(user) subject @@ -188,19 +186,31 @@ def get_epics stub_licensed_features(epics: true, subepics: true) end - context 'when user is guest' do + context 'when user is not a member' do it 'returns 403' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'when user is a guest' do + before do group.add_guest(user) + end + it 'returns 201 status' do subject - expect(response).to have_gitlab_http_status(:forbidden) + expect(response).to have_gitlab_http_status(:created) + expect(response).to match_response_schema('public_api/v4/linked_epic', dir: 'ee') + expect(epic.reload.children).to include(Epic.last) end end - context 'when user is developer' do + context 'when user is a reporter' do before do - group.add_developer(user) + group.add_reporter(user) end it 'returns 201 status' do @@ -269,7 +279,7 @@ def get_epics context 'when user has permissions to reorder epics' do before do - group.add_developer(user) + group.add_guest(user) end it 'returns status 200' do @@ -284,16 +294,14 @@ def get_epics let_it_be(:other_child) { create(:epic, group: other_group, parent: epic, relative_position: 100) } let(:url) { "/groups/#{group_path}/epics/#{epic.iid}/epics/#{other_child.id}" } - it 'returns status 404 if user has guest access' do - other_group.add_guest(user) - + it 'returns status 404 if user is not a member' do subject expect(response).to have_gitlab_http_status(:not_found) end - it 'returns status 200 if user has reporter access' do - other_group.add_reporter(user) + it 'returns status 200 if user has guest access' do + other_group.add_guest(user) subject @@ -311,8 +319,6 @@ def get_epics context 'when user does not have permissions to reorder epics' do it 'returns status 403' do - group.add_guest(user) - subject expect(response).to have_gitlab_http_status(:forbidden) @@ -335,19 +341,17 @@ def get_epics stub_licensed_features(epics: true) end - context 'when user is guest' do + context 'when user is not a member' do it 'returns 403' do - group.add_guest(user) - subject expect(response).to have_gitlab_http_status(:forbidden) end end - context 'when user is developer' do + context 'when user is guest' do it 'returns 200 status' do - group.add_developer(user) + group.add_guest(user) subject @@ -355,15 +359,25 @@ def get_epics expect(response).to match_response_schema('public_api/v4/epic', dir: 'ee') expect(epic.reload.children).not_to include(child_epic) end + + context 'with confidential epic' do + let_it_be(:epic) { create(:epic, group: group, confidential: true) } + let_it_be(:child_epic) { create(:epic, group: group, parent: epic, confidential: true) } + + it 'returns status 403' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end context 'when child belongs to a different group hierarchy' do let_it_be(:child_epic) { create(:epic, group: other_group, parent: epic) } - context "when user has guest access to child's group" do + context "when user is not a member of the child's group" do before do - group.add_reporter(user) - other_group.add_guest(user) + group.add_guest(user) end it 'returns 404 status' do @@ -373,10 +387,10 @@ def get_epics end end - context "when user has reporter access to child's group" do + context "when user has guest access to child's group" do before do - group.add_reporter(user) - other_group.add_reporter(user) + group.add_guest(user) + other_group.add_guest(user) end it 'returns 200 status' do diff --git a/ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb b/ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb index 7b76acf0069ca6bd6824248d6eae6f16d1464891..66680503147ce86fe4e513f4a942d5d5d41d7726 100644 --- a/ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb @@ -60,16 +60,32 @@ def mutation_response it 'returns the error message' do post_graphql_mutation(mutation, current_user: current_user) - expect(mutation_response['errors']).to eq(['You don\'t have permissions to move the objects.']) + expect(mutation_response['errors']).to contain_exactly('You don\'t have permissions to move the objects.') end - end - context 'when the user has permission' do - before do - group.add_developer(current_user) + context 'when moving an issue' do + before do + group.add_guest(current_user) + variables[:moved][:id] = GitlabSchema.id_from_object(epic_issue2).to_s + variables[:moved][:adjacent_reference_id] = GitlabSchema.id_from_object(epic_issue1).to_s + end + + it_behaves_like 'a mutation that does not update the tree' + + it 'returns the error message' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(mutation_response['errors']).to contain_exactly('You don\'t have permissions to move the objects.') + end end + end + context 'when the user has permission' do context 'when moving an epic' do + before do + group.add_guest(current_user) + end + context 'when moving an epic is successful' do it 'updates the epics relative positions' do post_graphql_mutation(mutation, current_user: current_user) @@ -164,6 +180,7 @@ def mutation_response context 'when moving an issue' do before do + group.add_reporter(current_user) variables[:moved][:id] = GitlabSchema.id_from_object(epic_issue2).to_s variables[:moved][:adjacent_reference_id] = GitlabSchema.id_from_object(epic_issue1).to_s end @@ -207,6 +224,7 @@ def mutation_response let(:epic_issue2) { create(:epic_issue, relative_position: 20) } before do + group.add_reporter(current_user) variables[:moved][:id] = GitlabSchema.id_from_object(epic_issue2).to_s variables[:moved][:adjacent_reference_id] = GitlabSchema.id_from_object(epic_issue1).to_s end diff --git a/ee/spec/requests/groups/epics/epic_links_controller_spec.rb b/ee/spec/requests/groups/epics/epic_links_controller_spec.rb index d369d4244bac5c5c3476a389278870eaf27a7d03..5226620770974a2b626f43647a258fafdb335504 100644 --- a/ee/spec/requests/groups/epics/epic_links_controller_spec.rb +++ b/ee/spec/requests/groups/epics/epic_links_controller_spec.rb @@ -63,7 +63,7 @@ def get_epics context 'when user has access to epic' do before do - group.add_developer(user) + group.add_guest(user) subject end @@ -111,6 +111,19 @@ def get_epics expect(response).to have_gitlab_http_status(:not_found) end + + context 'when epic is confidential' do + let(:epic1) { build(:epic, group: group, confidential: true) } + let(:parent_epic) { build(:epic, group: group, confidential: true) } + + it 'returns 403 status when user is a guest' do + group.add_guest(user) + + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end context 'with children in different group hierarchies' do @@ -166,7 +179,7 @@ def get_epics context 'when user has permissions to create requested association' do before do - group.add_developer(user) + group.add_guest(user) end it 'returns correct response for the correct issue reference' do @@ -219,7 +232,7 @@ def get_epics context 'when user has permissions to reorder epics' do before do - group.add_developer(user) + group.add_guest(user) end it 'returns status 200' do @@ -271,7 +284,7 @@ def get_epics context 'when user has permissions to update the parent epic' do before do - group.add_developer(user) + group.add_guest(user) end it 'returns status 200' do @@ -309,7 +322,7 @@ def get_epics context 'when user has permissions to update the parent epic but epics feature is disabled' do before do stub_licensed_features(epics: false) - group.add_developer(user) + group.add_guest(user) end it 'does not destroy the link' do diff --git a/ee/spec/services/epics/epic_links/create_service_spec.rb b/ee/spec/services/epics/epic_links/create_service_spec.rb index 4625031e605ec9b6a20ab165ade09a60855bb686..74a34fdf25a50b16fb34b995397ebe0da2320b66 100644 --- a/ee/spec/services/epics/epic_links/create_service_spec.rb +++ b/ee/spec/services/epics/epic_links/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Epics::EpicLinks::CreateService do +RSpec.describe Epics::EpicLinks::CreateService, feature_category: :portfolio_management do include NestedEpicsHelper describe '#execute' do @@ -87,7 +87,7 @@ def add_epic(references) context 'when a user has permissions to add an epic' do before do - group.add_developer(user) + group.add_guest(user) end context 'when an epic from another group is given' do @@ -95,26 +95,14 @@ def add_epic(references) epic_to_add.update!(group: other_group) end - context 'when user has no permission to read child epic' do + context 'when user has no permission to admin_epic_tree_relation' do let(:params) { { target_issuable: epic_to_add } } subject { described_class.new(epic, user, params).execute } - before do - epic_to_add.update!(confidential: true) - end - - include_examples 'returns an error' - end - - context 'when user has no permission to admin_epic_link' do - let(:expected_code) { 409 } - let(:expected_error) do - "This epic cannot be added. You don't have access to perform this action." - end - before do other_group.add_guest(user) + epic_to_add.update!(confidential: true) end include_examples 'returns an error' @@ -127,9 +115,10 @@ def add_epic(references) end before do - other_group.add_developer(user) + other_group.add_guest(user) stub_licensed_features(epics: true) - allow(group).to receive(:licensed_feature_available?).with(:subepics).and_return(true) + + allow(group).to receive(:licensed_feature_available?).and_return(true) allow(other_group).to receive(:licensed_feature_available?).with(:subepics).and_return(false) end @@ -143,7 +132,7 @@ def add_epic(references) end before do - other_group.add_developer(user) + other_group.add_guest(user) stub_feature_flags(child_epics_from_different_hierarchies: false) epic_to_add.update!(group: other_group) end @@ -159,7 +148,7 @@ def add_epic(references) end before do - ancestor.add_developer(user) + ancestor.add_guest(user) epic_to_add.update!(group: ancestor) end @@ -319,22 +308,19 @@ def add_epic(references) context 'when an epic from a another group is given' do before do - other_group.add_developer(user) epic_to_add.update!(group: other_group) end context 'when user has insufficient permissions' do - before do - other_group.add_guest(user) - end - include_examples 'returns an error' end context 'when subepics feature is not available for child group' do before do + other_group.add_guest(user) stub_licensed_features(epics: true) - allow(group).to receive(:licensed_feature_available?).with(:subepics).and_return(true) + + allow(group).to receive(:licensed_feature_available?).and_return(true) allow(other_group).to receive(:licensed_feature_available?).with(:subepics).and_return(false) end @@ -344,6 +330,7 @@ def add_epic(references) context 'when child_epics_from_different_hierarchies feature flag is disabled' do before do stub_feature_flags(child_epics_from_different_hierarchies: false) + other_group.add_guest(user) epic_to_add.update!(group: other_group) end @@ -410,7 +397,7 @@ def add_epic(references) let_it_be_with_reload(:another_epic) { create(:epic, group: group) } before do - group.add_developer(user) + group.add_guest(user) end context 'when a correct reference is given' do @@ -447,7 +434,7 @@ def add_epic(references) context 'when an epic from ancestor group is given' do before do - ancestor.add_developer(user) + ancestor.add_guest(user) epic_to_add.update!(group: ancestor) end diff --git a/ee/spec/services/epics/epic_links/destroy_service_spec.rb b/ee/spec/services/epics/epic_links/destroy_service_spec.rb index e45999452a86749738d97e9809d8b9f0344f9285..ffb2b0bfe5c6df46c8902867e30b4e5a47f42dc8 100644 --- a/ee/spec/services/epics/epic_links/destroy_service_spec.rb +++ b/ee/spec/services/epics/epic_links/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Epics::EpicLinks::DestroyService do +RSpec.describe Epics::EpicLinks::DestroyService, feature_category: :portfolio_management do describe '#execute' do let(:group) { create(:group) } let(:user) { create(:user) } @@ -68,7 +68,7 @@ def remove_epic_relation(child_epic) context 'when user has permissions to remove epic relation' do before do - group.add_developer(user) + group.add_guest(user) end context 'when the child epic is nil' do diff --git a/ee/spec/services/epics/epic_links/update_service_spec.rb b/ee/spec/services/epics/epic_links/update_service_spec.rb index e11da9734f2e951f7c4e102f13883a2a658b43ee..744c971e3e04fbd419647c2e6e655b2ada6a9cda 100644 --- a/ee/spec/services/epics/epic_links/update_service_spec.rb +++ b/ee/spec/services/epics/epic_links/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Epics::EpicLinks::UpdateService do +RSpec.describe Epics::EpicLinks::UpdateService, feature_category: :portfolio_management do let(:user) { create(:user) } let(:group) { create(:group) } let(:parent_epic) { create(:epic, group: group) } @@ -25,7 +25,7 @@ def ordered_epics describe '#execute' do before do - group.add_developer(user) + group.add_guest(user) end shared_examples 'updating timestamps' do @@ -57,6 +57,16 @@ def ordered_epics stub_licensed_features(epics: true, subepics: true) end + context 'when user has insufficient permissions' do + before do + user.group_members.delete_all + end + + it 'returns an error' do + expect(subject).to eq(message: 'Epic not found for given params', status: :error, http_status: 404) + end + end + context 'when params are nil' do let(:params) { { move_before_id: nil, move_after_id: nil } } diff --git a/ee/spec/services/epics/tree_reorder_service_spec.rb b/ee/spec/services/epics/tree_reorder_service_spec.rb index 94f3f529617284d4f3705b9237337c2cc0cbe373..fb647363bbc6b38515f1b0891b8ec6eff853a9fe 100644 --- a/ee/spec/services/epics/tree_reorder_service_spec.rb +++ b/ee/spec/services/epics/tree_reorder_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Epics::TreeReorderService do +RSpec.describe Epics::TreeReorderService, feature_category: :portfolio_management do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:ancestor) { create(:group) } @@ -71,16 +71,16 @@ group.add_reporter(user) end - context 'when relative_position is not valid' do - let(:relative_position) { 'whatever' } - - it_behaves_like 'error for the tree update', 'Relative position is not valid.' - end - context 'when moving EpicIssue' do let!(:tree_object_1) { epic_issue1 } let!(:tree_object_2) { epic_issue2 } + context 'when relative_position is not valid' do + let(:relative_position) { 'whatever' } + + it_behaves_like 'error for the tree update', 'Relative position is not valid.' + end + context 'when object being moved is not the same type as the switched object' do let!(:tree_object_3) { epic1 } let!(:tree_object_4) { epic2 } @@ -237,6 +237,12 @@ stub_licensed_features(epics: true, subepics: true) end + context 'when relative_position is not valid' do + let(:relative_position) { 'whatever' } + + it_behaves_like 'error for the tree update', 'Relative position is not valid.' + end + context 'when user does not have permissions to admin the previous parent' do let(:other_epic) { create(:epic, group: ancestor) } let(:new_parent_id) { GitlabSchema.id_from_object(epic) } @@ -252,7 +258,7 @@ let(:new_parent_id) { GitlabSchema.id_from_object(epic) } before do - group.add_guest(user) + user.group_members.delete_all end it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'