Merge branch 'dz-cleanup-routing' into 'master'

Remove NamespacesController

* removes unnecessary NamespacesController. The main purpose of this controller was redirect to group or user page when URL like https://gitlab.com/gitlab-org was used. Now this functionality is handled by constrainers (like this https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/routes/user.rb#L17-21) and take user to correct controller right from the start.
* serve non existing API routes like `/api/v3/whatever` with Grape instead of Rails. Before this change wrong API url was served by rails with not obvious 404, 405 & 500 errors


See merge request !6733

CHANGELOG

@@ -87,6 +87,7 @@ v 8.13.0 (unreleased)
   - Grouped pipeline dropdown is a scrollable container
   - Cleanup Ci::ApplicationController. !6757 (Takuya Noguchi)
   - Fix a typo in doc/api/labels.md
+  - API: all unknown routing will be handled with 404 Not Found
 
 v 8.12.5 (unreleased)
 

app/controllers/namespaces_controller.rb

-class NamespacesController < ApplicationController
-  skip_before_action :authenticate_user!
-
-  def show
-    namespace = Namespace.find_by(path: params[:id])
-
-    if namespace
-      if namespace.is_a?(Group)
-        group = namespace
-      else
-        user = namespace.owner
-      end
-    end
-
-    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
-      render_404
-    end
-  end
-end

config/routes.rb

@@ -87,7 +87,5 @@ Rails.application.routes.draw do
   # Get all keys of user
   get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: /.*/ }
 
-  get ':id' => 'namespaces#show', constraints: { id: /(?:[^.]|\.(?!atom$))+/, format: /atom/ }
-
   root to: "root#index"
 end

doc/api/README.md

