diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-10-11 06:58:05 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-10-11 06:58:05 +0000 |
commit | 73adae0f62a3d6048abbee9d076e077185370325 (patch) | |
tree | 8f5e5bcbbd76aa356e50b44d838cb61cac9d89f4 | |
parent | 6f441ae1a10a00007fcb361626f826321b511d90 (diff) | |
parent | c69b81839430500de49d089df2049e778c8b7ef8 (diff) | |
download | gitlab-ce-73adae0f62a3d6048abbee9d076e077185370325.tar.gz |
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/namespaces_controller.rb | 25 | ||||
-rw-r--r-- | config/routes.rb | 2 | ||||
-rw-r--r-- | doc/api/README.md | 13 | ||||
-rw-r--r-- | lib/api/api.rb | 4 | ||||
-rw-r--r-- | spec/controllers/namespaces_controller_spec.rb | 118 | ||||
-rw-r--r-- | spec/requests/api/project_hooks_spec.rb | 5 | ||||
-rw-r--r-- | spec/requests/api/users_spec.rb | 50 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 5 | ||||
-rw-r--r-- | spec/routing/routing_spec.rb | 4 |
10 files changed, 60 insertions, 167 deletions
diff --git a/CHANGELOG b/CHANGELOG index dc06303aecf..06af4e4d6f4 100644 --- a/CHANGELOG +++ b/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) diff --git a/app/controllers/namespaces_controller.rb b/app/controllers/namespaces_controller.rb deleted file mode 100644 index 83eec1bf4a2..00000000000 --- a/app/controllers/namespaces_controller.rb +++ /dev/null @@ -1,25 +0,0 @@ -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 diff --git a/config/routes.rb b/config/routes.rb index bf7c5b76128..83c3a42c19f 100644 --- a/config/routes.rb +++ b/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 diff --git a/doc/api/README.md b/doc/api/README.md index bbd5bcfb386..9e907689c80 100644 --- a/doc/api/README.md +++ b/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 diff --git a/lib/api/api.rb b/lib/api/api.rb index 0bbf73a1b63..99722a0a65c 100644 --- a/lib/api/api.rb +++ b/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 diff --git a/spec/controllers/namespaces_controller_spec.rb b/spec/controllers/namespaces_controller_spec.rb deleted file mode 100644 index 2b334ed1172..00000000000 --- a/spec/controllers/namespaces_controller_spec.rb +++ /dev/null @@ -1,118 +0,0 @@ -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 diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 765dc8a8f66..cfcdcad74cd 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/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 diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index f4ea3bebb4c..b002949b41b 100644 --- a/spec/requests/api/users_spec.rb +++ b/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 diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index c0c1e62e910..413d06715b3 100644 --- a/spec/requests/git_http_spec.rb +++ b/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 diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 0dd00af878d..0ee1c811dfb 100644 --- a/spec/routing/routing_spec.rb +++ b/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 |