Code fixes

24 files changed, 178 insertions, 125 deletions

diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss
index d20b77ffae9..31084872367 100644
--- a/app/assets/stylesheets/framework/blocks.scss
+++ b/app/assets/stylesheets/framework/blocks.scss
@@ -115,6 +115,24 @@
     color: #4c4e54;
     font-size: 23px;
     line-height: 1.1;
+
+    h1 {
+      color: #313236;
+      margin-bottom: 6px;
+      font-size: 23px;
+    }
+
+    .visibility-icon {
+      display: inline-block;
+      margin-left: 5px;
+      font-size: 18px;
+      color: $gray;
+    }
+
+    p {
+      padding: 0 $gl-padding;
+      color: #5c5d5e;
+    }
   }
 
   .cover-desc {
diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss
index f4608cd80bb..ff551f151f1 100644
--- a/app/assets/stylesheets/framework/common.scss
+++ b/app/assets/stylesheets/framework/common.scss
@@ -385,25 +385,3 @@ table {
   margin-right: -$gl-padding;
   border-top: 1px solid $border-color;
 }
-
-.cover-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/groups_controller.rb b/app/controllers/groups_controller.rb
index 8243946c852..ba2057eb2c8 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -28,9 +28,9 @@ class GroupsController < Groups::ApplicationController
   end
 
   def create
-    @group = Group.new(group_params)
+    @group = Groups::CreateService.new(current_user, group_params).execute
 
-    if Groups::CreateService.new(@group, current_user, group_params).execute
+    if @group.persisted?
       redirect_to @group, notice: "Group '#{@group.name}' was successfully created."
     else
       render action: "new"
diff --git a/app/finders/contributed_projects_finder.rb b/app/finders/contributed_projects_finder.rb
index 4f7fe1c748b..f8b04dfa2aa 100644
--- a/app/finders/contributed_projects_finder.rb
+++ b/app/finders/contributed_projects_finder.rb
@@ -10,8 +10,9 @@ class ContributedProjectsFinder
   #                visible by this user.
   #
   # Returns an ActiveRecord::Relation.
+
   def execute(current_user = nil)
-    if current_user && !current_user.external?
+    if current_user
       relation = projects_visible_to_user(current_user)
     else
       relation = public_projects
@@ -24,9 +25,7 @@ class ContributedProjectsFinder
 
   def projects_visible_to_user(current_user)
     authorized = @user.contributed_projects.visible_to_user(current_user)
-
-    union = Gitlab::SQL::Union.
-      new([authorized.select(:id), public_projects.select(:id)])
+    union = Gitlab::SQL::Union.new([authorized.select(:id), public_projects.select(:id)])
 
     Project.where("projects.id IN (#{union.to_sql})")
   end
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index ce62f5e762f..30698f80231 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -14,9 +14,17 @@ class GroupsFinder
 
   def all_groups(current_user)
     if current_user
-      [current_user.authorized_groups, Group.unscoped.public_and_internal_only]
+      user_groups(current_user)
     else
       [Group.unscoped.public_only]
     end
   end
+
+  def user_groups(current_user)
+    if current_user.external?
+      [current_user.authorized_groups, Group.unscoped.public_only]
+    else
+      [current_user.authorized_groups, Group.unscoped.public_and_internal_only]
+    end
+  end
 end
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
index ff744689e3d..867eb661682 100644
--- a/app/finders/joined_groups_finder.rb
+++ b/app/finders/joined_groups_finder.rb
@@ -12,7 +12,7 @@ class JoinedGroupsFinder
   #
   # Returns an ActiveRecord::Relation.
   def execute(current_user = nil)
-    if current_user && !current_user.external?
+    if current_user
       relation = groups_visible_to_user(current_user)
     else
       relation = public_groups
@@ -29,7 +29,7 @@ class JoinedGroupsFinder
   # "@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
+    extra = current_user.external? ? public_groups : public_and_internal_groups
     union = Gitlab::SQL::Union.new([base.select(:id), extra.select(:id)])
 
     Group.where("namespaces.id IN (#{union.to_sql})")
diff --git a/app/finders/personal_projects_finder.rb b/app/finders/personal_projects_finder.rb
index 0e2d915da54..34f33e2353b 100644
--- a/app/finders/personal_projects_finder.rb
+++ b/app/finders/personal_projects_finder.rb
@@ -11,7 +11,7 @@ class PersonalProjectsFinder
   #
   # Returns an ActiveRecord::Relation.
   def execute(current_user = nil)
-    if current_user && !current_user.external?
+    if current_user
       relation = projects_visible_to_user(current_user)
     else
       relation = public_projects
@@ -23,10 +23,7 @@ class PersonalProjectsFinder
   private
 
   def projects_visible_to_user(current_user)
-    authorized = @user.personal_projects.visible_to_user(current_user)
-
-    union = Gitlab::SQL::Union.
-      new([authorized.select(:id), public_and_internal_projects.select(:id)])
+    union = Gitlab::SQL::Union.new(projects_for_user_ids(current_user))
 
     Project.where("projects.id IN (#{union.to_sql})")
   end
@@ -38,4 +35,14 @@ class PersonalProjectsFinder
   def public_and_internal_projects
     @user.personal_projects.public_and_internal_only
   end
+
+  def projects_for_user_ids(current_user)
+    authorized = @user.personal_projects.visible_to_user(current_user)
+
+    if current_user.external?
+      [authorized.select(:id), public_projects.select(:id)]
+    else
+      [authorized.select(:id), public_and_internal_projects.select(:id)]
+    end
+  end
 end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 134ae440c9c..ffcf05dcd33 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -295,11 +295,8 @@ class Ability
     end
 
     def can_read_group?(user, group)
-      if user.external?
-        group.public? || ProjectsFinder.new.execute(user, group: group).any?
-      else
-        user.admin? || group.public? || group.internal? || group.users.include?(user) || ProjectsFinder.new.execute(user, group: group).any?
-      end
+        user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
+        ProjectsFinder.new.execute(user, group: group).any?
     end
 
     def namespace_abilities(user, namespace)
diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb
index 644ec7c013e..053b6a05281 100644
--- a/app/services/groups/base_service.rb
+++ b/app/services/groups/base_service.rb
@@ -6,8 +6,20 @@ module Groups
       @group, @current_user, @params = group, user, params.dup
     end
 
-    def add_error_message(message)
-      group.errors.add(:visibility_level, message)
+    private
+
+    def visibility_allowed_for_user?(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
+      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 e2875aafb94..38742369d82 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -1,17 +1,16 @@
 module Groups
   class CreateService < Groups::BaseService
-    def execute
-      return false unless visibility_level_allowed?(params[:visibility_level])
-      @group.name = @group.path.dup unless @group.name
-      @group.save(params) && @group.add_owner(current_user)
+    def initialize(user, params = {})
+      @current_user, @params = user, params.dup
+      @group = Group.new(@params)
     end
 
-    private
-
-    def visibility_level_allowed?(level)
-      allowed = Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
-      add_error_message("Visibility level restricted by admin.") unless allowed
-      allowed
+    def execute
+      return @group unless visibility_allowed_for_user?(@params[:visibility_level])
+      @group.name = @group.path.dup unless @group.name
+      @group.save
+      @group.add_owner(@current_user)
+      @group
     end
   end
 end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index a7382c1e07c..b910e0fde98 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -14,24 +14,7 @@ module Groups
     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
+      visibility_allowed_for_project?(level) && visibility_allowed_for_user?(level)
     end
   end
 end
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index 2653a03059f..4be117667db 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -18,7 +18,7 @@
     %h1
       = @group.name
 
-      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{visibility_level_label(@group.visibility_level)} - #{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
@@ -28,14 +28,6 @@
     .cover-desc.description
       = markdown(@group.description, pipeline: :description)
 
-  %ul.nav-links
-    %li.active
-      = link_to "#activity", 'data-toggle' => 'tab' do
-        Activity
-    %li
-      = link_to "#projects", 'data-toggle' => 'tab' do
-        Projects
-
 - if can?(current_user, :read_group, @group)
   %div{ class: container_class }
     .top-area
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index fc808e228de..05f127d622a 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -90,10 +90,6 @@ 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
index b71322376fa..d5c78db0aa3 100644
--- a/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
+++ b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
@@ -13,15 +13,9 @@ class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
   end
 
   private
-  def allowed_visibility_level
-    default_visibility = Settings.gitlab.default_groups_features['visibility_level']
-    restricted_levels  = current_application_settings.restricted_visibility_levels
-    return default_visibility unless restricted_levels.present?
 
-    if restricted_levels.include?(default_visibility)
-      Gitlab::VisibilityLevel.values.select{ |vis_level| vis_level unless restricted_levels.include?(vis_level) }.last
-    else
-      default_visibility
-    end
+  def allowed_visibility_level
+    allowed_levels = Gitlab::VisibilityLevel.values - current_application_settings.restricted_visibility_levels
+    allowed_levels.max
   end
 end
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index a50d8e3d5a8..761b63e98f6 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -29,7 +29,6 @@ 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 f6e0dd6afc0..f193fb282ab 100644
--- a/lib/gitlab/visibility_level.rb
+++ b/lib/gitlab/visibility_level.rb
@@ -6,19 +6,18 @@
 module Gitlab
   module VisibilityLevel
     extend CurrentSettings
+    extend ActiveSupport::Concern
+
+    included do
+      scope :public_only, -> { where(visibility_level: PUBLIC) }
+      scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
+    end
 
     PRIVATE  = 0 unless const_defined?(:PRIVATE)
     INTERNAL = 10 unless const_defined?(:INTERNAL)
     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/features/security/group/internal_access_spec.rb b/spec/features/security/group/internal_access_spec.rb
index 4e781c23ee0..e44d4c32921 100644
--- a/spec/features/security/group/internal_access_spec.rb
+++ b/spec/features/security/group/internal_access_spec.rb
@@ -12,9 +12,12 @@ describe 'Internal group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
+
     end
 
     context "when user in group project" do
@@ -31,9 +34,11 @@ describe 'Internal group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -50,9 +55,11 @@ describe 'Internal group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -70,9 +77,11 @@ describe 'Internal group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -89,9 +98,11 @@ describe 'Internal group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb
index 0d01310b449..8d8c61a618f 100644
--- a/spec/features/security/group/private_access_spec.rb
+++ b/spec/features/security/group/private_access_spec.rb
@@ -14,9 +14,11 @@ describe 'Private group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
-      it { is_expected.to_not be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -33,9 +35,11 @@ describe 'Private group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
-      it { is_expected.to_not be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -52,9 +56,11 @@ describe 'Private group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
-      it { is_expected.to_not be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -72,9 +78,11 @@ describe 'Private group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
-      it { is_expected.to_not be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
@@ -91,9 +99,11 @@ describe 'Private group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
-      it { is_expected.to_not be_allowed_for :user }
-      it { is_expected.to_not be_allowed_for :visitor }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+      it { is_expected.to be_denied_for :external }
     end
 
     context "when user in group project" do
diff --git a/spec/features/security/group/public_access_spec.rb b/spec/features/security/group/public_access_spec.rb
index 75d208f2949..5ff982504c5 100644
--- a/spec/features/security/group/public_access_spec.rb
+++ b/spec/features/security/group/public_access_spec.rb
@@ -14,9 +14,11 @@ describe 'Public group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
       it { is_expected.to be_allowed_for :visitor }
+      it { is_expected.to be_allowed_for :external }
     end
 
     context "when user in group project" do
@@ -33,9 +35,11 @@ describe 'Public group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
       it { is_expected.to be_allowed_for :visitor }
+      it { is_expected.to be_allowed_for :external }
     end
 
     context "when user in group project" do
@@ -52,9 +56,11 @@ describe 'Public group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
       it { is_expected.to be_allowed_for :visitor }
+      it { is_expected.to be_allowed_for :external }
     end
 
     context "when user in group project" do
@@ -72,9 +78,11 @@ describe 'Public group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
       it { is_expected.to be_allowed_for :visitor }
+      it { is_expected.to be_allowed_for :external }
     end
 
     context "when user in group project" do
@@ -91,9 +99,11 @@ describe 'Public group access', feature: true do
       it { is_expected.to be_allowed_for group_member(:master) }
       it { is_expected.to be_allowed_for group_member(:reporter) }
       it { is_expected.to be_allowed_for group_member(:guest) }
+      it { is_expected.to be_allowed_for external_guest }
       it { is_expected.to be_allowed_for :admin }
       it { is_expected.to be_allowed_for :user }
       it { is_expected.to be_allowed_for :visitor }
+      it { is_expected.to be_allowed_for :external }
     end
 
     context "when user in group project" do
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index ed24954af7a..d0fd7af8cc3 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -18,7 +18,14 @@ describe GroupsFinder do
       describe 'with a user' do
         subject { finder.execute(user) }
 
-        it { is_expected.to eq([public_group, internal_group]) }
+        context 'normal user' do
+          it { is_expected.to eq([public_group, internal_group]) }
+        end
+
+        context 'external user' do
+          before { user.update_attribute(external: true) }
+          it { is_expected.to eq([public_group]) }
+        end
       end
     end
   end
diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb
index e2f6c593638..7b6fc837e5f 100644
--- a/spec/finders/joined_groups_finder_spec.rb
+++ b/spec/finders/joined_groups_finder_spec.rb
@@ -46,6 +46,25 @@ describe JoinedGroupsFinder do
 
         it { is_expected.to eq([public_group, private_group]) }
       end
+
+      context 'external users' do
+        before do
+          profile_visitor.update_attributes(external: true)
+          public_group.add_user(profile_owner, Gitlab::Access::MASTER)
+          internal_group.add_user(profile_owner, Gitlab::Access::MASTER)
+        end
+
+        subject { finder.execute(profile_visitor) }
+
+        it "doest not show internal groups if not member" do
+          expect(subject).to eq([public_group])
+        end
+
+        it "shows internal groups if authorized" do
+          internal_group.add_user(profile_visitor, Gitlab::Access::MASTER)
+          expect(subject).to eq([public_group, internal_group])
+        end
+      end
     end
   end
 end
diff --git a/spec/finders/personal_projects_finder_spec.rb b/spec/finders/personal_projects_finder_spec.rb
index 38817add456..8758f61903c 100644
--- a/spec/finders/personal_projects_finder_spec.rb
+++ b/spec/finders/personal_projects_finder_spec.rb
@@ -16,6 +16,11 @@ describe PersonalProjectsFinder do
                                path: 'B')
   end
 
+  let!(:internal_project) do
+    create(:project, :internal, namespace: source_user.namespace, name: 'c',
+                               path: 'C')
+  end
+
   before do
     private_project.team << [current_user, Gitlab::Access::DEVELOPER]
   end
@@ -29,6 +34,14 @@ describe PersonalProjectsFinder do
   describe 'with a current user' do
     subject { finder.execute(current_user) }
 
-    it { is_expected.to eq([private_project, public_project]) }
+    context 'normal user' do
+      it { is_expected.to eq([internal_project, private_project, public_project]) }
+    end
+
+    context 'external' do
+      before { current_user.update_attributes(external: true) }
+
+      it { is_expected.to eq([private_project, public_project]) }
+    end
   end
 end
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index 7dbc5297978..b938a2f0c05 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -1,22 +1,20 @@
 require 'spec_helper'
 
 describe Groups::CreateService, 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!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
 
   describe "execute" do
-    let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC ) }
+    let!(:service) { described_class.new(user, group_params ) }
     subject { service.execute }
 
     context "create groups without restricted visibility level" do
-      it { is_expected.to be_truthy }
+      it { is_expected.to be_persisted }
     end
 
     context "cannot create group with restricted visibility level" do
       before { allow(current_application_settings).to receive(:restricted_visibility_levels).and_return([Gitlab::VisibilityLevel::PUBLIC]) }
-      it { is_expected.to be_falsy }
+      it { is_expected.to_not be_persisted }
     end
   end
 end
diff --git a/spec/support/group_access_helper.rb b/spec/support/group_access_helper.rb
index a1a8fb2bd72..c8ed0e406a1 100644
--- a/spec/support/group_access_helper.rb
+++ b/spec/support/group_access_helper.rb
@@ -14,4 +14,8 @@ module GroupAccessHelper
 
     create(:user).tap { |user| grp.add_user(user, level) }
   end
+
+  def external_guest(grp=group())
+    create(:user, external: true).tap { |user| grp.add_user(user, Gitlab::Access::GUEST) }
+  end
 end