Merge EE master from dev.gitlab.org

CHANGELOG-EE.md

 Please view this file on the master branch, on stable branches it's out of date.
 
+## 11.7.2 (2019-01-29)
+
+### Security (6 changes)
+
+- Avoid leaking unauthorized approver group members. !766
+- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !791
+- Check access rights when creating/updating ProtectedRefs.
+- Fix locked file visibility issue for private repositories.
+- Filter out non-project member approvers.
+- Remove HTTP POST in JIRA OAuth access_token endpoint.
+
+
+## 11.7.1 (2019-01-28)
+
+### Security (6 changes)
+
+- Avoid leaking unauthorized approver group members. !766
+- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !791
+- Check access rights when creating/updating ProtectedRefs.
+- Fix locked file visibility issue for private repositories.
+- Filter out non-project member approvers.
+- Remove HTTP POST in JIRA OAuth access_token endpoint.
+
+
 ## 11.7.0 (2019-01-22)
 
 ### Security (1 change)
@@ -86,6 +110,10 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Gather deepest epic relationship data.
 
 
+## 11.6.8 (2019-01-30)
+
+- No changes.
+
 ## 11.6.5 (2019-01-17)
 
 ### Fixed (1 change)
@@ -222,6 +250,18 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Use new information-o icon for Security Dashboard.
 
 
+## 11.5.8 (2019-01-28)
+
+### Security (6 changes)
+
+- Avoid leaking unauthorized approver group members. !766
+- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !793
+- Check access rights when creating/updating ProtectedRefs.
+- Fix locked file visibility issue for private repositories.
+- Filter out non-project member approvers.
+- Remove HTTP POST in JIRA OAuth access_token endpoint.
+
+
 ## 11.5.5 (2018-12-20)
 
 - No changes.

CHANGELOG.md

@@ -4,33 +4,6 @@ entry.
 
 ## 11.7.2 (2019-01-29)
 
-### Security (24 changes)
-
-- Make potentially malicious links more visible in the UI and scrub RTLO chars from links. !2770
-- Don't process MR refs for guests in the notes. !2771
-- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !2828
-- Fixed XSS content in KaTex links.
-- Disallows unauthorized users from accessing the pipelines section.
-- Verify that LFS upload requests are genuine.
-- Extract GitLab Pages using RubyZip.
-- Prevent awarding emojis to notes whose parent is not visible to user.
-- Prevent unauthorized replies when discussion is locked or confidential.
-- Disable git v2 protocol temporarily.
-- Fix showing ci status for guest users when public pipline are not set.
-- Fix contributed projects info still visible when user enable private profile.
-- Add subresources removal to member destroy service.
-- Add more LFS validations to prevent forgery.
-- Use common error for unauthenticated users when creating issues.
-- Fix slow regex in project reference pattern.
-- Fix private user email being visible in push (and tag push) webhooks.
-- Fix wiki access rights when external wiki is enabled.
-- Group guests are no longer able to see merge requests they don't have access to at group level.
-- Fix path disclosure on project import error.
-- Restrict project import visibility based on its group.
-- Expose CI/CD trigger token only to the trigger owner.
-- Notify only users who can access the project on project move.
-- Alias GitHub and BitBucket OAuth2 callback URLs.
-
 ### Fixed (1 change)
 
 - Fix uninitialized constant with GitLab Pages.

app/services/notes/build_service.rb

@@ -2,8 +2,6 @@
 
 module Notes
   class BuildService < ::BaseService
-    prepend ::EE::Notes::BuildService # rubocop: disable Cop/InjectEnterpriseEditionModule
-
     def execute
       should_resolve = false
       in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)

app/services/protected_branches/api_service.rb

@@ -18,5 +18,3 @@ module ProtectedBranches
     end
   end
 end
-
-ProtectedBranches::ApiService.prepend(EE::ProtectedBranches::ApiService)

ee/app/controllers/ee/projects/issues_controller.rb

@@ -7,7 +7,12 @@ module EE
       extend ::Gitlab::Utils::Override
 
       prepended do
-        before_action :authenticate_user!, only: [:export_csv]
+        # Specifying before_action :authenticate_user! multiple times
+        # doesn't work, since the last filter will override the previous
+        # ones.
+        alias_method :export_csv_authenticate_user!, :authenticate_user!
+
+        before_action :export_csv_authenticate_user!, only: [:export_csv]
         before_action :check_export_issues_available!, only: [:export_csv]
         before_action :check_service_desk_available!, only: [:service_desk]
         before_action :whitelist_query_limiting_ee, only: [:update]

ee/app/controllers/oauth/jira/authorizations_controller.rb

