From b959ae553b1243e081d557b1e545d30830931e5b Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <zegerjan@gitlab.com>
Date: Fri, 18 Mar 2016 13:28:16 +0100
Subject: [PATCH] Improve group visibility level feature

---
 .../groups/application_controller.rb          |  2 +-
 app/controllers/groups_controller.rb          | 11 ------
 app/helpers/visibility_level_helper.rb        |  3 +-
 app/models/ability.rb                         |  2 +-
 app/models/group.rb                           | 22 ++++++++++++
 app/models/project.rb                         | 20 ++++++++---
 app/services/groups/base_service.rb           | 15 +++-----
 app/services/groups/create_service.rb         |  8 +++--
 app/services/groups/update_service.rb         | 17 ++++-----
 app/services/projects/create_service.rb       | 13 ++-----
 app/views/groups/show.html.haml               |  3 +-
 lib/api/entities.rb                           |  2 +-
 lib/gitlab/visibility_level.rb                |  4 +--
 spec/factories/groups.rb                      | 12 +++++++
 spec/finders/groups_finder_spec.rb            | 13 +++----
 spec/finders/personal_projects_finder_spec.rb | 19 ++++------
 spec/helpers/visibility_level_helper_spec.rb  |  9 ++---
 spec/models/group_spec.rb                     | 10 +++---
 spec/models/project_spec.rb                   |  8 ++---
 spec/services/groups/create_service_spec.rb   |  4 +--
 spec/services/groups/update_service_spec.rb   | 36 +++++++++----------
 21 files changed, 118 insertions(+), 115 deletions(-)

diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index be801858eaf..795ce50fe92 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -9,7 +9,7 @@ class Groups::ApplicationController < ApplicationController
   end
 
   def authorize_read_group!
-    unless @group and can?(current_user, :read_group, @group)
+    unless @group && can?(current_user, :read_group, @group)
       if current_user.nil?
         return authenticate_user!
       else
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index ba2057eb2c8..b635fb150ae 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -105,17 +105,6 @@ class GroupsController < Groups::ApplicationController
     @projects ||= ProjectsFinder.new.execute(current_user, group: group).sorted_by_activity
   end
 
-  # Dont allow unauthorized access to group
-  def authorize_read_group!
-    unless can?(current_user, :read_group, @group)
-      if current_user.nil?
-        return authenticate_user!
-      else
-        return render_404
-      end
-    end
-  end
-
   def authorize_create_group!
     unless can?(current_user, :create_group, nil)
       return render_404
diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb
index 930cc883634..7fa18ba9079 100644
--- a/app/helpers/visibility_level_helper.rb
+++ b/app/helpers/visibility_level_helper.rb
@@ -85,7 +85,6 @@ module VisibilityLevelHelper
   end
 
   def skip_level?(form_model, level)
-    form_model.is_a?(Project) &&
-    !form_model.visibility_level_allowed?(level)
+    form_model.is_a?(Project) && !form_model.visibility_level_allowed?(level)
   end
 end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index ffcf05dcd33..61d5e7dc859 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -295,7 +295,7 @@ class Ability
     end
 
     def can_read_group?(user, group)
-        user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
+      user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
         ProjectsFinder.new.execute(user, group: group).any?
     end
 
diff --git a/app/models/group.rb b/app/models/group.rb
index b094a65e3d6..17c69af4d1b 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -29,6 +29,8 @@ class Group < Namespace
   has_many :shared_projects, through: :project_group_links, source: :project
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
+  validate :visibility_level_allowed_by_projects
+
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   mount_uploader :avatar, AvatarUploader
@@ -80,6 +82,26 @@ class Group < Namespace
     visibility_level
   end
 
+  def visibility_level_allowed_by_projects
+    unless visibility_level_allowed?
+      level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
+      self.errors.add(:visibility_level, "#{level_name} is not allowed since there are projects with higher visibility.")
+    end
+  end
+
+  def visibility_level_allowed?
+    projects_visibility = self.projects.pluck(:visibility_level)
+
+    allowed_by_projects = projects_visibility.none? { |project_visibility| self.visibility_level < project_visibility }
+
+    unless allowed_by_projects
+      level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
+      self.errors.add(:visibility_level, "#{level_name} is not allowed since there are projects with higher visibility.")
+    end
+
+    allowed_by_projects
+  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 2828385a5f6..7c10ab35431 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -73,7 +73,7 @@ class Project < ActiveRecord::Base
     update_column(:last_activity_at, self.created_at)
   end
 
