Code improvements

8 files changed, 40 insertions, 28 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 13de19bc141..6532eee1602 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -9,7 +9,7 @@ class GroupsController < Groups::ApplicationController
   before_action :group, except: [:index, :new, :create]
 
   # Authorize
-  before_action :authorize_read_group!, except: [:index, :show, :new, :create, :autocomplete]
+  before_action :authorize_read_group!, except: [:index, :new, :create]
   before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
   before_action :authorize_create_group!, only: [:new, :create]
 
@@ -105,7 +105,7 @@ class GroupsController < Groups::ApplicationController
 
   # Dont allow unauthorized access to group
   def authorize_read_group!
-    unless @group and (@projects.present? or can?(current_user, :read_group, @group))
+    unless can?(current_user, :read_group, @group)
       if current_user.nil?
         return authenticate_user!
       else
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index e10c633690f..d26a1ce6737 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -3,6 +3,16 @@ class UsersController < ApplicationController
   before_action :set_user
 
   def show
+<<<<<<< HEAD
+=======
+    @contributed_projects = contributed_projects.joined(@user).reject(&:forked?)
+
+    @projects = PersonalProjectsFinder.new(@user).execute(current_user)
+    @projects = @projects.page(params[:page]).per(PER_PAGE)
+
+    @groups = @user.groups.order_id_desc
+
+>>>>>>> Code improvements
     respond_to do |format|
       format.html
 
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index a3a8cd541de..ce62f5e762f 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -1,6 +1,5 @@
 class GroupsFinder
   def execute(current_user = nil)
-
     segments = all_groups(current_user)
 
     if segments.length > 1
@@ -15,17 +14,9 @@ class GroupsFinder
 
   def all_groups(current_user)
     if current_user
-      [current_user.authorized_groups, public_and_internal_groups]
+      [current_user.authorized_groups, Group.unscoped.public_and_internal_only]
     else
-      [Group.public_only]
+      [Group.unscoped.public_only]
     end
   end
-
-  def public_groups
-    Group.unscoped.public_only
-  end
-
-  def public_and_internal_groups
-    Group.unscoped.public_and_internal_only
-  end
 end
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index b1f0a765bb9..d918d8acd22 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -28,7 +28,7 @@ module GroupsHelper
       group = Group.find_by(path: group)
     end
 
-    if group && group.avatar.present?
+    if group && can?(current_user, :read_group, group) && group.avatar.present?
       group.avatar.url
     else
       'no_group_avatar.png'
diff --git a/app/models/ability.rb b/app/models/ability.rb
index c84ded61606..ec5587d8fa5 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -83,7 +83,7 @@ class Ability
                 subject.group
               end
 
-      if group && group.projects.public_only.any?
+      if group && group.public?
         [:read_group]
       else
         []
@@ -271,16 +271,13 @@ class Ability
     def group_abilities(user, group)
       rules = []
 
-      if user.admin? || group.users.include?(user) || ProjectsFinder.new.execute(user, group: group).any?
-        rules << :read_group
-      end
+      rules << :read_group if can_read_group?(user, group)
 
       # Only group masters and group owners can create new projects and change permission level
       if group.has_master?(user) || group.has_owner?(user) || user.admin?
         rules += [
           :create_projects,
-          :admin_milestones,
-          :change_visibility_level
+          :admin_milestones
         ]
       end
 
@@ -289,13 +286,20 @@ class Ability
         rules += [
           :admin_group,
           :admin_namespace,
-          :admin_group_member
+          :admin_group_member,
+          :change_visibility_level
         ]
       end
 
       rules.flatten
     end
 
+    def can_read_group?(user, group)
+      is_project_member = ProjectsFinder.new.execute(user, group: group).any?
+      internal_group_allowed = group.internal? && user.present?
+      user.admin? || group.users.include?(user) || is_project_member || group.public? || internal_group_allowed
+    end
+
     def namespace_abilities(user, namespace)
       rules = []
 
diff --git a/app/models/concerns/shared_scopes.rb b/app/models/concerns/shared_scopes.rb
new file mode 100644
index 00000000000..f576d2c0821
--- /dev/null
+++ b/app/models/concerns/shared_scopes.rb
@@ -0,0 +1,8 @@
+module SharedScopes
+  extend ActiveSupport::Concern
+
+  included do
+    scope :public_only, -> { where(visibility_level: Group::PUBLIC) }
+    scope :public_and_internal_only, -> { where(visibility_level: [Group::PUBLIC, Group::INTERNAL] ) }
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index 26914f55541..d1a1817f0fa 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -21,7 +21,7 @@ class Group < Namespace
   include Gitlab::ConfigHelper
   include Gitlab::VisibilityLevel
   include Referable
-
+  include SharedScopes
 
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
   alias_method :members, :group_members
@@ -35,10 +35,6 @@ class Group < Namespace
   after_create :post_create_hook
   after_destroy :post_destroy_hook
 
-  scope :public_only, -> { where(visibility_level: Group::PUBLIC) }
-  scope :public_and_internal_only, -> { where(visibility_level: [Group::PUBLIC, Group::INTERNAL] ) }
-
-
   class << self
     def search(query)
       where("LOWER(namespaces.name) LIKE :query or LOWER(namespaces.path) LIKE :query", query: "%#{query.downcase}%")
@@ -69,6 +65,10 @@ class Group < Namespace
     name
   end
 
+  def visibility_level_field
+    visibility_level
+  end
+
   def avatar_url(size = nil)
     if avatar.present?
       [gitlab_config.url, avatar.url].join
diff --git a/app/models/project.rb b/app/models/project.rb
index 426464dee81..6e86c7cc883 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -52,6 +52,7 @@ class Project < ActiveRecord::Base
   include AfterCommitQueue
   include CaseSensitivity
   include TokenAuthenticatable
+  include SharedScopes
 
   extend Gitlab::ConfigHelper
 
@@ -213,8 +214,6 @@ class Project < ActiveRecord::Base
   scope :in_group_namespace, -> { joins(:group) }
   scope :personal, ->(user) { where(namespace_id: user.namespace_id) }
   scope :joined, ->(user) { where('namespace_id != ?', user.namespace_id) }
-  scope :public_only, -> { where(visibility_level: Project::PUBLIC) }
-  scope :public_and_internal_only, -> { where(visibility_level: Project.public_and_internal_levels) }
   scope :non_archived, -> { where(archived: false) }
   scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }