Merge branch 'group-information-leak' into 'master'

Don't leak private group existence by redirecting from namespace controller to group controller.

See merge request !440

5 files changed, 137 insertions, 7 deletions

diff --git a/CHANGELOG b/CHANGELOG
index afc5c456fdf..ae1d2c7bdd2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -50,6 +50,7 @@ v 7.10.0 (unreleased)
   - API: Add pagination to project events
   - Don't show commit comment button when user is not signed in.
   - Fix admin user projects lists.
+  - Don't leak private group existence by redirecting from namespace controller to group controller.
 
 v 7.9.0
   - Send EmailsOnPush email when branch or tag is created or deleted.
diff --git a/app/controllers/namespaces_controller.rb b/app/controllers/namespaces_controller.rb
index b7a9d8c1291..386d103ee5a 100644
--- a/app/controllers/namespaces_controller.rb
+++ b/app/controllers/namespaces_controller.rb
@@ -4,14 +4,22 @@ class NamespacesController < ApplicationController
   def show
     namespace = Namespace.find_by(path: params[:id])
 
-    unless namespace
-      return render_404
+    if namespace
+      if namespace.is_a?(Group)
+        group = namespace
+      else
+        user = namespace.owner
+      end
     end
 
-    if namespace.type == "Group"
-      redirect_to group_path(namespace)
+    if user
+      redirect_to user_path(user)
+    elsif group && can?(current_user, :read_group, group)
+      redirect_to group_path(group)
+    elsif current_user.nil?
+      authenticate_user!
     else
-      redirect_to user_path(namespace.owner)
+      render_404
     end
   end
 end
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 74900d4675d..d96e07034ec 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -52,7 +52,7 @@ module Mentionable
       if identifier == "all"
         users.push(*project.team.members.flatten)
       elsif namespace = Namespace.find_by(path: identifier)
-        if namespace.type == "Group"
+        if namespace.is_a?(Group)
           users.push(*namespace.users)
         else
           users << namespace.owner
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 41bb8d08924..1ff8651e6a3 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -237,7 +237,7 @@ module Gitlab
         link_to("@all", namespace_project_url(project.namespace, project), options)
       elsif namespace = Namespace.find_by(path: identifier)
         url =
-          if namespace.type == "Group"
+          if namespace.is_a?(Group)
             group_url(identifier)
           else 
             user_url(identifier)
diff --git a/spec/controllers/namespaces_controller_spec.rb b/spec/controllers/namespaces_controller_spec.rb
new file mode 100644
index 00000000000..9c8619722cd
--- /dev/null
+++ b/spec/controllers/namespaces_controller_spec.rb
@@ -0,0 +1,121 @@
+require 'spec_helper'
+
+describe NamespacesController do
+  let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+
+  describe "GET show" do
+    context "when the namespace belongs to a user" do
+      let!(:other_user) { create(:user) }
+
+      it "redirects to the user's page" do
+        get :show, id: other_user.username
+
+        expect(response).to redirect_to(user_path(other_user))
+      end
+    end
+
+    context "when the namespace belongs to a group" do
+      let!(:group) { create(:group) }
+      let!(:project) { create(:project, namespace: group) }
+
+      context "when the group has public projects" do
+        before do
+          project.update_attribute(:visibility_level, Project::PUBLIC)
+        end
+
+        context "when not signed in" do
+          it "redirects to the group's page" do
+            get :show, id: group.path
+
+            expect(response).to redirect_to(group_path(group))
+          end
+        end
+
+        context "when signed in" do
+          before do
+            sign_in(user)
+          end
+
+          it "redirects to the group's page" do
+            get :show, id: group.path
+
+            expect(response).to redirect_to(group_path(group))
+          end
+        end
+      end
+
+      context "when the project doesn't have public projects" do
+        context "when not signed in" do
+          it "redirects to the sign in page" do
+            get :show, id: group.path
+
+            expect(response).to redirect_to(new_user_session_path)
+          end
+        end
+
+        context "when signed in" do
+          before do
+            sign_in(user)
+          end
+
+          context "when the user has access to the project" do
+            before do
+              project.team << [user, :master]
+            end
+
+            context "when the user is blocked" do
+              before do
+                user.block
+                project.team << [user, :master]
+              end
+
+              it "redirects to the sign in page" do
+                get :show, id: group.path
+
+                expect(response).to redirect_to(new_user_session_path)
+              end
+            end
+
+            context "when the user isn't blocked" do
+              it "redirects to the group's page" do
+                get :show, id: group.path
+
+                expect(response).to redirect_to(group_path(group))
+              end
+            end
+          end
+
+          context "when the user doesn't have access to the project" do
+            it "responds with status 404" do
+              get :show, id: group.path
+
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+    end
+
+    context "when the namespace doesn't exist" do
+      context "when signed in" do
+        before do
+          sign_in(user)
+        end
+
+        it "responds with status 404" do
+          get :show, id: "doesntexist"
+
+          expect(response.status).to eq(404)
+        end
+      end
+
+      context "when not signed in" do
+        it "redirects to the sign in page" do
+          get :show, id: "doesntexist"
+
+          expect(response).to redirect_to(new_user_session_path)
+        end
+      end
+    end
+  end
+end