@@ -29,13 +29,18 @@ class Oauth::Jira::AuthorizationsController < ApplicationController
 
   # 3. Rewire and adjust access_token request accordingly.
   def access_token
-    auth_params = params
-                    .slice(:code, :client_id, :client_secret)
-                    .merge(grant_type: 'authorization_code', redirect_uri: oauth_jira_callback_url)
+    # We have to modify request.parameters because Doorkeeper::Server reads params from there
+    request.parameters[:redirect_uri] = oauth_jira_callback_url
+
+    begin
+      strategy = Doorkeeper::Server.new(self).token_request('authorization_code')
+      auth_response = strategy.authorize.body
+    rescue Doorkeeper::Errors::DoorkeeperError
+      auth_response = {}
+    end
 
-    auth_response = Gitlab::HTTP.post(oauth_token_url, body: auth_params, allow_local_requests: true)
     token_type, scope, token = auth_response['token_type'], auth_response['scope'], auth_response['access_token']
 
-    render plain: "access_token=#{token}&scope=#{scope}&token_type=#{token_type}"
+    render body: "access_token=#{token}&scope=#{scope}&token_type=#{token_type}"
   end
 end

ee/app/controllers/projects/path_locks_controller.rb

@@ -6,6 +6,7 @@ class Projects::PathLocksController < Projects::ApplicationController
 
   # Authorize
   before_action :require_non_empty_project
+  before_action :authorize_download_code!
   before_action :authorize_push_code!, only: [:toggle]
 
   before_action :check_license

ee/app/models/concerns/ee/protected_ref_access.rb

@@ -27,6 +27,9 @@ module EE
         validates :group, :user,
                   absence: true,
                   unless: :protected_refs_for_users_required_and_available
+
+        validate :validate_group_membership, if: :protected_refs_for_users_required_and_available
+        validate :validate_user_membership, if: :protected_refs_for_users_required_and_available
       end
     end
 
@@ -81,5 +84,21 @@ module EE
     def protected_refs_for_users_required_and_available
       type != :role && project.feature_available?(:protected_refs_for_users)
     end
+
+    def validate_group_membership
+      return unless group
+
+      unless project.project_group_links.where(group: group).exists?
+        self.errors.add(:group, 'does not have access to the project')
+      end
+    end
+
+    def validate_user_membership
+      return unless user
+
+      unless project.members.where(user: user).exists?
+        self.errors.add(:user, 'is not a member of the project')
+      end
+    end
   end
 end

ee/app/models/concerns/visible_approvable.rb

@@ -31,17 +31,19 @@ module VisibleApprovable
 
     if approvers_overwritten?
       code_owners ||= [] # already persisted into database, no need to recompute
-      approvers_relation = approvers
+      approvers_relation = approver_users
     else
       code_owners ||= self.code_owners.dup
-      approvers_relation = target_project.approvers
+      approvers_relation = target_project.approver_users
     end
 
+    approvers_relation = project.members_among(approvers_relation)
+
     if authors.any? && !authors_can_approve?
-      approvers_relation = approvers_relation.where.not(user_id: authors.select(:id))
+      approvers_relation = approvers_relation.where.not(id: authors.select(:id))
     end
 
-    results = code_owners.concat(approvers_relation.includes(:user).map(&:user))
+    results = code_owners.concat(approvers_relation)
     results.uniq!
     results
   end

ee/app/models/ee/merge_request.rb

@@ -16,6 +16,7 @@ module EE
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
       has_many :draft_notes

ee/app/models/ee/project.rb

@@ -44,6 +44,7 @@ module EE
 
       has_many :reviews, inverse_of: :project
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalProjectRule'
       has_many :audit_events, as: :entity

ee/app/presenters/merge_request_approver_presenter.rb

@@ -40,7 +40,9 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
   private
 
   def users
-    @users ||= users_from_git_log_authors
+    strong_memoize(:users) do
+      merge_request.project.members_among(users_from_git_log_authors)
+    end
   end
 
   def code_owner_enabled?

ee/app/services/ee/notes/build_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Notes
-    module BuildService
-      extend ActiveSupport::Concern
-      extend ::Gitlab::Utils::Override
-
-      override :noteable_without_project?
-      def noteable_without_project?(noteable)
-        return true if noteable.is_a?(Epic) && can?(current_user, :create_note, noteable)
-
-        super
-      end
-    end
-  end
-end

ee/app/services/ee/protected_branches/access_level_params.rb

@@ -10,14 +10,6 @@ module EE
         ce_style_access_level + ee_style_access_levels
       end
 
-      def group_ids
-        ids_for('group_id')
-      end
-
-      def user_ids
-        ids_for('user_id')
-      end
-
       private
 
       override :use_default_access_level?
