Merge branch 'fix/drop-project-authorized-for-user' into 'master'

Use authorized projects in ProjectTeam

Closes #23938 and #23636

See merge request !7586

14 files changed, 121 insertions, 185 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 73b0f1c6572..4248e1162d8 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -65,7 +65,9 @@ class Group < Namespace
 
     def select_for_project_authorization
       if current_scope.joins_values.include?(:shared_projects)
-        select("members.user_id, projects.id AS project_id, project_group_links.group_access")
+        joins('INNER JOIN namespaces project_namespace ON project_namespace.id = projects.namespace_id')
+          .where('project_namespace.share_with_group_lock = ?',  false)
+          .select("members.user_id, projects.id AS project_id, LEAST(project_group_links.group_access, members.access_level) AS access_level")
       else
         super
       end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 6e8f5d3c422..dd0cb75f9a8 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -93,7 +93,7 @@ class Issue < ActiveRecord::Base
 
     # Check if we are scoped to a specific project's issues
     if owner_project
-      if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER)
+      if owner_project.team.member?(user, Gitlab::Access::REPORTER)
         # If the project is authorized for the user, they can see all issues in the project
         return all
       else
diff --git a/app/models/member.rb b/app/models/member.rb
index 7be2665bf48..df93aaee847 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -246,7 +246,7 @@ class Member < ActiveRecord::Base
   end
 
   def post_update_hook
-    # override in subclass
+    UserProjectAccessChangedService.new(user.id).execute if access_level_changed?
   end
 
   def post_destroy_hook
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index b67049f0f55..99da26a89fb 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -27,6 +27,7 @@ class Namespace < ActiveRecord::Base
   delegate :name, to: :owner, allow_nil: true, prefix: true
 
   after_update :move_dir, if: :path_changed?
+  after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') }
 
   # Save the storage paths before the projects are destroyed to use them on after destroy
   before_destroy(prepend: true) { @old_repository_storage_paths = repository_storage_paths }
@@ -175,4 +176,11 @@ class Namespace < ActiveRecord::Base
       end
     end
   end
+
+  def refresh_access_of_projects_invited_groups
+    Group.
+      joins(project_group_links: :project).
+      where(projects: { namespace_id: id }).
+      find_each(&:refresh_members_authorized_projects)
+  end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 76c1fc4945d..bd9fcb2f3b7 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -126,6 +126,8 @@ class Project < ActiveRecord::Base
   has_many :hooks,              dependent: :destroy, class_name: 'ProjectHook'
   has_many :protected_branches, dependent: :destroy
 
+  has_many :project_authorizations, dependent: :destroy
+  has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
   has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source
   alias_method :members, :project_members
   has_many :users, through: :project_members
@@ -1293,14 +1295,6 @@ class Project < ActiveRecord::Base
     end
   end
 
-  # Checks if `user` is authorized for this project, with at least the
-  # `min_access_level` (if given).
-  def authorized_for_user?(user, min_access_level = nil)
-    return false unless user
-
-    user.authorized_project?(self, min_access_level)
-  end
-
   def append_or_update_attribute(name, value)
     old_values = public_send(name.to_s)
 
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 65549d8cd08..8a53e974b6f 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -80,19 +80,19 @@ class ProjectTeam
   alias_method :users, :members
 
   def guests
-    @guests ||= fetch_members(:guests)
+    @guests ||= fetch_members(Gitlab::Access::GUEST)
   end
 
   def reporters
-    @reporters ||= fetch_members(:reporters)
+    @reporters ||= fetch_members(Gitlab::Access::REPORTER)
   end
 
   def developers
-    @developers ||= fetch_members(:developers)
+    @developers ||= fetch_members(Gitlab::Access::DEVELOPER)
   end
 
   def masters
-    @masters ||= fetch_members(:masters)
+    @masters ||= fetch_members(Gitlab::Access::MASTER)
   end
 
   def import(source_project, current_user = nil)
@@ -141,8 +141,12 @@ class ProjectTeam
     max_member_access(user.id) == Gitlab::Access::MASTER
   end
 
-  def member?(user, min_member_access = Gitlab::Access::GUEST)
-    max_member_access(user.id) >= min_member_access
+  # Checks if `user` is authorized for this project, with at least the
+  # `min_access_level` (if given).
+  def member?(user, min_access_level = Gitlab::Access::GUEST)
+    return false unless user
+
+    user.authorized_project?(project, min_access_level)
   end
 
   def human_max_access(user_id)