-  # update visibility_levet of forks
+  # update visibility_level of forks
   after_update :update_forks_visibility_level
   def update_forks_visibility_level
     return unless visibility_level < visibility_level_was
@@ -197,6 +197,7 @@ class Project < ActiveRecord::Base
   validate :avatar_type,
     if: ->(project) { project.avatar.present? && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
+  validate :visibility_level_allowed_in_group
 
   add_authentication_token_field :runners_token
   before_save :ensure_runners_token
@@ -446,6 +447,12 @@ class Project < ActiveRecord::Base
     errors[:base] << ("Can't check your ability to create project")
   end
 
+  def visibility_level_allowed_in_group
+    unless visibility_level_allowed?
+      self.errors.add(:visibility_level, "#{self.visibility_level} is not allowed in a #{self.group.visibility_level} group.")
+    end
+  end
+
   def to_param
     path
   end
@@ -961,9 +968,14 @@ class Project < ActiveRecord::Base
     issues.opened.count
   end
 
-  def visibility_level_allowed?(level)
-    allowed_by_forks = forked? ? Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i) : true
-    allowed_by_groups = group.present? ? level.to_i <= group.visibility_level : true
+  def visibility_level_allowed?(level = self.visibility_level)
+    allowed_by_forks =  if forked?
+                          Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level)
+                        else
+                          true
+                        end
+
+    allowed_by_groups = group.present? ? level <= group.visibility_level : true
 
     allowed_by_forks && allowed_by_groups
   end
diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb
index 053b6a05281..1db81216084 100644
--- a/app/services/groups/base_service.rb
+++ b/app/services/groups/base_service.rb
@@ -8,18 +8,13 @@ module Groups
 
     private
 
-    def visibility_allowed_for_user?(level)
+    def visibility_allowed_for_user?
+      level = group.visibility_level
       allowed_by_user  = Gitlab::VisibilityLevel.allowed_for?(current_user, level)
-      @group.errors.add(:visibility_level, "You are not authorized to set this permission level.") unless allowed_by_user
+
+      group.errors.add(:visibility_level, "#{level} has been restricted by your GitLab administrator.") unless allowed_by_user
+      
       allowed_by_user
     end
