Merge branch...

Merge branch 'dakotadux/add-rest-api-to-update-status-of-external-requirement-controller' into 'master' 

Add a REST controller for external API access

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



Merged-by: Douglas Barbosa Alexandre <dbalexandre@gmail.com>
Approved-by: Douglas Barbosa Alexandre <dbalexandre@gmail.com>
Reviewed-by: Huzaifa Iftikhar <hiftikhar@gitlab.com>
Reviewed-by: GitLab Duo <gitlab-duo@gitlab.com>
Reviewed-by: Dakota Dux <ddux@gitlab.com>
Co-authored-by: dakotadux <ddux@gitlab.com>

ee/app/models/compliance_management/compliance_framework/project_settings.rb

@@ -16,6 +16,7 @@ class ProjectSettings < ApplicationRecord
       delegate :full_path, to: :project
 
       scope :by_framework_and_project, ->(project_id, framework_id) { where(project_id: project_id, framework_id: framework_id) }
+      scope :by_project_id, ->(project_id) { where(project_id: project_id) }
 
       def self.find_or_create_by_project(project, framework)
         find_or_initialize_by(project: project).tap do |setting|

ee/app/services/compliance_management/compliance_framework/compliance_requirements_controls/update_status_service.rb

@@ -23,8 +23,6 @@ def execute
 
           audit_changes
           success
-        rescue ArgumentError => e
-          error(e.message)
         end
 
         private

ee/lib/api/compliance_external_controls.rb

+# frozen_string_literal: true
+
+module API
+  class ComplianceExternalControls < ::API::Base
+    VALID_STATUS_VALUES = %w[pass fail].freeze
+    VERIFICATION_TIMESTAMP_EXPIRY = 15.seconds
+    HMAC_ALGORITHM = 'SHA256'
+    SIGNATURE_HEADER = 'X-Gitlab-Hmac-Sha256'
+    TIMESTAMP_HEADER = 'X-Gitlab-Timestamp'
+    NONCE_HEADER = 'X-Gitlab-Nonce'
+    NONCE_NAMESPACE = 'control_statuses:nonce'
+    NONCE_LENGTH = 32
+    feature_category :compliance_management
+
+    params do
+      requires :id, types: [String, Integer],
+        desc: 'The ID or URL-encoded path of the project',
+        documentation: { example: 1 }
+    end
+    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      helpers do
+        def verify_hmac_signature!(control)
+          return error!('Control is not external', :forbidden) unless control.external?
+          return error!('Missing required headers', :unauthorized) unless valid_hmac_headers?
+
+          timestamp = headers[TIMESTAMP_HEADER]
+          nonce = headers[NONCE_HEADER]
+
+          return error!('Invalid nonce', :unauthorized) if invalid_nonce?(nonce)
+
+          store_nonce(nonce)
+
+          error!('Invalid signature', :unauthorized) unless valid_signature?(control, params, timestamp, nonce)
+          error!('Invalid timestamp', :unauthorized) if invalid_timestamp?(timestamp)
+          error!('Request has expired', :unauthorized) if expired_request?(timestamp)
+        end
+
+        def expired_request?(timestamp)
+          Time.current.to_i - timestamp.to_i >= VERIFICATION_TIMESTAMP_EXPIRY
+        end
+
+        def invalid_timestamp?(timestamp)
+          timestamp.to_i == 0 || timestamp.to_i > Time.current.to_i
+        end
+
+        def valid_hmac_headers?
+          [TIMESTAMP_HEADER, NONCE_HEADER, SIGNATURE_HEADER].all? { |header| headers[header].present? }
+        end
+
+        def valid_signature?(control, params, timestamp, nonce)
+          path = "/api/v4/projects/#{params[:id]}/compliance_external_controls/#{params[:control_id]}/status"
+          data = "status=#{params[:status]}"
+          sign_payload = "#{timestamp}#{nonce}#{path}#{data}"
+
+          expected_signature = OpenSSL::HMAC.hexdigest(
+            HMAC_ALGORITHM,
+            control.secret_token,
+            sign_payload
+          )
+
+          ActiveSupport::SecurityUtils.secure_compare(headers[SIGNATURE_HEADER], expected_signature)
+        end
+
+        def valid_project(control)
+          project = find_project(params[:id])
+          error!('Project not found', :not_found) if project.nil?
+
+          unless project.licensed_feature_available?(:custom_compliance_frameworks)
+            return error!('Not permitted to update compliance control status',
+              :unauthorized)
+          end
+
+          return project if control.compliance_requirement.framework.project_settings.by_project_id(project.id).any?
+
+          error!('Project not found', :not_found)
+        end
+
+        def valid_control(control_id)
+          control = ComplianceManagement::ComplianceFramework::ComplianceRequirementsControl.find_by_id(control_id)
+          error!('Control not found', :not_found) if control.nil?
+
+          control
+        end
+
+        def invalid_nonce?(nonce)
+          return true if nonce.blank? || nonce.length != NONCE_LENGTH
+
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.exists?(nonce_key(nonce)) # rubocop:disable CodeReuse/ActiveRecord -- not using ActiveRecord
+          end
+        end
+
+        def store_nonce(nonce)
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(nonce_key(nonce), '1', ex: VERIFICATION_TIMESTAMP_EXPIRY.to_i + 1)
+          end
+        end
+
+        def nonce_key(nonce)
+          "#{NONCE_NAMESPACE}:#{nonce}"
+        end
+      end
+
+      desc "Update the status of a control"
+      params do
+        requires :control_id, type: Integer, desc: 'The ID of the control'
+        requires :status, type: String, values: VALID_STATUS_VALUES, desc: 'The status of the control'
+      end
+      patch ':id/compliance_external_controls/:control_id/status' do
+        control = valid_control(params[:control_id])
+        verify_hmac_signature!(control)
+        project = valid_project(control)
+        user = ::Gitlab::Audit::UnauthenticatedAuthor.new
+        status = ComplianceManagement::ComplianceFramework::ComplianceRequirementsControls::UpdateStatusService.new(
+          current_user: user,
+          control: control,
+          project: project,
+          status_value: params[:status]
+        ).execute
+
+        if status.success?
+          status.payload
+        else
+          error!(status.message, :bad_request)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/api.rb

