diff options
64 files changed, 1593 insertions, 427 deletions
diff --git a/app/assets/javascripts/layout_nav.js b/app/assets/javascripts/layout_nav.js index 2cb8c37f192..1f58065a505 100644 --- a/app/assets/javascripts/layout_nav.js +++ b/app/assets/javascripts/layout_nav.js @@ -1,5 +1,4 @@ import $ from 'jquery'; -import { setNotification } from './whats_new/utils/notification'; function hideEndFade($scrollingTabs) { $scrollingTabs.each(function scrollTabsLoop() { @@ -86,27 +85,8 @@ function initInviteMembers() { .catch(() => {}); } -function initWhatsNewComponent() { - const appEl = document.getElementById('whats-new-app'); - if (!appEl) return; - - setNotification(appEl); - - const triggerEl = document.querySelector('.js-whats-new-trigger'); - if (!triggerEl) return; - - triggerEl.addEventListener('click', () => { - import(/* webpackChunkName: 'whatsNewApp' */ '~/whats_new') - .then(({ default: initWhatsNew }) => { - initWhatsNew(appEl); - }) - .catch(() => {}); - }); -} - function initDeferred() { initScrollingTabs(); - initWhatsNewComponent(); initInviteMembers(); } diff --git a/app/assets/javascripts/members/components/action_buttons/remove_group_link_button.vue b/app/assets/javascripts/members/components/action_buttons/remove_group_link_button.vue index 2f10a333bf4..c76b928ad3d 100644 --- a/app/assets/javascripts/members/components/action_buttons/remove_group_link_button.vue +++ b/app/assets/javascripts/members/components/action_buttons/remove_group_link_button.vue @@ -36,7 +36,7 @@ export default { :title="$options.i18n.buttonTitle" :aria-label="$options.i18n.buttonTitle" icon="remove" - data-qa-selector="remove_group_link_button" + data-testid="remove-group-link-button" @click="showRemoveGroupLinkModal(groupLink)" /> </template> diff --git a/app/assets/javascripts/members/components/modals/remove_group_link_modal.vue b/app/assets/javascripts/members/components/modals/remove_group_link_modal.vue index 18db8fe9cfb..55f75bc819c 100644 --- a/app/assets/javascripts/members/components/modals/remove_group_link_modal.vue +++ b/app/assets/javascripts/members/components/modals/remove_group_link_modal.vue @@ -15,7 +15,7 @@ export default { text: s__('Members|Remove group'), attributes: { variant: 'danger', - 'data-qa-selector': 'remove_group_button', + 'data-testid': 'remove-group-button', }, }, csrf, @@ -69,7 +69,7 @@ export default { :action-primary="$options.actionPrimary" :action-cancel="$options.actionCancel" size="sm" - data-qa-selector="remove_group_link_modal_content" + data-testid="remove-group-link-modal-content" @primary="handlePrimary" @hide="hideRemoveGroupLinkModal" > diff --git a/app/assets/javascripts/members/components/modals/remove_member_modal.vue b/app/assets/javascripts/members/components/modals/remove_member_modal.vue index ecc769174f4..d3079dc7d0a 100644 --- a/app/assets/javascripts/members/components/modals/remove_member_modal.vue +++ b/app/assets/javascripts/members/components/modals/remove_member_modal.vue @@ -72,7 +72,7 @@ export default { text: this.actionText, attributes: { variant: 'danger', - 'data-qa-selector': 'remove_member_button', + 'data-testid': 'remove-member-button', }, }; }, @@ -104,7 +104,7 @@ export default { :action-primary="actionPrimary" :title="actionText" :visible="removeMemberModalVisible" - data-qa-selector="remove_member_modal" + data-testid="remove-member-modal" @primary="submitForm" @hide="hideRemoveMemberModal" > diff --git a/app/assets/javascripts/members/components/table/max_role.vue b/app/assets/javascripts/members/components/table/max_role.vue index 504e68f4f09..89780108518 100644 --- a/app/assets/javascripts/members/components/table/max_role.vue +++ b/app/assets/javascripts/members/components/table/max_role.vue @@ -107,13 +107,13 @@ export default { :header-text="__('Change role')" :disabled="disabled" :loading="busy" - data-qa-selector="access_level_dropdown" + data-testid="access-level-dropdown" :items="accessLevelOptions.formatted" :selected="selectedRole" @select="handleSelect" > <template #list-item="{ item }"> - <span data-qa-selector="access_level_link">{{ item.text }}</span> + <span data-testid="access-level-link">{{ item.text }}</span> </template> <template #footer> <ldap-dropdown-footer diff --git a/app/assets/javascripts/members/components/table/members_table.vue b/app/assets/javascripts/members/components/table/members_table.vue index 149947397ad..1bccb8a0c4b 100644 --- a/app/assets/javascripts/members/components/table/members_table.vue +++ b/app/assets/javascripts/members/components/table/members_table.vue @@ -143,7 +143,6 @@ export default { ...this.tableAttrs.tr, ...(member?.id && { 'data-testid': `members-table-row-${member.id}`, - 'data-qa-selector': 'member_row', }), }; }, diff --git a/app/assets/javascripts/super_sidebar/components/help_center.vue b/app/assets/javascripts/super_sidebar/components/help_center.vue index e354d82b76a..4e0d05add85 100644 --- a/app/assets/javascripts/super_sidebar/components/help_center.vue +++ b/app/assets/javascripts/super_sidebar/components/help_center.vue @@ -47,6 +47,7 @@ export default { return { showWhatsNewNotification: this.shouldShowWhatsNewNotification(), helpCenterState, + toggleWhatsNewDrawer: null, }; }, computed: { @@ -177,12 +178,11 @@ export default { this.showWhatsNewNotification = false; if (!this.toggleWhatsNewDrawer) { - const appEl = document.getElementById('whats-new-app'); const { default: toggleWhatsNewDrawer } = await import( /* webpackChunkName: 'whatsNewApp' */ '~/whats_new' ); this.toggleWhatsNewDrawer = toggleWhatsNewDrawer; - this.toggleWhatsNewDrawer(appEl); + this.toggleWhatsNewDrawer(this.sidebarData.whats_new_version_digest); } else { this.toggleWhatsNewDrawer(); } diff --git a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue index d153c17ea1e..6a1f5f0bb44 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue @@ -1,7 +1,7 @@ <script> -import { GlButton, GlSprintf } from '@gitlab/ui'; +import { GlForm, GlButton, GlSprintf } from '@gitlab/ui'; import { createAlert } from '~/alert'; -import { visitUrl } from '~/lib/utils/url_utility'; +import csrf from '~/lib/utils/csrf'; import { STATUS_MERGED } from '~/issues/constants'; import { BV_SHOW_MODAL } from '~/lib/utils/constants'; import { HTTP_STATUS_UNAUTHORIZED } from '~/lib/utils/http_status'; @@ -23,7 +23,9 @@ export default { StateContainer, GlButton, GlSprintf, + GlForm, }, + csrf, mixins: [approvalsMixin, glFeatureFlagsMixin()], props: { mr: { @@ -169,16 +171,15 @@ export default { .join(', ') .concat('.'); }, + samlApprovalPath() { + return this.mr.samlApprovalPath; + }, requireSamlAuthToApprove() { return this.mr.requireSamlAuthToApprove; }, }, methods: { approve() { - if (this.requireSamlAuthToApprove) { - this.approveWithSamlAuth(); - return; - } if (this.requirePasswordToApprove) { this.$root.$emit(BV_SHOW_MODAL, this.modalId); return; @@ -195,7 +196,7 @@ export default { }, approveWithSamlAuth() { // Intentionally direct to SAML Identity Provider for renewed authorization even if SSO session exists - visitUrl(this.mr.samlApprovalPath); + this.$refs.form.$el.submit(); }, approveWithAuth(data) { this.updateApproval( @@ -270,17 +271,40 @@ export default { <template v-else> <div class="gl-display-flex gl-flex-direction-column"> <div class="gl-display-flex gl-flex-direction-row gl-align-items-center"> - <gl-button - v-if="action" - :variant="action.variant" - :category="action.category" - :loading="isApproving" - class="gl-mr-3" - data-testid="approve-button" - @click="action.action" - > - {{ action.text }} - </gl-button> + <div v-if="requireSamlAuthToApprove && showApprove"> + <gl-form + ref="form" + :action="samlApprovalPath" + method="post" + data-testid="approve-form" + > + <gl-button + v-if="action" + :variant="action.variant" + :category="action.category" + :loading="isApproving" + class="gl-mr-3" + data-testid="approve-button" + type="submit" + > + {{ action.text }} + </gl-button> + <input :value="$options.csrf.token" type="hidden" name="authenticity_token" /> + </gl-form> + </div> + <span v-if="!requireSamlAuthToApprove || showUnapprove"> + <gl-button + v-if="action" + :variant="action.variant" + :category="action.category" + :loading="isApproving" + class="gl-mr-3" + data-testid="approve-button" + @click="action.action" + > + {{ action.text }} + </gl-button> + </span> <approvals-summary-optional v-if="isOptional" :can-approve="hasAction" diff --git a/app/assets/javascripts/whats_new/index.js b/app/assets/javascripts/whats_new/index.js index bc9e2d5c3b1..57db8cde110 100644 --- a/app/assets/javascripts/whats_new/index.js +++ b/app/assets/javascripts/whats_new/index.js @@ -1,34 +1,22 @@ import Vue from 'vue'; -// eslint-disable-next-line no-restricted-imports -import { mapState } from 'vuex'; -import App from './components/app.vue'; +import WhatsNewApp from './components/app.vue'; import store from './store'; -import { getVersionDigest, setNotification } from './utils/notification'; let whatsNewApp; -export default (el) => { +export default (versionDigest) => { if (whatsNewApp) { store.dispatch('openDrawer'); } else { + const el = document.createElement('div'); + document.body.append(el); whatsNewApp = new Vue({ el, store, - components: { - App, - }, - computed: { - ...mapState(['open']), - }, - watch: { - open() { - setNotification(el); - }, - }, render(createElement) { - return createElement('app', { + return createElement(WhatsNewApp, { props: { - versionDigest: getVersionDigest(el), + versionDigest, }, }); }, diff --git a/app/assets/javascripts/whats_new/utils/notification.js b/app/assets/javascripts/whats_new/utils/notification.js index 1621c4d5f27..fb6ce7454dc 100644 --- a/app/assets/javascripts/whats_new/utils/notification.js +++ b/app/assets/javascripts/whats_new/utils/notification.js @@ -1,21 +1 @@ export const STORAGE_KEY = 'display-whats-new-notification'; - -export const getVersionDigest = (appEl) => appEl.dataset.versionDigest; - -export const setNotification = (appEl) => { - const versionDigest = getVersionDigest(appEl); - const notificationEl = document.querySelector('.header-help'); - if (!notificationEl) return; - - let notificationCountEl = notificationEl.querySelector('.js-whats-new-notification-count'); - - if (localStorage.getItem(STORAGE_KEY) === versionDigest || versionDigest === undefined) { - notificationEl.classList.remove('with-notifications'); - if (notificationCountEl) { - notificationCountEl.parentElement.removeChild(notificationCountEl); - notificationCountEl = null; - } - } else { - notificationEl.classList.add('with-notifications'); - } -}; diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index a97516fddff..907ece1a06e 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -8,6 +8,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController include KnownSignIn include AcceptsPendingInvitations include Onboarding::Redirectable + include InternalRedirect + + ACTIVE_SINCE_KEY = 'active_since' after_action :verify_known_sign_in @@ -113,14 +116,21 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController super end + def store_redirect_to + # overridden in EE + end + def omniauth_flow(auth_module, identity_linker: nil) if fragment = request.env.dig('omniauth.params', 'redirect_fragment').presence store_redirect_fragment(fragment) end + store_redirect_to + if current_user return render_403 unless link_provider_allowed?(oauth['provider']) + set_session_active_since(oauth['provider']) if ::AuthHelper.saml_providers.include?(oauth['provider'].to_sym) track_event(current_user, oauth['provider'], 'succeeded') if Gitlab::CurrentSettings.admin_mode @@ -167,6 +177,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end + # Overrided in EE + def set_session_active_since(id); end + def sign_in_user_flow(auth_user_class) auth_user = build_auth_user(auth_user_class) new_user = auth_user.new? @@ -181,6 +194,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController Gitlab::Tracking.event(self.class.name, "#{oauth['provider']}_sso", user: @user) if new_user set_remember_me(@user) + set_session_active_since(oauth['provider']) if ::AuthHelper.saml_providers.include?(oauth['provider'].to_sym) if @user.two_factor_enabled? && !auth_user.bypass_two_factor? prompt_for_two_factor(@user) diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb index 5f8bf423219..855b9824cf2 100644 --- a/app/controllers/projects/group_links_controller.rb +++ b/app/controllers/projects/group_links_controller.rb @@ -3,7 +3,7 @@ class Projects::GroupLinksController < Projects::ApplicationController layout 'project_settings' before_action :authorize_admin_project!, except: [:destroy] - before_action :authorize_admin_project_group_link!, only: [:destroy] + before_action :authorize_manage_destroy!, only: [:destroy] before_action :authorize_admin_project_member!, only: [:update] feature_category :groups_and_projects @@ -20,8 +20,8 @@ class Projects::GroupLinksController < Projects::ApplicationController else render json: {} end - elsif result.reason == :not_found - render json: { message: result.message }, status: :not_found + else + render json: { message: result.message }, status: result.reason end end @@ -47,7 +47,7 @@ class Projects::GroupLinksController < Projects::ApplicationController end format.js do - render json: { message: result.message }, status: :not_found if result.reason == :not_found + render json: { message: result.message }, status: result.reason end end end @@ -55,8 +55,8 @@ class Projects::GroupLinksController < Projects::ApplicationController protected - def authorize_admin_project_group_link! - render_404 unless can?(current_user, :admin_project_group_link, group_link) + def authorize_manage_destroy! + render_404 unless can?(current_user, :manage_destroy, group_link) end def group_link diff --git a/app/finders/timelogs/timelogs_finder.rb b/app/finders/timelogs/timelogs_finder.rb new file mode 100644 index 00000000000..610dba43317 --- /dev/null +++ b/app/finders/timelogs/timelogs_finder.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Timelogs + class TimelogsFinder + attr_reader :issuable, :params + + def initialize(issuable, params = {}) + @issuable = issuable + @params = params + end + + def execute + timelogs = issuable&.timelogs || Timelog.all + timelogs = by_time(timelogs) + timelogs = by_user(timelogs) + timelogs = by_group(timelogs) + timelogs = by_project(timelogs) + apply_sorting(timelogs) + end + + private + + def by_time(timelogs) + return timelogs unless params[:start_time] || params[:end_time] + + validate_time_difference! + + timelogs = timelogs.at_or_after(params[:start_time]) if params[:start_time] + timelogs = timelogs.at_or_before(params[:end_time]) if params[:end_time] + + timelogs + end + + def by_user(timelogs) + return timelogs unless params[:username] + + user = User.find_by_username(params[:username]) + timelogs.for_user(user) + end + + def by_group(timelogs) + return timelogs unless params[:group_id] + + group = Group.find_by_id(params[:group_id]) + raise(ActiveRecord::RecordNotFound, "Group with id '#{params[:group_id]}' could not be found") unless group + + timelogs.in_group(group) + end + + def by_project(timelogs) + return timelogs unless params[:project_id] + + timelogs.in_project(params[:project_id]) + end + + def apply_sorting(timelogs) + return timelogs unless params[:sort] + + timelogs.sort_by_field(params[:sort]) + end + + def validate_time_difference! + return unless end_time_before_start_time? + + raise ArgumentError, 'Start argument must be before End argument' + end + + def end_time_before_start_time? + times_provided? && params[:end_time] < params[:start_time] + end + + def times_provided? + params[:start_time] && params[:end_time] + end + end +end diff --git a/app/graphql/mutations/container_registry/protection/rule/update.rb b/app/graphql/mutations/container_registry/protection/rule/update.rb new file mode 100644 index 00000000000..b4464e5b5f4 --- /dev/null +++ b/app/graphql/mutations/container_registry/protection/rule/update.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Mutations + module ContainerRegistry + module Protection + module Rule + class Update < ::Mutations::BaseMutation + graphql_name 'UpdateContainerRegistryProtectionRule' + description 'Updates a container registry protection rule to restrict access to project containers. ' \ + 'You can prevent users without certain roles from altering containers. ' \ + 'Available only when feature flag `container_registry_protected_containers` is enabled.' + + authorize :admin_container_image + + argument :id, + ::Types::GlobalIDType[::ContainerRegistry::Protection::Rule], + required: true, + description: 'Global ID of the container registry protection rule to be updated.' + + argument :repository_path_pattern, + GraphQL::Types::String, + required: false, + validates: { allow_blank: false }, + description: + 'Container\'s repository path pattern of the protection rule. ' \ + 'For example, `my-scope/my-project/container-dev-*`. ' \ + 'Wildcard character `*` allowed.' + + argument :delete_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + required: false, + validates: { allow_blank: false }, + description: + 'Maximum GitLab access level prevented from deleting a container. ' \ + 'For example, `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + argument :push_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + required: false, + validates: { allow_blank: false }, + description: + 'Maximum GitLab access level prevented from pushing a container. ' \ + 'For example, `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + field :container_registry_protection_rule, + Types::ContainerRegistry::Protection::RuleType, + null: true, + description: 'Container registry protection rule after mutation.' + + def resolve(id:, **kwargs) + container_registry_protection_rule = authorized_find!(id: id) + + if Feature.disabled?(:container_registry_protected_containers, container_registry_protection_rule.project) + raise_resource_not_available_error!("'container_registry_protected_containers' feature flag is disabled") + end + + response = ::ContainerRegistry::Protection::UpdateRuleService.new(container_registry_protection_rule, + current_user: current_user, params: kwargs).execute + + { container_registry_protection_rule: response.payload[:container_registry_protection_rule], + errors: response.errors } + end + end + end + end + end +end diff --git a/app/graphql/resolvers/timelog_resolver.rb b/app/graphql/resolvers/timelog_resolver.rb index 4f52db6801d..c2be582742b 100644 --- a/app/graphql/resolvers/timelog_resolver.rb +++ b/app/graphql/resolvers/timelog_resolver.rb @@ -3,6 +3,7 @@ module Resolvers class TimelogResolver < BaseResolver include LooksAhead + include Gitlab::Graphql::Authorize::AuthorizeResource type ::Types::TimelogType.connection_type, null: false @@ -42,21 +43,30 @@ module Resolvers def resolve_with_lookahead(**args) validate_args!(object, args) - timelogs = object&.timelogs || Timelog.all - args = parse_datetime_args(args) - timelogs = apply_user_filter(timelogs, args) - timelogs = apply_project_filter(timelogs, args) - timelogs = apply_time_filter(timelogs, args) - timelogs = apply_group_filter(timelogs, args) - timelogs = apply_sorting(timelogs, args) + timelogs = Timelogs::TimelogsFinder.new(object, finder_params(args)).execute apply_lookahead(timelogs) + rescue ArgumentError => e + raise_argument_error(e.message) + rescue ActiveRecord::RecordNotFound + raise_resource_not_available_error! end private + def finder_params(args) + { + username: args[:username], + start_time: args[:start_time], + end_time: args[:end_time], + group_id: args[:group_id]&.model_id, + project_id: args[:project_id]&.model_id, + sort: args[:sort] + } + end + def preloads { note: [:note] @@ -95,58 +105,6 @@ module Resolvers args[:start_time] && args[:end_time] end - def validate_time_difference!(args) - return unless end_time_before_start_time?(args) - - raise_argument_error('Start argument must be before End argument') - end - - def end_time_before_start_time?(args) - times_provided?(args) && args[:end_time] < args[:start_time] - end - - def apply_project_filter(timelogs, args) - return timelogs unless args[:project_id] - - timelogs.in_project(args[:project_id].model_id) - end - - def apply_group_filter(timelogs, args) - return timelogs unless args[:group_id] - - group = Group.find_by_id(args[:group_id].model_id) - timelogs.in_group(group) - end - - def apply_user_filter(timelogs, args) - return timelogs unless args[:username] - - user = UserFinder.new(args[:username]).find_by_username - timelogs.for_user(user) - end - - def apply_time_filter(timelogs, args) - return timelogs unless args[:start_time] || args[:end_time] - - validate_time_difference!(args) - - if args[:start_time] - timelogs = timelogs.at_or_after(args[:start_time]) - end - - if args[:end_time] - timelogs = timelogs.at_or_before(args[:end_time]) - end - - timelogs - end - - def apply_sorting(timelogs, args) - return timelogs unless args[:sort] - - timelogs.sort_by_field(args[:sort]) - end - def raise_argument_error(message) raise Gitlab::Graphql::Errors::ArgumentError, message end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index d6c064f6882..8ec41af0be4 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -140,6 +140,7 @@ module Types mount_mutation Mutations::ContainerExpirationPolicies::Update mount_mutation Mutations::ContainerRegistry::Protection::Rule::Create, alpha: { milestone: '16.6' } mount_mutation Mutations::ContainerRegistry::Protection::Rule::Delete, alpha: { milestone: '16.7' } + mount_mutation Mutations::ContainerRegistry::Protection::Rule::Update, alpha: { milestone: '16.7' } mount_mutation Mutations::ContainerRepositories::Destroy mount_mutation Mutations::ContainerRepositories::DestroyTags mount_mutation Mutations::Ci::Catalog::Resources::Create, alpha: { milestone: '15.11' } diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 2f042ea6417..e931d95d38a 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -279,7 +279,7 @@ module MergeRequestsHelper target_branch = link_to merge_request.target_branch, project_tree_path(merge_request.target_project, merge_request.target_branch), title: merge_request.target_branch, class: 'ref-container gl-display-inline-block gl-text-truncate gl-max-w-26 gl-mx-2' - _('%{author} requested to merge %{source_branch} %{copy_button} into %{target_branch} %{created_at}').html_safe % { author: link_to_author.html_safe, source_branch: merge_request_source_branch(merge_request).html_safe, copy_button: copy_button.html_safe, target_branch: target_branch.html_safe, created_at: time_ago_with_tooltip(merge_request.created_at, html_class: 'gl-display-inline-block').html_safe } + safe_format('%{author} • %{source_branch} %{copy_button} ➔ %{target_branch} %{created_at}', author: link_to_author, source_branch: merge_request_source_branch(merge_request), copy_button: copy_button, target_branch: target_branch, created_at: time_ago_with_tooltip(merge_request.created_at, html_class: 'gl-display-inline-block')) end def sticky_header_data diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb index 69d8c0db55b..de323d98584 100644 --- a/app/models/project_group_link.rb +++ b/app/models/project_group_link.rb @@ -11,8 +11,7 @@ class ProjectGroupLink < ApplicationRecord validates :project_id, presence: true validates :group, presence: true validates :group_id, uniqueness: { scope: [:project_id], message: N_("already shared with this group") } - validates :group_access, presence: true - validates :group_access, inclusion: { in: Gitlab::Access.values }, presence: true + validates :group_access, inclusion: { in: Gitlab::Access.all_values }, presence: true validate :different_group scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) } @@ -22,20 +21,16 @@ class ProjectGroupLink < ApplicationRecord alias_method :shared_with_group, :group alias_method :shared_from, :project - def self.access_options - Gitlab::Access.options - end - - def self.default_access - Gitlab::Access::DEVELOPER - end - def self.search(query) joins(:group).merge(Group.search(query)) end def human_access - self.class.access_options.key(self.group_access) + Gitlab::Access.human_access(self.group_access) + end + + def owner_access? + group_access.to_i == Gitlab::Access::OWNER end private diff --git a/app/policies/project_group_link_policy.rb b/app/policies/project_group_link_policy.rb index 7ad2985ecc5..98d5bdebdc9 100644 --- a/app/policies/project_group_link_policy.rb +++ b/app/policies/project_group_link_policy.rb @@ -1,16 +1,39 @@ # frozen_string_literal: true class ProjectGroupLinkPolicy < BasePolicy # rubocop:disable Gitlab/NamespacedClass + condition(:group_owner) { group_owner? } condition(:group_owner_or_project_admin) { group_owner? || project_admin? } condition(:can_read_group) { can?(:read_group, @subject.group) } condition(:project_member) { @subject.project.member?(@user) } + condition(:can_manage_owners) { can_manage_owners? } + condition(:can_manage_group_link_with_owner_access) do + next true unless @subject.owner_access? - rule { group_owner_or_project_admin }.enable :admin_project_group_link + can_manage_owners? + end + + rule { can_manage_owners }.enable :manage_owners + + rule { can_manage_group_link_with_owner_access }.enable :manage_group_link_with_owner_access + + # `manage_destroy` specifies the very basic permission that a user needs to destroy a link. + rule { group_owner_or_project_admin }.enable :manage_destroy + + # `destroy_project_group_link` combines the basic permission, ie `manage_destroy` AND + # the specific permissions a user needs to destroy a link that has `OWNER` access level. + # link.project's owner, or link.group's owner can delete a link with any access level, including OWNER + rule { can?(:manage_destroy) & (can?(:manage_group_link_with_owner_access) | group_owner) }.policy do + enable :destroy_project_group_link + end rule { can_read_group | project_member }.enable :read_shared_with_group private + def can_manage_owners? + can?(:manage_owners, @subject.project) + end + def group_owner? can?(:admin_group, @subject.group) end diff --git a/app/serializers/group_link/group_group_link_entity.rb b/app/serializers/group_link/group_group_link_entity.rb index f855d89f593..f6989e8afcd 100644 --- a/app/serializers/group_link/group_group_link_entity.rb +++ b/app/serializers/group_link/group_group_link_entity.rb @@ -8,14 +8,22 @@ module GroupLink GroupEntity.represent(group_link.shared_from, only: [:id, :full_name, :web_url]) end - private + expose :valid_roles do |group_link| + group_link.class.access_options + end - def can_admin_group_link?(group_link, options) - can?(current_user, admin_permission_name, group_link.shared_from) + expose :can_update do |group_link, options| + can_admin_group_link?(group_link, options) + end + + expose :can_remove do |group_link, options| + can_admin_group_link?(group_link, options) end - def admin_permission_name - :admin_group_member + private + + def can_admin_group_link?(group_link, options) + direct_member?(group_link, options) && can?(current_user, :admin_group_member, group_link.shared_from) end end end diff --git a/app/serializers/group_link/group_link_entity.rb b/app/serializers/group_link/group_link_entity.rb index 66645e736a9..1b8313c2536 100644 --- a/app/serializers/group_link/group_link_entity.rb +++ b/app/serializers/group_link/group_link_entity.rb @@ -15,10 +15,6 @@ module GroupLink expose :group_access, as: :integer_value end - expose :valid_roles do |group_link| - group_link.class.access_options - end - expose :is_shared_with_group_private do |group_link| !can_read_shared_group?(group_link) end @@ -43,14 +39,6 @@ module GroupLink end end - expose :can_update do |group_link, options| - can_admin_shared_from?(group_link, options) - end - - expose :can_remove do |group_link, options| - direct_member?(group_link, options) && can_admin_group_link?(group_link, options) - end - expose :is_direct_member do |group_link, options| direct_member?(group_link, options) end @@ -68,10 +56,5 @@ module GroupLink def direct_member?(group_link, options) group_link.shared_from == options[:source] end - - def can_admin_shared_from?(group_link, options) - direct_member?(group_link, options) && - can?(current_user, admin_permission_name, group_link.shared_from) - end end end diff --git a/app/serializers/group_link/project_group_link_entity.rb b/app/serializers/group_link/project_group_link_entity.rb index fbad69bf2c5..d763809d123 100644 --- a/app/serializers/group_link/project_group_link_entity.rb +++ b/app/serializers/group_link/project_group_link_entity.rb @@ -8,14 +8,23 @@ module GroupLink ProjectEntity.represent(group_link.shared_from, only: [:id, :full_name]) end - private + expose :valid_roles do |group_link| + if can?(current_user, :manage_owners, group_link) + Gitlab::Access.options_with_owner + else + Gitlab::Access.options + end + end - def can_admin_group_link?(group_link, options) - can?(current_user, :admin_project_group_link, group_link) + expose :can_update do |group_link, options| + direct_member?(group_link, options) && + can?(current_user, :admin_project_member, group_link.project) && + can?(current_user, :manage_group_link_with_owner_access, group_link) end - def admin_permission_name - :admin_project_member + expose :can_remove do |group_link, options| + direct_member?(group_link, options) && + can?(current_user, :destroy_project_group_link, group_link) end end end diff --git a/app/services/container_registry/protection/update_rule_service.rb b/app/services/container_registry/protection/update_rule_service.rb new file mode 100644 index 00000000000..af74e542ac7 --- /dev/null +++ b/app/services/container_registry/protection/update_rule_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + class UpdateRuleService + include Gitlab::Allowable + + ALLOWED_ATTRIBUTES = %i[ + repository_path_pattern + delete_protected_up_to_access_level + push_protected_up_to_access_level + ].freeze + + def initialize(container_registry_protection_rule, current_user:, params:) + if container_registry_protection_rule.blank? || current_user.blank? + raise ArgumentError, + 'container_registry_protection_rule and current_user must be set' + end + + @container_registry_protection_rule = container_registry_protection_rule + @current_user = current_user + @params = params || {} + end + + def execute + unless can?(current_user, :admin_container_image, container_registry_protection_rule.project) + error_message = _('Unauthorized to update a container registry protection rule') + return service_response_error(message: error_message) + end + + container_registry_protection_rule.update(params.slice(*ALLOWED_ATTRIBUTES)) + + if container_registry_protection_rule.errors.present? + return service_response_error(message: container_registry_protection_rule.errors.full_messages) + end + + ServiceResponse.success(payload: { container_registry_protection_rule: container_registry_protection_rule }) + rescue StandardError => e + service_response_error(message: e.message) + end + + private + + attr_reader :container_registry_protection_rule, :current_user, :params + + def service_response_error(message:) + ServiceResponse.error( + message: message, + payload: { container_registry_protection_rule: nil } + ) + end + end + end +end diff --git a/app/services/projects/group_links/create_service.rb b/app/services/projects/group_links/create_service.rb index c9642fb495a..cc7478540d2 100644 --- a/app/services/projects/group_links/create_service.rb +++ b/app/services/projects/group_links/create_service.rb @@ -11,12 +11,30 @@ module Projects super(project, user, params) end + def execute + if adding_a_group_as_owner? && cannot_assign_owner_responsibilities_to_member_in_project? + error('403 Forbidden', 403) + else + super + end + end + private delegate :root_ancestor, to: :project + def adding_a_group_as_owner? + params[:link_group_access].to_i == Gitlab::Access::OWNER + end + + def cannot_assign_owner_responsibilities_to_member_in_project? + !current_user.can?(:manage_owners, project) + end + def valid_to_create? - can?(current_user, :read_namespace_via_membership, shared_with_group) && sharing_allowed? + can?(current_user, :admin_project, project) && + can?(current_user, :read_namespace_via_membership, shared_with_group) && + sharing_allowed? end def build_link diff --git a/app/services/projects/group_links/destroy_service.rb b/app/services/projects/group_links/destroy_service.rb index e0218ae087e..f0ac28c9216 100644 --- a/app/services/projects/group_links/destroy_service.rb +++ b/app/services/projects/group_links/destroy_service.rb @@ -4,8 +4,14 @@ module Projects module GroupLinks class DestroyService < BaseService def execute(group_link, skip_authorization: false) - unless valid_to_destroy?(group_link, skip_authorization) - return ServiceResponse.error(message: 'Not found', reason: :not_found) + return not_found! unless group_link + + unless skip_authorization + return not_found! unless allowed_to_manage_destroy?(group_link) + + unless allowed_to_destroy_link?(group_link) + return ServiceResponse.error(message: 'Forbidden', reason: :forbidden) + end end if group_link.project.private? @@ -30,11 +36,16 @@ module Projects private - def valid_to_destroy?(group_link, skip_authorization) - return false unless group_link - return true if skip_authorization + def not_found! + ServiceResponse.error(message: 'Not found', reason: :not_found) + end + + def allowed_to_manage_destroy?(group_link) + current_user.can?(:manage_destroy, group_link) + end - current_user.can?(:admin_project_group_link, group_link) + def allowed_to_destroy_link?(group_link) + current_user.can?(:destroy_project_group_link, group_link) end def refresh_project_authorizations_asynchronously(project) diff --git a/app/services/projects/group_links/update_service.rb b/app/services/projects/group_links/update_service.rb index 04f1552d929..1d657f2396d 100644 --- a/app/services/projects/group_links/update_service.rb +++ b/app/services/projects/group_links/update_service.rb @@ -10,7 +10,13 @@ module Projects end def execute(group_link_params) - return ServiceResponse.error(message: 'Not found', reason: :not_found) unless allowed_to_update? + if group_link.blank? || !allowed_to_update? + return ServiceResponse.error(message: 'Not found', reason: :not_found) + end + + unless allowed_to_update_to_or_from_owner?(group_link_params) + return ServiceResponse.error(message: 'Forbidden', reason: :forbidden) + end group_link.update!(group_link_params) @@ -24,7 +30,13 @@ module Projects attr_reader :group_link def allowed_to_update? - current_user.can?(:admin_project_member, project) + current_user.can?(:admin_project_member, group_link.project) + end + + def allowed_to_update_to_or_from_owner?(params) + return current_user.can?(:manage_owners, group_link) if upgrading_to_owner?(params) || touching_an_owner? + + true end def refresh_authorizations @@ -41,6 +53,14 @@ module Projects def requires_authorization_refresh?(params) params.include?(:group_access) end + + def upgrading_to_owner?(params) + params[:group_access].to_i == Gitlab::Access::OWNER + end + + def touching_an_owner? + group_link.owner_access? + end end end end diff --git a/app/views/groups/_home_panel.html.haml b/app/views/groups/_home_panel.html.haml index 5d1ebd60413..6b56fe0b75b 100644 --- a/app/views/groups/_home_panel.html.haml +++ b/app/views/groups/_home_panel.html.haml @@ -5,8 +5,7 @@ .group-home-panel .gl-display-flex.gl-justify-content-space-between.gl-flex-wrap.gl-sm-flex-direction-column.gl-md-flex-direction-row.gl-gap-3.gl-my-5 .home-panel-title-row.gl-display-flex - .avatar-container.rect-avatar.s48.home-panel-avatar.gl-flex-shrink-0.float-none{ class: 'gl-mr-3!' } - = render Pajamas::AvatarComponent.new(@group, alt: @group.name, size: 48, avatar_options: { itemprop: 'logo' }) + = render Pajamas::AvatarComponent.new(@group, alt: @group.name, size: 48, class: 'float-none gl-flex-shrink-0 gl-mr-3', avatar_options: { itemprop: 'logo' }) %div %h1.home-panel-title.gl-font-size-h-display.gl-mt-3.gl-mb-2.gl-ml-2.gl-display-flex.gl-align-items-center.gl-flex-wrap.gl-gap-3.gl-word-break-word{ itemprop: 'name' } = @group.name diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index 06d9a53efdb..a7caa797a46 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -7,9 +7,6 @@ - sidebar_data = super_sidebar_context(current_user, group: group, project: @project, panel: sidebar_panel, panel_type: nav).to_json %aside.js-super-sidebar.super-sidebar.super-sidebar-loading{ data: { root_path: root_path, sidebar: sidebar_data, force_desktop_expanded_sidebar: @force_desktop_expanded_sidebar.to_s, command_palette: command_palette_data(project: @project).to_json } } - - if display_whats_new? - #whats-new-app{ data: { version_digest: whats_new_version_digest } } - = render_if_exists "layouts/tanuki_bot_chat" - elsif defined?(nav) && nav diff --git a/app/views/projects/merge_requests/_code_dropdown.html.haml b/app/views/projects/merge_requests/_code_dropdown.html.haml index bfa33f26453..50f4e313bc5 100644 --- a/app/views/projects/merge_requests/_code_dropdown.html.haml +++ b/app/views/projects/merge_requests/_code_dropdown.html.haml @@ -1,6 +1,6 @@ .gl-md-ml-3.dropdown.gl-dropdown{ class: "gl-display-none! gl-md-display-flex!" } #js-check-out-modal{ data: how_merge_modal_data(@merge_request) } - = button_tag type: 'button', class: "btn dropdown-toggle btn-confirm gl-button gl-dropdown-toggle", data: { toggle: 'dropdown', testid: 'mr-code-dropdown' } do + = button_tag type: 'button', class: "btn dropdown-toggle btn-confirm-secondary gl-button gl-dropdown-toggle", data: { toggle: 'dropdown', testid: 'mr-code-dropdown' } do %span.gl-dropdown-button-text= _('Code') = sprite_icon "chevron-down", size: 16, css_class: "dropdown-icon gl-icon gl-ml-2 gl-mr-0!" .dropdown-menu.dropdown-menu-right diff --git a/app/views/projects/merge_requests/_widget.html.haml b/app/views/projects/merge_requests/_widget.html.haml index 9ec4363fa9a..721446eb017 100644 --- a/app/views/projects/merge_requests/_widget.html.haml +++ b/app/views/projects/merge_requests/_widget.html.haml @@ -22,6 +22,7 @@ window.gl.mrWidgetData.can_view_false_positive = '#{@merge_request.project.licensed_feature_available?(:sast_fp_reduction).to_s}'; window.gl.mrWidgetData.user_preferences_gitpod_path = '#{profile_preferences_path(anchor: 'user_gitpod_enabled')}'; window.gl.mrWidgetData.user_profile_enable_gitpod_path = '#{profile_path(user: { gitpod_enabled: true })}'; + window.gl.mrWidgetData.saml_approval_path = window.gl.mrWidgetData.saml_approval_path %h2#merge-request-widgets-heading.gl-sr-only = _("Merge request reports") diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 62101b4f3a6..9ea2a7a0281 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -7464,6 +7464,34 @@ Input type: `UpdateContainerExpirationPolicyInput` | <a id="mutationupdatecontainerexpirationpolicycontainerexpirationpolicy"></a>`containerExpirationPolicy` | [`ContainerExpirationPolicy`](#containerexpirationpolicy) | Container expiration policy after mutation. | | <a id="mutationupdatecontainerexpirationpolicyerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.updateContainerRegistryProtectionRule` + +Updates a container registry protection rule to restrict access to project containers. You can prevent users without certain roles from altering containers. Available only when feature flag `container_registry_protected_containers` is enabled. + +WARNING: +**Introduced** in 16.7. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `UpdateContainerRegistryProtectionRuleInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationupdatecontainerregistryprotectionruleclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationupdatecontainerregistryprotectionruledeleteprotecteduptoaccesslevel"></a>`deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel`](#containerregistryprotectionruleaccesslevel) | Maximum GitLab access level prevented from deleting a container. For example, `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| <a id="mutationupdatecontainerregistryprotectionruleid"></a>`id` | [`ContainerRegistryProtectionRuleID!`](#containerregistryprotectionruleid) | Global ID of the container registry protection rule to be updated. | +| <a id="mutationupdatecontainerregistryprotectionrulepushprotecteduptoaccesslevel"></a>`pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel`](#containerregistryprotectionruleaccesslevel) | Maximum GitLab access level prevented from pushing a container. For example, `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| <a id="mutationupdatecontainerregistryprotectionrulerepositorypathpattern"></a>`repositoryPathPattern` | [`String`](#string) | Container's repository path pattern of the protection rule. For example, `my-scope/my-project/container-dev-*`. Wildcard character `*` allowed. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationupdatecontainerregistryprotectionruleclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationupdatecontainerregistryprotectionrulecontainerregistryprotectionrule"></a>`containerRegistryProtectionRule` | [`ContainerRegistryProtectionRule`](#containerregistryprotectionrule) | Container registry protection rule after mutation. | +| <a id="mutationupdatecontainerregistryprotectionruleerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | + ### `Mutation.updateDependencyProxyImageTtlGroupPolicy` These settings can be adjusted by the group Owner or Maintainer. diff --git a/doc/user/project/pages/custom_domains_ssl_tls_certification/index.md b/doc/user/project/pages/custom_domains_ssl_tls_certification/index.md index 69fd974a21e..345a30da198 100644 --- a/doc/user/project/pages/custom_domains_ssl_tls_certification/index.md +++ b/doc/user/project/pages/custom_domains_ssl_tls_certification/index.md @@ -273,7 +273,7 @@ meet these requirements. **Do not** open certificates or encryption keys in regular text editors. Always use code editors (such as -Sublime Text, Atom, Dreamweaver, Brackets, etc). +Sublime Text, Dreamweaver, Brackets, etc). ## Force HTTPS for GitLab Pages websites diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 3b80fd125ca..de1ed75ca70 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -755,7 +755,7 @@ module API end params do requires :group_id, type: Integer, desc: 'The ID of a group', documentation: { example: 1 } - requires :group_access, type: Integer, values: Gitlab::Access.values, as: :link_group_access, desc: 'The group access level' + requires :group_access, type: Integer, values: Gitlab::Access.all_values, as: :link_group_access, desc: 'The group access level' optional :expires_at, type: Date, desc: 'Share expiration date' end post ":id/share", feature_category: :groups_and_projects do @@ -798,8 +798,7 @@ module API result = ::Projects::GroupLinks::DestroyService.new(user_project, current_user).execute(link) if result.error? - status = :not_found if result.reason == :not_found - render_api_error!(result.message, status) + render_api_error!(result.message, result.reason) end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 57289a90aff..cfe69105890 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -550,9 +550,6 @@ msgstr "" msgid "%{author} has added you as an approver." msgstr "" -msgid "%{author} requested to merge %{source_branch} %{copy_button} into %{target_branch} %{created_at}" -msgstr "" - msgid "%{board_target} not found" msgstr "" @@ -51736,6 +51733,9 @@ msgstr "" msgid "Unauthorized to delete a package protection rule" msgstr "" +msgid "Unauthorized to update a container registry protection rule" +msgstr "" + msgid "Unauthorized to update a package protection rule" msgstr "" diff --git a/qa/qa/page/component/members/members_table.rb b/qa/qa/page/component/members/members_table.rb index 655a28f947e..7d6431d7b32 100644 --- a/qa/qa/page/component/members/members_table.rb +++ b/qa/qa/page/component/members/members_table.rb @@ -18,12 +18,12 @@ module QA end base.view 'app/assets/javascripts/members/components/table/members_table.vue' do - element :member_row + element 'members-table' end base.view 'app/assets/javascripts/members/components/table/max_role.vue' do - element :access_level_dropdown - element :access_level_link + element 'access-level-dropdown' + element 'access-level-link' end base.view 'app/assets/javascripts/members/components/action_dropdowns/user_action_dropdown.vue' do @@ -43,23 +43,23 @@ module QA end base.view 'app/assets/javascripts/members/components/action_buttons/remove_group_link_button.vue' do - element :remove_group_link_button + element('remove-group-link-button') end end def update_access_level(username, access_level) search_member(username) - within_element(:member_row, text: username) do - click_element :access_level_dropdown - click_element :access_level_link, text: access_level + within_element('members-table', text: username) do + click_element('access-level-dropdown') + click_element('access-level-link', text: access_level) end click_confirmation_ok_button_if_present end def remove_member(username) - within_element(:member_row, text: username) do + within_element('members-table', text: username) do click_element 'user-action-dropdown' click_element 'delete-member-dropdown-item' end @@ -68,13 +68,13 @@ module QA end def approve_access_request(username) - within_element(:member_row, text: username) do + within_element('members-table', text: username) do click_element 'approve-access-request-button' end end def deny_access_request(username) - within_element(:member_row, text: username) do + within_element('members-table', text: username) do click_element 'delete-member-button' end @@ -84,8 +84,8 @@ module QA def remove_group(group_name) click_element 'groups-list-tab' - within_element(:member_row, text: group_name) do - click_element :remove_group_link_button + within_element('members-table', text: group_name) do + click_element('remove-group-link-button') end confirm_remove_group @@ -93,7 +93,7 @@ module QA def has_group?(group_name) click_element 'groups-list-tab' - has_element?(:member_row, text: group_name) + has_element?('members-table', text: group_name) end end end diff --git a/qa/qa/page/component/members/remove_group_modal.rb b/qa/qa/page/component/members/remove_group_modal.rb index 03c51757b62..b2be79604b6 100644 --- a/qa/qa/page/component/members/remove_group_modal.rb +++ b/qa/qa/page/component/members/remove_group_modal.rb @@ -11,16 +11,16 @@ module QA super base.view 'app/assets/javascripts/members/components/modals/remove_group_link_modal.vue' do - element :remove_group_link_modal_content - element :remove_group_button + element('remove-group-link-modal-content') + element('remove-group-button') end end def confirm_remove_group - within_element(:remove_group_link_modal_content) do + within_element('remove-group-link-modal-content') do wait_for_enabled_remove_group_button - click_element :remove_group_button + click_element('remove-group-button') end end @@ -28,7 +28,7 @@ module QA def wait_for_enabled_remove_group_button retry_until(sleep_interval: 1, message: 'Waiting for remove group button to be enabled') do - has_element?(:remove_group_button, disabled: false, wait: 3) + has_element?('remove-group-button', disabled: false, wait: 3) end end end diff --git a/qa/qa/page/component/members/remove_member_modal.rb b/qa/qa/page/component/members/remove_member_modal.rb index b7b81040ba7..414e1ba5b08 100644 --- a/qa/qa/page/component/members/remove_member_modal.rb +++ b/qa/qa/page/component/members/remove_member_modal.rb @@ -11,16 +11,16 @@ module QA super base.view 'app/assets/javascripts/members/components/modals/remove_member_modal.vue' do - element :remove_member_modal - element :remove_member_button + element 'remove-member-modal' + element 'remove-member-button' end end def confirm_remove_member - within_element(:remove_member_modal) do + within_element('remove-member-modal') do wait_for_enabled_remove_member_button - click_element :remove_member_button + click_element('remove-member-button') end end @@ -28,7 +28,7 @@ module QA def wait_for_enabled_remove_member_button retry_until(sleep_interval: 1, message: 'Waiting for remove member button to be enabled') do - has_element?(:remove_member_button, disabled: false, wait: 3) + has_element?('remove-member-button', disabled: false, wait: 3) end end end diff --git a/qa/qa/tools/reliable_report.rb b/qa/qa/tools/reliable_report.rb index 96e9d53cac3..7569095b56f 100644 --- a/qa/qa/tools/reliable_report.rb +++ b/qa/qa/tools/reliable_report.rb @@ -31,6 +31,7 @@ module QA PROJECT_ID = 278964 FEATURES_DIR = 'https://gitlab.com/gitlab-org/gitlab/-/blob/master/qa/qa/specs/features/' + # @param [Integer] range amount of days for results range def initialize(range) @range = range.to_i @slack_channel = "#quality-reports" diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 4510e9e646e..e7a08c55a70 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -76,6 +76,17 @@ RSpec.describe Projects::GroupLinksController, feature_category: :system_access expect(response).to have_gitlab_http_status(:not_found) expect(json_response['message']).to eq('404 Not Found') end + + context 'when MAINTAINER tries to update the link to OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it 'returns 403' do + update_link + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq('Forbidden') + end + end end describe '#destroy' do @@ -167,12 +178,41 @@ RSpec.describe Projects::GroupLinksController, feature_category: :system_access sign_in(user) end - it 'renders 404' do - destroy_link + it 'returns 404' do + expect { destroy_link }.to not_change { project.reload.project_group_links.count } expect(response).to have_gitlab_http_status(:not_found) end end end + + context 'when the user is a project maintainer' do + before do + project.add_maintainer(user) + sign_in(user) + end + + context 'when they try to destroy a link with OWNER access level' do + let(:group_access) { Gitlab::Access::OWNER } + + it 'does not destroy the link' do + expect { destroy_link }.to not_change { project.reload.project_group_links.count } + + expect(response).to redirect_to(project_project_members_path(project, tab: :groups)) + expect(flash[:alert]).to include('The project-group link could not be removed.') + end + + context 'when format is js' do + let(:format) { :js } + + it 'returns 403' do + expect { destroy_link }.to not_change { project.reload.project_group_links.count } + + expect(json_response).to eq({ "message" => "Forbidden" }) + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + end end end diff --git a/spec/factories/project_group_links.rb b/spec/factories/project_group_links.rb index 5edd57d5fe1..89204f81a6b 100644 --- a/spec/factories/project_group_links.rb +++ b/spec/factories/project_group_links.rb @@ -11,6 +11,7 @@ FactoryBot.define do trait(:reporter) { group_access { Gitlab::Access::REPORTER } } trait(:developer) { group_access { Gitlab::Access::DEVELOPER } } trait(:maintainer) { group_access { Gitlab::Access::MAINTAINER } } + trait(:owner) { group_access { Gitlab::Access::OWNER } } after(:create) do |project_group_link| project_group_link.run_after_commit_or_now do diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb index eab5cee976e..fdc996ef39b 100644 --- a/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/spec/features/merge_request/user_creates_merge_request_spec.rb @@ -98,7 +98,7 @@ RSpec.describe 'User creates a merge request', :js, feature_category: :code_revi click_button('Create merge request') - expect(page).to have_content(title).and have_content("requested to merge #{forked_project.full_path}:fix into master") + expect(page).to have_content(title).and have_content("• #{forked_project.full_path}:fix ➔ master") end end end diff --git a/spec/features/merge_request/user_edits_merge_request_spec.rb b/spec/features/merge_request/user_edits_merge_request_spec.rb index 584a17ae33d..31b382be611 100644 --- a/spec/features/merge_request/user_edits_merge_request_spec.rb +++ b/spec/features/merge_request/user_edits_merge_request_spec.rb @@ -95,7 +95,7 @@ RSpec.describe 'User edits a merge request', :js, feature_category: :code_review click_button('Save changes') - expect(page).to have_content("requested to merge #{merge_request.source_branch} into merge-test") + expect(page).to have_content("• #{merge_request.source_branch} ➔ merge-test") expect(page).to have_content("changed target branch from #{merge_request.target_branch} to merge-test") end diff --git a/spec/finders/timelogs/timelogs_finder_spec.rb b/spec/finders/timelogs/timelogs_finder_spec.rb new file mode 100644 index 00000000000..35691a46e23 --- /dev/null +++ b/spec/finders/timelogs/timelogs_finder_spec.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Timelogs::TimelogsFinder, feature_category: :team_planning do + let_it_be(:current_user) { create(:user) } + let_it_be(:group_a) { create(:group) } + let_it_be(:group_b) { create(:group) } + let_it_be(:project_a) { create(:project, :empty_repo, :public, group: group_a) } + let_it_be(:project_b) { create(:project, :empty_repo, :public, group: group_a) } + let_it_be(:project_c) { create(:project, :empty_repo, :public, group: group_b) } + + let_it_be(:issue_a) { create(:issue, project: project_a) } + let_it_be(:issue_b) { create(:issue, project: project_b) } + let_it_be(:issue_c) { create(:issue, project: project_c) } + let_it_be(:merge_request) { create(:merge_request, source_project: project_a) } + + let_it_be(:timelog1) do + create(:issue_timelog, issue: issue_a, user: current_user, spent_at: 2.days.ago.beginning_of_day, time_spent: 3000) + end + + let_it_be(:timelog2) do + create(:issue_timelog, issue: issue_a, user: create(:user), spent_at: 2.days.ago.end_of_day, time_spent: 4000) + end + + let_it_be(:timelog3) do + create(:merge_request_timelog, + merge_request: merge_request, + user: current_user, + spent_at: 10.days.ago, + time_spent: 2000) + end + + let_it_be(:timelog4) do + create(:issue_timelog, issue: issue_b, user: current_user, spent_at: 1.hour.ago, time_spent: 500) + end + + let_it_be(:timelog5) do + create(:issue_timelog, issue: issue_c, user: create(:user), spent_at: 7.days.ago.end_of_day, time_spent: 6000) + end + + subject(:finder_results) { described_class.new(issuable, params).execute } + + describe '#execute' do + let(:issuable) { nil } + let(:params) { {} } + + context 'when params is empty' do + it 'returns all timelogs' do + expect(finder_results).to contain_exactly(timelog1, timelog2, timelog3, timelog4, timelog5) + end + end + + context 'when an issuable is provided' do + let(:issuable) { issue_a } + + it 'returns the issuable timelogs' do + expect(finder_results).to contain_exactly(timelog1, timelog2) + end + end + + context 'when a username is provided' do + let(:params) { { username: current_user.username } } + + it 'returns all timelogs created by the user' do + expect(finder_results).to contain_exactly(timelog1, timelog3, timelog4) + end + end + + context 'when a group is provided' do + let(:params) { { group_id: group_a.id } } + + it 'returns all timelogs of issuables inside that group' do + expect(finder_results).to contain_exactly(timelog1, timelog2, timelog3, timelog4) + end + + context 'when the group does not exist' do + let(:params) { { group_id: non_existing_record_id } } + + it 'raises an exception' do + expect { finder_results }.to raise_error( + ActiveRecord::RecordNotFound, /Group with id '\d+' could not be found/) + end + end + end + + context 'when a project is provided' do + let(:params) { { project_id: project_a.id } } + + it 'returns all timelogs of issuables inside that project' do + expect(finder_results).to contain_exactly(timelog1, timelog2, timelog3) + end + + context 'when the project does not exist' do + let(:params) { { project_id: non_existing_record_id } } + + it 'returns an empty list and does not raise an exception' do + expect(finder_results).to be_empty + expect { finder_results }.not_to raise_error + end + end + end + + context 'when a start datetime is provided' do + let(:params) { { start_time: 3.days.ago.beginning_of_day } } + + it 'returns all timelogs created after that date' do + expect(finder_results).to contain_exactly(timelog1, timelog2, timelog4) + end + end + + context 'when an end datetime is provided' do + let(:params) { { end_time: 3.days.ago.beginning_of_day } } + + it 'returns all timelogs created before that date' do + expect(finder_results).to contain_exactly(timelog3, timelog5) + end + end + + context 'when both a start and an end datetime are provided' do + let(:params) { { start_time: 2.days.ago.beginning_of_day, end_time: 1.day.ago.beginning_of_day } } + + it 'returns all timelogs created between those dates' do + expect(finder_results).to contain_exactly(timelog1, timelog2) + end + + context 'when start time is after end time' do + let(:params) { { start_time: 1.day.ago.beginning_of_day, end_time: 2.days.ago.beginning_of_day } } + + it 'raises an exception' do + expect { finder_results }.to raise_error(ArgumentError, /Start argument must be before End argument/) + end + end + end + + context 'when sort is provided' do + let(:params) { { sort: sort_value } } + + context 'when sorting by spent_at desc' do + let(:sort_value) { :spent_at_desc } + + it 'returns timelogs sorted accordingly' do + expect(finder_results).to eq([timelog4, timelog2, timelog1, timelog5, timelog3]) + end + end + + context 'when sorting by spent_at asc' do + let(:sort_value) { :spent_at_asc } + + it 'returns timelogs sorted accordingly' do + expect(finder_results).to eq([timelog3, timelog5, timelog1, timelog2, timelog4]) + end + end + + context 'when sorting by time_spent desc' do + let(:sort_value) { :time_spent_desc } + + it 'returns timelogs sorted accordingly' do + expect(finder_results).to eq([timelog5, timelog2, timelog1, timelog3, timelog4]) + end + end + + context 'when sorting by time_spent asc' do + let(:sort_value) { :time_spent_asc } + + it 'returns timelogs sorted accordingly' do + expect(finder_results).to eq([timelog4, timelog3, timelog1, timelog2, timelog5]) + end + end + end + end +end diff --git a/spec/fixtures/api/schemas/group_link/group_group_link.json b/spec/fixtures/api/schemas/group_link/group_group_link.json index 689679cbc0f..d67fdaaf762 100644 --- a/spec/fixtures/api/schemas/group_link/group_group_link.json +++ b/spec/fixtures/api/schemas/group_link/group_group_link.json @@ -1,21 +1,45 @@ { "type": "object", "allOf": [ - { "$ref": "group_link.json" }, + { + "$ref": "group_link.json" + }, { "required": [ - "source" + "source", + "valid_roles", + "can_update", + "can_remove" ], "properties": { "source": { "type": "object", - "required": ["id", "full_name", "web_url"], + "required": [ + "id", + "full_name", + "web_url" + ], "properties": { - "id": { "type": "integer" }, - "full_name": { "type": "string" }, - "web_url": { "type": "string" } + "id": { + "type": "integer" + }, + "full_name": { + "type": "string" + }, + "web_url": { + "type": "string" + } }, "additionalProperties": false + }, + "valid_roles": { + "type": "object" + }, + "can_update": { + "type": "boolean" + }, + "can_remove": { + "type": "boolean" } } } diff --git a/spec/fixtures/api/schemas/group_link/group_link.json b/spec/fixtures/api/schemas/group_link/group_link.json index 4db38952ecc..421864b2bd7 100644 --- a/spec/fixtures/api/schemas/group_link/group_link.json +++ b/spec/fixtures/api/schemas/group_link/group_link.json @@ -5,9 +5,6 @@ "created_at", "expires_at", "access_level", - "valid_roles", - "can_update", - "can_remove", "is_direct_member" ], "properties": { @@ -41,9 +38,6 @@ }, "additionalProperties": false }, - "valid_roles": { - "type": "object" - }, "is_shared_with_group_private": { "type": "boolean" }, @@ -82,12 +76,6 @@ }, "additionalProperties": false }, - "can_update": { - "type": "boolean" - }, - "can_remove": { - "type": "boolean" - }, "is_direct_member": { "type": "boolean" } diff --git a/spec/fixtures/api/schemas/group_link/project_group_link.json b/spec/fixtures/api/schemas/group_link/project_group_link.json index 615c808e5aa..854ae3c8693 100644 --- a/spec/fixtures/api/schemas/group_link/project_group_link.json +++ b/spec/fixtures/api/schemas/group_link/project_group_link.json @@ -1,20 +1,41 @@ { "type": "object", "allOf": [ - { "$ref": "group_link.json" }, + { + "$ref": "group_link.json" + }, { "required": [ - "source" + "source", + "valid_roles", + "can_update", + "can_remove" ], "properties": { "source": { "type": "object", - "required": ["id", "full_name"], + "required": [ + "id", + "full_name" + ], "properties": { - "id": { "type": "integer" }, - "full_name": { "type": "string" } + "id": { + "type": "integer" + }, + "full_name": { + "type": "string" + } }, "additionalProperties": false + }, + "valid_roles": { + "type": "object" + }, + "can_update": { + "type": "boolean" + }, + "can_remove": { + "type": "boolean" } } } diff --git a/spec/frontend/fixtures/static/whats_new_notification.html b/spec/frontend/fixtures/static/whats_new_notification.html deleted file mode 100644 index bc8a27c779f..00000000000 --- a/spec/frontend/fixtures/static/whats_new_notification.html +++ /dev/null @@ -1,7 +0,0 @@ -<div class='whats-new-notification-fixture-root'> - <div class='app' data-version-digest='version-digest'></div> - <div data-testid='without-digest'></div> - <div class='header-help'> - <div class='js-whats-new-notification-count'></div> - </div> -</div> diff --git a/spec/frontend/super_sidebar/components/help_center_spec.js b/spec/frontend/super_sidebar/components/help_center_spec.js index 8ef742ba946..8e9e3e8ba20 100644 --- a/spec/frontend/super_sidebar/components/help_center_spec.js +++ b/spec/frontend/super_sidebar/components/help_center_spec.js @@ -168,14 +168,13 @@ describe('HelpCenter component', () => { describe('showWhatsNew', () => { beforeEach(() => { - beforeEach(() => { - createWrapper({ ...sidebarData, show_version_check: true }); - }); + createWrapper({ ...sidebarData, show_version_check: true }); + findButton("What's new 5").click(); }); it('shows the "What\'s new" slideout', () => { - expect(toggleWhatsNewDrawer).toHaveBeenCalledWith(expect.any(Object)); + expect(toggleWhatsNewDrawer).toHaveBeenCalledWith(sidebarData.whats_new_version_digest); }); it('shows the existing "What\'s new" slideout instance on subsequent clicks', () => { diff --git a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js index c81f4328d2a..c3ed131d6e3 100644 --- a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js +++ b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js @@ -1,11 +1,11 @@ import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import { GlButton, GlSprintf } from '@gitlab/ui'; -import { shallowMount } from '@vue/test-utils'; import { createMockSubscription as createMockApolloSubscription } from 'mock-apollo-client'; import approvedByCurrentUser from 'test_fixtures/graphql/merge_requests/approvals/approvals.query.graphql.json'; -import { visitUrl } from '~/lib/utils/url_utility'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; +import { stubComponent } from 'helpers/stub_component'; import waitForPromises from 'helpers/wait_for_promises'; import { getIdFromGraphQLId } from '~/graphql_shared/utils'; import { createAlert } from '~/alert'; @@ -29,11 +29,6 @@ jest.mock('~/alert', () => ({ dismiss: mockAlertDismiss, })), })); -jest.mock('~/lib/utils/url_utility', () => ({ - ...jest.requireActual('~/lib/utils/url_utility'), - visitUrl: jest.fn(), -})); - const TEST_HELP_PATH = 'help/path'; const testApprovedBy = () => [1, 7, 10].map((id) => ({ id })); const testApprovals = () => ({ @@ -53,6 +48,9 @@ describe('MRWidget approvals', () => { let wrapper; let service; let mr; + const submitSpy = jest.fn().mockImplementation((e) => { + e.preventDefault(); + }); const createComponent = (options = {}, responses = { query: approvedByCurrentUser }) => { mockedSubscription = createMockApolloSubscription(); @@ -68,7 +66,7 @@ describe('MRWidget approvals', () => { apolloProvider.defaultClient.setRequestHandler(query, stream); }); - wrapper = shallowMount(Approvals, { + wrapper = shallowMountExtended(Approvals, { apolloProvider, propsData: { mr, @@ -78,7 +76,18 @@ describe('MRWidget approvals', () => { provide, stubs: { GlSprintf, + GlForm: { + data() { + return { submitSpy }; + }, + // Workaround jsdom not implementing form submit + template: '<form @submit="submitSpy"><slot></slot></form>', + }, + GlButton: stubComponent(GlButton, { + template: '<button><slot></slot></button>', + }), }, + attachTo: document.body, }); }; @@ -257,11 +266,11 @@ describe('MRWidget approvals', () => { }); describe('when SAML auth is required and user clicks Approve with SAML', () => { - const fakeGroupSamlPath = '/example_group_saml'; + const fakeSamlPath = '/example_group_saml'; beforeEach(async () => { mr.requireSamlAuthToApprove = true; - mr.samlApprovalPath = fakeGroupSamlPath; + mr.samlApprovalPath = fakeSamlPath; createComponent({}, { query: createCanApproveResponse() }); await waitForPromises(); @@ -269,9 +278,10 @@ describe('MRWidget approvals', () => { it('redirects the user to the group SAML path', async () => { const action = findAction(); - action.vm.$emit('click'); - await nextTick(); - expect(visitUrl).toHaveBeenCalledWith(fakeGroupSamlPath); + + await action.trigger('click'); + + expect(submitSpy).toHaveBeenCalled(); }); }); diff --git a/spec/frontend/whats_new/utils/notification_spec.js b/spec/frontend/whats_new/utils/notification_spec.js deleted file mode 100644 index 020d833c578..00000000000 --- a/spec/frontend/whats_new/utils/notification_spec.js +++ /dev/null @@ -1,73 +0,0 @@ -import htmlWhatsNewNotification from 'test_fixtures_static/whats_new_notification.html'; -import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; -import { useLocalStorageSpy } from 'helpers/local_storage_helper'; -import { setNotification, getVersionDigest } from '~/whats_new/utils/notification'; - -describe('~/whats_new/utils/notification', () => { - useLocalStorageSpy(); - - let wrapper; - - const findNotificationEl = () => wrapper.querySelector('.header-help'); - const findNotificationCountEl = () => wrapper.querySelector('.js-whats-new-notification-count'); - const getAppEl = () => wrapper.querySelector('.app'); - - beforeEach(() => { - setHTMLFixture(htmlWhatsNewNotification); - wrapper = document.querySelector('.whats-new-notification-fixture-root'); - }); - - afterEach(() => { - wrapper.remove(); - resetHTMLFixture(); - }); - - describe('setNotification', () => { - const subject = () => setNotification(getAppEl()); - - it("when storage key doesn't exist it adds notifications class", () => { - const notificationEl = findNotificationEl(); - - expect(notificationEl.classList).not.toContain('with-notifications'); - - subject(); - - expect(findNotificationCountEl()).not.toBe(null); - expect(notificationEl.classList).toContain('with-notifications'); - }); - - it('removes class and count element when storage key has current digest', () => { - const notificationEl = findNotificationEl(); - - notificationEl.classList.add('with-notifications'); - localStorage.setItem('display-whats-new-notification', 'version-digest'); - - expect(findNotificationCountEl()).not.toBe(null); - - subject(); - - expect(findNotificationCountEl()).toBe(null); - expect(notificationEl.classList).not.toContain('with-notifications'); - }); - - it('removes class and count element when no records and digest undefined', () => { - const notificationEl = findNotificationEl(); - - notificationEl.classList.add('with-notifications'); - localStorage.setItem('display-whats-new-notification', 'version-digest'); - - expect(findNotificationCountEl()).not.toBe(null); - - setNotification(wrapper.querySelector('[data-testid="without-digest"]')); - - expect(findNotificationCountEl()).toBe(null); - expect(notificationEl.classList).not.toContain('with-notifications'); - }); - }); - - describe('getVersionDigest', () => { - it('retrieves the storage key data attribute from the el', () => { - expect(getVersionDigest(getAppEl())).toBe('version-digest'); - }); - }); -}); diff --git a/spec/graphql/resolvers/timelog_resolver_spec.rb b/spec/graphql/resolvers/timelog_resolver_spec.rb index 798d8a56cf5..c4253f4b9bc 100644 --- a/spec/graphql/resolvers/timelog_resolver_spec.rb +++ b/spec/graphql/resolvers/timelog_resolver_spec.rb @@ -29,6 +29,14 @@ RSpec.describe Resolvers::TimelogResolver, feature_category: :team_planning do expect(timelogs).to contain_exactly(timelog1) end + context 'when the project does not exist' do + let(:extra_args) { { project_id: "gid://gitlab/Project/#{non_existing_record_id}" } } + + it 'returns an empty set' do + expect(timelogs).to be_empty + end + end + context 'when no dates specified' do let(:args) { {} } @@ -137,6 +145,20 @@ RSpec.describe Resolvers::TimelogResolver, feature_category: :team_planning do expect(timelogs).to contain_exactly(timelog1) end + context 'when the group does not exist' do + let_it_be(:error_class) { Gitlab::Graphql::Errors::ResourceNotAvailable } + + let(:extra_args) { { group_id: "gid://gitlab/Group/#{non_existing_record_id}" } } + + it 'returns an error' do + expect_graphql_error_to_be_created(error_class, + "The resource that you are attempting to access does not exist or " \ + "you don't have permission to perform this action") do + timelogs + end + end + end + context 'when only start_date is present' do let(:args) { { start_date: short_time_ago } } diff --git a/spec/models/project_group_link_spec.rb b/spec/models/project_group_link_spec.rb index f141b8e83d6..bfbb7c91f47 100644 --- a/spec/models/project_group_link_spec.rb +++ b/spec/models/project_group_link_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ProjectGroupLink do +RSpec.describe ProjectGroupLink, feature_category: :groups_and_projects do describe "Associations" do it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:project) } @@ -18,6 +18,7 @@ RSpec.describe ProjectGroupLink do it { is_expected.to validate_uniqueness_of(:group_id).scoped_to(:project_id).with_message(/already shared/) } it { is_expected.to validate_presence_of(:group) } it { is_expected.to validate_presence_of(:group_access) } + it { is_expected.to validate_inclusion_of(:group_access).in_array(Gitlab::Access.all_values) } it "doesn't allow a project to be shared with the group it is in" do project_group_link.group = group @@ -30,12 +31,6 @@ RSpec.describe ProjectGroupLink do expect(project_group_link).not_to be_valid end - - it 'does not allow a project to be shared with `OWNER` access level' do - project_group_link.group_access = Gitlab::Access::OWNER - - expect(project_group_link).not_to be_valid - end end describe 'scopes' do @@ -62,4 +57,27 @@ RSpec.describe ProjectGroupLink do it { expect(described_class.search(group.name)).to eq([project_group_link]) } it { expect(described_class.search('not-a-group-name')).to be_empty } end + + describe '#owner_access?' do + it 'returns true for links with OWNER access' do + link = create(:project_group_link, :owner) + + expect(link.owner_access?).to eq(true) + end + + it 'returns false for links without OWNER access' do + link = create(:project_group_link, :guest) + + expect(link.owner_access?).to eq(false) + end + end + + describe '#human_access' do + it 'delegates to Gitlab::Access' do + project_group_link = create(:project_group_link, :reporter) + expect(Gitlab::Access).to receive(:human_access).with(project_group_link.group_access).and_call_original + + expect(project_group_link.human_access).to eq('Reporter') + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e6e3bba799a..d263bcde0ca 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -5433,8 +5433,7 @@ RSpec.describe User, feature_category: :user_profile do before do shared_project.project_group_links.create!( - group: group2, - group_access: ProjectGroupLink.default_access + group: group2 ) group2.add_member(user, GroupMember::OWNER) diff --git a/spec/policies/project_group_link_policy_spec.rb b/spec/policies/project_group_link_policy_spec.rb index 1047d3acb1e..9fcd6ead524 100644 --- a/spec/policies/project_group_link_policy_spec.rb +++ b/spec/policies/project_group_link_policy_spec.rb @@ -7,21 +7,75 @@ RSpec.describe ProjectGroupLinkPolicy, feature_category: :system_access do let_it_be(:group2) { create(:group, :private) } let_it_be(:project) { create(:project, :private) } - let(:project_group_link) do - create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::DEVELOPER) + subject(:policy) { described_class.new(user, project_group_link) } + + describe 'destroy_project_group_link' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::DEVELOPER) + end + + context 'when the user is a group owner' do + before_all do + group2.add_owner(user) + end + + it 'can destroy group_project_link' do + expect(policy).to be_allowed(:destroy_project_group_link) + end + + context 'when group link has owner access' do + it 'can destroy group_project_link' do + project_group_link.update!(group_access: Gitlab::Access::OWNER) + + expect(policy).to be_allowed(:destroy_project_group_link) + end + end + end + + context 'when user is a project maintainer' do + before do + project_group_link.project.add_maintainer(user) + end + + context 'when group link has owner access' do + it 'cannot destroy group_project_link' do + project_group_link.update!(group_access: Gitlab::Access::OWNER) + + expect(policy).to be_disallowed(:destroy_project_group_link) + end + end + + context 'when group link has maintainer access' do + it 'can destroy group_project_link' do + project_group_link.update!(group_access: Gitlab::Access::MAINTAINER) + + expect(policy).to be_allowed(:destroy_project_group_link) + end + end + end + + context 'when user is not a project maintainer' do + it 'cannot destroy group_project_link' do + project_group_link.project.add_developer(user) + + expect(policy).to be_disallowed(:destroy_project_group_link) + end + end end - subject(:policy) { described_class.new(user, project_group_link) } + describe 'manage_destroy' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::DEVELOPER) + end - describe 'admin_project_group_link' do context 'when the user is a group owner' do before_all do group2.add_owner(user) end context 'when user is not project maintainer' do - it 'can admin group_project_link' do - expect(policy).to be_allowed(:admin_project_group_link) + it 'can manage_destroy' do + expect(policy).to be_allowed(:manage_destroy) end end @@ -30,32 +84,50 @@ RSpec.describe ProjectGroupLinkPolicy, feature_category: :system_access do project_group_link.project.add_maintainer(user) end - it 'can admin group_project_link' do - expect(policy).to be_allowed(:admin_project_group_link) + it 'can admin manage_destroy' do + expect(policy).to be_allowed(:manage_destroy) end end end context 'when user is not a group owner' do context 'when user is a project maintainer' do - it 'can admin group_project_link' do + before do project_group_link.project.add_maintainer(user) + end + + context 'when group link has owner access' do + it 'can manage_destroy' do + project_group_link.update!(group_access: Gitlab::Access::OWNER) - expect(policy).to be_allowed(:admin_project_group_link) + expect(policy).to be_allowed(:manage_destroy) + end + end + + context 'when group link has maintainer access' do + it 'can manage_destroy' do + project_group_link.update!(group_access: Gitlab::Access::MAINTAINER) + + expect(policy).to be_allowed(:manage_destroy) + end end end context 'when user is not a project maintainer' do - it 'cannot admin group_project_link' do + it 'cannot manage_destroy' do project_group_link.project.add_developer(user) - expect(policy).to be_disallowed(:admin_project_group_link) + expect(policy).to be_disallowed(:manage_destroy) end end end end describe 'read_shared_with_group' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::MAINTAINER) + end + context 'when the user is a project member' do before_all do project.add_guest(user) @@ -83,9 +155,9 @@ RSpec.describe ProjectGroupLinkPolicy, feature_category: :system_access do end context 'when the group is public' do - let_it_be(:group2) { create(:group, :public) } - it 'can read_shared_with_group' do + group2.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + expect(policy).to be_allowed(:read_shared_with_group) end end @@ -102,4 +174,68 @@ RSpec.describe ProjectGroupLinkPolicy, feature_category: :system_access do end end end + + describe 'manage_owners' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::MAINTAINER) + end + + context 'when the user is a project owner' do + before_all do + project.add_owner(user) + end + + it 'can manage_owners' do + expect(policy).to be_allowed(:manage_owners) + end + end + + context 'when the user is a project maintainer' do + before_all do + project.add_maintainer(user) + end + + it 'cannot manage_owners' do + expect(policy).to be_disallowed(:manage_owners) + end + end + end + + describe 'manage_group_link_with_owner_access' do + context 'when group link has owner access' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::OWNER) + end + + context 'when the user is a project owner' do + before_all do + project.add_owner(user) + end + + it 'can manage_group_link_with_owner_access' do + expect(policy).to be_allowed(:manage_group_link_with_owner_access) + end + end + + context 'when the user is a project maintainer' do + before_all do + project.add_maintainer(user) + end + + it 'cannot manage_group_link_with_owner_access' do + expect(policy).to be_disallowed(:manage_group_link_with_owner_access) + end + end + end + + context 'when group link has maintainer access' do + let_it_be(:project_group_link) do + create(:project_group_link, project: project, group: group2, group_access: Gitlab::Access::MAINTAINER) + end + + it 'can manage_group_link_with_owner_access' do + expect(policy).to be_allowed(:manage_group_link_with_owner_access) + end + end + end end diff --git a/spec/requests/api/graphql/mutations/container_registry/protection/rule/update_spec.rb b/spec/requests/api/graphql/mutations/container_registry/protection/rule/update_spec.rb new file mode 100644 index 00000000000..cd2c8b9f0a2 --- /dev/null +++ b/spec/requests/api/graphql/mutations/container_registry/protection/rule/update_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Updating the container registry protection rule', :aggregate_failures, feature_category: :container_registry do + include GraphqlHelpers + + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, push_protected_up_to_access_level: :developer) + end + + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + + let(:container_registry_protection_rule_attributes) do + build_stubbed(:container_registry_protection_rule, project: project) + end + + let(:mutation) do + graphql_mutation(:update_container_registry_protection_rule, input, + <<~QUERY + containerRegistryProtectionRule { + repositoryPathPattern + deleteProtectedUpToAccessLevel + pushProtectedUpToAccessLevel + } + clientMutationId + errors + QUERY + ) + end + + let(:input) do + { + id: container_registry_protection_rule.to_global_id, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-updated", + delete_protected_up_to_access_level: 'OWNER', + push_protected_up_to_access_level: 'MAINTAINER' + } + end + + let(:mutation_response) { graphql_mutation_response(:update_container_registry_protection_rule) } + + subject { post_graphql_mutation(mutation, current_user: current_user) } + + shared_examples 'a successful response' do + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it 'returns the updated container registry protection rule' do + subject + + expect(mutation_response).to include( + 'containerRegistryProtectionRule' => { + 'repositoryPathPattern' => input[:repository_path_pattern], + 'deleteProtectedUpToAccessLevel' => input[:delete_protected_up_to_access_level], + 'pushProtectedUpToAccessLevel' => input[:push_protected_up_to_access_level] + } + ) + end + + it do + subject.tap do + expect(container_registry_protection_rule.reload).to have_attributes( + repository_path_pattern: input[:repository_path_pattern], + push_protected_up_to_access_level: input[:push_protected_up_to_access_level].downcase + ) + end + end + end + + shared_examples 'an erroneous reponse' do + it { subject.tap { expect(mutation_response).to be_blank } } + it { expect { subject }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful response' + + context 'with other existing container registry protection rule with same repository_path_pattern' do + let_it_be_with_reload(:other_existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-other") + end + + let(:input) do + super().merge(repository_path_pattern: other_existing_container_registry_protection_rule.repository_path_pattern) + end + + it { is_expected.tap { expect_graphql_errors_to_be_empty } } + + it 'returns a blank container registry protection rule' do + is_expected.tap { expect(mutation_response['containerRegistryProtectionRule']).to be_blank } + end + + it 'includes error message in response' do + is_expected.tap { expect(mutation_response['errors']).to eq ['Repository path pattern has already been taken'] } + end + end + + context 'with invalid input param `pushProtectedUpToAccessLevel`' do + let(:input) { super().merge(push_protected_up_to_access_level: nil) } + + it_behaves_like 'an erroneous reponse' + + it { is_expected.tap { expect_graphql_errors_to_include(/pushProtectedUpToAccessLevel can't be blank/) } } + end + + context 'with invalid input param `repositoryPathPattern`' do + let(:input) { super().merge(repository_path_pattern: '') } + + it_behaves_like 'an erroneous reponse' + + it { is_expected.tap { expect_graphql_errors_to_include(/repositoryPathPattern can't be blank/) } } + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it { is_expected.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } + end + end + + context "when feature flag ':container_registry_protected_containers' disabled" do + before do + stub_feature_flags(container_registry_protected_containers: false) + end + + it_behaves_like 'an erroneous reponse' + + it 'returns error of disabled feature flag' do + is_expected.tap do + expect_graphql_errors_to_include(/'container_registry_protected_containers' feature flag is disabled/) + end + end + end +end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 650b21aa0e6..b8e029385e3 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -3658,11 +3658,15 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and expect(json_response['error']).to eq 'group_access does not have a valid value' end - it "returns a 400 error when the project-group share is created with an OWNER access level" do - post api(path, user), params: { group_id: group.id, group_access: Gitlab::Access::OWNER } + it 'returns a 403 when a maintainer tries to create a link with OWNER access' do + user = create(:user) + project.add_maintainer(user) - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq 'group_access does not have a valid value' + expect do + post api(path, user), params: { group_id: group.id, group_access: Gitlab::Access::OWNER } + end.to not_change { project.reload.project_group_links.count } + + expect(response).to have_gitlab_http_status(:forbidden) end it "returns a 409 error when link is not saved" do @@ -3691,11 +3695,12 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and context 'for a valid group' do let_it_be(:group) { create(:group, :private) } let_it_be(:group_user) { create(:user) } + let(:group_access) { Gitlab::Access::DEVELOPER } before do group.add_developer(group_user) - create(:project_group_link, group: group, project: project) + create(:project_group_link, group: group, project: project, group_access: group_access) end it 'returns 204 when deleting a group share' do @@ -3726,6 +3731,21 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and expect(response).to have_gitlab_http_status(:not_found) expect(json_response['message']).to eq '404 Not Found' end + + context 'when a MAINTAINER tries to destroy a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it 'returns 403' do + user = create(:user) + project.add_maintainer(user) + + expect do + delete api("/projects/#{project.id}/share/#{group.id}", user) + end.to not_change { project.reload.project_group_links.count } + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end it 'returns a 400 when group id is not an integer' do diff --git a/spec/serializers/group_link/group_group_link_entity_spec.rb b/spec/serializers/group_link/group_group_link_entity_spec.rb index 8f31c53e841..c1ee92e55ba 100644 --- a/spec/serializers/group_link/group_group_link_entity_spec.rb +++ b/spec/serializers/group_link/group_group_link_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe GroupLink::GroupGroupLinkEntity do +RSpec.describe GroupLink::GroupGroupLinkEntity, feature_category: :groups_and_projects do include_context 'group_group_link' let_it_be(:current_user) { create(:user) } @@ -17,6 +17,10 @@ RSpec.describe GroupLink::GroupGroupLinkEntity do expect(entity.to_json).to match_schema('group_link/group_group_link') end + it 'correctly exposes `valid_roles`' do + expect(entity.as_json[:valid_roles]).to include(Gitlab::Access.options_with_owner) + end + context 'source' do it 'exposes `source`' do expect(as_json[:source]).to include( @@ -59,7 +63,7 @@ RSpec.describe GroupLink::GroupGroupLinkEntity do allow(entity).to receive(:direct_member?).and_return(false) end - it 'exposes `can_update` and `can_remove` as `true`' do + it 'exposes `can_update` and `can_remove` as `false`' do expect(as_json[:can_update]).to be false expect(as_json[:can_remove]).to be false end diff --git a/spec/serializers/group_link/group_link_entity_spec.rb b/spec/serializers/group_link/group_link_entity_spec.rb index 941445feaa2..01c01f492aa 100644 --- a/spec/serializers/group_link/group_link_entity_spec.rb +++ b/spec/serializers/group_link/group_link_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe GroupLink::GroupLinkEntity do +RSpec.describe GroupLink::GroupLinkEntity, feature_category: :groups_and_projects do include_context 'group_group_link' let(:entity) { described_class.new(group_group_link) } @@ -12,10 +12,6 @@ RSpec.describe GroupLink::GroupLinkEntity do expect(entity.to_json).to match_schema('group_link/group_link') end - it 'correctly exposes `valid_roles`' do - expect(entity_hash[:valid_roles]).to include(Gitlab::Access.options_with_owner) - end - it 'correctly exposes `shared_with_group.avatar_url`' do avatar_url = 'https://gitlab.com/uploads/-/system/group/avatar/24/foobar.png?width=40' allow(shared_with_group).to receive(:avatar_url).with(only_path: false, size: Member::AVATAR_SIZE).and_return(avatar_url) diff --git a/spec/serializers/group_link/project_group_link_entity_spec.rb b/spec/serializers/group_link/project_group_link_entity_spec.rb index 00bfc43f17e..9f09020b91d 100644 --- a/spec/serializers/group_link/project_group_link_entity_spec.rb +++ b/spec/serializers/group_link/project_group_link_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe GroupLink::ProjectGroupLinkEntity do +RSpec.describe GroupLink::ProjectGroupLinkEntity, feature_category: :groups_and_projects do let_it_be(:current_user) { create(:user) } let_it_be(:project_group_link) { create(:project_group_link) } @@ -16,32 +16,86 @@ RSpec.describe GroupLink::ProjectGroupLinkEntity do expect(entity.to_json).to match_schema('group_link/project_group_link') end - context 'when current user is a project maintainer' do - before_all do - project_group_link.project.add_maintainer(current_user) + context 'when current user is a direct member' do + before do + allow(entity).to receive(:direct_member?).and_return(true) + allow(entity).to receive(:can?).and_call_original end - it 'exposes `can_update` and `can_remove` as `true`' do - expect(as_json[:can_update]).to be true - expect(as_json[:can_remove]).to be true - end - end + describe 'can_update' do + using RSpec::Parameterized::TableSyntax + + where( + :can_admin_project_member, + :can_manage_group_link_with_owner_access, + :expected_can_update + ) do + false | false | false + true | false | false + true | true | true + end - context 'when current user is a group owner' do - before_all do - project_group_link.group.add_owner(current_user) + with_them do + before do + allow(entity) + .to receive(:can?) + .with(current_user, :admin_project_member, project_group_link.shared_from) + .and_return(can_admin_project_member) + allow(entity) + .to receive(:can?) + .with(current_user, :manage_group_link_with_owner_access, project_group_link) + .and_return(can_manage_group_link_with_owner_access) + end + + it "exposes `can_update` as `#{params[:expected_can_update]}`" do + expect(entity.as_json[:can_update]).to be expected_can_update + end + end end - it 'exposes `can_remove` as true' do - expect(as_json[:can_remove]).to be true + describe 'can_remove' do + context 'when current user has `destroy_project_group_link` ability' do + before do + allow(entity) + .to receive(:can?) + .with(current_user, :destroy_project_group_link, project_group_link) + .and_return(true) + end + + it 'exposes `can_remove` as `true`' do + expect(entity.as_json[:can_remove]).to be(true) + end + end + + context 'when current user does not have `destroy_project_group_link` ability' do + before do + allow(entity) + .to receive(:can?) + .with(current_user, :destroy_project_group_link, project_group_link) + .and_return(false) + end + + it 'exposes `can_remove` as `false`' do + expect(entity.as_json[:can_remove]).to be(false) + end + end end end - context 'when current user is not a group owner' do - it 'exposes `can_remove` as false' do - expect(as_json[:can_remove]).to be false + context 'when current user is not a direct member' do + before do + allow(entity).to receive(:direct_member?).and_return(false) end + it 'exposes `can_update` and `can_remove` as `false`' do + json = entity.as_json + + expect(json[:can_update]).to be false + expect(json[:can_remove]).to be false + end + end + + context 'when current user is not a project member' do context 'when group is public' do it 'does expose shared_with_group details' do expect(as_json[:shared_with_group].keys).to include(:id, :avatar_url, :web_url, :name) diff --git a/spec/services/container_registry/protection/update_rule_service_spec.rb b/spec/services/container_registry/protection/update_rule_service_spec.rb new file mode 100644 index 00000000000..28933b5764a --- /dev/null +++ b/spec/services/container_registry/protection/update_rule_service_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::UpdateRuleService, '#execute', feature_category: :container_registry do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_reload(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + let(:service) { described_class.new(container_registry_protection_rule, current_user: current_user, params: params) } + + let(:params) do + attributes_for( + :container_registry_protection_rule, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-updated", + delete_protected_up_to_access_level: 'owner', + push_protected_up_to_access_level: 'owner' + ) + end + + subject(:service_execute) { service.execute } + + shared_examples 'a successful service response' do + let(:expected_attributes) { params } + + it { is_expected.to be_success } + + it do + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { + container_registry_protection_rule: + be_a(ContainerRegistry::Protection::Rule) + .and(have_attributes(expected_attributes)) + } + ) + end + + it { expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } } + + it { subject.tap { expect(container_registry_protection_rule.reload).to have_attributes expected_attributes } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + + it do + is_expected.to have_attributes( + errors: be_present, + message: be_present, + payload: { container_registry_protection_rule: nil } + ) + end + + it { expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } } + it { expect { subject }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful service response' + + context 'with disallowed params' do + let(:params) { super().merge!(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { params.except(:project_id, :unsupported_param) } + end + end + + context 'with invalid params' do + using RSpec::Parameterized::TableSyntax + + where(:params_invalid, :message_expected) do + { repository_path_pattern: '' } | [/Repository path pattern can't be blank/] + { delete_protected_up_to_access_level: 1000 } | /not a valid delete_protected_up_to_access_level/ + { push_protected_up_to_access_level: 1000 } | /not a valid push_protected_up_to_access_level/ + end + + with_them do + let(:params) do + super().merge(params_invalid) + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: message_expected } + end + end + + context 'with empty params' do + let(:params) { {} } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { container_registry_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + context 'with nil params' do + let(:params) { nil } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { container_registry_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + context 'when updated field `repository_path_pattern` is already taken' do + let_it_be_with_reload(:other_existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-other") + end + + let(:params) do + { repository_path_pattern: other_existing_container_registry_protection_rule.repository_path_pattern } + end + + it_behaves_like 'an erroneous service response' + + it do + expect { service_execute }.not_to( + change { other_existing_container_registry_protection_rule.reload.repository_path_pattern } + ) + end + + it do + is_expected.to have_attributes( + errors: match_array([/Repository path pattern has already been taken/]), + message: match_array([/Repository path pattern has already been taken/]) + ) + end + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes errors: match_array(/Unauthorized/), message: /Unauthorized/ } + end + end + + context 'without container registry protection rule' do + let(:container_registry_protection_rule) { nil } + let(:params) { {} } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end +end diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index e3f170ef3fe..6e3a8aa1733 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -103,6 +103,43 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category it_behaves_like 'shareable' end end + + context 'when sharing it to a group with OWNER access' do + let(:opts) do + { + link_group_access: Gitlab::Access::OWNER, + expires_at: nil + } + end + + it 'does not share and returns a forbiden error' do + expect do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(403) + end.not_to change { project.reload.project_group_links.count } + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + it_behaves_like 'shareable' + + context 'when sharing it to a group with OWNER access' do + let(:opts) do + { + link_group_access: Gitlab::Access::OWNER, + expires_at: nil + } + end + + it_behaves_like 'shareable' + end end end diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 0cd003f6142..4fc441c9e26 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -19,8 +19,10 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end - shared_examples_for 'returns not_found' do - it do + context 'if group_link is blank' do + let!(:group_link) { nil } + + it 'returns 404 not found' do expect do result = subject.execute(group_link) @@ -30,14 +32,15 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end - context 'if group_link is blank' do - let!(:group_link) { nil } - - it_behaves_like 'returns not_found' - end - context 'if the user does not have access to destroy the link' do - it_behaves_like 'returns not_found' + it 'returns 404 not found' do + expect do + result = subject.execute(group_link) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:not_found) + end.not_to change { project.reload.project_group_links.count } + end end context 'when the user has proper permissions to remove a group-link from a project' do @@ -111,6 +114,41 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end end + + context 'on trying to destroy a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it 'does not remove the group from project' do + expect do + result = subject.execute(group_link) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:forbidden) + end.not_to change { project.reload.project_group_links.count } + end + + context 'if the user is an OWNER of the group' do + before do + group.add_owner(user) + end + + it_behaves_like 'removes group from project' + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + it_behaves_like 'removes group from project' + + context 'on trying to destroy a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it_behaves_like 'removes group from project' + end end end diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index b02614fa062..86ad1bcf286 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -20,8 +20,8 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category subject { described_class.new(link, user).execute(group_link_params) } - shared_examples_for 'returns not_found' do - it do + context 'when the user does not have proper permissions to update a project group link' do + it 'returns 404 not found' do result = subject expect(result[:status]).to eq(:error) @@ -29,10 +29,6 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category end end - context 'when the user does not have proper permissions to update a project group link' do - it_behaves_like 'returns not_found' - end - context 'when user has proper permissions to update a project group link' do context 'when the user is a MAINTAINER in the project' do before do @@ -92,6 +88,86 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category subject end end + + context 'updating a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + shared_examples_for 'returns :forbidden' do + it do + expect do + result = subject + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:forbidden) + end.to not_change { link.expires_at }.and not_change { link.group_access } + end + end + + context 'updating expires_at' do + let(:group_link_params) do + { expires_at: 7.days.from_now } + end + + it_behaves_like 'returns :forbidden' + end + + context 'updating group_access' do + let(:group_link_params) do + { group_access: Gitlab::Access::MAINTAINER } + end + + it_behaves_like 'returns :forbidden' + end + + context 'updating both expires_at and group_access' do + it_behaves_like 'returns :forbidden' + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + context 'updating expires_at' do + let(:group_link_params) do + { expires_at: 7.days.from_now.to_date } + end + + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.expires_at }.to(group_link_params[:expires_at]) + end + end + + context 'updating group_access' do + let(:group_link_params) do + { group_access: Gitlab::Access::MAINTAINER } + end + + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.group_access }.to(group_link_params[:group_access]) + end + end + + context 'updating both expires_at and group_access' do + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.group_access }.to(group_link_params[:group_access]) + .and change { link.reload.expires_at }.to(group_link_params[:expires_at]) + end + end end end end diff --git a/spec/support/shared_examples/security/policies_shared_examples.rb b/spec/support/shared_examples/security/policies_shared_examples.rb new file mode 100644 index 00000000000..6edd2aa5c59 --- /dev/null +++ b/spec/support/shared_examples/security/policies_shared_examples.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +# Requires the url to the policies list: +# - path_to_policies_list +RSpec.shared_examples 'policies list' do + before do + allow_next_found_instance_of(Security::OrchestrationPolicyConfiguration) do |policy| + allow(policy).to receive(:policy_configuration_valid?).and_return(true) + allow(policy).to receive(:policy_hash).and_return(policy_yaml) + allow(policy).to receive(:policy_last_updated_at).and_return(Time.current) + end + sign_in(owner) + stub_licensed_features(security_orchestration_policies: true) + end + + it "shows the policies list with policies" do + visit(path_to_policies_list) + + # Scan Execution Policy from ee/spec/fixtures/security_orchestration.yml + expect(page).to have_content 'Run DAST in every pipeline' + # Scan Result Policy from ee/spec/fixtures/security_orchestration.yml + expect(page).to have_content 'critical vulnerability CS approvals' + end +end + +# Requires the url to the policy editor: +# - path_to_policy_editor +RSpec.shared_examples 'policy editor' do + before do + sign_in(owner) + stub_licensed_features(security_orchestration_policies: true) + end + + it "can create a policy when a policy project exists" do + visit(path_to_policy_editor) + page.within(".gl-card:nth-child(1)") do + click_button _('Select policy') + end + fill_in _('Name'), with: 'Prevent vulnerabilities' + click_button _('Select scan type') + select_listbox_item _('Security Scan') + page.within(find_by_testid('actions-section')) do + click_button _('Remove') + end + click_button _('Configure with a merge request') + expect(page).to have_current_path(project_merge_request_path(policy_management_project, 1)) + end +end |