diff options
| author | Douwe Maan <douwe@gitlab.com> | 2016-07-07 22:37:30 +0000 |
|---|---|---|
| committer | Douwe Maan <douwe@gitlab.com> | 2016-07-07 22:37:30 +0000 |
| commit | bf89e06a459556fc55c0f5582a552ede8f6675c8 (patch) | |
| tree | 5bfaeb0186c5bf201ce3cb58c403c982b7ac32ea | |
| parent | 39fbec941939f76ec08a25f537cc3f4a308e21f5 (diff) | |
| parent | b1c81f849e5e5b03f56e89cdcefba029ed5c0543 (diff) | |
| download | gitlab-ce-bf89e06a459556fc55c0f5582a552ede8f6675c8.tar.gz | |
Merge branch '18627-wildcard-branch-protection' into 'master'
Allow specifying protected branches using wildcards
Closes #18627
# Tasks
- [ ] #18627 !4665 Allow specifying protected branches using wildcards
- [x] Find existing usages of protected branches
- Protecting branches
- `ProtectedBranchesController` is used to mark a branch protected/unprotected
- `API::Branches` can be used to mark a branch protected/unprotected
- Enforcing branch protection
- `Gitlab::GitAccess` has helpers (`can_push_to_branch?`, `check`) that are used to deny pushes if a branch is protected
- Over SSH: `gitlab-shell` receives a push, and calls `/allowed` on the GitLab API, which calls `GitAccess.check`
- Over HTTP:
- `gitlab-workhorse` receives the request, and forwards it to rails
- Rails (in the `GitHttpController#git-recieve-pack`) runs basic checks (is the user logged in, not protected branch checks) and returns ok with `GL_ID` and `RepoPath`
- `gitlab-workhorse` looks at the response, and calls the relevant `gitlab-shell` action from `git-http/handlePostRPC`
- Rest of this flow is the same as the SSH flow above
- [x] Implementation
- [x] Backend
- [x] Change `project#protected_branch?` to look at wildcard protected branches
- [x] Change `project#developers_can_push_to_protected_branch?`
- [x] Change `project#open_branches`
- [x] Better error message when creating a disallowed branch from the Web UI
- [x] Frontend
- [x] Protected branches page should allow typing out a wildcard pattern
- [x] Add help text explaining the use of wildcards
- [x] Show matching branches for each protected branch
- [x] ~~On the index page~~
- [x] On a show page
- [x] Index?
- [x] Can't have the "last commit" column for wildcard protected branches
- [x] Fix / write tests
- [x] What happens if a hook is missing in dev?
- [x] Refactor
- [x] Test workflows
- Create a branch matching a wildcard pattern
- Push to a branch matching a wildcard pattern
- Force push to a branch matching a wildcard pattern
- Delete a branch matching a wildcard pattern
- [x] Test using Web UI
- [x] Test over SSH
- [x] Test over HTTP
- [x] Test as developer and master
- [x] Investigate performance
- [x] Test with a large number of protected branches / branches
- [x] Paginate list of protected branches
- [x] ~~Possibly rewrite `open_branches`~~
- [x] Add `iid`s to existing `ProtectedBranch`es
- [x] Add documentation
- [x] Add CHANGELOG entry
- [x] Add screenshots
- [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/2f753e3ed2ce681b4444944d521f4419e8ed37f7/builds) passes
- [x] Assign to endboss for review
- [x] Address @DouweM's comments
- [x] `protected_branch_params`
- [x] `exact_match` instead of `explicit_match`
- [x] When would self.name be blank?
- [x] Move `protected_branches.each` to a partial
- [x] Move `matching_branches.each` to a partial
- [x] If the branch is in @matching_branches, it's not been removed
- [x] move this regex to a method and memoize it
- [x] `commit_sha` directly for exact matches
- [x] Number of matches for wildcard matches, with a link
- [x] Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/43f9ce0e88194b8f719bb1c1e656b7fc13278d56/builds) to pass
- [x] Respond to @DouweM's comments
- [x] Don't use iid
- [x] Controller should use `@project.protected_branches.new`
- [x] move the memoization to `def wildcard_regex`
- [x] render with `collection: @protected_branches`
- [x] Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/f7beedf122fa0c7aa89e86181fe7499321fb10ca/builds) to pass
- [x] Wait for @DouweM's review
- [x] Wait for @jschatz1's review
- [x] Respond to @jschatz1's comments
- [x] Use the new dropdown style
- [x] description should be moved to the description section without the styling
- [x] Protect button should be disabled when no branch is selected
- [x] Update screenshots
- [x] Merge conflicts
- [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/20f3cfe8d5540eab64c2ba548043d600b28c61ba/builds) passes
- [ ] Revisit performance, possibly with staging/production data
- [ ] Get a dump of staging / run against staging live
- [ ] Get SSH access to staging
- [ ] Wait for review/merge
# Screenshots
## Creating wildcard protected branches
### Using the `GLDropdown` component
## Enforcing wildcard protected branches
### From the Web UI
### Over SSH
### Over HTTPS
## Listing matching branches
See merge request !4665
24 files changed, 585 insertions, 70 deletions
diff --git a/CHANGELOG b/CHANGELOG index 491692204a6..7fbfa5e7377 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -25,6 +25,7 @@ v 8.10.0 (unreleased) - Updated layout for Projects, Groups, Users on Admin area !4424 - Fix changing issue state columns in milestone view - Add notification settings dropdown for groups + - Wildcards for protected branches. !4665 - Allow importing from Github using Personal Access Tokens. (Eric K Idema) - API: Todos !3188 (Robert Schilling) - Add "Enabled Git access protocols" to Application Settings diff --git a/app/assets/javascripts/gl_dropdown.js.coffee b/app/assets/javascripts/gl_dropdown.js.coffee index ed9dfcc917e..1c65e833d47 100644 --- a/app/assets/javascripts/gl_dropdown.js.coffee +++ b/app/assets/javascripts/gl_dropdown.js.coffee @@ -56,6 +56,7 @@ class GitLabDropdownFilter return BLUR_KEYCODES.indexOf(keyCode) >= 0 filter: (search_text) -> + @options.onFilter(search_text) if @options.onFilter data = @options.data() if data? and not @options.filterByText @@ -195,6 +196,7 @@ class GitLabDropdown @filter = new GitLabDropdownFilter @filterInput, filterInputBlur: @filterInputBlur filterByText: @options.filterByText + onFilter: @options.onFilter remote: @options.filterRemote query: @options.data keys: searchFields @@ -530,7 +532,7 @@ class GitLabDropdown if $el.length e.preventDefault() e.stopImmediatePropagation() - $(selector, @dropdown)[0].click() + $el.first().trigger('click') addArrowKeyEvent: -> ARROW_KEY_CODES = [38, 40] diff --git a/app/assets/javascripts/protected_branch_select.js.coffee b/app/assets/javascripts/protected_branch_select.js.coffee new file mode 100644 index 00000000000..6d45770ace9 --- /dev/null +++ b/app/assets/javascripts/protected_branch_select.js.coffee @@ -0,0 +1,40 @@ +class @ProtectedBranchSelect + constructor: (currentProject) -> + $('.dropdown-footer').hide(); + @dropdown = $('.js-protected-branch-select').glDropdown( + data: @getProtectedBranches + filterable: true + remote: false + search: + fields: ['title'] + selectable: true + toggleLabel: (selected) -> if (selected and 'id' of selected) then selected.title else 'Protected Branch' + fieldName: 'protected_branch[name]' + text: (protected_branch) -> _.escape(protected_branch.title) + id: (protected_branch) -> _.escape(protected_branch.id) + onFilter: @toggleCreateNewButton + clicked: () -> $('.protect-branch-btn').attr('disabled', false) + ) + + $('.create-new-protected-branch').on 'click', (event) => + # Refresh the dropdown's data, which ends up calling `getProtectedBranches` + @dropdown.data('glDropdown').remote.execute() + @dropdown.data('glDropdown').selectRowAtIndex(event, 0) + + getProtectedBranches: (term, callback) => + if @selectedBranch + callback(gon.open_branches.concat(@selectedBranch)) + else + callback(gon.open_branches) + + toggleCreateNewButton: (branchName) => + @selectedBranch = { title: branchName, id: branchName, text: branchName } + + if branchName is '' + $('.protected-branch-select-footer-list').addClass('hidden') + $('.dropdown-footer').hide(); + else + $('.create-new-protected-branch').text("Create Protected Branch: #{branchName}") + $('.protected-branch-select-footer-list').removeClass('hidden') + $('.dropdown-footer').show(); + diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index 5753c9d4e72..79c2306e4d2 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -11,7 +11,8 @@ $ -> dataType: "json" data: id: id - developers_can_push: checked + protected_branch: + developers_can_push: checked success: -> row = $(e.target) diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index efa7bf14d0f..80dad758afa 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -2,12 +2,14 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController # Authorize before_action :require_non_empty_project before_action :authorize_admin_project! + before_action :load_protected_branch, only: [:show, :update, :destroy] layout "project_settings" def index - @branches = @project.protected_branches.to_a + @protected_branches = @project.protected_branches.order(:name).page(params[:page]) @protected_branch = @project.protected_branches.new + gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } }) end def create @@ -16,26 +18,24 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController @project) end - def update - protected_branch = @project.protected_branches.find(params[:id]) - - if protected_branch && - protected_branch.update_attributes( - developers_can_push: params[:developers_can_push] - ) + def show + @matching_branches = @protected_branch.matching(@project.repository.branches) + end + def update + if @protected_branch && @protected_branch.update_attributes(protected_branch_params) respond_to do |format| - format.json { render json: protected_branch, status: :ok } + format.json { render json: @protected_branch, status: :ok } end else respond_to do |format| - format.json { render json: protected_branch.errors, status: :unprocessable_entity } + format.json { render json: @protected_branch.errors, status: :unprocessable_entity } end end end def destroy - @project.protected_branches.find(params[:id]).destroy + @protected_branch.destroy respond_to do |format| format.html { redirect_to namespace_project_protected_branches_path } @@ -45,6 +45,10 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController private + def load_protected_branch + @protected_branch = @project.protected_branches.find(params[:id]) + end + def protected_branch_params params.require(:protected_branch).permit(:name, :developers_can_push) end diff --git a/app/models/project.rb b/app/models/project.rb index 029026a4e56..5005ee7cd32 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -802,18 +802,12 @@ class Project < ActiveRecord::Base @repo_exists = false end + # Branches that are not _exactly_ matched by a protected branch. def open_branches - # We're using a Set here as checking values in a large Set is faster than - # checking values in a large Array. - protected_set = Set.new(protected_branch_names) - - repository.branches.reject do |branch| - protected_set.include?(branch.name) - end - end - - def protected_branch_names - @protected_branch_names ||= protected_branches.pluck(:name) + exact_protected_branch_names = protected_branches.reject(&:wildcard?).map(&:name) + branch_names = repository.branches.map(&:name) + non_open_branch_names = Set.new(exact_protected_branch_names).intersection(Set.new(branch_names)) + repository.branches.reject { |branch| non_open_branch_names.include? branch.name } end def root_ref?(branch) @@ -830,11 +824,12 @@ class Project < ActiveRecord::Base # Check if current branch name is marked as protected in the system def protected_branch?(branch_name) - protected_branch_names.include?(branch_name) + @protected_branches ||= self.protected_branches.to_a + ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present? end def developers_can_push_to_protected_branch?(branch_name) - protected_branches.any? { |pb| pb.name == branch_name && pb.developers_can_push } + protected_branches.matching(branch_name).any?(&:developers_can_push) end def forked? diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 33cf046fa75..b7011d7afdf 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -8,4 +8,51 @@ class ProtectedBranch < ActiveRecord::Base def commit project.commit(self.name) end + + # Returns all protected branches that match the given branch name. + # This realizes all records from the scope built up so far, and does + # _not_ return a relation. + # + # This method optionally takes in a list of `protected_branches` to search + # through, to avoid calling out to the database. + def self.matching(branch_name, protected_branches: nil) + (protected_branches || all).select { |protected_branch| protected_branch.matches?(branch_name) } + end + + # Returns all branches (among the given list of branches [`Gitlab::Git::Branch`]) + # that match the current protected branch. + def matching(branches) + branches.select { |branch| self.matches?(branch.name) } + end + + # Checks if the protected branch matches the given branch name. + def matches?(branch_name) + return false if self.name.blank? + + exact_match?(branch_name) || wildcard_match?(branch_name) + end + + # Checks if this protected branch contains a wildcard + def wildcard? + self.name && self.name.include?('*') + end + + protected + + def exact_match?(branch_name) + self.name == branch_name + end + + def wildcard_match?(branch_name) + wildcard_regex === branch_name + end + + def wildcard_regex + @wildcard_regex ||= begin + name = self.name.gsub('*', 'STAR_DONT_ESCAPE') + quoted_name = Regexp.quote(name) + regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?') + /\A#{regex_string}\z/ + end + end end diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 565905cbe7b..97cb1a9052b 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -1,6 +1,6 @@ %h5.prepend-top-0 - Already Protected (#{@branches.size}) -- if @branches.empty? + Already Protected (#{@protected_branches.size}) +- if @protected_branches.empty? %p.settings-message.text-center No branches are protected, protect a branch with the form above. - else @@ -9,33 +9,18 @@ %table.table.protected-branches-list %colgroup %col{ width: "30%" } - %col{ width: "30%" } + %col{ width: "25%" } %col{ width: "25%" } - if can_admin_project %col %thead %tr - %th Branch - %th Last commit - %th Developers can push + %th Protected Branch + %th Commit + %th Developers Can Push - if can_admin_project %th %tbody - - @branches.each do |branch| - - @url = namespace_project_protected_branch_path(@project.namespace, @project, branch) - %tr - %td - = link_to(branch.name, namespace_project_commits_path(@project.namespace, @project, branch.name)) - - if @project.root_ref?(branch.name) - %span.label.label-info.prepend-left-5 default - %td - - if commit = branch.commit - = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id') - #{time_ago_with_tooltip(commit.committed_date)} - - else - (branch was removed from repository) - %td - = check_box_tag("developers_can_push", branch.id, branch.developers_can_push, data: { url: @url }) - - if can_admin_project - %td - = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm" + = render partial: @protected_branches, locals: { can_admin_project: can_admin_project } + + = paginate @protected_branches, theme: 'gitlab' diff --git a/app/views/projects/protected_branches/_dropdown.html.haml b/app/views/projects/protected_branches/_dropdown.html.haml new file mode 100644 index 00000000000..b803d932e67 --- /dev/null +++ b/app/views/projects/protected_branches/_dropdown.html.haml @@ -0,0 +1,17 @@ += f.hidden_field(:name) + += dropdown_tag("Protected Branch", + options: { title: "Pick protected branch", toggle_class: 'js-protected-branch-select js-filter-submit', + filter: true, dropdown_class: "dropdown-menu-selectable", placeholder: "Search protected branches", + footer_content: true, + data: { show_no: true, show_any: true, show_upcoming: true, + selected: params[:protected_branch_name], + project_id: @project.try(:id) } }) do + + %ul.dropdown-footer-list.hidden.protected-branch-select-footer-list + %li + = link_to '#', title: "New Protected Branch", class: "create-new-protected-branch" do + Create new + +:javascript + new ProtectedBranchSelect(); diff --git a/app/views/projects/protected_branches/_matching_branch.html.haml b/app/views/projects/protected_branches/_matching_branch.html.haml new file mode 100644 index 00000000000..8a5332ca5bb --- /dev/null +++ b/app/views/projects/protected_branches/_matching_branch.html.haml @@ -0,0 +1,9 @@ +%tr + %td + = link_to matching_branch.name, namespace_project_tree_path(@project.namespace, @project, matching_branch.name) + - if @project.root_ref?(matching_branch.name) + %span.label.label-info.prepend-left-5 default + %td + - commit = @project.commit(matching_branch.name) + = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id') + = time_ago_with_tooltip(commit.committed_date) diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml new file mode 100644 index 00000000000..474aec3a97c --- /dev/null +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -0,0 +1,21 @@ +- url = namespace_project_protected_branch_path(@project.namespace, @project, protected_branch) +%tr + %td + = protected_branch.name + - if @project.root_ref?(protected_branch.name) + %span.label.label-info.prepend-left-5 default + %td + - if protected_branch.wildcard? + - matching_branches = protected_branch.matching(repository.branches) + = link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch) + - else + - if commit = protected_branch.commit + = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id') + = time_ago_with_tooltip(commit.committed_date) + - else + (branch was removed from repository) + %td + = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url }) + - if can_admin_project + %td + = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right" diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index c7d317dbaee..5669713d9a1 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -4,30 +4,38 @@ .col-lg-3 %h4.prepend-top-0 = page_title - %p Keep stable branches secure and force developers to use Merge Requests - .col-lg-9 - %h5.prepend-top-0 - Protect a branch - .account-well.append-bottom-default - %p.light-header.append-bottom-0 Protected branches are designed to + %p Keep stable branches secure and force developers to use merge requests. + %p.prepend-top-20 + Protected branches are designed to: %ul %li prevent pushes from everybody except #{link_to "masters", help_page_path("permissions", "permissions"), class: "vlink"} %li prevent anyone from force pushing to the branch %li prevent anyone from deleting the branch %p.append-bottom-0 Read more about #{link_to "project permissions", help_page_path("permissions", "permissions"), class: "underlined-link"} + .col-lg-9 + %h5.prepend-top-0 + Protect a branch - if can? current_user, :admin_project, @project = form_for [@project.namespace.becomes(Namespace), @project, @protected_branch] do |f| = form_errors(@protected_branch) .form-group = f.label :name, "Branch", class: "label-light" - = f.select(:name, @project.open_branches.map { |br| [br.name, br.name] } , {include_blank: true}, {class: "select2", data: {placeholder: "Select branch"}}) + = render partial: "dropdown", locals: { f: f } + %p.help-block + = link_to "Wildcards", help_page_path(category: 'workflow', file: 'protected_branches', format: 'md', anchor: "wildcard-protected-branches") + such as + %code *-stable + or + %code production/* + are supported. + .form-group = f.check_box :developers_can_push, class: "pull-left" .prepend-left-20 = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" %p.light.append-bottom-0 Allow developers to push to this branch - = f.submit "Protect", class: "btn-create btn" + = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true %hr = render "branches_list" diff --git a/app/views/projects/protected_branches/show.html.haml b/app/views/projects/protected_branches/show.html.haml new file mode 100644 index 00000000000..4d8169815b3 --- /dev/null +++ b/app/views/projects/protected_branches/show.html.haml @@ -0,0 +1,25 @@ +- page_title @protected_branch.name, "Protected Branches" + +.row.prepend-top-default.append-bottom-default + .col-lg-3 + %h4.prepend-top-0 + = @protected_branch.name + + .col-lg-9 + %h5 Matching Branches + - if @matching_branches.present? + .table-responsive + %table.table.protected-branches-list + %colgroup + %col{ width: "30%" } + %col{ width: "30%" } + %thead + %tr + %th Branch + %th Last commit + %tbody + - @matching_branches.each do |matching_branch| + = render partial: "matching_branch", object: matching_branch + - else + %p.settings-message.text-center + Couldn't find any matching branches. diff --git a/config/routes.rb b/config/routes.rb index 1572656b8c5..18a4ead2b37 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -720,7 +720,7 @@ Rails.application.routes.draw do resource :release, only: [:edit, :update] end - resources :protected_branches, only: [:index, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } + resources :protected_branches, only: [:index, :show, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } resources :variables, only: [:index, :show, :update, :create, :destroy] resources :triggers, only: [:index, :create, :destroy] diff --git a/doc/workflow/protected_branches.md b/doc/workflow/protected_branches.md index d854ec1e025..67adfc2f43a 100644 --- a/doc/workflow/protected_branches.md +++ b/doc/workflow/protected_branches.md @@ -1,4 +1,4 @@ -# Protected branches +# Protected Branches Permissions in GitLab are fundamentally defined around the idea of having read or write permission to the repository and branches. @@ -28,4 +28,28 @@ For those workflows, you can allow everyone with write access to push to a prote On already protected branches you can also allow developers to push to the repository by selecting the `Developers can push` check box. -
\ No newline at end of file + + +## Wildcard Protected Branches + +>**Note:** +This feature was added in GitLab 8.10. + +1. You can specify a wildcard protected branch, which will protect all branches matching the wildcard. For example: + + | Wildcard Protected Branch | Matching Branches | + |---------------------------+--------------------------------------------------------| + | `*-stable` | `production-stable`, `staging-stable` | + | `production/*` | `production/app-server`, `production/load-balancer` | + | `*gitlab*` | `gitlab`, `gitlab/staging`, `master/gitlab/production` | + +1. Protected branch settings (like "Developers Can Push") apply to all matching branches. + +1. Two different wildcards can potentially match the same branch. For example, `*-stable` and `production-*` would both match a `production-stable` branch. + >**Note:** + If _any_ of these protected branches have "Developers Can Push" set to true, then `production-stable` has it set to true. + +1. If you click on a protected branch's name, you will be presented with a list of all matching branches: + +  + diff --git a/doc/workflow/protected_branches/protected_branches1.png b/doc/workflow/protected_branches/protected_branches1.png Binary files differindex bb3ab7d7913..c00443803de 100644 --- a/doc/workflow/protected_branches/protected_branches1.png +++ b/doc/workflow/protected_branches/protected_branches1.png diff --git a/doc/workflow/protected_branches/protected_branches2.png b/doc/workflow/protected_branches/protected_branches2.png Binary files differindex 58ace31ac57..a4f664d3b21 100644 --- a/doc/workflow/protected_branches/protected_branches2.png +++ b/doc/workflow/protected_branches/protected_branches2.png diff --git a/doc/workflow/protected_branches/protected_branches3.png b/doc/workflow/protected_branches/protected_branches3.png Binary files differnew file mode 100644 index 00000000000..2a50cb174bb --- /dev/null +++ b/doc/workflow/protected_branches/protected_branches3.png diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index 57c41b41298..9b681e636c7 100644 --- a/lib/gitlab/git/hook.rb +++ b/lib/gitlab/git/hook.rb @@ -15,7 +15,7 @@ module Gitlab end def trigger(gl_id, oldrev, newrev, ref) - return true unless exists? + return [true, nil] unless exists? case name when "pre-receive", "post-receive" @@ -70,13 +70,10 @@ module Gitlab end def call_update_hook(gl_id, oldrev, newrev, ref) - status = nil - Dir.chdir(repo_path) do - status = system({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev) + stdout, stderr, status = Open3.capture3({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev) + [status.success?, stderr.presence || stdout] end - - [status, nil] end def retrieve_error_message(stderr, stdout) diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb new file mode 100644 index 00000000000..d94dee0c797 --- /dev/null +++ b/spec/features/protected_branches_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' + +feature 'Projected Branches', feature: true, js: true do + let(:user) { create(:user, :admin) } + let(:project) { create(:project) } + + before { login_as(user) } + + def set_protected_branch_name(branch_name) + find(".js-protected-branch-select").click + find(".dropdown-input-field").set(branch_name) + click_on "Create Protected Branch: #{branch_name}" + end + + describe "explicit protected branches" do + it "allows creating explicit protected branches" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('some-branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('some-branch') } + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.name).to eq('some-branch') + end + + it "displays the last commit on the matching branch if it exists" do + commit = create(:commit, project: project) + project.repository.add_branch(user, 'some-branch', commit.id) + + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('some-branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content(commit.id[0..7]) } + end + + it "displays an error message if the named branch does not exist" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('some-branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('branch was removed') } + end + end + + describe "wildcard protected branches" do + it "allows creating protected branches with a wildcard" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('*-stable') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('*-stable') } + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.name).to eq('*-stable') + end + + it "displays the number of matching branches" do + project.repository.add_branch(user, 'production-stable', 'master') + project.repository.add_branch(user, 'staging-stable', 'master') + + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('*-stable') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content("2 matching branches") } + end + + it "displays all the branches matching the wildcard" do + project.repository.add_branch(user, 'production-stable', 'master') + project.repository.add_branch(user, 'staging-stable', 'master') + project.repository.add_branch(user, 'development', 'master') + create(:protected_branch, project: project, name: "*-stable") + + visit namespace_project_protected_branches_path(project.namespace, project) + click_on "2 matching branches" + + within(".protected-branches-list") do + expect(page).to have_content("production-stable") + expect(page).to have_content("staging-stable") + expect(page).not_to have_content("development") + end + end + end +end diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb new file mode 100644 index 00000000000..a15aa173fbd --- /dev/null +++ b/spec/lib/gitlab/git/hook_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' +require 'fileutils' + +describe Gitlab::Git::Hook, lib: true do + describe "#trigger" do + let(:project) { create(:project) } + let(:user) { create(:user) } + + def create_hook(name) + FileUtils.mkdir_p(File.join(project.repository.path, 'hooks')) + File.open(File.join(project.repository.path, 'hooks', name), 'w', 0755) do |f| + f.write('exit 0') + end + end + + def create_failing_hook(name) + FileUtils.mkdir_p(File.join(project.repository.path, 'hooks')) + File.open(File.join(project.repository.path, 'hooks', name), 'w', 0755) do |f| + f.write(<<-HOOK) + echo 'regular message from the hook' + echo 'error message from the hook' 1>&2 + exit 1 + HOOK + end + end + + ['pre-receive', 'post-receive', 'update'].each do |hook_name| + + context "when triggering a #{hook_name} hook" do + context "when the hook is successful" do + it "returns success with no errors" do + create_hook(hook_name) + hook = Gitlab::Git::Hook.new(hook_name, project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be true + expect(errors).to be_blank + end + end + + context "when the hook is unsuccessful" do + it "returns failure with errors" do + create_failing_hook(hook_name) + hook = Gitlab::Git::Hook.new(hook_name, project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be false + expect(errors).to eq("error message from the hook\n") + end + end + end + end + + context "when the hook doesn't exist" do + it "returns success with no errors" do + hook = Gitlab::Git::Hook.new('unknown_hook', project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be true + expect(errors).to be_nil + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1b434a726dc..5a27ccbab0a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -449,6 +449,14 @@ describe Project, models: true do it { expect(project.open_branches.map(&:name)).to include('feature') } it { expect(project.open_branches.map(&:name)).not_to include('master') } + + it "includes branches matching a protected branch wildcard" do + expect(project.open_branches.map(&:name)).to include('feature') + + create(:protected_branch, name: 'feat*', project: project) + + expect(Project.find(project.id).open_branches.map(&:name)).to include('feature') + end end describe '#star_count' do @@ -949,15 +957,67 @@ describe Project, models: true do describe '#protected_branch?' do let(:project) { create(:empty_project) } - it 'returns true when a branch is a protected branch' do + it 'returns true when the branch matches a protected branch via direct match' do project.protected_branches.create!(name: 'foo') expect(project.protected_branch?('foo')).to eq(true) end - it 'returns false when a branch is not a protected branch' do + it 'returns true when the branch matches a protected branch via wildcard match' do + project.protected_branches.create!(name: 'production/*') + + expect(project.protected_branch?('production/some-branch')).to eq(true) + end + + it 'returns false when the branch does not match a protected branch via direct match' do expect(project.protected_branch?('foo')).to eq(false) end + + it 'returns false when the branch does not match a protected branch via wildcard match' do + project.protected_branches.create!(name: 'production/*') + + expect(project.protected_branch?('staging/some-branch')).to eq(false) + end + end + + describe "#developers_can_push_to_protected_branch?" do + let(:project) { create(:empty_project) } + + context "when the branch matches a protected branch via direct match" do + it "returns true if 'Developers can Push' is turned on" do + create(:protected_branch, name: "production", project: project, developers_can_push: true) + + expect(project.developers_can_push_to_protected_branch?('production')).to be true + end + + it "returns false if 'Developers can Push' is turned off" do + create(:protected_branch, name: "production", project: project, developers_can_push: false) + + expect(project.developers_can_push_to_protected_branch?('production')).to be false + end + end + + context "when the branch matches a protected branch via wilcard match" do + it "returns true if 'Developers can Push' is turned on" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: true) + + expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true + end + + it "returns false if 'Developers can Push' is turned off" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: false) + + expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false + end + end + + context "when the branch does not match a protected branch" do + it "returns false" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: true) + + expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false + end + end end describe '#container_registry_path_with_namespace' do diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index b523834c6e9..8bf0d24a128 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe ProtectedBranch, models: true do + subject { build_stubbed(:protected_branch) } + describe 'Associations' do it { is_expected.to belong_to(:project) } end @@ -12,4 +14,127 @@ describe ProtectedBranch, models: true do it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:name) } end + + describe "#matches?" do + context "when the protected branch setting is not a wildcard" do + let(:protected_branch) { build(:protected_branch, name: "production/some-branch") } + + it "returns true for branch names that are an exact match" do + expect(protected_branch.matches?("production/some-branch")).to be true + end + + it "returns false for branch names that are not an exact match" do + expect(protected_branch.matches?("staging/some-branch")).to be false + end + end + + context "when the protected branch name contains wildcard(s)" do + context "when there is a single '*'" do + let(:protected_branch) { build(:protected_branch, name: "production/*") } + + it "returns true for branch names matching the wildcard" do + expect(protected_branch.matches?("production/some-branch")).to be true + expect(protected_branch.matches?("production/")).to be true + end + + it "returns false for branch names not matching the wildcard" do + expect(protected_branch.matches?("staging/some-branch")).to be false + expect(protected_branch.matches?("production")).to be false + end + end + + context "when the wildcard contains regex symbols other than a '*'" do + let(:protected_branch) { build(:protected_branch, name: "pro.duc.tion/*") } + + it "returns true for branch names matching the wildcard" do + expect(protected_branch.matches?("pro.duc.tion/some-branch")).to be true + end + + it "returns false for branch names not matching the wildcard" do + expect(protected_branch.matches?("production/some-branch")).to be false + expect(protected_branch.matches?("proXducYtion/some-branch")).to be false + end + end + + context "when there are '*'s at either end" do + let(:protected_branch) { build(:protected_branch, name: "*/production/*") } + + it "returns true for branch names matching the wildcard" do + expect(protected_branch.matches?("gitlab/production/some-branch")).to be true + expect(protected_branch.matches?("/production/some-branch")).to be true + expect(protected_branch.matches?("gitlab/production/")).to be true + expect(protected_branch.matches?("/production/")).to be true + end + + it "returns false for branch names not matching the wildcard" do + expect(protected_branch.matches?("gitlabproductionsome-branch")).to be false + expect(protected_branch.matches?("production/some-branch")).to be false + expect(protected_branch.matches?("gitlab/production")).to be false + expect(protected_branch.matches?("production")).to be false + end + end + + context "when there are arbitrarily placed '*'s" do + let(:protected_branch) { build(:protected_branch, name: "pro*duction/*/gitlab/*") } + + it "returns true for branch names matching the wildcard" do + expect(protected_branch.matches?("production/some-branch/gitlab/second-branch")).to be true + expect(protected_branch.matches?("proXYZduction/some-branch/gitlab/second-branch")).to be true + expect(protected_branch.matches?("proXYZduction/gitlab/gitlab/gitlab")).to be true + expect(protected_branch.matches?("proXYZduction//gitlab/")).to be true + expect(protected_branch.matches?("proXYZduction/some-branch/gitlab/")).to be true + expect(protected_branch.matches?("proXYZduction//gitlab/some-branch")).to be true + end + + it "returns false for branch names not matching the wildcard" do + expect(protected_branch.matches?("production/some-branch/not-gitlab/second-branch")).to be false + expect(protected_branch.matches?("prodXYZuction/some-branch/gitlab/second-branch")).to be false + expect(protected_branch.matches?("proXYZduction/gitlab/some-branch/gitlab")).to be false + expect(protected_branch.matches?("proXYZduction/gitlab//")).to be false + expect(protected_branch.matches?("proXYZduction/gitlab/")).to be false + expect(protected_branch.matches?("proXYZduction//some-branch/gitlab")).to be false + end + end + end + end + + describe "#matching" do + context "for direct matches" do + it "returns a list of protected branches matching the given branch name" do + production = create(:protected_branch, name: "production") + staging = create(:protected_branch, name: "staging") + + expect(ProtectedBranch.matching("production")).to include(production) + expect(ProtectedBranch.matching("production")).not_to include(staging) + end + + it "accepts a list of protected branches to search from, so as to avoid a DB call" do + production = build(:protected_branch, name: "production") + staging = build(:protected_branch, name: "staging") + + expect(ProtectedBranch.matching("production")).to be_empty + expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).not_to include(staging) + end + end + + context "for wildcard matches" do + it "returns a list of protected branches matching the given branch name" do + production = create(:protected_branch, name: "production/*") + staging = create(:protected_branch, name: "staging/*") + + expect(ProtectedBranch.matching("production/some-branch")).to include(production) + expect(ProtectedBranch.matching("production/some-branch")).not_to include(staging) + end + + it "accepts a list of protected branches to search from, so as to avoid a DB call" do + production = build(:protected_branch, name: "production/*") + staging = build(:protected_branch, name: "staging/*") + + expect(ProtectedBranch.matching("production/some-branch")).to be_empty + expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).not_to include(staging) + end + end + end end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 9f9ef20f99b..6b99b0f24cb 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -63,7 +63,7 @@ module TestEnv end def disable_pre_receive - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(true) + allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) end # Clean /tmp/tests |
