Merge branch 'tpresa-expose-reject-unsigned-commits' into 'master'

Expose reject_unsigned_commits on the API

Closes #13036

See merge request gitlab-org/gitlab-ee!14165

doc/api/projects.md

@@ -1676,18 +1676,20 @@ GET /projects/:id/push_rule
   "author_email_regex": "",
   "file_name_regex": "",
   "max_file_size": 5,
-  "commit_committer_check": false
+  "commit_committer_check": false,
+  "reject_unsigned_commits": false
 }
 ```
 
 Users on GitLab [Premium, Silver, or higher](https://about.gitlab.com/pricing/) will also see
-the `commit_committer_check` parameter:
+the `commit_committer_check` and `reject_unsigned_commits` parameters:
 
 ```json
 {
   "id": 1,
   "project_id": 3,
-  "commit_committer_check": false
+  "commit_committer_check": false,
+  "reject_unsigned_commits": false
   ...
 }
 ```
@@ -1713,6 +1715,7 @@ POST /projects/:id/push_rule
 | `file_name_regex` **(STARTER)**               | string         | no       | All commited filenames must **not** match this, e.g. `(jar|exe)$` |
 | `max_file_size` **(STARTER)**                 | integer        | no       | Maximum file size (MB) |
 | `commit_committer_check` **(PREMIUM)**        | boolean        | no       | Users can only push commits to this repository that were committed with one of their own verified emails. |
+| `reject_unsigned_commits` **(PREMIUM)**        | boolean        | no       | Reject commit when it is not signed through GPG. |
 
 ### Edit project push rule
 
@@ -1735,6 +1738,7 @@ PUT /projects/:id/push_rule
 | `file_name_regex` **(STARTER)**               | string         | no       | All commited filenames must **not** match this, e.g. `(jar|exe)$` |
 | `max_file_size` **(STARTER)**                 | integer        | no       | Maximum file size (MB) |
 | `commit_committer_check` **(PREMIUM)**        | boolean        | no       | Users can only push commits to this repository that were committed with one of their own verified emails. |
+| `reject_unsigned_commits` **(PREMIUM)**        | boolean        | no       | Reject commits when they are not GPG signed. |
 
 ### Delete project push rule
 

ee/app/policies/ee/project_policy.rb

@@ -200,6 +200,8 @@ module EE
 
       rule { admin | (reject_unsigned_commits_disabled_globally & can?(:maintainer_access)) }.enable :change_reject_unsigned_commits
 
+      rule { reject_unsigned_commits_available }.enable :read_reject_unsigned_commits
+
       rule { ~reject_unsigned_commits_available }.prevent :change_reject_unsigned_commits
 
       rule { admin | (commit_committer_check_disabled_globally & can?(:maintainer_access)) }.policy do

ee/changelogs/unreleased/tpresa-expose-reject-unsigned-commits.yml

+---
+title: Expose reject_unsigned_commits option via the API
+merge_request: 14165
+author:
+type: added

ee/lib/api/project_push_rule.rb

@@ -5,11 +5,7 @@ module API
     before { authenticate! }
     before { authorize_admin_project }
     before { check_project_feature_available!(:push_rules) }
-    before do
-      if params.has_key?(:commit_committer_check)
-        authorize! :change_commit_committer_check, user_project
-      end
-    end
+    before { authorize_change_param(user_project, :commit_committer_check, :reject_unsigned_commits) }
 
     params do
       requires :id, type: String, desc: 'The ID of a project'
@@ -27,10 +23,12 @@ module API
           optional :file_name_regex, type: String, desc: 'All commited filenames must not match this'
           optional :max_file_size, type: Integer, desc: 'Maximum file size (MB)'
           optional :commit_committer_check, type: Boolean, desc: 'Users may only push their own commits'
+          optional :reject_unsigned_commits, type: Boolean, desc: 'Only GPG signed commits can be pushed to this project'
           at_least_one_of :deny_delete_tag, :member_check, :prevent_secrets,
                           :commit_message_regex, :commit_message_negative_regex, :branch_name_regex, :author_email_regex,
                           :file_name_regex, :max_file_size,
-                          :commit_committer_check
+                          :commit_committer_check,
+                          :reject_unsigned_commits
         end
       end
 

ee/lib/ee/api/entities.rb

@@ -21,6 +21,16 @@ module EE
         end
       end
 
+      module EntityHelpers
+        def can_read(attr, &block)
+          ->(obj, opts) { Ability.allowed?(opts[:user], "read_#{attr}".to_sym, yield(obj)) }
+        end
+
+        def expose_restricted(attr, &block)
+          expose attr, if: can_read(attr, &block)
+        end
+      end
+
       module UserPublic
         extend ActiveSupport::Concern
 
@@ -206,13 +216,13 @@ module EE
       # EE-specific entities #
       ########################
       class ProjectPushRule < Grape::Entity
+        extend EntityHelpers
         expose :id, :project_id, :created_at
         expose :commit_message_regex, :commit_message_negative_regex, :branch_name_regex, :deny_delete_tag
         expose :member_check, :prevent_secrets, :author_email_regex
         expose :file_name_regex, :max_file_size
-        expose :commit_committer_check, if: ->(rule, opts) do
-          Ability.allowed?(opts[:user], :read_commit_committer_check, rule.project)
-        end
+        expose_restricted :commit_committer_check, &:project
+        expose_restricted :reject_unsigned_commits, &:project
       end
 
       class LdapGroupLink < Grape::Entity

ee/lib/ee/api/helpers.rb

@@ -50,6 +50,12 @@ module EE
         not_found! unless user_project.feature_available?(feature)
       end
 
+      def authorize_change_param(subject, *keys)
+        keys.each do |key|
+          authorize!("change_#{key}".to_sym, subject) if params.has_key?(key)
+        end
+      end
+
       def check_sha_param!(params, merge_request)
         if params[:sha] && merge_request.diff_head_sha != params[:sha]
           render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)

ee/spec/lib/ee/api/helpers_spec.rb

@@ -90,4 +90,32 @@ describe EE::API::Helpers do
       expect(JSON.parse(last_response.body)).to eq({ 'message' => '401 Unauthorized' })
     end
   end
+
+  describe '#authorize_change_param' do
+    subject { Class.new.include(described_class).new }
+    let(:project) { create(:project) }
+
+    before do
+      allow(subject).to receive(:params).and_return({ change_commit_committer_check: true })
+    end
+
+    it 'does not throw exception if param is authorized' do
+      allow(subject).to receive(:authorize!).and_return(nil)
+
+      expect { subject.authorize_change_param(project, :change_commit_committer_check) }.not_to raise_error
+    end
+
+    context 'unauthorized param' do
+      before do
+        allow(subject).to receive(:authorize!).and_raise(Exception.new("Forbidden"))
+      end
+      it 'throws exception if unauthorized param is present' do
+        expect { subject.authorize_change_param(project, :change_commit_committer_check) }.to raise_error
+      end
+
+      it 'does not throw exception is unauthorized param is not present' do
+        expect { subject.authorize_change_param(project, :reject_unsigned_commit) }.not_to raise_error
+      end
+    end
+  end
 end

ee/spec/policies/project_policy_spec.rb

@@ -896,4 +896,35 @@ describe ProjectPolicy do
       it { is_expected.to be_allowed(:read_commit_committer_check) }
     end
   end
+
+  context 'reject_unsigned_commits is not enabled by the current license' do
+    before do
+      stub_licensed_features(reject_unsigned_commits: false)
+    end
+
+    let(:current_user) { maintainer }
+
+    it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
+    it { is_expected.not_to be_allowed(:read_reject_unsigned_commits) }
+  end
+
+  context 'reject_unsigned_commits is enabled by the current license' do
+    before do
+      stub_licensed_features(reject_unsigned_commits: true)
+    end
+
+    context 'the user is a maintainer' do
+      let(:current_user) { maintainer }
+
+      it { is_expected.to be_allowed(:change_reject_unsigned_commits) }
+      it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
+    end
+
+    context 'the user is a developer' do
+      let(:current_user) { developer }
+
+      it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
+      it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
+    end
+  end
 end

ee/spec/requests/api/project_push_rule_spec.rb

@@ -8,13 +8,15 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
 
   before do
     stub_licensed_features(push_rules: push_rules_enabled,
-                           commit_committer_check: ccc_enabled)
+                           commit_committer_check: ccc_enabled,
+                           reject_unsigned_commits: ruc_enabled)
     project.add_maintainer(user)
     project.add_developer(user3)
   end
 
   let(:push_rules_enabled) { true }
   let(:ccc_enabled) { true }
+  let(:ruc_enabled) { true }
 
   describe "GET /projects/:id/push_rule" do
     before do
@@ -47,15 +49,26 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
         end
       end
 
-      context 'the commit_committer_check feature is not enabled' do
-        let(:ccc_enabled) { false }
+      context 'the reject_unsigned_commits feature is enabled' do
+        let(:ruc_enabled) { true }
+
+        it 'returns the reject_unsigned_commits information' do
+          subset = attributes
+            .slice(:reject_unsigned_commits)
+            .transform_keys(&:to_s)
+          expect(json_response).to include(subset)
+        end
+      end
+
+      context 'the reject_unsigned_commits feature is not enabled' do
+        let(:ruc_enabled) { false }
 
         it 'succeeds' do
           expect(response).to have_gitlab_http_status(200)
         end
 
-        it 'does not return the commit_committer_check information' do
-          expect(json_response).not_to have_key('commit_committer_check')
+        it 'does not return the reject_unsigned_commits information' do
+          expect(json_response).not_to have_key('reject_unsigned_commits')
         end
       end
 
@@ -87,7 +100,8 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
         author_email_regex: '[a-zA-Z0-9]+@gitlab.com',
         file_name_regex: '[a-zA-Z0-9]+.key',
         max_file_size: 5,
-        commit_committer_check: true }
+        commit_committer_check: true,
+        reject_unsigned_commits: true }
     end
 
     let(:expected_response) do
@@ -107,6 +121,14 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
         end
       end
 
+      context 'reject_unsigned_commits not allowed by License' do
+        let(:ruc_enabled) { false }
+
+        it "is forbidden to use this service" do
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+
       it "is accepted" do
         expect(response).to have_gitlab_http_status(201)
       end
@@ -143,6 +165,31 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
           end
         end
       end
+
+      context 'reject_unsigned_commits is not enabled' do
+        let(:ruc_enabled) { false }
+
+        it "is forbidden to send the the :reject_unsigned_commits parameter" do
+          expect(response).to have_gitlab_http_status(403)
+        end
+
+        context "without the :reject_unsigned_commits parameter" do
+          let(:rules_params) do
+            { deny_delete_tag: true,
+              member_check: true,
+              prevent_secrets: true,
+              commit_message_regex: 'JIRA\-\d+',
+              branch_name_regex: '(feature|hotfix)\/*',
+              author_email_regex: '[a-zA-Z0-9]+@gitlab.com',
+              file_name_regex: '[a-zA-Z0-9]+.key',
+              max_file_size: 5 }
+          end
+
+          it "sets all given parameters" do
+            expect(json_response).to include(expected_response)
+          end
+        end
+      end
     end
 
     it 'adds push rule to project with no file size' do
@@ -220,6 +267,26 @@ describe API::ProjectPushRule, 'ProjectPushRule', api: true do
       context 'the commit_committer_check feature is not enabled' do
         let(:ccc_enabled) { false }
 
+        it "is an error to provide this parameter" do
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context "setting reject_unsigned_commits" do
+      let(:new_settings) { { reject_unsigned_commits: true } }
+
+      it "is successful" do
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it "sets the reject_unsigned_commits" do
+        expect(json_response).to include('reject_unsigned_commits' => true)
+      end
+
+      context 'the reject_unsigned_commits feature is not enabled' do
+        let(:ruc_enabled) { false }
+
         it "is an error to provide the this parameter" do
           expect(response).to have_gitlab_http_status(403)
         end