@@ -165,112 +169,29 @@ class ProjectTeam
 
     # Lookup only the IDs we need
     user_ids = user_ids - access.keys
+    users_access = project.project_authorizations.
+      where(user: user_ids).
+      group(:user_id).
+      maximum(:access_level)
 
-    if user_ids.present?
-      user_ids.each { |id| access[id] = Gitlab::Access::NO_ACCESS }
-
-      member_access = project.members.access_for_user_ids(user_ids)
-      merge_max!(access, member_access)
-
-      if group
-        group_access = group.members.access_for_user_ids(user_ids)
-        merge_max!(access, group_access)
-      end
-
-      # Each group produces a list of maximum access level per user. We take the
-      # max of the values produced by each group.
-      if project_shared_with_group?
-        project.project_group_links.each do |group_link|
-          invited_access = max_invited_level_for_users(group_link, user_ids)
-          merge_max!(access, invited_access)
-        end
-      end
-    end
-
+    access.merge!(users_access)
     access
   end
 
   def max_member_access(user_id)
-    max_member_access_for_user_ids([user_id])[user_id]
+    max_member_access_for_user_ids([user_id])[user_id] || Gitlab::Access::NO_ACCESS
   end
 
   private
 
-  # For a given group, return the maximum access level for the user. This is the min of
-  # the invited access level of the group and the access level of the user within the group.
-  # For example, if the group has been given DEVELOPER access but the member has MASTER access,
-  # the user should receive only DEVELOPER access.
-  def max_invited_level_for_users(group_link, user_ids)
-    invited_group = group_link.group
-    capped_access_level = group_link.group_access
-    access = invited_group.group_members.access_for_user_ids(user_ids)
-
-    # If the user is not in the list, assume he/she does not have access
-    missing_users = user_ids - access.keys
-    missing_users.each { |id| access[id] = Gitlab::Access::NO_ACCESS }
-
-    # Cap the maximum access by the invited level access
-    access.each { |key, value| access[key] = [value, capped_access_level].min }
-  end
-
   def fetch_members(level = nil)
-    project_members = project.members
-    group_members = group ? group.members : []
-
-    if level
-      project_members = project_members.public_send(level)
-      group_members = group_members.public_send(level) if group
-    end
-
-    user_ids = project_members.pluck(:user_id)
-
-    invited_members = fetch_invited_members(level)
-    user_ids.push(*invited_members.map(&:user_id)) if invited_members.any?
-
-    user_ids.push(*group_members.pluck(:user_id)) if group
+    members = project.authorized_users
+    members = members.where(project_authorizations: { access_level: level }) if level
 
-    User.where(id: user_ids)
+    members
   end
 
   def group
     project.group
   end