-
-    def visibility_allowed_for_project?(level)
-      projects_visibility = group.projects.pluck(:visibility_level)
-
-      allowed_by_projects = !projects_visibility.any? { |project_visibility| level.to_i < project_visibility }
-      @group.errors.add(:visibility_level, "Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects
-      allowed_by_projects
-    end
   end
 end
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index 38742369d82..f605ccca81b 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -2,14 +2,16 @@ module Groups
   class CreateService < Groups::BaseService
     def initialize(user, params = {})
       @current_user, @params = user, params.dup
-      @group = Group.new(@params)
     end
 
     def execute
-      return @group unless visibility_allowed_for_user?(@params[:visibility_level])
+      @group = Group.new(params)
+
+      return @group unless visibility_allowed_for_user?
+
       @group.name = @group.path.dup unless @group.name
       @group.save
-      @group.add_owner(@current_user)
+      @group.add_owner(current_user)
       @group
     end
   end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index b910e0fde98..0b0c5a35d37 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -1,20 +1,15 @@
-#Checks visibility level permission check before updating a group
-#Do not allow to put Group visibility level smaller than its projects
-#Do not allow unauthorized permission levels
+# Checks visibility level permission check before updating a group
+# Do not allow to put Group visibility level smaller than its projects
+# Do not allow unauthorized permission levels
 
 module Groups
   class UpdateService < Groups::BaseService
     def execute
-      return false unless visibility_level_allowed?(params[:visibility_level])
-      group.update_attributes(params)
-    end
+      group.assign_attributes(params)
 
-    private
+      return false unless visibility_allowed_for_user?
 
-    def visibility_level_allowed?(level)
-      return true unless level.present?
-
-      visibility_allowed_for_project?(level) && visibility_allowed_for_user?(level)
+      group.save
     end
   end
 end
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 4c121106bda..cebfc432002 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -9,13 +9,8 @@ module Projects
 
       @project = Project.new(params)
 
-      # Make sure that the user is allowed to use the specified visibility
-      # level
-
-      unless visibility_level_allowed?
-        deny_visibility_level(@project)
-        return @project
-      end
+      # Make sure that the user is allowed to use the specified visibility level
+      return @project unless visibility_level_allowed?
 
       # Set project name from path
       if @project.name.present? && @project.path.present?
@@ -55,9 +50,7 @@ module Projects
         @project.save
 
         if @project.persisted? && !@project.import?
-          unless @project.create_repository
-            raise 'Failed to create repository'
-          end
+          raise 'Failed to create repository' unless @project.create_repository
         end
       end
 
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index 4be117667db..222c3e4a40e 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -17,8 +17,7 @@
   .cover-title
     %h1
       = @group.name
-
-      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{group_visibility_description(@group)}"}
+      %span.visibility-icon.has_tooltip{ data: { container: 'body', placement: 'left' }, title: group_visibility_description(@group) }
         = visibility_level_icon(@group.visibility_level, fw: false)
 
   .cover-desc.username
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 20565e368dd..197e826e5bc 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -85,7 +85,7 @@ module API
     end
 
     class Group < Grape::Entity
-      expose :id, :name, :path, :description
+      expose :id, :name, :path, :description, :visibility_level
       expose :avatar_url
 
       expose :web_url do |group, options|
diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb
index f193fb282ab..58cd8df5219 100644
--- a/lib/gitlab/visibility_level.rb
+++ b/lib/gitlab/visibility_level.rb
@@ -9,8 +9,8 @@ module Gitlab
     extend ActiveSupport::Concern
 
     included do
-      scope :public_only, -> { where(visibility_level: PUBLIC) }
-      scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
+      scope :public_only,               -> { where(visibility_level: PUBLIC) }
+      scope :public_and_internal_only,  -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
     end
 
     PRIVATE  = 0 unless const_defined?(:PRIVATE)
diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb
index 4a3a155d7ff..2d47a6f6c4c 100644
--- a/spec/factories/groups.rb
+++ b/spec/factories/groups.rb
@@ -3,5 +3,17 @@ FactoryGirl.define do
     sequence(:name) { |n| "group#{n}" }
     path { name.downcase.gsub(/\s/, '_') }
     type 'Group'
+
+    trait :public do
+      visibility_level Gitlab::VisibilityLevel::PUBLIC
+    end
+
+    trait :internal do
+      visibility_level Gitlab::VisibilityLevel::INTERNAL
+    end
+
+    trait :private do
+      visibility_level Gitlab::VisibilityLevel::PRIVATE
+    end
   end
 end
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index d0fd7af8cc3..d5d111e8d15 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -2,11 +2,11 @@ require 'spec_helper'
 
 describe GroupsFinder do
   describe '#execute' do
-    let(:user) { create(:user) }
-    let!(:private_group) { create(:group, visibility_level: 0) }
-    let!(:internal_group) { create(:group, visibility_level: 10) }
-    let!(:public_group) { create(:group, visibility_level: 20) }
-    let(:finder) { described_class.new }
+    let(:user)            { create(:user) }
+    let!(:private_group)  { create(:group, :private) }
+    let!(:internal_group) { create(:group, :internal) }
+    let!(:public_group)   { create(:group, :public) }
+    let(:finder)          { described_class.new }
 
     describe 'execute' do
       describe 'without a user' do
@@ -23,7 +23,8 @@ describe GroupsFinder do
         end
 
         context 'external user' do
-          before { user.update_attribute(external: true) }
+          let(:user) { create(:user, external: true) }
+
           it { is_expected.to eq([public_group]) }
         end
       end
diff --git a/spec/finders/personal_projects_finder_spec.rb b/spec/finders/personal_projects_finder_spec.rb
index 8758f61903c..a4681fe59d8 100644
--- a/spec/finders/personal_projects_finder_spec.rb
+++ b/spec/finders/personal_projects_finder_spec.rb
@@ -1,24 +1,17 @@
 require 'spec_helper'
 
 describe PersonalProjectsFinder do
-  let(:source_user) { create(:user) }
-  let(:current_user) { create(:user) }
-
-  let(:finder) { described_class.new(source_user) }
-
-  let!(:public_project) do
-    create(:project, :public, namespace: source_user.namespace, name: 'A',
-                              path: 'A')
-  end
+  let(:source_user)     { create(:user) }
+  let(:current_user)    { create(:user) }
+  let(:finder)          { described_class.new(source_user) }
+  let!(:public_project) { create(:project, :public, namespace: source_user.namespace) }
 
   let!(:private_project) do
-    create(:project, :private, namespace: source_user.namespace, name: 'B',
-                               path: 'B')
+    create(:project, :private, namespace: source_user.namespace, path: 'mepmep')
   end
 
   let!(:internal_project) do
-    create(:project, :internal, namespace: source_user.namespace, name: 'c',
-                               path: 'C')
+    create(:project, :internal, namespace: source_user.namespace, path: 'C')
   end
 
   before do
diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb
index ef60ef75fbe..ff98249570d 100644
--- a/spec/helpers/visibility_level_helper_spec.rb
+++ b/spec/helpers/visibility_level_helper_spec.rb
@@ -66,13 +66,8 @@ describe VisibilityLevelHelper do
 
   describe "skip_level?" do
     describe "forks" do
-      let(:project) { create(:project, :internal) }
-      let(:fork_project) { create(:forked_project_with_submodules) }
-
-      before do
-        fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
-        fork_project.save
-      end
+      let(:project)       { create(:project, :internal) }
+      let(:fork_project)  { create(:project, forked_from_project: project) }
 
       it "skips levels" do
         expect(skip_level?(fork_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 135d298e10f..0591aa089d8 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -57,18 +57,18 @@ describe Group, models: true do
   end
 
   describe 'scopes' do
-    let!(:private_group)  { create(:group, visibility_level: 0)  }
-    let!(:internal_group) { create(:group, visibility_level: 10) }
-    let!(:public_group)   { create(:group, visibility_level: 20) }
+    let!(:private_group)  { create(:group, :private)  }
+    let!(:internal_group) { create(:group, :internal) }
+    let!(:public_group)   { create(:group, :public) }
 
     describe 'public_only' do
-      subject { described_class.public_only }
+      subject { described_class.public_only.to_a }
 
       it{ is_expected.to eq([public_group]) }
     end
 
     describe 'public_and_internal_only' do
-      subject { described_class.public_and_internal_only }
+      subject { described_class.public_and_internal_only.to_a }
 
       it{ is_expected.to eq([public_group, internal_group]) }
     end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 7fd3726c6ad..74383204250 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -571,12 +571,8 @@ describe Project, models: true do
     end
 
     context 'when checking on forked project' do
-      let(:forked_project) { create :forked_project_with_submodules }
-
-      before do
-        forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id)
-        forked_project.save
-      end
+      let(:project)        { create(:project, :internal) }
+      let(:forked_project) { create(:project, forked_from_project: project) }
 
       it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy }
       it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy }
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index b938a2f0c05..6aefb48a4e8 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe Groups::CreateService, services: true do
-    let!(:user)         { create(:user) }
-    let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
+  let!(:user)         { create(:user) }
+  let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
 
   describe "execute" do
     let!(:service) { described_class.new(user, group_params ) }
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index c759e32342d..9d427ff2d90 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe Groups::UpdateService, services: true do
-    let!(:user)    { create(:user) }
-    let!(:private_group)    { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
-    let!(:internal_group)   { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
-    let!(:public_group)     { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
+  let!(:user)             { create(:user) }
+  let!(:private_group)    { create(:group, :private) }
+  let!(:internal_group)   { create(:group, :internal) }
+  let!(:public_group)     { create(:group, :public) }
 
   describe "execute" do
     context "project visibility_level validation" do
@@ -14,28 +14,28 @@ describe Groups::UpdateService, services: true do
 
         before do
           public_group.add_user(user, Gitlab::Access::MASTER)
-          create(:project, :public, group: public_group, name: 'B', path: 'B')
+          create(:project, :public, group: public_group)
         end
 
         it "cant downgrade permission level" do
           expect(service.execute).to be_falsy
-          expect(public_group.errors.count).to eq(1)
+          expect(public_group.errors.count).to eq(2)
         end
       end
 
-    context "internal group with internal project" do
-        let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE ) }
-
-        before do
-          internal_group.add_user(user, Gitlab::Access::MASTER)
-          create(:project, :internal, group: internal_group, name: 'B', path: 'B')
+      context "internal group with internal project" do
+          let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE ) }
+  
+          before do
+            internal_group.add_user(user, Gitlab::Access::MASTER)
+            create(:project, :internal, group: internal_group)
+          end
+  
+          it "cant downgrade permission level" do
+            expect(service.execute).to be_falsy
+            expect(internal_group.errors.count).to eq(2)
+          end
         end
-
-        it "cant downgrade permission level" do
-          expect(service.execute).to be_falsy
-          expect(internal_group.errors.count).to eq(1)
-        end
-      end
     end
   end