Prevent projects to have higher visibility than groups

Prevent Groups to have smaller visibility than projects
Add default_group_visibility_level to configuration
Code improvements

30 files changed, 304 insertions, 34 deletions

diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss
index c98e43ad09f..18044b25b87 100644
--- a/app/assets/stylesheets/framework/common.scss
+++ b/app/assets/stylesheets/framework/common.scss
@@ -383,3 +383,35 @@ table {
   margin-right: -$gl-padding;
   border-top: 1px solid $border-color;
 }
+.message {
+  border: 1px solid #ccc;
+  padding: 10px;
+  color: #333;
+}
+.message {
+  border: 1px solid #ccc;
+  padding: 10px;
+  color: #333;
+}
+
+.group-projects-show-title{
+    h1 {
+      color: #313236;
+      margin: 0;
+      margin-bottom: 6px;
+      font-size: 23px;
+      font-weight: normal;
+    }
+
+    .visibility-icon {
+      display: inline-block;
+      margin-left: 5px;
+      font-size: 18px;
+      color: $gray;
+    }
+
+    p {
+      padding: 0 $gl-padding;
+      color: #5c5d5e;
+    }
+  }
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index 04a99d8c84a..ed9f6031389 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -61,6 +61,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :session_expire_delay,
       :default_project_visibility,
       :default_snippet_visibility,
+      :default_group_visibility,
       :restricted_signup_domains_raw,
       :version_check_enabled,
       :admin_notification_email,
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 6532eee1602..54f14e62ead 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -79,7 +79,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def update
-    if @group.update_attributes(group_params)
+    if Groups::UpdateService.new(@group, current_user, group_params).execute
       redirect_to edit_group_path(@group), notice: "Group '#{@group.name}' was successfully updated."
     else
       render action: "edit"
diff --git a/app/controllers/namespaces_controller.rb b/app/controllers/namespaces_controller.rb
index 282012c60a1..5a94dcb0dbd 100644
--- a/app/controllers/namespaces_controller.rb
+++ b/app/controllers/namespaces_controller.rb
@@ -14,7 +14,7 @@ class NamespacesController < ApplicationController
 
     if user
       redirect_to user_path(user)
-    elsif group
+    elsif group && can?(current_user, :read_group, namespace)
       redirect_to group_path(group)
     elsif current_user.nil?
       authenticate_user!
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d26a1ce6737..7b32572f822 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -3,16 +3,13 @@ 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
+    @groups =  JoinedGroupsFinder.new(@user).execute(current_user)
 
->>>>>>> Code improvements
     respond_to do |format|
       format.html
 
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
new file mode 100644
index 00000000000..131b518563e
--- /dev/null
+++ b/app/finders/joined_groups_finder.rb
@@ -0,0 +1,45 @@
+#Shows only authorized groups of a user
+class JoinedGroupsFinder
+  def initialize(user = nil)
+    @user = user
+  end
+
+  # Finds the groups of the source user, optionally limited to those visible to
+  # the current user.
+  #
+  # current_user - If given the groups of "@user" will only include the groups
+  #                "current_user" can also see.
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil)
+    if current_user
+      relation = groups_visible_to_user(current_user)
+    else
+      relation = public_groups
+    end
+
+    relation.order_id_desc
+  end
+
+  private
+
+  # Returns the groups the user in "current_user" can see.
+  #
+  # This list includes all public/internal projects as well as the projects of
+  # "@user" that "current_user" also has access to.
+  def groups_visible_to_user(current_user)
+    base  = @user.authorized_groups.visible_to_user(current_user)
+    extra = public_and_internal_groups
+    union = Gitlab::SQL::Union.new([base.select(:id), extra.select(:id)])
+
+    Group.where("namespaces.id IN (#{union.to_sql})")
+  end
+
+  def public_groups
+    @user.authorized_groups.public_only
+  end
+
+  def public_and_internal_groups
+    @user.authorized_groups.public_and_internal_only
+  end
+end
diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb
index c47342534a8..930cc883634 100644
--- a/app/helpers/visibility_level_helper.rb
+++ b/app/helpers/visibility_level_helper.rb
@@ -80,6 +80,10 @@ module VisibilityLevelHelper
     current_application_settings.default_snippet_visibility
   end
 
+  def default_group_visibility
+    current_application_settings.default_group_visibility
+  end
+
   def skip_level?(form_model, level)
     form_model.is_a?(Project) &&
     !form_model.visibility_level_allowed?(level)
diff --git a/app/models/ability.rb b/app/models/ability.rb
index ec5587d8fa5..1c9b15069aa 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -296,8 +296,7 @@ class Ability
 
     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