@@ -80,6 +80,7 @@ module API
         mount ::API::Chat
         mount ::API::DuoCodeReview
         mount ::API::SecurityScans
+        mount ::API::ComplianceExternalControls
         mount ::API::VirtualRegistries::Packages::Maven::Registries
         mount ::API::VirtualRegistries::Packages::Maven::Upstreams
         mount ::API::VirtualRegistries::Packages::Maven::Cache::Entries

ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb

@@ -53,20 +53,26 @@
   end
 
   describe 'scopes' do
-    describe '.by_framework_and_project' do
-      let_it_be(:framework1) do
-        create(:compliance_framework, namespace: project.group.root_ancestor, name: 'framework1')
-      end
+    let_it_be(:framework1) do
+      create(:compliance_framework, namespace: project.group.root_ancestor, name: 'framework1')
+    end
 
-      let_it_be(:setting) do
-        create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework1)
-      end
+    let_it_be(:setting) do
+      create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework1)
+    end
 
+    describe '.by_framework_and_project' do
       it 'returns the setting' do
         expect(described_class.by_framework_and_project(project.id, framework1.id))
           .to eq([setting])
       end
     end
+
+    describe '.by_project_id' do
+      it 'returns the setting' do
+        expect(described_class.by_project_id(project.id)).to eq([setting])
+      end
+    end
   end
 
   describe 'creation of ComplianceManagement::Framework record' do

