From c0da8130ade00691e2732d0f8b081894065c01a6 Mon Sep 17 00:00:00 2001 From: smriti Date: Fri, 2 Sep 2022 16:45:46 +0530 Subject: [PATCH] API endpoints to manage customizable roles - experimental Removed puts statement Exposed id field as well for deletion to be easy Merge request URL added Added MR link in config file Changes for spec Find by id method used Else condition for save removed Spacing issues fixed Changes for group owner authorization and improvement in specs Root group check added Updated Rollback issue url Renamed feature flag and corrected created logic Changes for adding check in before action and specs Removed unused method Suggestions for spacing applied Moved feature flag yml to ee directory Changes for processing errors on record save --- .../development/customizable_roles.yml | 8 + ee/lib/api/member_roles.rb | 72 +++++ ee/lib/ee/api/api.rb | 1 + ee/lib/ee/api/entities/member_role.rb | 14 + .../lib/ee/api/entities/member_role_spec.rb | 24 ++ ee/spec/requests/api/member_roles_spec.rb | 280 ++++++++++++++++++ 6 files changed, 399 insertions(+) create mode 100644 ee/config/feature_flags/development/customizable_roles.yml create mode 100644 ee/lib/api/member_roles.rb create mode 100644 ee/lib/ee/api/entities/member_role.rb create mode 100644 ee/spec/lib/ee/api/entities/member_role_spec.rb create mode 100644 ee/spec/requests/api/member_roles_spec.rb diff --git a/ee/config/feature_flags/development/customizable_roles.yml b/ee/config/feature_flags/development/customizable_roles.yml new file mode 100644 index 00000000000000..0682a09db0aef1 --- /dev/null +++ b/ee/config/feature_flags/development/customizable_roles.yml @@ -0,0 +1,8 @@ +--- +name: customizable_roles +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96996 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/372548 +milestone: '15.4' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb new file mode 100644 index 00000000000000..886a54861d4271 --- /dev/null +++ b/ee/lib/api/member_roles.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module API + class MemberRoles < ::API::Base + before { authenticate! } + before { authorize_admin_group } + before { not_found! unless Feature.enabled?(:customizable_roles, user_group) } + before { bad_request! unless user_group.root? } + + feature_category :authentication_and_authorization + + params do + requires :id, type: String, desc: 'The ID of a group' + end + + resource :groups do + desc 'Get Member Roles for a group' do + success EE::API::Entities::MemberRole + end + get ":id/member_roles" do + group = find_group(params[:id]) + + member_roles = group.member_roles + + present member_roles, with: EE::API::Entities::MemberRole + end + + desc 'Create Member Role for a group' do + success EE::API::Entities::MemberRole + end + params do + requires 'base_access_level', type: Integer, values: Gitlab::Access.all_values, + desc: 'Base Access Level for the configured role ' + optional 'download_code', type: Boolean, + desc: 'Permission to download code' + end + + post ":id/member_roles" do + group = find_group(params[:id]) + + member_role = group.member_roles.new(declared_params) + + if member_role.save + present member_role, with: EE::API::Entities::MemberRole + else + render_api_error!(member_role.errors.full_messages.first, 400) + end + end + + desc 'Delete Member Role for a group' do + success EE::API::Entities::MemberRole + end + params do + requires 'member_role_id', type: Integer, + desc: 'The ID of the Member Role to be deleted' + end + + delete ":id/member_roles/:member_role_id" do + group = find_group(params[:id]) + + member_role = group.member_roles.find_by_id(params[:member_role_id]) + + if member_role + member_role.destroy + no_content! + else + render_api_error!('Linked Member Role not found', 404) + end + end + end + end +end diff --git a/ee/lib/ee/api/api.rb b/ee/lib/ee/api/api.rb index 2245a8a73fe4c9..017d9548a06849 100644 --- a/ee/lib/ee/api/api.rb +++ b/ee/lib/ee/api/api.rb @@ -30,6 +30,7 @@ module API mount ::API::ProjectPushRule mount ::API::GroupPushRule mount ::API::MergeTrains + mount ::API::MemberRoles mount ::API::GroupHooks mount ::API::MergeRequestApprovalSettings mount ::API::Scim diff --git a/ee/lib/ee/api/entities/member_role.rb b/ee/lib/ee/api/entities/member_role.rb new file mode 100644 index 00000000000000..a32121ab0f05c1 --- /dev/null +++ b/ee/lib/ee/api/entities/member_role.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module EE + module API + module Entities + class MemberRole < Grape::Entity + expose :id + expose :namespace_id, as: :group_id + expose :base_access_level + expose :download_code + end + end + end +end diff --git a/ee/spec/lib/ee/api/entities/member_role_spec.rb b/ee/spec/lib/ee/api/entities/member_role_spec.rb new file mode 100644 index 00000000000000..4b96d657bf3001 --- /dev/null +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::API::Entities::MemberRole do + describe 'exposes access_level and download_code fields' do + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } + + let(:member_role) { create(:member_role) } + let(:entity) { described_class.new(member_role) } + + subject { entity.as_json } + + it 'exposes the attributes' do + group.add_owner(user) + + expect(subject[:id]).to eq member_role.id + expect(subject[:base_access_level]).to eq member_role.base_access_level + expect(subject[:download_code]).to eq member_role.download_code + expect(subject[:group_id]).to eq(member_role.namespace.id) + end + end +end diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb new file mode 100644 index 00000000000000..60f526dd497d7c --- /dev/null +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -0,0 +1,280 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe API::MemberRoles, api: true do + include ApiHelpers + + let_it_be(:owner) { create(:user) } + let_it_be(:user) { create(:user) } + let(:current_user) { nil } + + let_it_be(:group_with_member_roles) do + group = create(:group) + group.add_owner(owner) + group + end + + let_it_be(:child_group) { create :group, parent: group_with_member_roles } + + let_it_be(:member_role_1) do + create(:member_role, namespace: group_with_member_roles, + base_access_level: ::Gitlab::Access::REPORTER, + download_code: 0) + end + + let_it_be(:member_role_2) do + create(:member_role, namespace: group_with_member_roles, + base_access_level: ::Gitlab::Access::REPORTER, + download_code: 1) + end + + let_it_be(:group_id) { group_with_member_roles.id } + + shared_examples 'fails for non-root group' do + context 'with non-root group' do + let(:group_id) { child_group.id } + let(:current_user) { owner } + + before do + stub_feature_flags(customizable_roles: [child_group]) + end + + it 'returns bad request' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + end + + describe "GET /groups/:id/member_roles" do + subject { get api("/groups/#{group_id}/member_roles", current_user) } + + it_behaves_like 'fails for non-root group' + + context "when unauthorized" do + it "returns forbidden error" do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context "when a less privileged user" do + let(:current_user) { user } + + it "returns forbidden error" do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context "when owner of the group" do + let(:current_user) { owner } + + it "returns associated member roles" do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to( + match([ + { + "id" => member_role_1.id, + "base_access_level" => ::Gitlab::Access::REPORTER, + "download_code" => false, + "group_id" => group_id + }, + { + "id" => member_role_2.id, + "base_access_level" => ::Gitlab::Access::REPORTER, + "download_code" => true, + "group_id" => group_id + } + ]) + ) + end + + context "when group does not have any associated member_roles" do + let_it_be(:group_with_no_member_roles) { create(:group) } + let_it_be(:group_id) { group_with_no_member_roles.id } + + before do + group_with_no_member_roles.add_owner owner + end + + it "returns empty array as response" do + subject + + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to(match([])) + end + end + end + end + end + + describe "POST /groups/:id/member_roles" do + let_it_be(:params) { { base_access_level: 40, download_code: 1 } } + + subject { post api("/groups/#{group_id}/member_roles", current_user), params: params } + + it_behaves_like 'fails for non-root group' + + context "when feature flag is enabled" do + before do + stub_feature_flags(customizable_roles: [group_with_member_roles]) + end + + context "when unauthorized" do + it "returns unauthorized error" do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context "when a less privileged user" do + let(:current_user) { user } + + it "does not allow less privileged user to add member roles" do + expect do + subject + end.not_to change { group_with_member_roles.member_roles.count } + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context "when owner of the group" do + let(:current_user) { owner } + + it "returns ok and add member role" do + expect do + subject + end.to change { group_with_member_roles.member_roles.count }.by(1) + + aggregate_failures "testing response" do + expect(response).to have_gitlab_http_status(:created) + expect(json_response['base_access_level']).to eq(::Gitlab::Access::MAINTAINER) + expect(json_response['download_code']).to eq(true) + end + end + + context "when params are missing" do + let(:params) { { download_code: 0 } } + + it "returns a 400 error when params are missing" do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to match(/base_access_level is missing/) + end + end + + context "when params are invalid" do + let(:params) { { base_access_level: 1, download_code: 1 } } + + it "returns a 400 error when params are invalid" do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('base_access_level does not have a valid value') + end + end + + context "when errors during creation of new record" do + before do + allow_next_instance_of(MemberRole) do |instance| + instance.errors.add(:base, 'validation error') + + allow(instance).to receive(:valid?).and_return(false) + end + end + + it "returns a error message with 400 code" do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq('validation error') + end + end + end + end + + context "when feature flag is disabled" do + before do + stub_feature_flags(customizable_roles: false) + end + + let(:current_user) { owner } + + it "returns unauthorized error" do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe "DELETE /groups/:id/member_roles/:member_role_id" do + let_it_be(:member_role_id) { member_role_1.id } + + subject { delete api("/groups/#{group_id}/member_roles/#{member_role_id}", current_user) } + + it_behaves_like 'fails for non-root group' + + context "when feature flag is enabled" do + before do + stub_feature_flags(customizable_roles: [group_with_member_roles]) + end + + context "when unauthorized" do + it "returns unauthorized error" do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context "when a less privileged user" do + let(:current_user) { user } + + it "does not remove the member role" do + expect do + subject + end.not_to change { group_with_member_roles.member_roles.count } + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context "when owner of the group" do + let(:current_user) { owner } + + it "removes member role" do + expect do + subject + + expect(response).to have_gitlab_http_status(:no_content) + end.to change { group_with_member_roles.member_roles.count }.by(-1) + end + + context "when invalid group name is passed" do + let(:member_role_id) { (member_role_1.id + 10) } + + it "returns 404 if SAML group can not used for a SAML group link" do + expect do + subject + end.not_to change { group_with_member_roles.member_roles.count } + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('Linked Member Role not found') + end + end + end + end + end +end -- GitLab