+      user.admin? || group.public? || group.internal? || group.users.include?(user)
     end
 
     def namespace_abilities(user, namespace)
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 269056e0e77..c4879598c4e 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -18,6 +18,7 @@
 #  max_attachment_size               :integer          default(10), not null
 #  default_project_visibility        :integer
 #  default_snippet_visibility        :integer
+#  default_group_visibility          :integer
 #  restricted_signup_domains         :text
 #  user_oauth_applications           :boolean          default(TRUE)
 #  after_sign_out_path               :string(255)
diff --git a/app/models/concerns/shared_scopes.rb b/app/models/concerns/shared_scopes.rb
deleted file mode 100644
index f576d2c0821..00000000000
--- a/app/models/concerns/shared_scopes.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-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 d1a1817f0fa..02b9a968dcd 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -21,7 +21,6 @@ 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
diff --git a/app/models/project.rb b/app/models/project.rb
index 6e86c7cc883..ae5f6e2417d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -52,7 +52,6 @@ class Project < ActiveRecord::Base
   include AfterCommitQueue
   include CaseSensitivity
   include TokenAuthenticatable
-  include SharedScopes
 
   extend Gitlab::ConfigHelper
 
@@ -934,8 +933,10 @@ class Project < ActiveRecord::Base
   end
 
   def visibility_level_allowed?(level)
-    return true unless forked?
-    Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i)
+    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
+
+    allowed_by_forks && allowed_by_groups
   end
 
   def runners_token
diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb
new file mode 100644
index 00000000000..5becd475d3a
--- /dev/null
+++ b/app/services/groups/base_service.rb
@@ -0,0 +1,9 @@
+module Groups
+  class BaseService
+    attr_accessor :group, :current_user, :params
+
+    def initialize(group, user, params = {})
+      @group, @current_user, @params = group, user, params.dup
+    end
+  end
+end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
new file mode 100644
index 00000000000..acb6c529c17
--- /dev/null
+++ b/app/services/groups/update_service.rb
@@ -0,0 +1,44 @@
+#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
+      visibility_level_allowed?(params[:visibility_level]) ? group.update_attributes(params) : false
+    end
+
+    private
+
+    def visibility_level_allowed?(level)
+      return true unless level.present?
+
+      allowed_by_projects = visibility_by_project(level)
+      allowed_by_user     = visibility_by_user(level)
+
+      allowed_by_projects && allowed_by_user
+    end
+
+    def visibility_by_project(level)
+      projects_visibility = group.projects.pluck(:visibility_level)
+
+      allowed_by_projects = !projects_visibility.any?{|project_visibility| level.to_i < project_visibility }
+      add_error_message("Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects
+      allowed_by_projects
+    end
+
+    def visibility_by_user(level)
+      allowed_by_user  = Gitlab::VisibilityLevel.allowed_for?(current_user, level)
+      add_error_message("You are not authorized to set this permission level.") unless allowed_by_user
+      allowed_by_user
+    end
+
+    def add_error_message(message)
+      level_name = Gitlab::VisibilityLevel.level_name(params[:visibility_level])
+      group.errors.add(:visibility_level, message)
+    end
+  end
+end
+
+
+
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index a6820183bee..522fae79503 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -11,8 +11,8 @@ module Projects
 
       # Make sure that the user is allowed to use the specified visibility
       # level
-      unless Gitlab::VisibilityLevel.allowed_for?(current_user,
-                                                  params[:visibility_level])
+
+      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) && @project.visibility_level_allowed?(@project.visibility_level)
         deny_visibility_level(@project)
         return @project
       end
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index b30dfd109ea..0350995d03d 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -19,6 +19,10 @@
       = f.label :default_snippet_visibility, class: 'control-label col-sm-2'
       .col-sm-10
         = render('shared/visibility_radios', model_method: :default_snippet_visibility, form: f, selected_level: @application_setting.default_snippet_visibility, form_model: ProjectSnippet.new)
+    .form-group.group-visibility-level-holder
+      = f.label :default_group_visibility, class: 'control-label col-sm-2'
+      .col-sm-10
+        = render('shared/visibility_radios', model_method: :default_group_visibility, form: f, selected_level: @application_setting.default_group_visibility, form_model: Group.new)
     .form-group
       = f.label :restricted_visibility_levels, class: 'control-label col-sm-2'
       .col-sm-10
diff --git a/app/views/groups/new.html.haml b/app/views/groups/new.html.haml
index 1526ca42634..30ab8aeba13 100644
--- a/app/views/groups/new.html.haml
+++ b/app/views/groups/new.html.haml
@@ -17,7 +17,7 @@
     .col-sm-10
       = render 'shared/choose_group_avatar_button', f: f
 
