Merge branch '217155-role-can-t-be-changed-for-users-not-meeting-domain-restriction' into 'master'

Add error handling to update member service

See merge request gitlab-org/gitlab!52014

app/assets/javascripts/members/store/actions.js

@@ -11,7 +11,7 @@ export const updateMemberRole = async ({ state, commit }, { memberId, accessLeve
 
     commit(types.RECEIVE_MEMBER_ROLE_SUCCESS, { memberId, accessLevel });
   } catch (error) {
-    commit(types.RECEIVE_MEMBER_ROLE_ERROR);
+    commit(types.RECEIVE_MEMBER_ROLE_ERROR, { error });
 
     throw error;
   }
@@ -37,7 +37,7 @@ export const updateMemberExpiration = async ({ state, commit }, { memberId, expi
       expiresAt: expiresAt ? formatDate(expiresAt, 'isoUtcDateTime') : null,
     });
   } catch (error) {
-    commit(types.RECEIVE_MEMBER_EXPIRATION_ERROR);
+    commit(types.RECEIVE_MEMBER_EXPIRATION_ERROR, { error });
 
     throw error;
   }

app/assets/javascripts/members/store/mutations.js

@@ -13,10 +13,10 @@ export default {
 
     Vue.set(member, 'accessLevel', accessLevel);
   },
