Branch permissions not properly checked for fast-forward merges
Zendesk issue: https://gitlab.zendesk.com/agent/tickets/34231
A customer has branch permissions on master set so developers can merge, but not push. They reported that merge requests would spin indefinitely and would not get merged if a developer chose 'Accept Merge Request'. The logs showed:
2016-08-18_21:36:11.09623 2016-08-18T21:36:11.096Z 6052 TID-25sva4 WARN: {"class":"MergeWorker","args":[3,2,{"utf8":"✓","authenticity_token":"/jwJE0RBoZEDlrNRsC15p06L87NR+KC98jiHh5BNN3gnv7AQUfLWC2bbrz/AlVBx++40y3Ya7X/v3IAFLcYSqw==","sha":"37fc0007713846ea4fe1d086e540cec6f2316c4d","merge_when_build_succeeds":"","button":"","controller":"projects/merge_requests","action":"merge","namespace_id":"root","project_id":"test","id":"3"}],"retry":true,"queue":"default","jid":"6ffdc2abd4389c5edb023877","created_at":1471556167.7439814,"enqueued_at":1471556167.7442226,"error_message":"GitLab: You are not allowed to push code to protected branches on this project.\n","error_class":"GitHooksService::PreReceiveError","failed_at":1471556171.0846725,"retry_count":0}
2016-08-18_21:36:11.09668 2016-08-18T21:36:11.096Z 6052 TID-25sva4 WARN: GitHooksService::PreReceiveError: GitLab: You are not allowed to push code to protected branches on this project.
2016-08-18_21:36:11.09670
2016-08-18_21:36:11.09741 2016-08-18T21:36:11.097Z 6052 TID-25sva4 WARN: /opt/gitlab/embedded/service/gitlab-rails/app/services/git_hooks_service.rb:15:in `block in execute'
2016-08-18_21:36:11.09743 /opt/gitlab/embedded/service/gitlab-rails/app/services/git_hooks_service.rb:11:in `each'
2016-08-18_21:36:11.09743 /opt/gitlab/embedded/service/gitlab-rails/app/services/git_hooks_service.rb:11:in `execute'
2016-08-18_21:36:11.09743 /opt/gitlab/embedded/service/gitlab-rails/app/models/repository.rb:1192:in `block in commit_with_hooks'
2016-08-18_21:36:11.09744 /opt/gitlab/embedded/service/gitlab-rails/app/models/repository.rb:1167:in `with_tmp_ref'
2016-08-18_21:36:11.09744 /opt/gitlab/embedded/service/gitlab-rails/app/models/repository.rb:1184:in `commit_with_hooks'
2016-08-18_21:36:11.09744 /opt/gitlab/embedded/service/gitlab-rails/app/models/repository.rb:894:in `ff_merge'
2016-08-18_21:36:11.09744 /opt/gitlab/embedded/service/gitlab-rails/app/services/merge_requests/ff_merge_service.rb:12:in `commit'
2016-08-18_21:36:11.09745 /opt/gitlab/embedded/service/gitlab-rails/app/services/merge_requests/merge_service.rb:22:in `block in execute'
2016-08-18_21:36:11.09745 /opt/gitlab/embedded/service/gitlab-rails/app/models/merge_request.rb:724:in `in_locked_state'
2016-08-18_21:36:11.09745 /opt/gitlab/embedded/service/gitlab-rails/app/services/merge_requests/merge_service.rb:21:in `execute'
2016-08-18_21:36:11.09746 /opt/gitlab/embedded/service/gitlab-rails/app/services/merge_requests/merge_service.rb:13:in `execute'
2016-08-18_21:36:11.09746 /opt/gitlab/embedded/service/gitlab-rails/app/workers/merge_worker.rb:12:in `perform'
2016-08-18_21:36:11.09747 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:152:in `execute_job'
2016-08-18_21:36:11.09747 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:134:in `block (2 levels) in process'
2016-08-18_21:36:11.09747 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:128:in `block in invoke'
2016-08-18_21:36:11.09748 /opt/gitlab/embedded/service/gitlab-rails/lib/gitlab/sidekiq_middleware/memory_killer.rb:17:in `call'
2016-08-18_21:36:11.09749 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:130:in `block in invoke'
2016-08-18_21:36:11.09750 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/server/active_record.rb:6:in `call'
2016-08-18_21:36:11.09750 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:130:in `block in invoke'
2016-08-18_21:36:11.09750 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/server/retry_jobs.rb:74:in `call'
2016-08-18_21:36:11.09750 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:130:in `block in invoke'
2016-08-18_21:36:11.09751 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/server/logging.rb:11:in `block in call'
2016-08-18_21:36:11.09751 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/logging.rb:32:in `with_context'
2016-08-18_21:36:11.09751 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/server/logging.rb:7:in `call'
2016-08-18_21:36:11.09751 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:130:in `block in invoke'
2016-08-18_21:36:11.09751 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:133:in `call'
2016-08-18_21:36:11.09752 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/middleware/chain.rb:133:in `invoke'
2016-08-18_21:36:11.09752 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:129:in `block in process'
2016-08-18_21:36:11.09752 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:168:in `stats'
2016-08-18_21:36:11.09752 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:128:in `process'
2016-08-18_21:36:11.09753 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:80:in `process_one'
2016-08-18_21:36:11.09753 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/processor.rb:68:in `run'
2016-08-18_21:36:11.09753 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/util.rb:17:in `watchdog'
2016-08-18_21:36:11.09753 /opt/gitlab/embedded/service/gem/ruby/2.1.0/gems/sidekiq-4.1.4/lib/sidekiq/util.rb:25:in `block in safe_thread'Initially, I could not reproduce this issue. Then, I noticed during one of our tests the customer had to click 'Rebase' on a merge request. We used the Rails console to create a Gitlab::UserAccess object and call can_merge_to_branch? and it returned true. I then started to suspect the if matching_merge_request? conditional at https://gitlab.com/gitlab-org/gitlab-ee/blob/master/lib/gitlab/checks/change_access.rb#L35. I notice that this checks for a merge request with some specific attributes. Notably - in_progress_merge_commit_sha. This value is normally set at https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/models/repository.rb#L934, but fast-forward merges go through the #ff_merge method, instead.
The Repository#ff_merge class (https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/models/repository.rb#L902) does not set an in_progress_merge_commit_sha which is why this problem occurs.
Possible solution
Set in_progress_merge_commit_sha to the sha of their_commit in #ff_merge. The downside I can see to this is that the attribute really doesn't make as much sense, since technically fast-foward merges don't have a 'merge commit'. Do we care about that technicality?
Activity
Reassigned to @timothyandrew
@timothyandrew It looks like you did a lot of the branch permission work. Can you have a look, please? I think this is a pretty critical bug - it will affect anyone using FF-merge plus new branch permissions. Do you think we can get it fixed for 8.11?
cc/ @DouweM
@dblessing I'll take a look right away.
mentioned in merge request !671 (merged)
-
@dblessing: Fixed the issue in #900 (closed). I'm not sure if there's an easy way to test this behavior, but that shouldn't block this fix. I'll open a new issue regarding testing cases like this.
(edit: created gitlab-org/gitlab-ce#21081)
Edited by username-removed-407765 Status changed to closed by merge request !671 (merged)
mentioned in commit 4894540a
mentioned in commit 4d3f34bd
Mentioned in commit pfjason/gitlab-ce@0e9d202b
Mentioned in commit 4894540a
mentioned in commit test-nick/gitlab-ce@db0bda3b