ee/spec/requests/api/compliance_external_controls_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::ComplianceExternalControls, feature_category: :compliance_management do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:project2) { create(:project) }
+  let_it_be(:framework) { create(:compliance_framework, projects: [project]) }
+  let_it_be(:requirement) { create(:compliance_requirement, framework: framework) }
+  let_it_be(:control) do
+    create(:compliance_requirements_control, control_type: :external, secret_token: 'foo',
+      compliance_requirement: requirement, external_url: 'https://example.com')
+  end
+
+  let_it_be(:project_control_status) do
+    create(:project_control_compliance_status,
+      project: project,
+      compliance_requirements_control: control,
+      compliance_requirement: requirement,
+      status: 'pending')
+  end
+
+  let_it_be(:number_used_once) { SecureRandom.hex(16) }
+
+  describe 'PATCH /projects/:id/compliance_external_controls/:control_id/status' do
+    let(:path) { api("/projects/#{project.id}/compliance_external_controls/#{control.id}/status") }
+    let(:mock_redis) { instance_double(Redis) }
+
+    def generate_headers(
+      path:,
+      data:,
+      secret: control.secret_token,
+      timestamp: Time.now.to_i.to_s,
+      nonce: number_used_once
+    )
+      sign_payload = "#{timestamp}#{nonce}#{path}#{data}"
+      signature = OpenSSL::HMAC.hexdigest('SHA256', secret, sign_payload)
+
+      {
+        'X-Gitlab-Timestamp' => timestamp,
+        'X-Gitlab-Nonce' => nonce,
+        'X-Gitlab-Hmac-Sha256' => signature
+      }
+    end
+
+    before do
+      allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(mock_redis)
+
+      allow(mock_redis).to receive(:exists?)
+        .with("control_statuses:nonce:#{number_used_once}")
+        .and_return(false)
+
+      allow(mock_redis).to receive(:set)
+        .with("control_statuses:nonce:#{number_used_once}", '1', ex: 16)
+        .and_return('OK')
+      stub_licensed_features(custom_compliance_frameworks: true)
+    end
+
+    describe 'updates the control status' do
+      it 'updates the control status' do
+        %w[pass fail].each do |status|
+          data = "status=#{status}"
+          headers = generate_headers(path:, data:)
+
+          patch path, params: { status: }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response.body).to eq({ status: }.to_json)
+            expect(project_control_status.reload.status).to eq(status)
+          end
+        end
+      end
+
+      it 'calls UpdateStatusService with correct parameters' do
+        service_double = instance_double(
+          ComplianceManagement::ComplianceFramework::ComplianceRequirementsControls::UpdateStatusService
+        )
+        expect(ComplianceManagement::ComplianceFramework::ComplianceRequirementsControls::UpdateStatusService)
+          .to receive(:new)
+          .with(hash_including(
+            control: control,
+            project: project,
+            status_value: 'pass'
+          )) do |args|
+            expect(args[:current_user]).to be_a(::Gitlab::Audit::UnauthenticatedAuthor)
+            service_double
+          end
+        expect(service_double).to receive(:execute).and_return(ServiceResponse.success(payload: { status: 'pass' }))
+        data = "status=pass"
+        headers = generate_headers(path:, data:)
+
+        patch path, params: { status: 'pass' }, headers: headers
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+
+      it 'stores the nonce in Redis' do
+        expect(mock_redis).to receive(:set).with("control_statuses:nonce:#{number_used_once}", '1', ex: 16)
+        headers = generate_headers(path: path, data: "status=pass")
+
+        patch path, params: { status: 'pass' }, headers: headers
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
+
+    describe 'returns errors' do
+      before do
+        allow(project_control_status).to receive(:update!).and_raise("Unexpected update attempt")
+      end
+
+      context 'with control type restrictions' do
+        let_it_be(:internal_control) { create(:compliance_requirements_control, control_type: :internal) }
+        let(:path) { api("/projects/#{project.id}/compliance_external_controls/#{internal_control.id}/status") }
+
+        it 'returns forbidden for internal controls' do
+          data = "status=pass"
+          headers = generate_headers(path:, data:)
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:forbidden)
+            expect(json_response['error']).to eq('Control is not external')
+          end
+        end
+      end
+
+      context 'with invalid request' do
+        it 'does not update the control status with invalid status' do
+          data = "status=invalid"
+          headers = generate_headers(path:, data:)
+
+          patch path, params: { status: 'invalid' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['error']).to eq('status does not have a valid value')
+          end
+        end
+
+        it 'returns bad request when the status is not provided' do
+          data = "status="
+          headers = generate_headers(path:, data:)
+
+          patch path, params: { status: }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['error']).to eq('status does not have a valid value')
+          end
+        end
+      end
+
+      context 'with resource not found' do
+        it 'returns not found when the control id is not provided' do
+          data = "status=pass"
+          missing_control_path = "/projects/#{project.id}/compliance_external_controls/"
+          headers = generate_headers(path: missing_control_path, data: data)
+
+          patch api(missing_control_path), params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['error']).to eq('404 Not Found')
+          end
+        end
+
+        it 'returns not found when the control id is not found' do
+          data = "status=pass"
+          missing_control_path = "/projects/#{project.id}/compliance_external_controls/123/status"
+          headers = generate_headers(path: missing_control_path, data: data)
+
+          patch api(missing_control_path), params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['error']).to eq('Control not found')
+          end
+        end
+
+        it 'returns not found when the project id is not provided' do
+          data = "status=pass"
+          missing_project_path = "/projects/compliance_external_controls/#{control.id}/status"
+          headers = generate_headers(path: missing_project_path, data: data)
+
+          patch api(missing_project_path), params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['error']).to eq('404 Not Found')
+          end
+        end
+
+        it 'returns not found when the project id is not found' do
+          data = "status=pass"
+          missing_project_path = api("/projects/123/compliance_external_controls/#{control.id}/status")
+          headers = generate_headers(path: missing_project_path, data: data)
+
+          patch missing_project_path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['error']).to eq('Project not found')
+          end
+        end
+
+        it 'returns not found when the project id does not match the control id' do
+          data = "status=pass"
+          missing_project_path = api("/projects/#{project2.id}/compliance_external_controls/#{control.id}/status")
+          headers = generate_headers(path: missing_project_path, data: data)
+
+          patch missing_project_path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['error']).to eq('Project not found')
+          end
+        end
+      end
+
+      context 'with unauthorized request' do
+        it 'returns unauthorized when the sha256 header is missing' do
+          data = "status=pass"
+          headers = generate_headers(path: path, data: data)
+          headers.delete('X-Gitlab-Hmac-Sha256')
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(json_response['error']).to eq('Missing required headers')
+          end
+        end
+
+        it 'returns unauthorized when secret keys do not match' do
+          data = "status=pass"
+          wrong_secret = 'wrong_secret'
+          headers = generate_headers(path: path, data: data, secret: wrong_secret)
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(json_response['error']).to eq('Invalid signature')
+          end
+        end
+
+        it 'returns unauthorized when the timestamp is too old' do
+          [15, 16].each do |duration|
+            data = "status=pass"
+            headers = generate_headers(path: path, data: data, timestamp: (Time.now.to_i - duration).to_s)
+
+            patch path, params: { status: 'pass' }, headers: headers
+
+            aggregate_failures do
+              expect(response).to have_gitlab_http_status(:unauthorized)
+              expect(json_response['error']).to eq('Request has expired')
+            end
+          end
+        end
+
+        it 'returns bad request when the nonce is invalid' do
+          data = "status=pass"
+          headers = generate_headers(path: path, data: data, nonce: nil)
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(json_response['error']).to eq('Missing required headers')
+          end
+        end
+
+        it 'returns unauthorized with invalid nonces' do
+          ['too_short', 'a' * 64].each do |invalid_nonce|
+            data = "status=pass"
+            headers = generate_headers(path: path, data: data, nonce: invalid_nonce)
+
+            patch path, params: { status: 'pass' }, headers: headers
+
+            aggregate_failures do
+              expect(response).to have_gitlab_http_status(:unauthorized)
+              expect(json_response['error']).to eq('Invalid nonce')
+            end
+          end
+        end
+
+        it 'returns unauthorized when the nonce is already used' do
+          allow(mock_redis).to receive(:exists?)
+            .with("control_statuses:nonce:#{number_used_once}")
+            .and_return(true)
+          headers = generate_headers(path: path, data: "status=pass")
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(json_response['error']).to eq('Invalid nonce')
+          end
+        end
+
+        it 'returns unauthorized when the feature is not licensed' do
+          stub_licensed_features(custom_compliance_frameworks: false)
+          data = "status=pass"
+          headers = generate_headers(path:, data:)
+
+          patch path, params: { status: 'pass' }, headers: headers
+
+          aggregate_failures do
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(json_response['error']).to eq('Not permitted to update compliance control status')
+          end
+        end
+
+        it 'returns unauthorized when the timestamp is in the future' do
+          [0, (Time.now.to_i + 60).to_s].each do |invalid_timestamp|
+            data = "status=pass"
+            headers = generate_headers(path: path, data: data, timestamp: invalid_timestamp)
+
+            patch path, params: { status: 'pass' }, headers: headers
+
+            aggregate_failures do
+              expect(response).to have_gitlab_http_status(:unauthorized)
+              expect(json_response['error']).to eq('Invalid timestamp')
+            end
+          end
+        end
+      end
+
+      it 'returns error when the UpdateStatusService fails' do
+        allow_next_instance_of(
+          ComplianceManagement::ComplianceFramework::ComplianceRequirementsControls::UpdateStatusService
+        ) do |instance|
+          allow(instance).to receive(:execute).and_return(
+            ServiceResponse.error(message: 'Failed to update status')
+          )
+        end
+        data = "status=pass"
+        headers = generate_headers(path: path, data: data)
+
+        patch path, params: { status: 'pass' }, headers: headers
+
+        aggregate_failures do
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['error']).to eq('Failed to update status')
+        end
+      end
+    end
+  end
+end

ee/spec/services/compliance_management/compliance_framework/compliance_requirements_controls/update_status_service_spec.rb

@@ -157,18 +157,5 @@
         expect(::Gitlab::Audit::Auditor).not_to have_received(:audit)
       end
     end
-
-    context 'when an ArgumentError is raised' do
-      before do
-        allow(service).to receive(:update_control_status).and_raise(ArgumentError, 'test error message')
-      end
-
-      it 'returns an error response with the error message' do
-        result = service.execute
-
-        expect(result).to be_error
-        expect(result.message).to eq('Failed to update compliance control status. Error: test error message')
-      end
-    end
   end
 end