From e116a356b8ac07bd3a935c40ceb274d67d808c83 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Wed, 18 Nov 2015 12:30:24 +0100
Subject: [PATCH] Refactor User#authorized_groups/projects

These methods no longer include public groups/projects (that don't
belong to the actual user) as this is handled by the various finder
classes now. This also removes the need for passing extra arguments.

Note that memoizing was removed _explicitly_. For whatever reason doing
so messes up the users controller to a point where it claims a certain
user does _not_ have access to certain groups/projects when it does have
access. Existing code shouldn't be affected as these methods are only
called in ways that they'd run queries anyway (e.g. a combination of
"any?" and "each" which would run 2 queries regardless of memoizing).
---
 app/models/user.rb       | 35 ++++++-----------------------------
 spec/models/user_spec.rb | 26 ++++----------------------
 2 files changed, 10 insertions(+), 51 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index d523b3f0491..20a2457eec9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -389,40 +389,17 @@ class User < ActiveRecord::Base
     end
   end
 
-  # Returns the groups a user has access to, optionally including any public
-  # groups.
-  #
-  # public_internal - When set to "true" all public groups and groups of public
-  #                   projects are also included.
-  #
-  # Returns an ActiveRecord::Relation
-  def authorized_groups(public_internal = false)
+  # Returns the groups a user has access to
+  def authorized_groups
     union = Gitlab::SQL::Union.
-      new([groups.select(:id), authorized_projects(public_internal).
-           select(:namespace_id)])
-
-    sql = "namespaces.id IN (#{union.to_sql})"
-
-    if public_internal
-      sql << ' OR public IS TRUE'
-    end
+      new([groups.select(:id), authorized_projects.select(:namespace_id)])
 
-    Group.where(sql)
+    Group.where("namespaces.id IN (#{union.to_sql})")
   end
 
   # Returns the groups a user is authorized to access.
-  #
-  # public_internal - When set to "true" all public/internal projects will also
-  #                   be included.
-  def authorized_projects(public_internal = false)
-    base = "projects.id IN (#{projects_union.to_sql})"
-
-    if public_internal
-      Project.where("#{base} OR projects.visibility_level IN (?)",
-                    Project.public_and_internal_levels)
-    else
-      Project.where(base)
-    end
+  def authorized_projects
+    Project.where("projects.id IN (#{projects_union.to_sql})")
   end
 
   def owned_projects
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 71160f8dfef..4631b12faf1 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -762,44 +762,26 @@ describe User do
   describe '#authorized_groups' do
     let!(:user) { create(:user) }
     let!(:private_group) { create(:group) }
-    let!(:public_group) { create(:group, public: true) }
 
     before do
       private_group.add_user(user, Gitlab::Access::MASTER)
     end
 
-    describe 'excluding public groups' do
-      subject { user.authorized_groups }
+    subject { user.authorized_groups }
 
-      it { is_expected.to eq([private_group]) }
-    end
-
-    describe 'including public groups' do
-      subject { user.authorized_groups(true) }
-
-      it { is_expected.to eq([public_group, private_group]) }
-    end
+    it { is_expected.to eq([private_group]) }
   end
 
   describe '#authorized_projects' do
     let!(:user) { create(:user) }
     let!(:private_project) { create(:project, :private) }
-    let!(:public_project) { create(:project, :public) }
 
     before do
       private_project.team << [user, Gitlab::Access::MASTER]
     end
 
-    describe 'excluding public projects' do
-      subject { user.authorized_projects }
+    subject { user.authorized_projects }
 
-      it { is_expected.to eq([private_project]) }
-    end
-
-    describe 'including public projects' do
-      subject { user.authorized_projects(true) }
-
-      it { is_expected.to eq([public_project, private_project]) }
-    end
+    it { is_expected.to eq([private_project]) }
   end
 end
-- 
GitLab