-  = render 'shared/visibility_level', f: f, visibility_level: @group.visibility_level, can_change_visibility_level: true, form_model: @group
+  = render 'shared/visibility_level', f: f, visibility_level: default_group_visibility, can_change_visibility_level: true, form_model: @group
 
   .form-group
     .col-sm-offset-2.col-sm-10
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index 6148d8cb3d2..0a8c2da7207 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -1,8 +1,5 @@
 - @no_container = true
 
-- unless can?(current_user, :read_group, @group)
-  - @disable_search_panel = true
-
 = content_for :meta_tags do
   - if current_user
     = auto_discovery_link_tag(:atom, group_url(@group, format: :atom, private_token: current_user.private_token), title: "#{@group.name} activity")
@@ -17,8 +14,12 @@
   .avatar-holder
     = link_to group_icon(@group), target: '_blank' do
       = image_tag group_icon(@group), class: "avatar group-avatar s90"
-  .cover-title
-    = @group.name
+  .group-projects-show-title
+    %h1
+      = @group.name
+
+      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{visibility_level_label(@group.visibility_level)} - #{project_visibility_level_description(@group.visibility_level)}"}
+        = visibility_level_icon(@group.visibility_level, fw: false)
 
   .cover-desc.username
     @#{@group.path}
@@ -27,7 +28,6 @@
     .cover-desc.description
       = markdown(@group.description, pipeline: :description)
 
-
   %ul.nav-links
     %li.active
       = link_to "#activity", 'data-toggle' => 'tab' do
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index 77d01a7736c..714da410f56 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -7,9 +7,8 @@
 
       .navbar-collapse.collapse
         %ul.nav.navbar-nav.pull-right
-          - unless @disable_search_panel
-            %li.hidden-sm.hidden-xs
-              = render 'layouts/search'
+          %li.hidden-sm.hidden-xs
+            = render 'layouts/search'
           %li.visible-sm.visible-xs
             = link_to search_path, title: 'Search', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do
               = icon('search')
diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml
index b45df44f270..dd16f504c92 100644
--- a/app/views/projects/_home_panel.html.haml
+++ b/app/views/projects/_home_panel.html.haml
@@ -2,7 +2,7 @@
 .project-home-panel.cover-block.clearfix{:class => ("empty-project" if empty_repo)}
   .project-identicon-holder
     = project_icon(@project, alt: '', class: 'project-avatar avatar s90')
-  .project-home-desc
+  .group-projects-show-title
     %h1
       = @project.name
       %span.visibility-icon.has_tooltip{data: { container: 'body' },
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 05f127d622a..fc808e228de 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -90,6 +90,10 @@ production: &base
       snippets: false
       builds: true
 
+    ## Default group features settings
+    default_groups_features:
+      visibility_level: 20
+
     ## Webhook settings
     # Number of seconds to wait for HTTP response after sending webhook HTTP POST request (default: 10)
     # webhook_timeout: 10
diff --git a/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
new file mode 100644
index 00000000000..c2bdd7b31fa
--- /dev/null
+++ b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
@@ -0,0 +1,11 @@
+class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
+  def up
+    add_column :application_settings, :default_group_visibility, :integer
+    visibility = Settings.gitlab.default_groups_features['visibility_level']
+    execute("update application_settings set default_group_visibility = #{visibility}")
+  end
+
+  def down
+    remove_column :application_settings, :default_group_visibility
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index d2f311e42da..1d4e18c5f95 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160301124843) do
+ActiveRecord::Schema.define(version: 20160308212903) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -75,6 +75,7 @@ ActiveRecord::Schema.define(version: 20160301124843) do
     t.boolean  "akismet_enabled",                   default: false
     t.string   "akismet_api_key"
     t.boolean  "email_author_in_body",              default: false
+    t.integer  "default_group_visibility"
   end
 
   create_table "audit_events", force: :cascade do |t|
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 5b5b8bd044b..efad2390127 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -332,6 +332,7 @@ module API
       expose :session_expire_delay
       expose :default_project_visibility
       expose :default_snippet_visibility
+      expose :default_group_visibility
       expose :restricted_signup_domains
       expose :user_oauth_applications
       expose :after_sign_out_path
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index 761b63e98f6..a50d8e3d5a8 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -29,6 +29,7 @@ module Gitlab
         session_expire_delay: Settings.gitlab['session_expire_delay'],
         default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'],
         default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'],
+        default_group_visibility: Settings.gitlab.default_groups_features['visibility_level'],
         restricted_signup_domains: Settings.gitlab['restricted_signup_domains'],
         import_sources: ['github','bitbucket','gitlab','gitorious','google_code','fogbugz','git'],
         shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'],
diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb
index 3160a3c7582..f6e0dd6afc0 100644
--- a/lib/gitlab/visibility_level.rb
+++ b/lib/gitlab/visibility_level.rb
@@ -12,6 +12,13 @@ module Gitlab
     PUBLIC   = 20 unless const_defined?(:PUBLIC)
 
     class << self
+      def included(base)
+        base.class_eval do
+          scope :public_only, -> { where(visibility_level: PUBLIC) }
+          scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
+        end
+      end
+
       def values
         options.values
       end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index e7ead824d20..91db3fd1ee2 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -54,6 +54,7 @@ describe GroupsController do
     let(:group) { create(:group, visibility_level: 20) }
 
     it 'checks if group can be updated' do
+      expect_any_instance_of(Groups::UpdateService).to receive(:execute)
       expect(controller).to receive(:authorize_admin_group!)
       put :update, id: group.path, group: { name: 'test' }
     end
diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb
new file mode 100644
index 00000000000..e2f6c593638
--- /dev/null
+++ b/spec/finders/joined_groups_finder_spec.rb
@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+describe JoinedGroupsFinder do
+  describe '#execute' do
+    let!(:profile_owner)    { create(:user) }
+    let!(:profile_visitor)  { create(:user) }
+
+    let!(:private_group)    { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
+    let!(:private_group_2)  { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
+    let!(:internal_group)   { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
+    let!(:internal_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
+    let!(:public_group)     { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
+    let!(:public_group_2)   { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
+    let!(:finder) { described_class.new(profile_owner) }
+
+    describe 'execute' do
+      context 'without a user only shows public groups from profile owner' do
+        before { public_group.add_user(profile_owner, Gitlab::Access::MASTER)}
+        subject { finder.execute }
+
+        it { is_expected.to eq([public_group]) }
+      end
+
+      context 'only shows groups where both users are authorized to see' do
+        subject { finder.execute(profile_visitor) }
+
+        before do
+          private_group.add_user(profile_owner, Gitlab::Access::MASTER)
+          private_group.add_user(profile_visitor, Gitlab::Access::DEVELOPER)
+          internal_group.add_user(profile_owner, Gitlab::Access::MASTER)
+          public_group.add_user(profile_owner, Gitlab::Access::MASTER)
+        end
+
+        it { is_expected.to eq([public_group, internal_group, private_group]) }
+      end
+
+      context 'shows group if profile visitor is in one of its projects' do
+        before do
+          public_group.add_user(profile_owner, Gitlab::Access::MASTER)
+          private_group.add_user(profile_owner, Gitlab::Access::MASTER)
+          project = create(:project, :private, group: private_group, name: 'B', path: 'B')
+          project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER)
+        end
+
+        subject { finder.execute(profile_visitor) }
+
+        it { is_expected.to eq([public_group, private_group]) }
+      end
+    end
+  end
+end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2fa38a5d3d3..9efaffbb577 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -583,6 +583,21 @@ describe Project, models: true do
       it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PUBLIC)).to be_falsey }
     end
 
+    context 'when checking projects from groups' do
+      let(:private_group)    { create(:group, visibility_level: 0)  }
+      let(:internal_group)   { create(:group, visibility_level: 10) }
+
+      let(:private_project)  { create :project, group: private_group, visibility_level: Gitlab::VisibilityLevel::PRIVATE   }
+      let(:internal_project) { create :project, group: internal_group, visibility_level: Gitlab::VisibilityLevel::INTERNAL }
+
+      context 'when group is private project can not be internal' do
+        it { expect(private_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_falsey }
+      end
+
+      context 'when group is internal project can not be public' do
+        it { expect(internal_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PUBLIC)).to be_falsey }
+      end
+    end
   end
 
   describe '#rename_repo' do
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
new file mode 100644
index 00000000000..c759e32342d
--- /dev/null
+++ b/spec/services/groups/update_service_spec.rb
@@ -0,0 +1,51 @@
+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) }
+
+  describe "execute" do
+    context "project visibility_level validation" do
+
+      context "public group with public projects" do
+        let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL ) }
+
+        before do
+          public_group.add_user(user, Gitlab::Access::MASTER)
+          create(:project, :public, group: public_group, name: 'B', path: 'B')
+        end
+
+        it "cant downgrade permission level" do
+          expect(service.execute).to be_falsy
+          expect(public_group.errors.count).to eq(1)
+        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')
+        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
+
+  context "unauthorized visibility_level validation" do
+    let!(:service) { described_class.new(internal_group, user, visibility_level: 99 ) }
+    before { internal_group.add_user(user, Gitlab::Access::MASTER) }
+
+    it "does not change permission level" do
+      expect(service.execute).to be_falsy
+      expect(internal_group.errors.count).to eq(1)
+    end
+  end
+end