From 42e02ce5ea3c573eab41f33fa35b0f0ef6d98686 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 6 Jan 2023 21:39:49 +0800 Subject: [PATCH 1/6] Support "Minimal access" role when inviting new users Co-authored-by: Andrew Smith --- .../groups/_invite_members_modal.html.haml | 2 +- ee/spec/models/ee/group_spec.rb | 21 +++++++++++++++++++ lib/api/invitations.rb | 12 +++++++++-- spec/models/group_spec.rb | 16 ++++++++++++++ 4 files changed, 48 insertions(+), 3 deletions(-) diff --git a/app/views/groups/_invite_members_modal.html.haml b/app/views/groups/_invite_members_modal.html.haml index bf0e8b627fdc5d..f0fd9026b30787 100644 --- a/app/views/groups/_invite_members_modal.html.haml +++ b/app/views/groups/_invite_members_modal.html.haml @@ -1,6 +1,6 @@ - return unless can_admin_group_member?(group) .js-invite-members-modal{ data: { is_project: 'false', - access_levels: GroupMember.access_level_roles.to_json, + access_levels: group.access_level_roles.to_json, reload_page_on_submit: local_assigns.fetch(:reload_page_on_submit, false).to_s, help_link: help_page_url('user/permissions') }.merge(common_invite_modal_dataset(group)).merge(users_filter_data(group)) } diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 711def1e0e0d23..5f10b026e934fc 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -2199,6 +2199,27 @@ def webhook_headers it { is_expected.to match([user.email]) } end + describe "#access_level_roles" do + let(:group) { create(:group) } + + before do + stub_licensed_features(minimal_access_role: true) + end + + it "returns the correct roles" do + expect(group.access_level_roles).to eq( + { + "Minimal Access" => 5, + "Guest" => 10, + "Reporter" => 20, + "Developer" => 30, + "Maintainer" => 40, + "Owner" => 50 + } + ) + end + end + describe 'Releases Stats' do context 'when there are no releases' do describe '#releases_count' do diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index ec563e6a5c5058..8ef2eb49ddc85b 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -10,6 +10,12 @@ class Invitations < ::API::Base helpers ::API::Helpers::MembersHelpers + helpers do + def valid_access_level_values(source) + source.is_a?(Group) ? source.access_level_values : Gitlab::Access.all_values + end + end + %w[group project].each do |source_type| params do requires :id, type: String, desc: "The #{source_type} ID" @@ -21,7 +27,7 @@ class Invitations < ::API::Base tags %w[invitations] end params do - requires :access_level, type: Integer, values: Gitlab::Access.all_values, desc: 'A valid access level (defaults: `30`, developer access level)' + requires :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)' optional :email, type: Array[String], email_or_email_list: true, coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The email address to invite, or multiple emails separated by comma' optional :user_id, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The user ID of the new member or multiple IDs separated by commas.' optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY' @@ -35,6 +41,7 @@ class Invitations < ::API::Base bad_request!('Must provide either email or user_id as a parameter') if params[:email].blank? && params[:user_id].blank? source = find_source(source_type, params[:id]) + bad_request!('access_level does not have a valid value') unless params[:access_level].in?(valid_access_level_values(source)) authorize_admin_source!(source_type, source) create_service_params = params.merge(source: source) @@ -71,11 +78,12 @@ class Invitations < ::API::Base end params do requires :email, type: String, desc: 'The email address of the invitation' - optional :access_level, type: Integer, values: Gitlab::Access.all_values, desc: 'A valid access level (defaults: `30`, developer access level)' + optional :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)' optional :expires_at, type: DateTime, desc: 'Date string in ISO 8601 format (`YYYY-MM-DDTHH:MM:SSZ`)' end put ":id/invitations/:email", requirements: { email: %r{[^/]+} } do source = find_source(source_type, params.delete(:id)) + bad_request!('access_level does not have a valid value') unless params[:access_level].nil? || params[:access_level].in?(valid_access_level_values(source)) invite_email = params[:email] authorize_admin_source!(source_type, source) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 4605c086763ba1..9975086ff219be 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -2913,6 +2913,22 @@ def define_cache_expectations(cache_key) end end + describe "#access_level_roles" do + let(:group) { create(:group) } + + it "returns the correct roles" do + expect(group.access_level_roles).to eq( + { + 'Guest' => 10, + 'Reporter' => 20, + 'Developer' => 30, + 'Maintainer' => 40, + 'Owner' => 50 + } + ) + end + end + describe '#membership_locked?' do it 'returns false' do expect(build(:group)).not_to be_membership_locked -- GitLab From fb8d6784da9d4247a9bd6dc952e9022088f17a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Tue, 10 Jan 2023 11:04:06 +0800 Subject: [PATCH 2/6] Remove verification of access_level in controller --- lib/api/invitations.rb | 8 -------- spec/requests/api/invitations_spec.rb | 5 +++-- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index 8ef2eb49ddc85b..9587e3bd187325 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -10,12 +10,6 @@ class Invitations < ::API::Base helpers ::API::Helpers::MembersHelpers - helpers do - def valid_access_level_values(source) - source.is_a?(Group) ? source.access_level_values : Gitlab::Access.all_values - end - end - %w[group project].each do |source_type| params do requires :id, type: String, desc: "The #{source_type} ID" @@ -41,7 +35,6 @@ def valid_access_level_values(source) bad_request!('Must provide either email or user_id as a parameter') if params[:email].blank? && params[:user_id].blank? source = find_source(source_type, params[:id]) - bad_request!('access_level does not have a valid value') unless params[:access_level].in?(valid_access_level_values(source)) authorize_admin_source!(source_type, source) create_service_params = params.merge(source: source) @@ -83,7 +76,6 @@ def valid_access_level_values(source) end put ":id/invitations/:email", requirements: { email: %r{[^/]+} } do source = find_source(source_type, params.delete(:id)) - bad_request!('access_level does not have a valid value') unless params[:access_level].nil? || params[:access_level].in?(valid_access_level_values(source)) invite_email = params[:email] authorize_admin_source!(source_type, source) diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index bb0f557cfee4c3..27aafa7f2e52cf 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -397,11 +397,12 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: expect(response).to have_gitlab_http_status(:bad_request) end - it 'returns 400 when access_level is not valid' do + it 'returns error when access_level is not valid' do post invitations_url(source, maintainer), params: { email: email, access_level: non_existing_record_access_level } - expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['status']).to eq 'error' + expect(json_response['message'][email]).to include('Access level is not included in the list') end end end -- GitLab From af313b5bad34e9664df4ec8d2f7fa8af39130ea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Tue, 10 Jan 2023 23:25:22 +0800 Subject: [PATCH 3/6] Revert changes to PUT API & add spec to POST API --- ee/spec/requests/api/invitations_spec.rb | 29 ++++++++++++++++++++++++ lib/api/invitations.rb | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/ee/spec/requests/api/invitations_spec.rb b/ee/spec/requests/api/invitations_spec.rb index 0ee1027a18b355..7dab6e74149cc2 100644 --- a/ee/spec/requests/api/invitations_spec.rb +++ b/ee/spec/requests/api/invitations_spec.rb @@ -159,6 +159,35 @@ end end end + + context 'with minimal access level' do + before do + stub_licensed_features(minimal_access_role: true) + end + + context 'when group has no parent' do + it 'return success' do + post api(url, admin), + params: { email: invite_email, access_level: Member::MINIMAL_ACCESS } + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['status']).to eq("success") + end + end + + context 'when group has parent' do + let(:parent_group) { create(:group) } + let(:group) { create(:group, parent: parent_group) } + + it 'return error' do + post api(url, admin), + params: { email: invite_email, access_level: Member::MINIMAL_ACCESS } + + expect(json_response['status']).to eq 'error' + expect(json_response['message'][invite_email]).to include('Access level is not included in the list') + end + end + end end describe 'POST /projects/:id/invitations' do diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index 9587e3bd187325..a7e7adbb4ac1aa 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -71,7 +71,7 @@ class Invitations < ::API::Base end params do requires :email, type: String, desc: 'The email address of the invitation' - optional :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)' + optional :access_level, type: Integer, values: Gitlab::Access.all_values, desc: 'A valid access level (defaults: `30`, developer access level)' optional :expires_at, type: DateTime, desc: 'Date string in ISO 8601 format (`YYYY-MM-DDTHH:MM:SSZ`)' end put ":id/invitations/:email", requirements: { email: %r{[^/]+} } do -- GitLab From 5756282b0f25f16f6ef8749adadfdc26c8a0995a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 13 Jan 2023 16:12:34 +0800 Subject: [PATCH 4/6] Keep the 'values' param for invitations api --- lib/api/invitations.rb | 2 +- spec/requests/api/invitations_spec.rb | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index a7e7adbb4ac1aa..87c9f086990679 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -21,7 +21,7 @@ class Invitations < ::API::Base tags %w[invitations] end params do - requires :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)' + requires :access_level, type: Integer, values: Gitlab::Access.all_values + [Gitlab::Access::MINIMAL_ACCESS], desc: 'A valid access level (defaults: `30`, developer access level)' optional :email, type: Array[String], email_or_email_list: true, coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The email address to invite, or multiple emails separated by comma' optional :user_id, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The user ID of the new member or multiple IDs separated by commas.' optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY' diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index 27aafa7f2e52cf..bb0f557cfee4c3 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -397,12 +397,11 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: expect(response).to have_gitlab_http_status(:bad_request) end - it 'returns error when access_level is not valid' do + it 'returns 400 when access_level is not valid' do post invitations_url(source, maintainer), params: { email: email, access_level: non_existing_record_access_level } - expect(json_response['status']).to eq 'error' - expect(json_response['message'][email]).to include('Access level is not included in the list') + expect(response).to have_gitlab_http_status(:bad_request) end end end -- GitLab From e79e76851e6130698d801700b7c0e4450e4ff533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 20 Jan 2023 19:59:52 +0800 Subject: [PATCH 5/6] Add 'MembersHelpers.group_member_access_levels' --- ee/lib/ee/api/helpers/members_helpers.rb | 9 +++++++++ ee/spec/lib/ee/api/helpers/members_helpers_spec.rb | 8 ++++++++ lib/api/helpers/members_helpers.rb | 4 ++++ lib/api/invitations.rb | 2 +- 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index d569d7464aeca0..a3e8b9f7fade8d 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -26,6 +26,15 @@ def member_sort_options end end + class_methods do + extend ::Gitlab::Utils::Override + + override :group_member_access_levels + def group_member_access_levels + super + [::Gitlab::Access::MINIMAL_ACCESS] + end + end + # rubocop: disable CodeReuse/ActiveRecord override :retrieve_members def retrieve_members(source, params:, deep: false) diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb index d55a4156b0dbcf..1784b695bca019 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -84,4 +84,12 @@ def can?(user, ability, members_source) end end end + + describe 'API::Helpers::MembersHelpers.group_member_access_levels.group_member_access_levels' do + it 'return all access levels including minimal access' do + expect(API::Helpers::MembersHelpers.group_member_access_levels).to eq( + ::Gitlab::Access.all_values + [::Gitlab::Access::MINIMAL_ACCESS] + ) + end + end end diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index b0ea4388d9bdc6..ca2bf7ff305c4c 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -98,6 +98,10 @@ def add_single_member?(user_id) user_id.present? end + def self.group_member_access_levels + Gitlab::Access.all_values + end + private def member_already_exists?(source, user_id) diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index 87c9f086990679..a4ecf9b6b4a147 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -21,7 +21,7 @@ class Invitations < ::API::Base tags %w[invitations] end params do - requires :access_level, type: Integer, values: Gitlab::Access.all_values + [Gitlab::Access::MINIMAL_ACCESS], desc: 'A valid access level (defaults: `30`, developer access level)' + requires :access_level, type: Integer, values: ::API::Helpers::MembersHelpers.group_member_access_levels, desc: 'A valid access level (defaults: `30`, developer access level)' optional :email, type: Array[String], email_or_email_list: true, coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The email address to invite, or multiple emails separated by comma' optional :user_id, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The user ID of the new member or multiple IDs separated by commas.' optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY' -- GitLab From d5d4386695deb9cf572ff82b6d5794e7f9ef693f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 27 Jan 2023 19:33:20 +0800 Subject: [PATCH 6/6] Rename MembersHelpers.member_access_levels --- ee/lib/ee/api/helpers/members_helpers.rb | 4 ++-- ee/spec/lib/ee/api/helpers/members_helpers_spec.rb | 4 ++-- ee/spec/requests/api/invitations_spec.rb | 6 ++++-- lib/api/helpers/members_helpers.rb | 2 +- lib/api/invitations.rb | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index a3e8b9f7fade8d..3fe4a88bd27939 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -29,8 +29,8 @@ def member_sort_options class_methods do extend ::Gitlab::Utils::Override - override :group_member_access_levels - def group_member_access_levels + override :member_access_levels + def member_access_levels super + [::Gitlab::Access::MINIMAL_ACCESS] end end diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb index 1784b695bca019..e5af422ed1d576 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -85,9 +85,9 @@ def can?(user, ability, members_source) end end - describe 'API::Helpers::MembersHelpers.group_member_access_levels.group_member_access_levels' do + describe '.member_access_levels' do it 'return all access levels including minimal access' do - expect(API::Helpers::MembersHelpers.group_member_access_levels).to eq( + expect(API::Helpers::MembersHelpers.member_access_levels).to eq( ::Gitlab::Access.all_values + [::Gitlab::Access::MINIMAL_ACCESS] ) end diff --git a/ee/spec/requests/api/invitations_spec.rb b/ee/spec/requests/api/invitations_spec.rb index 7dab6e74149cc2..4d14c909b6321d 100644 --- a/ee/spec/requests/api/invitations_spec.rb +++ b/ee/spec/requests/api/invitations_spec.rb @@ -168,7 +168,8 @@ context 'when group has no parent' do it 'return success' do post api(url, admin), - params: { email: invite_email, access_level: Member::MINIMAL_ACCESS } + params: { email: invite_email, + access_level: Member::MINIMAL_ACCESS } expect(response).to have_gitlab_http_status(:created) expect(json_response['status']).to eq("success") @@ -181,7 +182,8 @@ it 'return error' do post api(url, admin), - params: { email: invite_email, access_level: Member::MINIMAL_ACCESS } + params: { email: invite_email, + access_level: Member::MINIMAL_ACCESS } expect(json_response['status']).to eq 'error' expect(json_response['message'][invite_email]).to include('Access level is not included in the list') diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index ca2bf7ff305c4c..a406a62344ac9a 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -98,7 +98,7 @@ def add_single_member?(user_id) user_id.present? end - def self.group_member_access_levels + def self.member_access_levels Gitlab::Access.all_values end diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index a4ecf9b6b4a147..828f4b419ef333 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -21,7 +21,7 @@ class Invitations < ::API::Base tags %w[invitations] end params do - requires :access_level, type: Integer, values: ::API::Helpers::MembersHelpers.group_member_access_levels, desc: 'A valid access level (defaults: `30`, developer access level)' + requires :access_level, type: Integer, values: ::API::Helpers::MembersHelpers.member_access_levels, desc: 'A valid access level (defaults: `30`, developer access level)' optional :email, type: Array[String], email_or_email_list: true, coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The email address to invite, or multiple emails separated by comma' optional :user_id, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The user ID of the new member or multiple IDs separated by commas.' optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY' -- GitLab