@@ -28,10 +20,6 @@ module EE
       def ee_style_access_levels
         params[:"allowed_to_#{type}"] || []
       end
-
-      def ids_for(key)
-        ee_style_access_levels.select { |level| level[key] }.flat_map(&:values)
-      end
     end
   end
 end

ee/app/services/ee/protected_branches/api_service.rb

-# frozen_string_literal: true
-
-module EE
-  module ProtectedBranches
-    module ApiService
-      extend ::Gitlab::Utils::Override
-
-      GroupsNotAccessibleError = Class.new(StandardError)
-      UsersNotAccessibleError = Class.new(StandardError)
-
-      override :create
-      def create
-        super
-      rescue ::EE::ProtectedBranches::ApiService::GroupsNotAccessibleError,
-             ::EE::ProtectedBranches::ApiService::UsersNotAccessibleError
-        ::ProtectedBranch.new.tap do |protected_branch|
-          message = 'Cannot add users or groups unless they have access to the project'
-          protected_branch.errors.add(:base, message)
-        end
-      end
-
-      private
-
-      override :verify_params!
-      def verify_params!
-        raise GroupsNotAccessibleError.new unless groups_accessible?
-        raise UsersNotAccessibleError.new unless users_accessible?
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def groups_accessible?
-        group_ids = @merge_params.group_ids + @push_params.group_ids + @unprotect_params.group_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        allowed_groups = @project.invited_groups.where(id: group_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-        group_ids.uniq.count == allowed_groups.count
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def users_accessible?
-        user_ids = @merge_params.user_ids + @push_params.user_ids + @unprotect_params.user_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        allowed_users = @project.team.users.where(id: user_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-        user_ids.uniq.count == allowed_users.count
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-    end
-  end
-end

ee/app/views/notify/add_merge_request_approver_email.text.erb

@@ -3,6 +3,6 @@
 <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %>
 
 <%= merge_path_description(@merge_request, 'to') %>
-Author:    <%= @merge_request.author_name %>
-Assignee:  <%= @merge_request.assignee_name %>
+Author:    <%= sanitize_name(@merge_request.author_name) %>
+Assignee:  <%= sanitize_name(@merge_request.assignee_name) %>
 <%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %>

ee/app/views/notify/approved_merge_request_email.text.haml

-Merge Request #{@merge_request.to_reference} was approved by #{@approved_by.name}
+Merge Request #{@merge_request.to_reference} was approved by #{sanitize_name(@approved_by.name)}
 
 Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

ee/app/views/notify/epic_status_changed_email.html.haml

 %p
-  Epic was #{@status} by #{@updated_by.name}
+  Epic was #{@status} by #{sanitize_name(@updated_by.name)}
 
 %p
   Epic

ee/app/views/notify/epic_status_changed_email.text.haml

-Epic was #{@status} by #{@updated_by.name}
+Epic was #{@status} by #{sanitize_name(@updated_by.name)}
 
 Epic #{@epic.to_reference}: #{group_epic_url(@epic.group, @epic)}

ee/app/views/notify/new_epic_email.html.haml

@@ -4,7 +4,7 @@
 
 - if @epic.assignee
   %p
-    Assignee: #{@epic.assignee.name}
+    Assignee: #{sanitize_name(@epic.assignee.name)}
 
 - if @epic.description
   %div

ee/app/views/notify/new_epic_email.text.erb

@@ -2,9 +2,9 @@ New Epic <%= @epic.to_reference(full: true) %> was created.
 
 <%= url_for(group_epic_url(@epic.group, @epic)) %>
 
-Author: <%= @epic.author_name %>
+Author: <%= sanitize_name(@epic.author_name) %>
 <% if @epic.assignee %>
-Assignee: <%= @epic.assignee.name %>
+Assignee: <%= sanitize_name(@epic.assignee.name) %>
 <% end %>
 
 <%= @epic.description %>

ee/app/views/notify/new_review_email.text.erb

-<%= "Merge request #{merge_request_url(@merge_request)} was reviewed by #{@author_name}" %>
+<%= "Merge request #{merge_request_url(@merge_request)} was reviewed by #{sanitize_name(@author_name)}" %>
 
 --
 <% @notes.each_with_index do |note, index| %>

ee/app/views/notify/project_mirror_user_changed_email.html.haml

 %p
-  The mirror user for #{@project.full_path} has been changed from #{@deleted_user_name} to yourself because their account was deleted.
+  The mirror user for #{@project.full_path} has been changed from #{sanitize_name(@deleted_user_name)} to yourself because their account was deleted.
 
 %p
   You can change this setting from the #{link_to("repository settings page", project_settings_repository_url(@project))}.

ee/app/views/notify/project_mirror_user_changed_email.text.erb

-The mirror user for <%= @project.full_path %> has been changed from <%= @deleted_user_name %> to yourself because their account was deleted.
+The mirror user for <%= @project.full_path %> has been changed from <%= sanitize_name(@deleted_user_name) %> to yourself because their account was deleted.
 
 You can change this setting from the repository settings page in <%= project_settings_repository_url(@project) %>.

ee/app/views/notify/service_desk_new_note_email.text.erb

 New response for issue #<%= @issue.iid %>:
 
-Author: <%= @note.author_name %>
+Author: <%= sanitize_name(@note.author_name) %>
 
 <%= @note.note %>
-<%# EE-specific start %><%= render 'layouts/mailer/additional_text' %><%# EE-specific end %>
\ No newline at end of file
+<%# EE-specific start %><%= render 'layouts/mailer/additional_text' %><%# EE-specific end %>

ee/app/views/notify/unapproved_merge_request_email.text.haml

@@ -4,5 +4,5 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

ee/changelogs/unreleased/security-22076-sanitize-url-in-names-ee.yml

+---
+title: Sanitize user full name to clean up any URL to prevent mail clients from auto-linking
+  URLs
+merge_request: 790
+author:
+type: security

ee/lib/ee/api/entities.rb

@@ -247,6 +247,12 @@ module EE
       end
 
       class MergeRequestApprovals < ::API::Entities::ProjectEntity
+        def initialize(merge_request, options = {})
+          presenter = merge_request.present(current_user: options[:current_user])
+
+          super(presenter, options)
+        end
+
         expose :merge_status
         expose :approvals_required
         expose :approvals_left

ee/spec/controllers/oauth/jira/authorizations_controller_spec.rb

@@ -30,15 +30,9 @@ describe Oauth::Jira::AuthorizationsController do
   end
 
   describe 'POST access_token' do
-    it 'send post call to oauth_token_url with correct params' do
-      expected_auth_params = { 'code' => 'code-123',
-                               'client_id' => 'client-123',
-                               'client_secret' => 'secret-123',
-                               'grant_type' => 'authorization_code',
-                               'redirect_uri' => 'http://test.host/login/oauth/callback' }
-
-      expect(Gitlab::HTTP).to receive(:post).with(oauth_token_url, allow_local_requests: true, body: ActionController::Parameters.new(expected_auth_params)) do
-        { 'access_token' => 'fake-123', 'scope' => 'foo', 'token_type' => 'bar' }
+    it 'returns oauth params in a format JIRA expects' do
+      expect_any_instance_of(Doorkeeper::Request::AuthorizationCode).to receive(:authorize) do
+        double(body: { 'access_token' => 'fake-123', 'scope' => 'foo', 'token_type' => 'bar' })
       end
 
       post :access_token, params: { code: 'code-123', client_id: 'client-123', client_secret: 'secret-123' }

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -41,7 +41,7 @@ shared_examples 'approvals' do
   describe 'approvals' do
     let!(:approval) { create(:approval, merge_request: merge_request, user: approver.user) }
 
-    before do
+    def get_approvals
       get :approvals,
           params: {
             namespace_id: project.namespace.to_param,
@@ -52,6 +52,8 @@ shared_examples 'approvals' do
     end
 
     it 'shows approval information' do
+      get_approvals
+
       approvals = json_response
 
       expect(response).to be_success
@@ -63,6 +65,23 @@ shared_examples 'approvals' do
       expect(approvals['suggested_approvers'].size).to eq 1
       expect(approvals['suggested_approvers'][0]['username']).to eq user.username
     end
+
+    context 'with unauthorized group' do
+      let(:private_group) { create(:group_with_members, :private) }
+
+      before do
+        create(:approver_group, target: merge_request, group: private_group)
+      end
+
+      it 'does not expose approvers from a private group the current user has no access to' do
+        get_approvals
+
+        approvals = json_response
+
+        expect(response).to be_success
+        expect(approvals['suggested_approvers'].size).to eq(0)
+      end
+    end
   end
 
   describe 'unapprove' do

ee/spec/requests/projects/path_locks_controller_spec.rb → ee/spec/controllers/projects/path_locks_controller_spec.rb

 require 'rails_helper'
 
-describe Projects::PathLocksController, type: :request do
-  let(:project) { create(:project, :repository) }
+describe Projects::PathLocksController do
+  let(:project) { create(:project, :repository, :public) }
   let(:user)    { project.owner }
-  let(:viewer)  { user }
   let(:file_path) { 'files/lfs/lfs_object.iso' }
-  let(:blob_object) { project.repository.blob_at_branch('lfs', file_path) }
-  let!(:lfs_object) { create(:lfs_object, oid: blob_object.lfs_oid) }
-  let!(:lfs_objects_project) { create(:lfs_objects_project, project: project, lfs_object: lfs_object) }
 
   before do
-    login_as(viewer)
+    sign_in(user)
 
     allow_any_instance_of(Repository).to receive(:root_ref).and_return('lfs')
   end
 
+  describe 'GET #index' do
+    it 'displays the lock paths' do
+      get :index, params: { namespace_id: project.namespace, project_id: project }
+
+      expect(response).to have_gitlab_http_status(200)
+    end
+
+    context 'when the user does not have access' do
+      let(:project) { create(:project, :repository, :public, :repository_private) }
+      let(:user) { create(:user) }
+
+      it 'does not allow access' do
+        get :index, params: { namespace_id: project.namespace, project_id: project }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
   describe 'POST #toggle' do
     context 'when LFS is enabled' do
       before do
@@ -110,9 +125,20 @@ describe Projects::PathLocksController, type: :request do
         expect(response).to have_gitlab_http_status(200)
       end
     end
+
+    context 'when the user does not have access' do
+      let(:project) { create(:project, :repository, :public, :repository_private) }
+      let(:user) { create(:user) }
+
+      it 'does not allow access' do
+        toggle_lock(file_path)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
   end
 
   def toggle_lock(path)
-    post toggle_project_path_locks_path(project), params: { path: path }
+    post :toggle, params: { namespace_id: project.namespace, project_id: project, path: path }
   end
 end

ee/spec/factories/approvers.rb

@@ -4,5 +4,14 @@ FactoryBot.define do
   factory :approver do
     target factory: :merge_request
     user
+
+    after(:create) do |approver, evaluator|
+      case approver.target
+      when Project
+        approver.target.add_developer(approver.user)
+      else
+        approver.target.project.add_developer(approver.user)
+      end
+    end
   end
 end

ee/spec/features/projects/members/member_is_removed_from_project_spec.rb

@@ -3,9 +3,11 @@ require 'spec_helper'
 describe 'Projects > Members > Member is removed from project' do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
+  let(:other_user) { create(:user) }
 
   before do
     project.add_maintainer(user)
+    project.add_maintainer(other_user)
     sign_in(user)
     visit project_project_members_path(project)
   end
@@ -17,7 +19,6 @@ describe 'Projects > Members > Member is removed from project' do
   end
 
   context 'when the user has been specifically allowed to access a protected branch' do
-    let(:other_user) { create(:user) }
     let!(:matching_protected_branch) { create(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, project: project) }
     let!(:non_matching_protected_branch) { create(:protected_branch, authorize_user_to_push: other_user, authorize_user_to_merge: other_user, project: project) }
 

ee/spec/features/projects/members/member_leaves_project_spec.rb

@@ -6,6 +6,7 @@ describe 'Projects > Members > Member leaves project' do
 
   before do
     project.add_developer(user)
+    project.add_developer(other_user)
     sign_in(user)
     visit project_path(project)
   end

ee/spec/javascripts/fixtures/merge_requests.rb

@@ -9,7 +9,11 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
   let(:namespace) { create(:namespace, name: 'frontend-fixtures' )}
   let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: admin, approvals_before_merge: 2) }
-  let(:suggested_approvers) { create_list(:user, 3) }
+  let(:suggested_approvers) do
+    create_list(:user, 3).tap do |users|
+      users.each { |user| project.add_developer(user) }
+    end
+  end
 
   render_views
 

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -9,6 +9,7 @@ merge_requests:
 - approval_rules
 - approvals
 - approvers
+- approver_users
 - approver_groups
 - approved_by_users
 - draft_notes
@@ -54,6 +55,7 @@ project:
 - feature_usage
 - approval_rules
 - approvers
+- approver_users
 - pages_domains
 - audit_events
 - path_locks

ee/spec/models/ee/protected_branch_spec.rb

@@ -30,6 +30,7 @@ describe ProtectedBranch do
 
       it "does not count a user-based #{human_association_name} with an `access_level` set" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.add_developer(user)
 
         protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
@@ -40,6 +41,7 @@ describe ProtectedBranch do
       it "does not count a group-based #{human_association_name} with an `access_level` set" do
         group = create(:group)
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.project_group_links.create(group: group)
 
         protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
@@ -53,6 +55,9 @@ describe ProtectedBranch do
         first_protected_branch = create(:protected_branch, default_access_level: false)
         second_protected_branch = create(:protected_branch, default_access_level: false)
 
+        first_protected_branch.project.add_developer(user)
+        second_protected_branch.project.add_developer(user)
+
         first_protected_branch.send(association_name) << build(factory_name, user: user)
         second_protected_branch.send(association_name) << build(factory_name, user: user)
 
@@ -66,6 +71,7 @@ describe ProtectedBranch do
 
       it "ignores the `access_level` while validating a user-based #{human_association_name}" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.add_developer(user)
 
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
@@ -81,6 +87,9 @@ describe ProtectedBranch do
         first_protected_branch = create(:protected_branch, default_access_level: false)
         second_protected_branch = create(:protected_branch, default_access_level: false)
 
+        first_protected_branch.project.project_group_links.create(group: group)
+        second_protected_branch.project.project_group_links.create(group: group)
+
         first_protected_branch.send(association_name) << build(factory_name, group: group)
         second_protected_branch.send(association_name) << build(factory_name, group: group)
 
@@ -94,6 +103,7 @@ describe ProtectedBranch do
 
       it "ignores the `access_level` while validating a group-based #{human_association_name}" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.project_group_links.create(group: group)
 
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
@@ -146,6 +156,7 @@ describe ProtectedBranch do
 
     context 'multiple access levels' do
       before do
+        project.add_developer(user)
         subject.unprotect_access_levels.create!(user: maintainer)
         subject.unprotect_access_levels.create!(user: user)
       end

ee/spec/models/ee/protected_ref_access_spec.rb

@@ -10,6 +10,14 @@ describe EE::ProtectedRefAccess do
     context "in #{included_in_class}" do
       let(:factory_name) { included_in_class.name.underscore.tr('/', '_') }
       let(:access_level) { build(factory_name) }
+      let(:project) { access_level.project }
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+
+      before do
+        project.add_developer(user)
+        project.project_group_links.create(group: group)
+      end
 
       it "#{included_in_class} includes {described_class}" do
         expect(included_in_class.included_modules).to include(described_class)
@@ -20,17 +28,19 @@ describe EE::ProtectedRefAccess do
           stub_licensed_features(protected_refs_for_users: false)
         end
 
-        it "allows creating an #{included_in_class} with a group" do
-          access_level.group = create(:group)
+        it "does not allow to create an #{included_in_class} with a group" do
+          access_level.group = group
 
           expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
           expect(access_level.errors).to include(:group)
         end
 
-        it "allows creating an #{included_in_class} with a user" do
-          access_level.user = create(:user)
+        it "does not allow to create an #{included_in_class} with a user" do
+          access_level.user = user
 
           expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
           expect(access_level.errors).to include(:user)
         end
       end
@@ -41,16 +51,32 @@ describe EE::ProtectedRefAccess do
         end
 
         it "allows creating an #{included_in_class} with a group" do
-          access_level.group = create(:group)
+          access_level.group = group
 
           expect(access_level).to be_valid
         end
 
+        it 'does not allow to add non member groups' do
+          access_level.group = create(:group)
+
+          expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
+          expect(access_level.errors[:group].first).to eq 'does not have access to the project'
+        end
+
         it "allows creating an #{included_in_class} with a user" do
-          access_level.user = create(:user)
+          access_level.user = user
 
           expect(access_level).to be_valid
         end
+
+        it 'does not allow to add non member users' do
+          access_level.user = create(:user)
+
+          expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
+          expect(access_level.errors[:user].first).to eq 'is not a member of the project'
+        end
       end
 
       it 'requires access_level if no user or group is specified' do
@@ -60,13 +86,15 @@ describe EE::ProtectedRefAccess do
       end
 
       it "doesn't require access_level if user specified" do
-        subject = build(factory_name, access_level: nil, user: create(:user))
+        subject = build(factory_name, access_level: nil, user: user)
+        subject.project.add_developer(subject.user)
 
         expect(subject).to be_valid
       end
 
       it "doesn't require access_level if group specified" do
         subject = build(factory_name, access_level: nil, group: create(:group))
+        subject.project.project_group_links.create(group: subject.group)
 
         expect(subject).to be_valid
       end

ee/spec/models/ee/protected_ref_spec.rb

@@ -4,8 +4,18 @@ describe EE::ProtectedRef do
   context 'for protected branches' do
     it 'deletes all related access levels' do
       protected_branch = create(:protected_branch)
-      2.times { protected_branch.merge_access_levels.create!(group: create(:group)) }
-      2.times { protected_branch.push_access_levels.create!(user: create(:user)) }
+
+      2.times do
+        group = create(:group)
+        protected_branch.project.project_group_links.create(group: group)
+        protected_branch.merge_access_levels.create!(group: group)
+      end
+
+      2.times do
+        user = create(:user)
+        protected_branch.project.add_developer(user)
+        protected_branch.push_access_levels.create!(user: user)
+      end
 
       protected_branch.destroy
 

ee/spec/models/merge_request_spec.rb

@@ -12,6 +12,7 @@ describe MergeRequest do
     it { is_expected.to have_many(:reviews).inverse_of(:merge_request) }
     it { is_expected.to have_many(:approvals).dependent(:delete_all) }
     it { is_expected.to have_many(:approvers).dependent(:delete_all) }
+    it { is_expected.to have_many(:approver_users).through(:approvers) }
     it { is_expected.to have_many(:approver_groups).dependent(:delete_all) }
     it { is_expected.to have_many(:approved_by_users) }
   end

ee/spec/models/project_spec.rb

@@ -24,6 +24,8 @@ describe Project do
     it { is_expected.to have_many(:source_pipelines) }
     it { is_expected.to have_many(:audit_events).dependent(false) }
     it { is_expected.to have_many(:protected_environments) }
+    it { is_expected.to have_many(:approvers).dependent(:destroy) }
+    it { is_expected.to have_many(:approver_users).through(:approvers) }
     it { is_expected.to have_many(:approver_groups).dependent(:destroy) }
     it { is_expected.to have_many(:packages).class_name('Packages::Package') }
     it { is_expected.to have_many(:package_files).class_name('Packages::PackageFile') }

ee/spec/models/visible_approvable_spec.rb

@@ -43,6 +43,7 @@ describe VisibleApprovable do
 
     before do
       allow(resource).to receive(:code_owners).and_return([code_owner])
+      project.add_developer(project_approver.user)
     end
 
     subject { resource.overall_approvers }
@@ -95,10 +96,22 @@ describe VisibleApprovable do
     context 'when approvers are overwritten' do
       let!(:approver) { create(:approver, target: resource) }
 
+      before do
+        project.add_developer(approver.user)
+      end
+
       it 'returns the list of all the merge request user approvers' do
         is_expected.to contain_exactly(approver.user)
       end
     end
+
+    context 'when approver is no longer part of project' do
+      it 'excludes non-project members' do
+        project.team.find_member(project_approver.user).destroy!
+
+        is_expected.not_to include(project_approver.user)
+      end
+    end
   end
 
   describe '#overall_approver_groups' do
@@ -128,6 +141,10 @@ describe VisibleApprovable do
     let!(:approver_group) { create(:approver_group, target: resource, group: group) }
     let!(:approver) { create(:approver, target: resource) }
 
+    before do
+      project.add_developer(approver.user)
+    end
+
     subject { resource.all_approvers_including_groups }
 
     it 'only queries once' do
@@ -157,7 +174,8 @@ describe VisibleApprovable do
 
   describe '#reset_approval_cache!' do
     before do
-      create(:approver, target: resource)
+      approver = create(:approver, target: resource)
+      project.add_developer(approver.user)
     end
 
     subject { resource.reset_approval_cache! }

ee/spec/policies/protected_branch_policy_spec.rb

@@ -11,6 +11,7 @@ describe ProtectedBranchPolicy do
 
   before do
     project.add_maintainer(user)
+    project.project_group_links.create(group: allowed_group)
   end
 
   context 'when unprotection is limited by access levels' do

ee/spec/presenters/merge_request_approver_presenter_spec.rb

@@ -20,6 +20,9 @@ describe MergeRequestApproverPresenter do
     allow(merge_request).to receive(:modified_paths).and_return(file_paths)
     allow(merge_request).to receive(:approvals_required).and_return(approvals_required)
 
+    project.add_developer(committer_a)
+    project.add_developer(committer_b)
+
     stub_licensed_features(code_owners: enable_code_owner)
   end
 

ee/spec/requests/jira_authorizations_spec.rb

+require 'spec_helper'
+
+describe 'JIRA authorization requests' do
+  let(:user) { create :user }
+  let(:application) { create :oauth_application, scopes: 'api' }
+  let(:redirect_uri) { oauth_jira_callback_url(host: "http://www.example.com") }
+
+  def generate_access_grant
+    create :oauth_access_grant, application: application, resource_owner_id: user.id, redirect_uri: redirect_uri
+  end
+
+  describe 'POST access_token' do
+    it 'should return values similar to a POST to /oauth/token' do
+      post_data = {
+        client_id: application.uid,
+        client_secret: application.secret
+      }
+
+      post '/oauth/token', params: post_data.merge({
+        code: generate_access_grant.token,
+        grant_type: 'authorization_code',
+        redirect_uri: redirect_uri
+      })
+      oauth_response = json_response
+
+      post '/login/oauth/access_token', params: post_data.merge({
+        code: generate_access_grant.token
+      })
+      jira_response = response.body
+
+      access_token, scope, token_type = oauth_response.values_at('access_token', 'scope', 'token_type')
+      expect(jira_response).to eq("access_token=#{access_token}&scope=#{scope}&token_type=#{token_type}")
+    end
+  end
+end

spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -17,6 +17,7 @@ describe "User creates a merge request", :js do
 
   before do
     project.add_maintainer(user)
+    project.add_maintainer(user2)
     project.add_maintainer(approver)
     sign_in(user)
 

spec/lib/gitlab/git_access_spec.rb

@@ -808,6 +808,8 @@ describe Gitlab::GitAccess do
             .project_group_links
             .create(group: group, group_access: Gitlab::Access.sym_options[role])
 
+          protected_branch.save
+
           aggregate_failures do
             matrix.each do |action, allowed|
               check = -> { push_changes(changes[action]) }
@@ -889,25 +891,19 @@ describe Gitlab::GitAccess do
 
     [%w(feature exact), ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
       context do
-        before do
-          create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix)
       end
 
       context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
       context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_merge, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
 
         context "when a merge request exists for the given source/target branch" do
           context "when the merge request is in progress" do
@@ -934,20 +930,16 @@ describe Gitlab::GitAccess do
       end
 
       context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
       context "user-specific access control" do
-        context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
+        let(:user) { create(:user) }
 
-          before do
-            create(:protected_branch, authorize_user_to_push: user, name: protected_branch_name, project: project)
-          end
+        context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_push: user, name: protected_branch_name, project: project) }
 
           run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
                                                               guest: { push_protected_branch: false, merge_into_protected_branch: false },