-
-  def merge_max!(first_hash, second_hash)
-    first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new }
-  end
-
-  def project_shared_with_group?
-    project.invited_groups.any? && project.allowed_to_share_with_group?
-  end
-
-  def fetch_invited_members(level = nil)
-    invited_members = []
-
-    return invited_members unless project_shared_with_group?
-
-    project.project_group_links.includes(group: [:group_members]).each do |link|
-      invited_group_members = link.group.members
-
-      if level
-        numeric_level = GroupMember.access_level_roles[level.to_s.singularize.titleize]
-
-        # If we're asked for a level that's higher than the group's access,
-        # there's nothing left to do
-        next if numeric_level > link.group_access
-
-        # Make sure we include everyone _above_ the requested level as well
-        invited_group_members =
-          if numeric_level == link.group_access
-            invited_group_members.where("access_level >= ?", link.group_access)
-          else
-            invited_group_members.public_send(level)
-          end
-      end
-
-      invited_members << invited_group_members
-    end
-
-    invited_members.flatten.compact
-  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 29fb849940a..223d84ba916 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -926,7 +926,7 @@ class User < ActiveRecord::Base
   # Returns a union query of projects that the user is authorized to access
   def project_authorizations_union
     relations = [
-      personal_projects.select("#{id} AS user_id, projects.id AS project_id, #{Gitlab::Access::OWNER} AS access_level"),
+      personal_projects.select("#{id} AS user_id, projects.id AS project_id, #{Gitlab::Access::MASTER} AS access_level"),
       groups_projects.select_for_project_authorization,
       projects.select_for_project_authorization,
       groups.joins(:shared_projects).select_for_project_authorization
diff --git a/changelogs/unreleased/fix-drop-project-authorized-for-user.yml b/changelogs/unreleased/fix-drop-project-authorized-for-user.yml
new file mode 100644
index 00000000000..0d11969575a
--- /dev/null
+++ b/changelogs/unreleased/fix-drop-project-authorized-for-user.yml
@@ -0,0 +1,4 @@
+---
+title: Use authorized projects in ProjectTeam
+merge_request: 
+author: 
diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb
index 9739a5ac8d5..04c3690e152 100644
--- a/db/fixtures/development/06_teams.rb
+++ b/db/fixtures/development/06_teams.rb
@@ -1,20 +1,25 @@
-Gitlab::Seeder.quiet do
-  Group.all.each do |group|
-    User.all.sample(4).each do |user|
-      if group.add_user(user, Gitlab::Access.values.sample).persisted?
-        print '.'
-      else
-        print 'F'
+require 'sidekiq/testing'
+require './db/fixtures/support/serialized_transaction'
+
+Sidekiq::Testing.inline! do
+  Gitlab::Seeder.quiet do
+    Group.all.each do |group|
+      User.all.sample(4).each do |user|
+        if group.add_user(user, Gitlab::Access.values.sample).persisted?
+          print '.'
+        else
+          print 'F'
+        end
       end
     end
-  end
 
-  Project.all.each do |project|
-    User.all.sample(4).each do |user|
-      if project.team << [user, Gitlab::Access.values.sample]
-        print '.'
-      else
-        print 'F'
+    Project.all.each do |project|
+      User.all.sample(4).each do |user|
+        if project.team << [user, Gitlab::Access.values.sample]
+          print '.'
+        else
+          print 'F'
+        end
       end
     end
   end
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index ffca1c94da1..33934cdf8b1 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -10,7 +10,7 @@ describe MembersHelper do
   end
 
   describe '#remove_member_message' do
-    let(:requester) { build(:user) }
+    let(:requester) { create(:user) }
     let(:project) { create(:empty_project, :public, :access_requestable) }
     let(:project_member) { build(:project_member, project: project) }
     let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } }
@@ -31,7 +31,7 @@ describe MembersHelper do
   end
 
   describe '#remove_member_title' do
-    let(:requester) { build(:user) }
+    let(:requester) { create(:user) }
     let(:project) { create(:empty_project, :public, :access_requestable) }
     let(:project_member) { build(:project_member, project: project) }
     let(:project_member_request) { project.request_access(requester) }
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index fe3c39e38db..7e00e214c6e 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -186,6 +186,8 @@ project:
 - environments
 - deployments
 - project_feature
+- authorized_users
+- project_authorizations
 award_emoji:
 - awardable
 - user
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 300425767ed..89e93dce8c5 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -331,7 +331,7 @@ describe Issue, models: true do
     end
 
     context 'with a user' do
-      let(:user) { build(:user) }
+      let(:user) { create(:user) }
       let(:issue) { build(:issue) }
 
       it 'returns true when the issue is readable' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 5ba741f40ec..da38254d1bc 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1507,57 +1507,6 @@ describe Project, models: true do
     end
   end
 
