From a62ad80a0ff0765d91ba50de945267125ac14c08 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 28 Oct 2014 17:36:35 +0200 Subject: [PATCH 1/5] Git hooks for checking email --- .../projects/git_hooks_controller.rb | 2 +- app/views/projects/git_hooks/index.html.haml | 10 ++++++++++ ...3526_add_author_email_regex_to_git_hook.rb | 5 +++++ db/schema.rb | 3 ++- features/project/git_hooks.feature | 8 ++++++++ features/steps/project/git_hooks.rb | 19 +++++++++++++++++++ features/steps/shared/paths.rb | 4 ++++ lib/gitlab/git_access.rb | 10 +++++++--- 8 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20141027173526_add_author_email_regex_to_git_hook.rb create mode 100644 features/project/git_hooks.feature create mode 100644 features/steps/project/git_hooks.rb diff --git a/app/controllers/projects/git_hooks_controller.rb b/app/controllers/projects/git_hooks_controller.rb index c774b4233a..999e18dc1b 100644 --- a/app/controllers/projects/git_hooks_controller.rb +++ b/app/controllers/projects/git_hooks_controller.rb @@ -27,6 +27,6 @@ class Projects::GitHooksController < Projects::ApplicationController # Only allow a trusted parameter "white list" through. def git_hook_params - params.require(:git_hook).permit(:deny_delete_tag, :delete_branch_regex, :commit_message_regex, :force_push_regex) + params.require(:git_hook).permit(:deny_delete_tag, :delete_branch_regex, :commit_message_regex, :force_push_regex, :author_email_regex) end end diff --git a/app/views/projects/git_hooks/index.html.haml b/app/views/projects/git_hooks/index.html.haml index dedb6b6c2f..decfedacd0 100644 --- a/app/views/projects/git_hooks/index.html.haml +++ b/app/views/projects/git_hooks/index.html.haml @@ -44,5 +44,15 @@ If this field is empty it allows any commit message. For example you can require that an issue number is always mentioned in the commit message. + .form-group + = f.label :author_email_regex, "Commit author's email", class: 'control-label' + .col-sm-10 + = f.text_field :author_email_regex, class: "form-control", placeholder: 'Example: Fixes @my-company.com$' + %p.hint + All commit author's email must match this + = link_to 'Ruby regular expression', 'http://www.ruby-doc.org/core-2.1.1/Regexp.html' + to be pushed. + If this field is empty it allows any email. + .form-actions = f.submit "Save Git hooks", class: "btn btn-create" diff --git a/db/migrate/20141027173526_add_author_email_regex_to_git_hook.rb b/db/migrate/20141027173526_add_author_email_regex_to_git_hook.rb new file mode 100644 index 0000000000..bf58a13e81 --- /dev/null +++ b/db/migrate/20141027173526_add_author_email_regex_to_git_hook.rb @@ -0,0 +1,5 @@ +class AddAuthorEmailRegexToGitHook < ActiveRecord::Migration + def change + add_column :git_hooks, :author_email_regex, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 1d728f5c5b..dc2aff403d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20141010132608) do +ActiveRecord::Schema.define(version: 20141027173526) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -91,6 +91,7 @@ ActiveRecord::Schema.define(version: 20141010132608) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" + t.string "author_email_regex" end create_table "issues", force: true do |t| diff --git a/features/project/git_hooks.feature b/features/project/git_hooks.feature new file mode 100644 index 0000000000..b340b4a6c3 --- /dev/null +++ b/features/project/git_hooks.feature @@ -0,0 +1,8 @@ +Feature: Git Hooks + Background: + Given I sign in as a user + And I own project "Shop" + + Scenario: I should see git hook form + When I visit project git hooks page + Then I should see git hook form \ No newline at end of file diff --git a/features/steps/project/git_hooks.rb b/features/steps/project/git_hooks.rb new file mode 100644 index 0000000000..025bbb3e41 --- /dev/null +++ b/features/steps/project/git_hooks.rb @@ -0,0 +1,19 @@ +require 'webmock' + +class Spinach::Features::GitHooks < Spinach::FeatureSteps + include SharedAuthentication + include SharedProject + include SharedPaths + include RSpec::Matchers + include RSpec::Mocks::ExampleMethods + include WebMock::API + + + step 'I should see git hook form' do + page.should have_selector('input#git_hook_commit_message_regex') + page.should have_content "Commit message" + page.should have_content "Commit author's email" + end + + +end diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index cf51a9d369..de356d9577 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -228,6 +228,10 @@ module SharedPaths visit project_hooks_path(@project) end + step 'I visit project git hooks page' do + visit project_git_hooks_path(@project) + end + step 'I visit project deploy keys page' do visit project_deploy_keys_path(@project) end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 893f413dc5..9848e28e4a 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -109,11 +109,15 @@ module Gitlab end # Check commit messages unless its branch removal - if git_hook.commit_message_regex.present? && newrev !~ /00000000/ + if (git_hook.commit_message_regex.present? || git_hook.author_email_regex.present?) && newrev !~ /00000000/ commits = project.repository.commits_between(oldrev, newrev) commits.each do |commit| - unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex) - return false + if git_hook.commit_message_regex.present? + return false unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex) + end + if git_hook.author_email_regex.present? + return false unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex) + return false unless commit.author_email =~ Regexp.new(git_hook.author_email_regex) end end end From 76764338dd4dd4afb3c8323ab1d0158a3423b19a Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 31 Oct 2014 16:19:58 +0200 Subject: [PATCH 2/5] Git Hook to check whether author is a GitLab member --- .../projects/git_hooks_controller.rb | 3 ++- app/models/user.rb | 5 ++++ app/views/projects/git_hooks/index.html.haml | 24 +++++++++++++------ ...030133853_add_member_check_to_git_hooks.rb | 5 ++++ db/schema.rb | 5 +++- lib/gitlab/git_access.rb | 10 +++++++- spec/models/user_spec.rb | 21 ++++++++++++++++ 7 files changed, 63 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20141030133853_add_member_check_to_git_hooks.rb diff --git a/app/controllers/projects/git_hooks_controller.rb b/app/controllers/projects/git_hooks_controller.rb index 999e18dc1b..71c8faf0d0 100644 --- a/app/controllers/projects/git_hooks_controller.rb +++ b/app/controllers/projects/git_hooks_controller.rb @@ -27,6 +27,7 @@ class Projects::GitHooksController < Projects::ApplicationController # Only allow a trusted parameter "white list" through. def git_hook_params - params.require(:git_hook).permit(:deny_delete_tag, :delete_branch_regex, :commit_message_regex, :force_push_regex, :author_email_regex) + params.require(:git_hook).permit(:deny_delete_tag, :delete_branch_regex, + :commit_message_regex, :force_push_regex, :author_email_regex, :member_check) end end diff --git a/app/models/user.rb b/app/models/user.rb index f4215ceef8..a8117871b7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -213,6 +213,11 @@ class User < ActiveRecord::Base User.where(name: name).first end + def existing_member?(email) + !!(User.where(email: email).first || + User.joins(:emails).where(emails: { email: email }).first) + end + def filter(filter_name) case filter_name when "admins"; self.admins diff --git a/app/views/projects/git_hooks/index.html.haml b/app/views/projects/git_hooks/index.html.haml index decfedacd0..45a66a98cd 100644 --- a/app/views/projects/git_hooks/index.html.haml +++ b/app/views/projects/git_hooks/index.html.haml @@ -13,13 +13,23 @@ .form-group = f.label :deny_delete_tag, "Prevent tag removal", class: 'control-label' .col-sm-10 - .checkbox - = f.check_box :deny_delete_tag - %span.descr - Do not allow users to remove git tags with - = succeed '.' do - %code git push - Tags can still be deleted through the web UI. + %label + .checkbox + = f.check_box :deny_delete_tag + %span.descr + Do not allow users to remove git tags with + = succeed '.' do + %code git push + Tags can still be deleted through the web UI. + + .form-group + = f.label :member_check, "Restrict commit authors to existing Gitlab users", class: 'control-label' + .col-sm-10 + %label + .checkbox + = f.check_box :member_check + %span.descr + Check whether author is a GitLab member -#.form-group = f.label :force_push_regex, "Force push", class: 'control-label' diff --git a/db/migrate/20141030133853_add_member_check_to_git_hooks.rb b/db/migrate/20141030133853_add_member_check_to_git_hooks.rb new file mode 100644 index 0000000000..15ce6eec89 --- /dev/null +++ b/db/migrate/20141030133853_add_member_check_to_git_hooks.rb @@ -0,0 +1,5 @@ +class AddMemberCheckToGitHooks < ActiveRecord::Migration + def change + add_column :git_hooks, :member_check, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index dc2aff403d..31707d2862 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20141027173526) do +ActiveRecord::Schema.define(version: 20141030133853) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -91,7 +91,10 @@ ActiveRecord::Schema.define(version: 20141027173526) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" + t.string "username_regex" + t.string "email_regex" t.string "author_email_regex" + t.boolean "member_check", default: false, null: false end create_table "issues", force: true do |t| diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9848e28e4a..d2fde28963 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -109,7 +109,7 @@ module Gitlab end # Check commit messages unless its branch removal - if (git_hook.commit_message_regex.present? || git_hook.author_email_regex.present?) && newrev !~ /00000000/ + if (git_hook.commit_message_regex.present? || git_hook.author_email_regex.present? || git_hook.member_check) && newrev !~ /00000000/ commits = project.repository.commits_between(oldrev, newrev) commits.each do |commit| if git_hook.commit_message_regex.present? @@ -119,6 +119,14 @@ module Gitlab return false unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex) return false unless commit.author_email =~ Regexp.new(git_hook.author_email_regex) end + + # Check whether author is a GitLab member + if git_hook.member_check + return false unless User.existing_member?(commit.author_email) + if commit.author_email != commit.committer_email + return false unless User.existing_member?(commit.committer_email) + end + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6ad57b06e0..0ff84b8375 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -449,6 +449,27 @@ describe User do end end + describe "#existing_member?" do + it "returns true for exisitng user" do + create :user, email: "bruno@example.com" + + expect(User.existing_member?("bruno@example.com")).to be_true + end + + it "returns false for unknown exisitng user" do + create :user, email: "bruno@example.com" + + expect(User.existing_member?("rendom@example.com")).to be_false + end + + it "returns true if additional email exists" do + user = create :user + user.emails.create(email: "bruno@example.com") + + expect(User.existing_member?("bruno@example.com")).to be_true + end + end + describe "#sort" do before do User.delete_all From fd3109f8d7196dd44d7f3507079a4ccb8a02fe42 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 31 Oct 2014 16:26:51 +0200 Subject: [PATCH 3/5] updated changelog --- CHANGELOG-EE | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 06e03abc4b..6b322d3677 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -1,3 +1,7 @@ +v 7.5.0 + - Added an ability to check each author commit's email by regex + - Added an abulity to restrict commit authors to existing Gitlab users + v 7.4.0 - Support for multiple LDAP servers - Skip AD specific LDAP checks From cc34c09e69355b7d314cabed68a7397da87b5918 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 31 Oct 2014 16:36:01 +0200 Subject: [PATCH 4/5] update schema --- db/schema.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 31707d2862..8ae23b7d69 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -91,8 +91,6 @@ ActiveRecord::Schema.define(version: 20141030133853) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.string "username_regex" - t.string "email_regex" t.string "author_email_regex" t.boolean "member_check", default: false, null: false end From a7f0160f9e8f3f0898207e65d6c58abe4ef9aa57 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 3 Nov 2014 14:16:50 +0200 Subject: [PATCH 5/5] git hook: code style --- app/models/git_hook.rb | 4 ++++ app/models/user.rb | 3 +-- lib/gitlab/git_access.rb | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/models/git_hook.rb b/app/models/git_hook.rb index f45f57aa99..0be8337856 100644 --- a/app/models/git_hook.rb +++ b/app/models/git_hook.rb @@ -13,4 +13,8 @@ class GitHook < ActiveRecord::Base true end end + + def commit_validation? + commit_message_regex.present? || author_email_regex.present? || member_check + end end diff --git a/app/models/user.rb b/app/models/user.rb index a8117871b7..d9b0033fc4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -214,8 +214,7 @@ class User < ActiveRecord::Base end def existing_member?(email) - !!(User.where(email: email).first || - User.joins(:emails).where(emails: { email: email }).first) + User.where(email: email).any? || Email.where(email: email).any? end def filter(filter_name) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index d2fde28963..88e5f3642b 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -109,12 +109,13 @@ module Gitlab end # Check commit messages unless its branch removal - if (git_hook.commit_message_regex.present? || git_hook.author_email_regex.present? || git_hook.member_check) && newrev !~ /00000000/ + if git_hook.commit_validation? && newrev !~ /00000000/ commits = project.repository.commits_between(oldrev, newrev) commits.each do |commit| if git_hook.commit_message_regex.present? return false unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex) end + if git_hook.author_email_regex.present? return false unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex) return false unless commit.author_email =~ Regexp.new(git_hook.author_email_regex) @@ -156,5 +157,6 @@ module Gitlab nil end end + end end