@@ -955,11 +947,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific user is allowed to merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_merge: user, name: protected_branch_name, project: project) }
 
           before do
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_user_to_merge: user, name: protected_branch_name, project: project)
           end
 
           run_permission_checks(permissions_matrix.deep_merge(admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
@@ -970,11 +961,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific user is allowed to push & merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project) }
 
           before do
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project)
           end
 
           run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
@@ -984,14 +974,15 @@ describe Gitlab::GitAccess do
       end
 
       context "group-specific access control" do
-        context "when a specific group is allowed to push into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
-          let(:group) { create(:group) }
+        let(:user) { create(:user) }
+        let(:group) { create(:group) }
 
-          before do
-            group.add_maintainer(user)
-            create(:protected_branch, authorize_group_to_push: group, name: protected_branch_name, project: project)
-          end
+        before do
+          group.add_maintainer(user)
+        end
+
+        context "when a specific group is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_push: group, name: protected_branch_name, project: project) }
 
           permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
                                                                      guest: { push_protected_branch: false, merge_into_protected_branch: false },
@@ -1001,13 +992,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific group is allowed to merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
-          let(:group) { create(:group) }
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_merge: group, name: protected_branch_name, project: project) }
 
           before do
-            group.add_maintainer(user)
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_group_to_merge: group, name: protected_branch_name, project: project)
           end
 
           permissions = permissions_matrix.except(:admin).deep_merge(maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
@@ -1019,13 +1007,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific group is allowed to push & merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
-          let(:group) { create(:group) }
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_push: group, authorize_group_to_merge: group, name: protected_branch_name, project: project) }
 
           before do
