Merge branch 'id-refactor-find-certificate-logic-into-a-service' into 'master'

Refactor find certificate logic into a service

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/140063



Merged-by: Dylan Griffith <dyl.griffith@gmail.com>
Approved-by: Abhilash Kotte <akotte@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Reviewed-by: Igor Drozdov <idrozdov@gitlab.com>
Co-authored-by: Igor Drozdov <idrozdov@gitlab.com>

ee/app/services/groups/ssh_certificates/find_service.rb

+# frozen_string_literal: true
+
+module Groups
+  module SshCertificates
+    class FindService
+      def initialize(ca_fingerprint, user_identifier)
+        @ca_fingerprint = ca_fingerprint
+        @user_identifier = user_identifier
+      end
+
+      def execute
+        certificate = ::Groups::SshCertificate.find_by_fingerprint(ca_fingerprint)
+        return error('Certificate Not Found', :not_found) unless certificate
+
+        group = certificate.group
+        return error('Feature is not available', :forbidden) unless group.licensed_feature_available?(:ssh_certificates)
+
+        user = group.users.find_by_login(user_identifier)
+
+        return error('User Not Found', :not_found) unless user
+        return error('Not an Enterprise User of the group', :forbidden) unless user.enterprise_user_of_group?(group)
+
+        ServiceResponse.success(payload: { user: user, group: group })
+      end
+
+      private
+
+      attr_reader :ca_fingerprint, :user_identifier
+
+      def error(message, reason)
+        ServiceResponse.error(message: message, reason: reason)
+      end
+    end
+  end
+end

ee/lib/ee/api/internal/base.rb

@@ -88,15 +88,11 @@ def two_factor_push_otp_check
 
           namespace 'internal' do
             get '/authorized_certs', feature_category: :source_code_management, urgency: :high do
-              certificate = ::Groups::SshCertificate.find_by_fingerprint!(params[:key])
-              group = certificate.group
+              response = ::Groups::SshCertificates::FindService.new(params[:key], params[:user_identifier]).execute
 
-              forbidden!('Feature is not available') unless group.licensed_feature_available?(:ssh_certificates)
+              render_api_error!(response.message, response.reason) if response.error?
 
-              user = group.users.find_by_login(params[:user_identifier])
-
-              not_found!('User') unless user
-              forbidden!('Not an Enterprise User of the group') unless user.enterprise_user_of_group?(group)
+              group, user = response.payload.values_at(:group, :user)
 
               status 200
 

ee/spec/requests/api/internal/base_spec.rb

@@ -1047,7 +1047,7 @@ def lfs_auth_user(user_id, project)
           get(api('/internal/authorized_certs'), params: params, headers: gitlab_shell_internal_api_request_header)
 
           expect(response).to have_gitlab_http_status(:forbidden)
-          expect(json_response['message']).to eq('403 Forbidden - Not an Enterprise User of the group')
+          expect(json_response['message']).to eq('Not an Enterprise User of the group')
         end
       end
 
@@ -1071,7 +1071,7 @@ def lfs_auth_user(user_id, project)
           get(api('/internal/authorized_certs'), params: params, headers: gitlab_shell_internal_api_request_header)
 
           expect(response).to have_gitlab_http_status(:not_found)
-          expect(json_response['message']).to eq('404 Not found')
+          expect(json_response['message']).to eq('Certificate Not Found')
         end
       end
 
@@ -1082,7 +1082,7 @@ def lfs_auth_user(user_id, project)
           get(api('/internal/authorized_certs'), params: params, headers: gitlab_shell_internal_api_request_header)
 
           expect(response).to have_gitlab_http_status(:not_found)
-          expect(json_response['message']).to eq('404 User Not Found')
+          expect(json_response['message']).to eq('User Not Found')
         end
       end
     end
@@ -1092,7 +1092,7 @@ def lfs_auth_user(user_id, project)
         get(api('/internal/authorized_certs'), params: params, headers: gitlab_shell_internal_api_request_header)
 
         expect(response).to have_gitlab_http_status(:not_found)
-        expect(json_response['message']).to eq('404 User Not Found')
+        expect(json_response['message']).to eq('User Not Found')
       end
     end
 
@@ -1103,7 +1103,7 @@ def lfs_auth_user(user_id, project)
         get(api('/internal/authorized_certs'), params: params, headers: gitlab_shell_internal_api_request_header)
 
         expect(response).to have_gitlab_http_status(:forbidden)
-        expect(json_response['message']).to eq('403 Forbidden - Feature is not available')
+        expect(json_response['message']).to eq('Feature is not available')
       end
     end
   end

ee/spec/services/groups/ssh_certificates/find_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::SshCertificates::FindService, feature_category: :source_code_management do
+  let_it_be(:ssh_certificate) { create(:group_ssh_certificate) }
+  let_it_be(:group) { ssh_certificate.group }
+  let_it_be(:user) { create(:user, :enterprise_user, enterprise_group: group) }
+
+  let(:ca_fingerprint) { ssh_certificate.fingerprint }
+  let(:user_identifier) { user.username }
+  let(:service) { described_class.new(ca_fingerprint, user_identifier) }
+
+  before_all do
+    group.add_developer(user)
+  end
+
+  before do
+    stub_licensed_features(ssh_certificates: true)
+  end
+
+  describe '#execute' do
+    it 'returns successful response with payload' do
+      response = service.execute
+
+      expect(response).to be_success
+      expect(response.payload).to eq({ user: user, group: group })
+    end
+
+    context 'when a certificate not found' do
+      let(:ca_fingerprint) { 'does not exist' }
+
+      it 'returns not found error' do
+        response = service.execute
+
+        expect(response).to be_error
+        expect(response.message).to eq('Certificate Not Found')
+        expect(response.reason).to eq(:not_found)
+      end
+    end
+
+    context 'when ssh_certificates feature is not available' do
+      it 'returns forbidden error' do
+        stub_licensed_features(ssh_certificates: false)
+
+        response = service.execute
+
+        expect(response).to be_error
+        expect(response.message).to eq('Feature is not available')
+        expect(response.reason).to eq(:forbidden)
+      end
+    end
+
+    context 'when a user is not found' do
+      let(:user_identifier) { 'does not exist' }
+
+      it 'returns not found error' do
+        response = service.execute
+
+        expect(response).to be_error
+        expect(response.message).to eq('User Not Found')
+        expect(response.reason).to eq(:not_found)
+      end
+    end
+
+    context 'when a user is not a member' do
+      let_it_be(:user) { create(:user) }
+
+      it 'returns not found error' do
+        response = service.execute
+
+        expect(response).to be_error
+        expect(response.message).to eq('User Not Found')
+        expect(response.reason).to eq(:not_found)
+      end
+    end
+
+    context 'when a user is not an enterprise user' do
+      let_it_be(:user) { create(:user) }
+
+      it 'returns not found error' do
+        group.add_developer(user)
+
+        response = service.execute
+
+        expect(response).to be_error
+        expect(response.message).to eq('Not an Enterprise User of the group')
+        expect(response.reason).to eq(:forbidden)
+      end
+    end
+  end
+end