Code fixes

8 files changed, 96 insertions, 24 deletions

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