-  describe 'authorized_for_user' do
-    let(:group) { create(:group) }
-    let(:developer) { create(:user) }
-    let(:master) { create(:user) }
-    let(:personal_project) { create(:project, namespace: developer.namespace) }
-    let(:group_project) { create(:project, namespace: group) }
-    let(:members_project) { create(:project) }
-    let(:shared_project) { create(:project) }
-
-    before do
-      group.add_master(master)
-      group.add_developer(developer)
-
-      members_project.team << [developer, :developer]
-      members_project.team << [master, :master]
-
-      create(:project_group_link, project: shared_project, group: group, group_access: Gitlab::Access::DEVELOPER)
-    end
-
-    it 'returns false for no user' do
-      expect(personal_project.authorized_for_user?(nil)).to be(false)
-    end
-
-    it 'returns true for personal projects of the user' do
-      expect(personal_project.authorized_for_user?(developer)).to be(true)
-    end
-
-    it 'returns true for projects of groups the user is a member of' do
-      expect(group_project.authorized_for_user?(developer)).to be(true)
-    end
-
-    it 'returns true for projects for which the user is a member of' do
-      expect(members_project.authorized_for_user?(developer)).to be(true)
-    end
-
-    it 'returns true for projects shared on a group the user is a member of' do
-      expect(shared_project.authorized_for_user?(developer)).to be(true)
-    end
-
-    it 'checks for the correct minimum level access' do
-      expect(group_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
-      expect(group_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
-      expect(members_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
-      expect(members_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
-      expect(shared_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
-      expect(shared_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(false)
-      expect(shared_project.authorized_for_user?(developer, Gitlab::Access::DEVELOPER)).to be(true)
-      expect(shared_project.authorized_for_user?(master, Gitlab::Access::DEVELOPER)).to be(true)
-    end
-  end
-
   describe 'change_head' do
     let(:project) { create(:project) }
 
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index eb6b009c7cf..0475cecaa2d 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -37,7 +37,7 @@ describe ProjectTeam, models: true do
 
   context 'group project' do
     let(:group) { create(:group) }
-    let(:project) { create(:empty_project, group: group) }
+    let!(:project) { create(:empty_project, group: group) }
 
     before do
       group.add_master(master)
@@ -118,7 +118,7 @@ describe ProjectTeam, models: true do
 
     context 'group project' do
       let(:group) { create(:group) }
-      let(:project) { create(:empty_project, group: group) }
+      let!(:project) { create(:empty_project, group: group) }
 
       it 'returns project members' do
         group_member = create(:group_member, group: group)
@@ -178,9 +178,9 @@ describe ProjectTeam, models: true do
     it 'returns Master role' do
       user = create(:user)
       group = create(:group)
-      group.add_master(user)
+      project = create(:empty_project, namespace: group)
 
-      project = build_stubbed(:empty_project, namespace: group)
+      group.add_master(user)
 
       expect(project.team.human_max_access(user.id)).to eq 'Master'
     end
@@ -188,9 +188,9 @@ describe ProjectTeam, models: true do
     it 'returns Owner role' do
       user = create(:user)
       group = create(:group)
-      group.add_owner(user)
+      project = create(:empty_project, namespace: group)
 
-      project = build_stubbed(:empty_project, namespace: group)
+      group.add_owner(user)
 
       expect(project.team.human_max_access(user.id)).to eq 'Owner'
     end
@@ -244,7 +244,7 @@ describe ProjectTeam, models: true do
 
     context 'group project' do
       let(:group) { create(:group, :access_requestable) }
-      let(:project) { create(:empty_project, group: group) }
+      let!(:project) { create(:empty_project, group: group) }
 
       before do
         group.add_master(master)
@@ -261,6 +261,57 @@ describe ProjectTeam, models: true do
     end
   end
 
+  describe '#member?' do
+    let(:group) { create(:group) }
+    let(:developer) { create(:user) }
+    let(:master) { create(:user) }
+    let(:personal_project) { create(:project, namespace: developer.namespace) }
+    let(:group_project) { create(:project, namespace: group) }
+    let(:members_project) { create(:project) }
+    let(:shared_project) { create(:project) }
+
+    before do
+      group.add_master(master)
+      group.add_developer(developer)
+
+      members_project.team << [developer, :developer]
+      members_project.team << [master, :master]
+
+      create(:project_group_link, project: shared_project, group: group)
+    end
+
+    it 'returns false for no user' do
+      expect(personal_project.team.member?(nil)).to be(false)
+    end
+
+    it 'returns true for personal projects of the user' do
+      expect(personal_project.team.member?(developer)).to be(true)
+    end
+
+    it 'returns true for projects of groups the user is a member of' do
+      expect(group_project.team.member?(developer)).to be(true)
+    end
+
+    it 'returns true for projects for which the user is a member of' do
+      expect(members_project.team.member?(developer)).to be(true)
+    end
+
+    it 'returns true for projects shared on a group the user is a member of' do
+      expect(shared_project.team.member?(developer)).to be(true)
+    end
+
+    it 'checks for the correct minimum level access' do
+      expect(group_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(group_project.team.member?(master, Gitlab::Access::MASTER)).to be(true)
+      expect(members_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(members_project.team.member?(master, Gitlab::Access::MASTER)).to be(true)
+      expect(shared_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(shared_project.team.member?(master, Gitlab::Access::MASTER)).to be(false)
+      expect(shared_project.team.member?(developer, Gitlab::Access::DEVELOPER)).to be(true)
+      expect(shared_project.team.member?(master, Gitlab::Access::DEVELOPER)).to be(true)
+    end
+  end
+
   shared_examples_for "#max_member_access_for_users" do |enable_request_store|
     describe "#max_member_access_for_users" do
       before do