Merge branch 'fix/unexistent-group-500' into 'master'

Return a 404 instead of a 500 when accessing a non-existent group

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/3352

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/5600

See merge request !2614

4 files changed, 10 insertions, 0 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4b283f235a4..e6fb2e9f508 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,7 @@ v 8.5.0 (unreleased)
   - Fix diff comments loaded by AJAX to load comment with diff in discussion tab
   - Whitelist raw "abbr" elements when parsing Markdown (Benedict Etzel)
   - Don't vendor minified JS
+  - Display 404 error on group not found
   - Track project import failure
   - Fix visibility level text in admin area (Zeger-Jan van de Weg)
   - Update the ExternalIssue regex pattern (Blake Hitchcock)
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index f7c9e619755..632145c2b9b 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -82,6 +82,7 @@ class GroupsController < Groups::ApplicationController
 
   def group
     @group ||= Group.find_by(path: params[:id])
+    @group || render_404
   end
 
   def load_projects
diff --git a/features/groups.feature b/features/groups.feature
index c803e952980..55fffb012ae 100644
--- a/features/groups.feature
+++ b/features/groups.feature
@@ -3,6 +3,10 @@ Feature: Groups
     Given I sign in as "John Doe"
     And "John Doe" is owner of group "Owned"
 
+  Scenario: I should not see a group if it does not exist
+    When I visit group "NonExistentGroup" page
+    Then page status code should be 404
+
   Scenario: I should have back to group button
     When I visit group "Owned" page
     Then I should see back to dashboard button
diff --git a/features/steps/groups.rb b/features/steps/groups.rb
index 4c5122d1b7d..1e2a78a6029 100644
--- a/features/steps/groups.rb
+++ b/features/steps/groups.rb
@@ -120,6 +120,10 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
     expect(page).to have_xpath("//span[@class='label label-warning']", text: 'archived')
   end
 
+  step 'I visit group "NonExistentGroup" page' do
+    visit group_path(-1)
+  end
+
   private
 
   def assigned_to_me(key)