-  [types.RECEIVE_MEMBER_ROLE_ERROR](state) {
-    state.errorMessage = s__(
-      "Members|An error occurred while updating the member's role, please try again.",
-    );
+  [types.RECEIVE_MEMBER_ROLE_ERROR](state, { error }) {
+    state.errorMessage =
+      error.response?.data?.message ||
+      s__("Members|An error occurred while updating the member's role, please try again.");
     state.showError = true;
   },
   [types.RECEIVE_MEMBER_EXPIRATION_SUCCESS](state, { memberId, expiresAt }) {
@@ -28,8 +28,10 @@ export default {
 
     Vue.set(member, 'expiresAt', expiresAt);
   },
-  [types.RECEIVE_MEMBER_EXPIRATION_ERROR](state) {
-    state.errorMessage = s__(
+  [types.RECEIVE_MEMBER_EXPIRATION_ERROR](state, { error }) {
+    state.errorMessage =
+      error.response?.data?.message ||
+      s__(
         "Members|An error occurred while updating the member's expiration date, please try again.",
       );
     state.showError = true;

app/controllers/concerns/membership_actions.rb

@@ -18,18 +18,26 @@ module MembershipActions
   def update
     update_params = params.require(root_params_key).permit(:access_level, :expires_at)
     member = membershipable.members_and_requesters.find(params[:id])
-    member = Members::UpdateService
+    result = Members::UpdateService
       .new(current_user, update_params)
       .execute(member)
 
-    if member.expires?
-      render json: {
+    member = result[:member]
+
+    member_data = if member.expires?
+                    {
                       expires_in: helpers.distance_of_time_in_words_to_now(member.expires_at),
                       expires_soon: member.expires_soon?,
                       expires_at_formatted: member.expires_at.to_time.in_time_zone.to_s(:medium)
                     }
                   else
-      render json: {}
+                    {}
+                  end
+
+    if result[:status] == :success
+      render json: member_data
+    else
+      render json: { message: result[:message] }, status: :unprocessable_entity
     end
   end
 

app/services/members/update_service.rb

@@ -16,7 +16,11 @@ module Members
         enqueue_delete_todos(member) if downgrading_to_guest?
       end
 
-      member
+      if member.errors.any?
+        error(member.errors.full_messages.to_sentence, pass_back: { member: member })
+      else
+        success(member: member)
+      end
     end
 
     private

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -27,12 +27,16 @@ module EE
       # rubocop: disable CodeReuse/ActiveRecord
       def override
         member = membershipable_members.find_by!(id: params[:id])
-        updated_member = ::Members::UpdateService.new(current_user, override_params)
-          .execute(member, permission: :override)
 
-        if updated_member.valid?
+        result = ::Members::UpdateService.new(current_user, override_params).execute(member, permission: :override)
+
         respond_to do |format|
-            format.js { head :ok }
+          format.js do
+            if result[:status] == :success
+              head :ok
+            else
+              render json: result[:message], status: :unprocessable_entity
+            end
           end
         end
       end

ee/app/models/ee/group_member.rb

@@ -91,8 +91,8 @@ module EE
     end
 
     def email_does_not_match_any_allowed_domains(email)
-      _("email '%{email}' does not match the allowed domains of %{email_domains}" %
-        { email: email, email_domains: ::Gitlab::Utils.to_exclusive_sentence(group_allowed_email_domains.map(&:domain)) })
+      n_("email '%{email}' does not match the allowed domain of %{email_domains}", "email '%{email}' does not match the allowed domains: %{email_domains}", group_allowed_email_domains.size) %
+        { email: email, email_domains: group_allowed_email_domains.map(&:domain).join(', ') }
     end
 
     def email_not_verified

ee/changelogs/unreleased/217155-role-can-t-be-changed-for-users-not-meeting-domain-restriction.yml

+---
+title: Display error message if group member can not be updated because their email
+  does not match the list of allowed domains
+merge_request: 52014
+author:
+type: fixed

ee/lib/ee/api/members.rb

@@ -19,11 +19,17 @@ module EE
           post ":id/members/:user_id/override" do
             member = find_member(params)
 
-            updated_member = ::Members::UpdateService
+            result = ::Members::UpdateService
               .new(current_user, { override: true })
               .execute(member, permission: :override)
 
+            updated_member = result[:member]
+
+            if result[:status] == :success
               present_member(updated_member)
+            else
+              render_validation_error!(updated_member)
+            end
           end
 
           desc 'Remove an LDAP group member access level override.' do
@@ -35,11 +41,17 @@ module EE
           delete ":id/members/:user_id/override" do
             member = find_member(params)
 
-            updated_member = ::Members::UpdateService
+            result = ::Members::UpdateService
               .new(current_user, { override: false })
               .execute(member, permission: :override)
 
+            updated_member = result[:member]
+
+            if result[:status] == :success
               present_member(updated_member)
+            else
+              render_validation_error!(updated_member)
+            end
           end
 
           desc 'Gets a list of billable users of root group.' do

ee/spec/models/group_member_spec.rb

@@ -30,6 +30,21 @@ RSpec.describe GroupMember do
           expect(build(:group_member, group: group, user: acme_user)).to be_valid
         end
 
+        it 'shows proper error message' do
+          group_member = build(:group_member, group: group, user: gmail_user)
+
+          expect(group_member).to be_invalid
+          expect(group_member.errors[:user]).to include("email 'test@gmail.com' does not match the allowed domains: gitlab.com, acme.com")
+        end
+
+        it 'shows proper error message for single domain limitation' do
+          group.allowed_email_domains.last.destroy!
+          group_member = build(:group_member, group: group, user: gmail_user)
+
+          expect(group_member).to be_invalid
+          expect(group_member.errors[:user]).to include("email 'test@gmail.com' does not match the allowed domain of gitlab.com")
+        end
+
         it 'invited email must match at least one of the allowed domain emails' do
           expect(build(:group_member, group: group, user: nil, invite_email: 'user@gmail.com')).to be_invalid
           expect(build(:group_member, group: group, user: nil, invite_email: 'user@gitlab.com')).to be_valid

ee/spec/requests/groups/group_members_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::GroupMembersController do
+  include ExternalAuthorizationServiceHelpers
+
+  let(:user)  { create(:user) }
+  let(:group) { create(:group, :public) }
+  let(:membership) { create(:group_member, group: group) }
+
+  before do
+    group.add_owner(user)
+    sign_in(user)
+  end
+
+  describe 'PUT /groups/*group_id/-/group_members/:id' do
+    context 'when group has email domain feature enabled' do
+      let(:email) { 'test@gitlab.com' }
+      let(:member_user) { create(:user, email: email) }
+      let(:member) { group.add_guest(member_user) }
+
+      before do
+        stub_licensed_features(group_allowed_email_domains: true)
+        create(:allowed_email_domain, group: group)
+      end
+
+      subject do
+        put group_group_member_path(group_id: group, id: member), xhr: true, params: {
+                                          group_member: {
+                                            access_level: 50
+                                          }
+                                        }
+      end
+
+      context 'for a user with an email belonging to the allowed domain' do
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
+        let(:email) { 'test@gmail.com' }
+
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+
+        it 'returns error message' do
+          subject
+
+          expect(json_response).to eq({ 'message' => "User email 'test@gmail.com' does not match the allowed domain of gitlab.com" })
+        end
+      end
+    end
+  end
+end

lib/api/members.rb

@@ -137,12 +137,14 @@ module API
           authorize_admin_source!(source_type, source)
 
           member = source_members(source).find_by!(user_id: params[:user_id])
-          updated_member =
-            ::Members::UpdateService
+
+          result = ::Members::UpdateService
             .new(current_user, declared_params(include_missing: false))
             .execute(member)
 
-          if updated_member.valid?
+          updated_member = result[:member]
+
+          if result[:status] == :success
             present_members updated_member
           else
             render_validation_error!(updated_member)

locale/gitlab.pot

@@ -33715,8 +33715,10 @@ msgstr ""
 msgid "element is not a hierarchy"
 msgstr ""
 
-msgid "email '%{email}' does not match the allowed domains of %{email_domains}"
-msgstr ""
+msgid "email '%{email}' does not match the allowed domain of %{email_domains}"
+msgid_plural "email '%{email}' does not match the allowed domains: %{email_domains}"
+msgstr[0] ""
+msgstr[1] ""
 
 msgid "email '%{email}' is not a verified email."
 msgstr ""

spec/controllers/groups/group_members_controller_spec.rb

@@ -221,6 +221,18 @@ RSpec.describe Groups::GroupMembersController do
 
           expect(requester.reload.expires_at).not_to eq(expires_at.to_date)
         end
+
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+
+        it 'returns error message' do
+          subject
+
+          expect(json_response).to eq({ 'message' => 'Expires at cannot be a date in the past' })
+        end
       end
 
       context 'when set to a date in the future' do

spec/controllers/projects/project_members_controller_spec.rb

@@ -321,6 +321,18 @@ RSpec.describe Projects::ProjectMembersController do
 
           expect(requester.reload.expires_at).not_to eq(expires_at.to_date)
         end
+
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+
+        it 'returns error message' do
+          subject
+
+          expect(json_response).to eq({ 'message' => 'Expires at cannot be a date in the past' })
+        end
       end
 
       context 'when set to a date in the future' do

spec/frontend/members/components/app_spec.js

@@ -48,7 +48,7 @@ describe('MembersApp', () => {
     it('renders and scrolls to error alert', async () => {
       createComponent({ showError: false, errorMessage: '' });
 
-      store.commit(RECEIVE_MEMBER_ROLE_ERROR);
+      store.commit(RECEIVE_MEMBER_ROLE_ERROR, { error: new Error('Network Error') });
 
       await nextTick();
 

spec/frontend/members/store/actions_spec.js

@@ -57,15 +57,17 @@ describe('Vuex members actions', () => {
 
       describe('unsuccessful request', () => {
         it(`commits ${types.RECEIVE_MEMBER_ROLE_ERROR} mutation and throws error`, async () => {
-          mock.onPut().networkError();
+          const error = new Error('Network Error');
+          mock.onPut().reply(() => Promise.reject(error));
 
           await expect(
             testAction(updateMemberRole, payload, state, [
               {
                 type: types.RECEIVE_MEMBER_ROLE_ERROR,
+                payload: { error },
               },
             ]),
-          ).rejects.toThrowError(new Error('Network Error'));
+          ).rejects.toThrowError(error);
         });
       });
     });
@@ -108,15 +110,17 @@ describe('Vuex members actions', () => {
 
       describe('unsuccessful request', () => {
         it(`commits ${types.RECEIVE_MEMBER_EXPIRATION_ERROR} mutation and throws error`, async () => {
-          mock.onPut().networkError();
+          const error = new Error('Network Error');
+          mock.onPut().reply(() => Promise.reject(error));
 
           await expect(
             testAction(updateMemberExpiration, { memberId, expiresAt }, state, [
               {
                 type: types.RECEIVE_MEMBER_EXPIRATION_ERROR,
+                payload: { error },
               },
             ]),
-          ).rejects.toThrowError(new Error('Network Error'));
+          ).rejects.toThrowError(error);
         });
       });
     });

spec/frontend/members/store/mutations_spec.js

@@ -28,8 +28,11 @@ describe('Vuex members mutations', () => {
     });
 
     describe(types.RECEIVE_MEMBER_ROLE_ERROR, () => {
-      it('shows error message', () => {
-        mutations[types.RECEIVE_MEMBER_ROLE_ERROR](state);
+      describe('when error does not have a message', () => {
+        it('shows default error message', () => {
+          mutations[types.RECEIVE_MEMBER_ROLE_ERROR](state, {
+            error: new Error('Network Error'),
+          });
 
           expect(state.showError).toBe(true);
           expect(state.errorMessage).toBe(
@@ -38,6 +41,23 @@ describe('Vuex members mutations', () => {
         });
       });
 
+      describe('when error has a message', () => {
+        it('shows error message', () => {
+          const error = new Error('Request failed with status code 422');
+          const message =
+            'User email "john.smith@gmail.com" does not match the allowed domain of example.com';
+
+          error.response = {
+            data: { message },
+          };
+          mutations[types.RECEIVE_MEMBER_ROLE_ERROR](state, { error });
+
+          expect(state.showError).toBe(true);
+          expect(state.errorMessage).toBe(message);
+        });
+      });
+    });
+
     describe(types.RECEIVE_MEMBER_EXPIRATION_SUCCESS, () => {
       it('updates member', () => {
         const expiresAt = '2020-03-17T00:00:00Z';
@@ -52,8 +72,11 @@ describe('Vuex members mutations', () => {
     });
 
     describe(types.RECEIVE_MEMBER_EXPIRATION_ERROR, () => {
-      it('shows error message', () => {
-        mutations[types.RECEIVE_MEMBER_EXPIRATION_ERROR](state);
+      describe('when error does not have a message', () => {
+        it('shows default error message', () => {
+          mutations[types.RECEIVE_MEMBER_EXPIRATION_ERROR](state, {
+            error: new Error('Network Error'),
+          });
 
           expect(state.showError).toBe(true);
           expect(state.errorMessage).toBe(
@@ -61,6 +84,23 @@ describe('Vuex members mutations', () => {
           );
         });
       });
+
+      describe('when error has a message', () => {
+        it('shows error message', () => {
+          const error = new Error('Request failed with status code 422');
+          const message =
+            'User email "john.smith@gmail.com" does not match the allowed domain of example.com';
+
+          error.response = {
+            data: { message },
+          };
+          mutations[types.RECEIVE_MEMBER_EXPIRATION_ERROR](state, { error });
+
+          expect(state.showError).toBe(true);
+          expect(state.errorMessage).toBe(message);
+        });
+      });
+    });
   });
 
   describe(types.HIDE_ERROR, () => {

spec/services/members/update_service_spec.rb

@@ -13,9 +13,11 @@ RSpec.describe Members::UpdateService do
     { access_level: Gitlab::Access::MAINTAINER }
   end
 
+  subject { described_class.new(current_user, params).execute(member, permission: permission) }
+
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(current_user, params).execute(member, permission: permission) }
+      expect { subject }
         .to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
@@ -24,18 +26,24 @@ RSpec.describe Members::UpdateService do
     it 'updates the member' do
       expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name)
 
-      updated_member = described_class.new(current_user, params).execute(member, permission: permission)
+      updated_member = subject.fetch(:member)
 
       expect(updated_member).to be_valid
       expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER)
     end
 
+    it 'returns success status' do
+      result = subject.fetch(:status)
+
+      expect(result).to eq(:success)
+    end
+
     context 'when member is downgraded to guest' do
       shared_examples 'schedules to delete confidential todos' do
         it do
           expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once
 
-          updated_member = described_class.new(current_user, params).execute(member, permission: permission)
+          updated_member = subject.fetch(:member)
 
           expect(updated_member).to be_valid
           expect(updated_member.access_level).to eq(Gitlab::Access::GUEST)
@@ -62,6 +70,16 @@ RSpec.describe Members::UpdateService do
         expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"')
       end
     end
+
+    context 'when member is not valid' do
+      let(:params) { { expires_at: 2.days.ago } }
+
+      it 'returns error status' do
+        result = subject
+
+        expect(result[:status]).to eq(:error)
+      end
+    end
   end
 
   before do