@@ -355,6 +355,19 @@ follows:
 }
 ```
 
+## Unknown route
+
+When you try to access an API URL that does not exist you will receive 404 Not Found.
+
+```
+HTTP/1.1 404 Not Found
+Content-Type: application/json
+{
+    "error": "404 Not Found"
+}
+```
+
+
 ## Clients
 
 There are many unofficial GitLab API Clients for most of the popular

lib/api/api.rb

@@ -73,5 +73,9 @@ module API
     mount ::API::Triggers
     mount ::API::Users
     mount ::API::Variables
+
+    route :any, '*path' do
+      error!('404 Not Found', 404)
+    end
   end
 end

spec/controllers/namespaces_controller_spec.rb

-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) }
-
-      context "when the group is public" do
-        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 group is private" do
-        before do
-          group.update_attribute(:visibility_level, Group::PRIVATE)
-        end
-
-        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 group" do
-            before do
-              group.add_developer(user)
-            end
-
-            context "when the user is blocked" do
-              before do
-                user.block
-              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 group" do
-            it "responds with status 404" do
-              get :show, id: group.path
-
-              expect(response).to have_http_status(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).to have_http_status(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

spec/requests/api/project_hooks_spec.rb

@@ -163,9 +163,10 @@ describe API::API, 'ProjectHooks', api: true do
       expect(response).to have_http_status(404)
     end
 
-    it "returns a 405 error if hook id not given" do
+    it "returns a 404 error if hook id not given" do
       delete api("/projects/#{project.id}/hooks", user)
-      expect(response).to have_http_status(405)
+
+      expect(response).to have_http_status(404)
     end
 
     it "returns a 404 if a user attempts to delete project hooks he/she does not own" do

spec/requests/api/users_spec.rb

@@ -90,8 +90,9 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('404 Not found')
     end
 
-    it "returns a 404 if invalid ID" do
+    it "returns a 404 for invalid ID" do
       get api("/users/1ASDF", user)
+
       expect(response).to have_http_status(404)
     end
   end
@@ -340,8 +341,10 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('404 Not found')
     end
 
-    it "raises error for invalid ID" do
-      expect{put api("/users/ASDF", admin) }.to raise_error(ActionController::RoutingError)
+    it "returns a 404 if invalid ID" do
+      put api("/users/ASDF", admin)
+
+      expect(response).to have_http_status(404)
     end
 
     it 'returns 400 error if user does not validate' do
@@ -493,8 +496,9 @@ describe API::API, api: true  do
       end.to change{ user.emails.count }.by(1)
     end
 
-    it "raises error for invalid ID" do
+    it "returns a 400 for invalid ID" do
       post api("/users/999999/emails", admin)
+
       expect(response).to have_http_status(400)
     end
   end
@@ -525,9 +529,10 @@ describe API::API, api: true  do
         expect(json_response.first['email']).to eq(email.email)
       end
 
-      it "raises error for invalid ID" do
+      it "returns a 404 for invalid ID" do
         put api("/users/ASDF/emails", admin)
-        expect(response).to have_http_status(405)
+
+        expect(response).to have_http_status(404)
       end
     end
   end
@@ -566,8 +571,10 @@ describe API::API, api: true  do
         expect(json_response['message']).to eq('404 Email Not Found')
       end
 
-      it "raises error for invalid ID" do
-        expect{delete api("/users/ASDF/emails/bar", admin) }.to raise_error(ActionController::RoutingError)
+      it "returns a 404 for invalid ID" do
+        delete api("/users/ASDF/emails/bar", admin)
+
+        expect(response).to have_http_status(404)
       end
     end
   end
@@ -600,8 +607,10 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('404 User Not Found')
     end
 
-    it "raises error for invalid ID" do
-      expect{delete api("/users/ASDF", admin) }.to raise_error(ActionController::RoutingError)
+    it "returns a 404 for invalid ID" do
+      delete api("/users/ASDF", admin)
+
+      expect(response).to have_http_status(404)
     end
   end
 
@@ -654,6 +663,7 @@ describe API::API, api: true  do
 
     it "returns 404 Not Found within invalid ID" do
       get api("/user/keys/42", user)
+
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 Not found')
     end
@@ -669,6 +679,7 @@ describe API::API, api: true  do
 
     it "returns 404 for invalid ID" do
       get api("/users/keys/ASDF", admin)
+
       expect(response).to have_http_status(404)
     end
   end
@@ -727,8 +738,10 @@ describe API::API, api: true  do
       expect(response).to have_http_status(401)
     end
 
-    it "raises error for invalid ID" do
-      expect{delete api("/users/keys/ASDF", admin) }.to raise_error(ActionController::RoutingError)
+    it "returns a 404 for invalid ID" do
+      delete api("/users/keys/ASDF", admin)
+
+      expect(response).to have_http_status(404)
     end
   end
 
@@ -778,6 +791,7 @@ describe API::API, api: true  do
 
     it "returns 404 for invalid ID" do
       get api("/users/emails/ASDF", admin)
+
       expect(response).to have_http_status(404)
     end
   end
@@ -825,8 +839,10 @@ describe API::API, api: true  do
       expect(response).to have_http_status(401)
     end
 
-    it "raises error for invalid ID" do
-      expect{delete api("/users/emails/ASDF", admin) }.to raise_error(ActionController::RoutingError)
+    it "returns a 404 for invalid ID" do
+      delete api("/users/emails/ASDF", admin)
+
+      expect(response).to have_http_status(404)
     end
   end
 
@@ -891,8 +907,10 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('404 User Not Found')
     end
 
-    it "raises error for invalid ID" do
-      expect{put api("/users/ASDF/block", admin) }.to raise_error(ActionController::RoutingError)
+    it "returns a 404 for invalid ID" do
+      put api("/users/ASDF/block", admin)
+
+      expect(response).to have_http_status(404)
     end
   end
 end

spec/requests/git_http_spec.rb

@@ -412,10 +412,9 @@ describe 'Git HTTP requests', lib: true do
 
         context "when the params are anything else" do
           let(:params) { { service: 'git-implode-pack' } }
-          before { get path, params }
 
-          it "redirects to the sign-in page" do
-            expect(response).to redirect_to(new_user_session_path)
+          it "fails to find a route" do
+            expect { get(path, params) }.to raise_error(ActionController::RoutingError)
           end
         end
       end

spec/routing/routing_spec.rb

@@ -266,7 +266,9 @@ describe "Groups", "routing" do
   end
 
   it "also display group#show on the short path" do
-    expect(get('/1')).to route_to('namespaces#show', id: '1')
+    allow(Group).to receive(:find_by_path).and_return(true)
+
+    expect(get('/1')).to route_to('groups#show', id: '1')
   end
 end