-            group.add_maintainer(user)
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_group_to_push: group, authorize_group_to_merge: group, name: protected_branch_name, project: project)
           end
 
           permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },

spec/requests/api/protected_branches_spec.rb

@@ -74,6 +74,9 @@ describe API::ProtectedBranches do
         let(:unprotect_group) { create(:group) }
 
         before do
+          project.add_developer(push_user)
+          project.project_group_links.create(group: merge_group)
+          project.project_group_links.create(group: unprotect_group)
           protected_branch.push_access_levels.create!(user: push_user)
           protected_branch.merge_access_levels.create!(group: merge_group)
           protected_branch.unprotect_access_levels.create!(group: unprotect_group)
@@ -282,7 +285,7 @@ describe API::ProtectedBranches do
           post post_endpoint, params: { name: branch_name, allowed_to_merge: [{ user_id: push_user.id }] }
 
           expect(response).to have_gitlab_http_status(422)
-          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+          expect(json_response['message'][0]).to match(/is not a member of the project/)
         end
 
         it "fails if groups aren't all invited to the project" do
@@ -291,7 +294,7 @@ describe API::ProtectedBranches do
           post post_endpoint, params: { name: branch_name, allowed_to_merge: [{ group_id: merge_group.id }] }
 
           expect(response).to have_gitlab_http_status(422)
-          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+          expect(json_response['message'][0]).to match(/does not have access to the project/)
         end
 
         it 'avoids creating default access levels unless necessary' do