summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Kozono <mkozono@gmail.com>2017-05-01 13:46:30 -0700
committerMichael Kozono <mkozono@gmail.com>2017-05-05 12:11:57 -0700
commit7d02bcd2e0165a90a9f2c1edb34b064ff76afd69 (patch)
tree3b74cbbf74fca3b36effa5ea4b33d8bd29e22f73
parentf4a2dfb46f168d3fd7309aca8631cf680456fa82 (diff)
downloadgitlab-ce-7d02bcd2e0165a90a9f2c1edb34b064ff76afd69.tar.gz
Redirect from redirect routes to canonical routes
-rw-r--r--app/controllers/application_controller.rb2
-rw-r--r--app/controllers/concerns/routable_actions.rb11
-rw-r--r--app/controllers/groups/application_controller.rb21
-rw-r--r--app/controllers/groups_controller.rb2
-rw-r--r--app/controllers/projects/application_controller.rb14
-rw-r--r--app/controllers/users_controller.rb12
-rw-r--r--app/models/concerns/routable.rb24
-rw-r--r--app/models/redirect_route.rb10
-rw-r--r--app/models/user.rb5
-rw-r--r--db/migrate/20170427215854_create_redirect_routes.rb15
-rw-r--r--db/schema.rb8
-rw-r--r--lib/constraints/group_url_constrainer.rb2
-rw-r--r--lib/constraints/project_url_constrainer.rb2
-rw-r--r--lib/constraints/user_url_constrainer.rb2
-rw-r--r--spec/controllers/application_controller_spec.rb5
-rw-r--r--spec/controllers/groups_controller_spec.rb92
-rw-r--r--spec/controllers/projects_controller_spec.rb84
-rw-r--r--spec/controllers/users_controller_spec.rb181
-rw-r--r--spec/features/groups/group_settings_spec.rb3
-rw-r--r--spec/features/profiles/account_spec.rb5
-rw-r--r--spec/features/projects/features_visibility_spec.rb37
-rw-r--r--spec/features/projects/project_settings_spec.rb10
-rw-r--r--spec/lib/constraints/group_url_constrainer_spec.rb32
-rw-r--r--spec/lib/constraints/project_url_constrainer_spec.rb21
-rw-r--r--spec/lib/constraints/user_url_constrainer_spec.rb21
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml1
-rw-r--r--spec/models/concerns/routable_spec.rb52
-rw-r--r--spec/models/redirect_route_spec.rb16
-rw-r--r--spec/models/user_spec.rb59
-rw-r--r--spec/routing/project_routing_spec.rb6
30 files changed, 668 insertions, 87 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index d2c13da6917..65a1f640a76 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -58,7 +58,7 @@ class ApplicationController < ActionController::Base
if current_user
not_found
else
- redirect_to new_user_session_path
+ authenticate_user!
end
end
diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
new file mode 100644
index 00000000000..6f16377a156
--- /dev/null
+++ b/app/controllers/concerns/routable_actions.rb
@@ -0,0 +1,11 @@
+module RoutableActions
+ extend ActiveSupport::Concern
+
+ def ensure_canonical_path(routable, requested_path)
+ return unless request.get?
+
+ if routable.full_path != requested_path
+ redirect_to request.original_url.sub(requested_path, routable.full_path)
+ end
+ end
+end
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index 29ffaeb19c1..209d8b1a08a 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -1,4 +1,6 @@
class Groups::ApplicationController < ApplicationController
+ include RoutableActions
+
layout 'group'
skip_before_action :authenticate_user!
@@ -8,18 +10,15 @@ class Groups::ApplicationController < ApplicationController
def group
unless @group
- id = params[:group_id] || params[:id]
- @group = Group.find_by_full_path(id)
- @group_merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id).execute
+ given_path = params[:group_id] || params[:id]
+ @group = Group.find_by_full_path(given_path, follow_redirects: request.get?)
- unless @group && can?(current_user, :read_group, @group)
+ if @group && can?(current_user, :read_group, @group)
+ ensure_canonical_path(@group, given_path)
+ else
@group = nil
- if current_user.nil?
- authenticate_user!
- else
- render_404
- end
+ route_not_found
end
end
@@ -30,6 +29,10 @@ class Groups::ApplicationController < ApplicationController
@projects ||= GroupProjectsFinder.new(group: group, current_user: current_user).execute
end
+ def group_merge_requests
+ @group_merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id).execute
+ end
+
def authorize_admin_group!
unless can?(current_user, :admin_group, group)
return render_404
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 593001e6396..46c3ff10694 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -12,8 +12,8 @@ class GroupsController < Groups::ApplicationController
before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
before_action :authorize_create_group!, only: [:new, :create]
- # Load group projects
before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests]
+ before_action :group_merge_requests, only: [:merge_requests]
before_action :event_filter, only: [:activity]
before_action :user_actions, only: [:show, :subgroups]
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 89f1128ec36..dbdf68776f1 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -1,4 +1,6 @@
class Projects::ApplicationController < ApplicationController
+ include RoutableActions
+
skip_before_action :authenticate_user!
before_action :project
before_action :repository
@@ -24,20 +26,14 @@ class Projects::ApplicationController < ApplicationController
end
project_path = "#{namespace}/#{id}"
- @project = Project.find_by_full_path(project_path)
+ @project = Project.find_by_full_path(project_path, follow_redirects: request.get?)
if can?(current_user, :read_project, @project) && !@project.pending_delete?
- if @project.path_with_namespace != project_path
- redirect_to request.original_url.gsub(project_path, @project.path_with_namespace)
- end
+ ensure_canonical_path(@project, project_path)
else
@project = nil
- if current_user.nil?
- authenticate_user!
- else
- render_404
- end
+ route_not_found
end
end
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index a452bbba422..6b6b3b20a8d 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1,7 +1,9 @@
class UsersController < ApplicationController
+ include RoutableActions
+
skip_before_action :authenticate_user!
before_action :user, except: [:exists]
- before_action :authorize_read_user!, only: [:show]
+ before_action :authorize_read_user!, except: [:exists]
def show
respond_to do |format|
@@ -92,11 +94,15 @@ class UsersController < ApplicationController
private
def authorize_read_user!
- render_404 unless can?(current_user, :read_user, user)
+ if can?(current_user, :read_user, user)
+ ensure_canonical_path(user.namespace, params[:username])
+ else
+ render_404
+ end
end
def user
- @user ||= User.find_by_username!(params[:username])
+ @user ||= User.find_by_full_path(params[:username], follow_redirects: true)
end
def contributed_projects
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index b28e05d0c28..e351dbb45dd 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -5,6 +5,7 @@ module Routable
included do
has_one :route, as: :source, autosave: true, dependent: :destroy
+ has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy
validates_associated :route
validates :route, presence: true
@@ -26,16 +27,31 @@ module Routable
# Klass.find_by_full_path('gitlab-org/gitlab-ce')
#
# Returns a single object, or nil.
- def find_by_full_path(path)
+ def find_by_full_path(path, follow_redirects: false)
# On MySQL we want to ensure the ORDER BY uses a case-sensitive match so
# any literal matches come first, for this we have to use "BINARY".
# Without this there's still no guarantee in what order MySQL will return
# rows.
+ #
+ # Why do we do this?
+ #
+ # Even though we have Rails validation on Route for unique paths
+ # (case-insensitive), there are old projects in our DB (and possibly
+ # clients' DBs) that have the same path with different cases.
+ # See https://gitlab.com/gitlab-org/gitlab-ce/issues/18603. Also note that
+ # our unique index is case-sensitive in Postgres.
binary = Gitlab::Database.mysql? ? 'BINARY' : ''
-
order_sql = "(CASE WHEN #{binary} routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)"
-
- where_full_path_in([path]).reorder(order_sql).take
+ found = where_full_path_in([path]).reorder(order_sql).take
+ return found if found
+
+ if follow_redirects
+ if Gitlab::Database.postgresql?
+ joins(:redirect_routes).find_by("LOWER(redirect_routes.path) = LOWER(?)", path)
+ else
+ joins(:redirect_routes).find_by(path: path)
+ end
+ end
end
# Builds a relation to find multiple objects by their full paths.
diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb
new file mode 100644
index 00000000000..e36ca988701
--- /dev/null
+++ b/app/models/redirect_route.rb
@@ -0,0 +1,10 @@
+class RedirectRoute < ActiveRecord::Base
+ belongs_to :source, polymorphic: true
+
+ validates :source, presence: true
+
+ validates :path,
+ length: { within: 1..255 },
+ presence: true,
+ uniqueness: { case_sensitive: false }
+end
diff --git a/app/models/user.rb b/app/models/user.rb
index 43c5fdc038d..c6354b989ad 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -333,6 +333,11 @@ class User < ActiveRecord::Base
find_by(id: Key.unscoped.select(:user_id).where(id: key_id))
end
+ def find_by_full_path(path, follow_redirects: false)
+ namespace = Namespace.find_by_full_path(path, follow_redirects: follow_redirects)
+ namespace.owner if namespace && namespace.owner
+ end
+
def reference_prefix
'@'
end
diff --git a/db/migrate/20170427215854_create_redirect_routes.rb b/db/migrate/20170427215854_create_redirect_routes.rb
new file mode 100644
index 00000000000..57b79a0267f
--- /dev/null
+++ b/db/migrate/20170427215854_create_redirect_routes.rb
@@ -0,0 +1,15 @@
+class CreateRedirectRoutes < ActiveRecord::Migration
+
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ def change
+ create_table :redirect_routes do |t|
+ t.integer :source_id, null: false
+ t.string :source_type, null: false
+ t.string :path, null: false
+
+ t.timestamps null: false
+ end
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 588e2082ae0..3a8970b2235 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1040,6 +1040,14 @@ ActiveRecord::Schema.define(version: 20170504102911) do
add_index "protected_tags", ["project_id"], name: "index_protected_tags_on_project_id", using: :btree
+ create_table "redirect_routes", force: :cascade do |t|
+ t.integer "source_id", null: false
+ t.string "source_type", null: false
+ t.string "path", null: false
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
+ end
+
create_table "releases", force: :cascade do |t|
t.string "tag"
t.text "description"
diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb
index 1501f64d537..5f379756c11 100644
--- a/lib/constraints/group_url_constrainer.rb
+++ b/lib/constraints/group_url_constrainer.rb
@@ -4,6 +4,6 @@ class GroupUrlConstrainer
return false unless DynamicPathValidator.valid?(id)
- Group.find_by_full_path(id).present?
+ Group.find_by_full_path(id, follow_redirects: request.get?).present?
end
end
diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb
index d0ce2caffff..6f542f63f98 100644
--- a/lib/constraints/project_url_constrainer.rb
+++ b/lib/constraints/project_url_constrainer.rb
@@ -6,6 +6,6 @@ class ProjectUrlConstrainer
return false unless DynamicPathValidator.valid?(full_path)
- Project.find_by_full_path(full_path).present?
+ Project.find_by_full_path(full_path, follow_redirects: request.get?).present?
end
end
diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb
index 9ab5bcb12ff..28159dc0dec 100644
--- a/lib/constraints/user_url_constrainer.rb
+++ b/lib/constraints/user_url_constrainer.rb
@@ -1,5 +1,5 @@
class UserUrlConstrainer
def matches?(request)
- User.find_by_username(request.params[:username]).present?
+ User.find_by_full_path(request.params[:username], follow_redirects: request.get?).present?
end
end
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 1bf0533ca24..d40aae04fc3 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -106,10 +106,9 @@ describe ApplicationController do
controller.send(:route_not_found)
end
- it 'does redirect to login page if not authenticated' do
+ it 'does redirect to login page via authenticate_user! if not authenticated' do
allow(controller).to receive(:current_user).and_return(nil)
- expect(controller).to receive(:redirect_to)
- expect(controller).to receive(:new_user_session_path)
+ expect(controller).to receive(:authenticate_user!)
controller.send(:route_not_found)
end
end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index cad82a34fb0..3398878f330 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -49,6 +49,24 @@ describe GroupsController do
expect(assigns(:issues)).to eq [issue_2, issue_1]
end
end
+
+ context 'when requesting the canonical path with different casing' do
+ it 'redirects to the correct casing' do
+ get :issues, id: group.to_param.upcase
+
+ expect(response).to redirect_to(issues_group_path(group.to_param))
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+
+ it 'redirects to the canonical path' do
+ get :issues, id: redirect_route.path
+
+ expect(response).to redirect_to(issues_group_path(group.to_param))
+ end
+ end
end
describe 'GET #merge_requests' do
@@ -74,6 +92,24 @@ describe GroupsController do
expect(assigns(:merge_requests)).to eq [merge_request_2, merge_request_1]
end
end
+
+ context 'when requesting the canonical path with different casing' do
+ it 'redirects to the correct casing' do
+ get :merge_requests, id: group.to_param.upcase
+
+ expect(response).to redirect_to(merge_requests_group_path(group.to_param))
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+
+ it 'redirects to the canonical path' do
+ get :merge_requests, id: redirect_route.path
+
+ expect(response).to redirect_to(merge_requests_group_path(group.to_param))
+ end
+ end
end
describe 'DELETE #destroy' do
@@ -81,7 +117,7 @@ describe GroupsController do
it 'returns 404' do
sign_in(create(:user))
- delete :destroy, id: group.path
+ delete :destroy, id: group.to_param
expect(response.status).to eq(404)
end
@@ -94,15 +130,39 @@ describe GroupsController do
it 'schedules a group destroy' do
Sidekiq::Testing.fake! do
- expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+ expect { delete :destroy, id: group.to_param }.to change(GroupDestroyWorker.jobs, :size).by(1)
end
end
it 'redirects to the root path' do
- delete :destroy, id: group.path
+ delete :destroy, id: group.to_param
expect(response).to redirect_to(root_path)
end
+
+ context 'when requesting the canonical path with different casing' do
+ it 'does not 404' do
+ delete :destroy, id: group.to_param.upcase
+
+ expect(response).to_not have_http_status(404)
+ end
+
+ it 'does not redirect to the correct casing' do
+ delete :destroy, id: group.to_param.upcase
+
+ expect(response).to_not redirect_to(group_path(group.to_param))
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+
+ it 'returns not found' do
+ delete :destroy, id: redirect_route.path
+
+ expect(response).to have_http_status(404)
+ end
+ end
end
end
@@ -111,7 +171,7 @@ describe GroupsController do
sign_in(user)
end
- it 'updates the path succesfully' do
+ it 'updates the path successfully' do
post :update, id: group.to_param, group: { path: 'new_path' }
expect(response).to have_http_status(302)
@@ -125,5 +185,29 @@ describe GroupsController do
expect(assigns(:group).errors).not_to be_empty
expect(assigns(:group).path).not_to eq('new_path')
end
+
+ context 'when requesting the canonical path with different casing' do
+ it 'does not 404' do
+ post :update, id: group.to_param.upcase, group: { path: 'new_path' }
+
+ expect(response).to_not have_http_status(404)
+ end
+
+ it 'does not redirect to the correct casing' do
+ post :update, id: group.to_param.upcase, group: { path: 'new_path' }
+
+ expect(response).to_not redirect_to(group_path(group.to_param))
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+
+ it 'returns not found' do
+ post :update, id: redirect_route.path, group: { path: 'new_path' }
+
+ expect(response).to have_http_status(404)
+ end
+ end
end
end
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index eafc2154568..1b0dd7c6369 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -218,19 +218,32 @@ describe ProjectsController do
expect(response).to redirect_to(namespace_project_path)
end
end
+
+ context 'when requesting a redirected path' do
+ let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
+
+ it 'redirects to the canonical path' do
+ get :show, namespace_id: 'foo', id: 'bar'
+
+ expect(response).to redirect_to(public_project)
+ end
+ end
end
describe "#update" do
render_views
let(:admin) { create(:admin) }
+ let(:project) { create(:project, :repository) }
+ let(:new_path) { 'renamed_path' }
+ let(:project_params) { { path: new_path } }
+
+ before do
+ sign_in(admin)
+ end
it "sets the repository to the right path after a rename" do
- project = create(:project, :repository)
- new_path = 'renamed_path'
- project_params = { path: new_path }
controller.instance_variable_set(:@project, project)
- sign_in(admin)
put :update,
namespace_id: project.namespace,
@@ -241,6 +254,34 @@ describe ProjectsController do
expect(assigns(:repository).path).to eq(project.repository.path)
expect(response).to have_http_status(302)
end
+
+ context 'when requesting the canonical path' do
+ it "is case-insensitive" do
+ controller.instance_variable_set(:@project, project)
+
+ put :update,
+ namespace_id: 'FOo',
+ id: 'baR',
+ project: project_params
+
+ expect(project.repository.path).to include(new_path)
+ expect(assigns(:repository).path).to eq(project.repository.path)
+ expect(response).to have_http_status(302)
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
+
+ it 'returns not found' do
+ put :update,
+ namespace_id: 'foo',
+ id: 'bar',
+ project: project_params
+
+ expect(response).to have_http_status(404)
+ end
+ end
end
describe "#destroy" do
@@ -276,6 +317,31 @@ describe ProjectsController do
expect(merge_request.reload.state).to eq('closed')
end
end
+
+ context 'when requesting the canonical path' do
+ it "is case-insensitive" do
+ controller.instance_variable_set(:@project, project)
+ sign_in(admin)
+
+ orig_id = project.id
+ delete :destroy, namespace_id: project.namespace, id: project.path.upcase
+
+ expect { Project.find(orig_id) }.to raise_error(ActiveRecord::RecordNotFound)
+ expect(response).to have_http_status(302)
+ expect(response).to redirect_to(dashboard_projects_path)
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
+
+ it 'returns not found' do
+ sign_in(admin)
+ delete :destroy, namespace_id: 'foo', id: 'bar'
+
+ expect(response).to have_http_status(404)
+ end
+ end
end
describe 'PUT #new_issue_address' do
@@ -397,6 +463,16 @@ describe ProjectsController do
expect(parsed_body["Tags"]).to include("v1.0.0")
expect(parsed_body["Commits"]).to include("123456")
end
+
+ context 'when requesting a redirected path' do
+ let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
+
+ it 'redirects to the canonical path' do
+ get :refs, namespace_id: 'foo', id: 'bar'
+
+ expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project))
+ end
+ end
end
describe 'POST #preview_markdown' do
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index bbe9aaf737f..73fd5b6f90d 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -4,15 +4,6 @@ describe UsersController do
let(:user) { create(:user) }
describe 'GET #show' do
- it 'is case-insensitive' do
- user = create(:user, username: 'CamelCaseUser')
- sign_in(user)
-
- get :show, username: user.username.downcase
-
- expect(response).to be_success
- end
-
context 'with rendered views' do
render_views
@@ -61,6 +52,38 @@ describe UsersController do
end
end
end
+
+ context 'when requesting the canonical path' do
+ let(:user) { create(:user, username: 'CamelCaseUser') }
+
+ before { sign_in(user) }
+
+ context 'with exactly matching casing' do
+ it 'responds with success' do
+ get :show, username: user.username
+
+ expect(response).to be_success
+ end
+ end
+
+ context 'with different casing' do
+ it 'redirects to the correct casing' do
+ get :show, username: user.username.downcase
+
+ expect(response).to redirect_to(user)
+ end
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
+
+ it 'redirects to the canonical path' do
+ get :show, username: redirect_route.path
+
+ expect(response).to redirect_to(user)
+ end
+ end
end
describe 'GET #calendar' do
@@ -88,11 +111,43 @@ describe UsersController do
expect(assigns(:contributions_calendar).projects.count).to eq(2)
end
end
+
+ context 'when requesting the canonical path' do
+ let(:user) { create(:user, username: 'CamelCaseUser') }
+
+ before { sign_in(user) }
+
+ context 'with exactly matching casing' do
+ it 'responds with success' do
+ get :calendar, username: user.username
+
+ expect(response).to be_success
+ end
+ end
+
+ context 'with different casing' do
+ it 'redirects to the correct casing' do
+ get :calendar, username: user.username.downcase
+
+ expect(response).to redirect_to(user_calendar_path(user))
+ end
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
+
+ it 'redirects to the canonical path' do
+ get :calendar, username: redirect_route.path
+
+ expect(response).to redirect_to(user_calendar_path(user))
+ end
+ end
end
describe 'GET #calendar_activities' do
let!(:project) { create(:empty_project) }
- let!(:user) { create(:user) }
+ let(:user) { create(:user) }
before do
allow_any_instance_of(User).to receive(:contributed_projects_ids).and_return([project.id])
@@ -110,6 +165,36 @@ describe UsersController do
get :calendar_activities, username: user.username
expect(response).to render_template('calendar_activities')
end
+
+ context 'when requesting the canonical path' do
+ let(:user) { create(:user, username: 'CamelCaseUser') }
+
+ context 'with exactly matching casing' do
+ it 'responds with success' do
+ get :calendar_activities, username: user.username
+
+ expect(response).to be_success
+ end
+ end
+
+ context 'with different casing' do
+ it 'redirects to the correct casing' do
+ get :calendar_activities, username: user.username.downcase
+
+ expect(response).to redirect_to(user_calendar_activities_path(user))
+ end
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
+
+ it 'redirects to the canonical path' do
+ get :calendar_activities, username: redirect_route.path
+
+ expect(response).to redirect_to(user_calendar_activities_path(user))
+ end
+ end
end
describe 'GET #snippets' do
@@ -132,5 +217,81 @@ describe UsersController do
expect(JSON.parse(response.body)).to have_key('html')
end
end
+
+ context 'when requesting the canonical path' do
+ let(:user) { create(:user, username: 'CamelCaseUser') }
+
+ context 'with exactly matching casing' do
+ it 'responds with success' do
+ get :snippets, username: user.username
+
+ expect(response).to be_success
+ end
+ end
+
+ context 'with different casing' do
+ it 'redirects to the correct casing' do
+ get :snippets, username: user.username.downcase
+
+ expect(response).to redirect_to(user_snippets_path(user))
+ end
+ end
+ end
+
+ context 'when requesting a redirected path' do
+ let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
+
+ it 'redirects to the canonical path' do
+ get :snippets, username: redirect_route.path
+
+ expect(response).to redirect_to(user_snippets_path(user))
+ end
+ end
+ end
+
+ describe 'GET #exists' do
+ before do
+ sign_in(user)
+ end
+
+ context 'when user exists' do
+ it 'returns JSON indicating the user exists' do
+ get :exists, username: user.username
+
+ expected_json = { exists: true }.to_json
+ expect(response.body).to eq(expected_json)
+ end
+
+ context 'when the casing is different' do
+ let(:user) { create(:user, username: 'CamelCaseUser') }
+
+ it 'returns JSON indicating the user exists' do
+ get :exists, username: user.username.downcase
+
+ expected_json = { exists: true }.to_json
+ expect(response.body).to eq(expected_json)
+ end
+ end
+ end
+
+ context 'when the user does not exist' do
+ it 'returns JSON indicating the user does not exist' do
+ get :exists, username: 'foo'
+
+ expected_json = { exists: false }.to_json
+ expect(response.body).to eq(expected_json)
+ end
+
+ context 'when a user changed their username' do
+ let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
+
+ it 'returns JSON indicating a user by that username does not exist' do
+ get :exists, username: 'old-username'
+
+ expected_json = { exists: false }.to_json
+ expect(response.body).to eq(expected_json)
+ end
+ end
+ end
end
end
diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb
index f18f2f2310f..cc25db4ad60 100644
--- a/spec/features/groups/group_settings_spec.rb
+++ b/spec/features/groups/group_settings_spec.rb
@@ -52,6 +52,9 @@ feature 'Edit group settings', feature: true do
given!(:project) { create(:project, group: group, path: 'project') }
given(:old_project_full_path) { "/#{group.path}/#{project.path}" }
given(:new_project_full_path) { "/#{new_group_path}/#{project.path}" }
+
+ before(:context) { TestEnv.clean_test_path }
+ after(:example) { TestEnv.clean_test_path }
scenario 'the project is accessible via the new path' do
update_path(new_group_path)
diff --git a/spec/features/profiles/account_spec.rb b/spec/features/profiles/account_spec.rb
index b3902a6b37f..05a7587f8d4 100644
--- a/spec/features/profiles/account_spec.rb
+++ b/spec/features/profiles/account_spec.rb
@@ -31,9 +31,8 @@ feature 'Profile > Account', feature: true do
given(:new_project_path) { "/#{new_username}/#{project.path}" }
given(:old_project_path) { "/#{user.username}/#{project.path}" }
- after do
- TestEnv.clean_test_path
- end
+ before(:context) { TestEnv.clean_test_path }
+ after(:example) { TestEnv.clean_test_path }
scenario 'the project is accessible via the new path' do
update_username(new_username)
diff --git a/spec/features/projects/features_visibility_spec.rb b/spec/features/projects/features_visibility_spec.rb
index b080a8d500e..a6ca0f4131a 100644
--- a/spec/features/projects/features_visibility_spec.rb
+++ b/spec/features/projects/features_visibility_spec.rb
@@ -68,20 +68,23 @@ describe 'Edit Project Settings', feature: true do
end
describe 'project features visibility pages' do
- before do
- @tools =
- {
- builds: namespace_project_pipelines_path(project.namespace, project),
- issues: namespace_project_issues_path(project.namespace, project),
- wiki: namespace_project_wiki_path(project.namespace, project, :home),
- snippets: namespace_project_snippets_path(project.namespace, project),
- merge_requests: namespace_project_merge_requests_path(project.namespace, project),
- }
- end
+ let(:tools) {
+ {
+ builds: namespace_project_pipelines_path(project.namespace, project),
+ issues: namespace_project_issues_path(project.namespace, project),
+ wiki: namespace_project_wiki_path(project.namespace, project, :home),
+ snippets: namespace_project_snippets_path(project.namespace, project),
+ merge_requests: namespace_project_merge_requests_path(project.namespace, project),
+ }
+ }
context 'normal user' do
+ before do
+ login_as(member)
+ end
+
it 'renders 200 if tool is enabled' do
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::ENABLED)
visit url
expect(page.status_code).to eq(200)
@@ -89,7 +92,7 @@ describe 'Edit Project Settings', feature: true do
end
it 'renders 404 if feature is disabled' do
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::DISABLED)
visit url
expect(page.status_code).to eq(404)
@@ -99,21 +102,21 @@ describe 'Edit Project Settings', feature: true do
it 'renders 404 if feature is enabled only for team members' do
project.team.truncate
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE)
visit url
expect(page.status_code).to eq(404)
end
end
- it 'renders 200 if users is member of group' do
+ it 'renders 200 if user is member of group' do
group = create(:group)
project.group = group
project.save
group.add_owner(member)
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE)
visit url
expect(page.status_code).to eq(200)
@@ -128,7 +131,7 @@ describe 'Edit Project Settings', feature: true do
end
it 'renders 404 if feature is disabled' do
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::DISABLED)
visit url
expect(page.status_code).to eq(404)
@@ -138,7 +141,7 @@ describe 'Edit Project Settings', feature: true do
it 'renders 200 if feature is enabled only for team members' do
project.team.truncate
- @tools.each do |method_name, url|
+ tools.each do |method_name, url|
project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE)
visit url
expect(page.status_code).to eq(200)
diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb
index 364cc8ef0f2..c0311e73ef5 100644
--- a/spec/features/projects/project_settings_spec.rb
+++ b/spec/features/projects/project_settings_spec.rb
@@ -58,6 +58,9 @@ describe 'Edit Project Settings', feature: true do
# Not using empty project because we need a repo to exist
let(:project) { create(:project, namespace: user.namespace, name: 'gitlabhq') }
+ before(:context) { TestEnv.clean_test_path }
+ after(:example) { TestEnv.clean_test_path }
+
specify 'the project is accessible via the new path' do
rename_project(project, path: 'bar')
new_path = namespace_project_path(project.namespace, 'bar')
@@ -92,9 +95,10 @@ describe 'Edit Project Settings', feature: true do
# Not using empty project because we need a repo to exist
let(:project) { create(:project, namespace: user.namespace, name: 'gitlabhq') }
let(:group) { create(:group) }
- before do
- group.add_owner(user)
- end
+
+ before(:context) { TestEnv.clean_test_path }
+ before(:example) { group.add_owner(user) }
+ after(:example) { TestEnv.clean_test_path }
specify 'the project is accessible via the new path' do
transfer_project(project, group)
diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb
index f95adf3a84b..db680489a8d 100644
--- a/spec/lib/constraints/group_url_constrainer_spec.rb
+++ b/spec/lib/constraints/group_url_constrainer_spec.rb
@@ -29,9 +29,37 @@ describe GroupUrlConstrainer, lib: true do
it { expect(subject.matches?(request)).to be_falsey }
end
+
+ context 'when the request matches a redirect route' do
+ context 'for a root group' do
+ let!(:redirect_route) { group.redirect_routes.create!(path: 'gitlabb') }
+
+ context 'and is a GET request' do
+ let(:request) { build_request(redirect_route.path) }
+
+ it { expect(subject.matches?(request)).to be_truthy }
+ end
+
+ context 'and is NOT a GET request' do
+ let(:request) { build_request(redirect_route.path, 'POST') }
+
+ it { expect(subject.matches?(request)).to be_falsey }
+ end
+ end
+
+ context 'for a nested group' do
+ let!(:nested_group) { create(:group, path: 'nested', parent: group) }
+ let!(:redirect_route) { nested_group.redirect_routes.create!(path: 'gitlabb/nested') }
+ let(:request) { build_request(redirect_route.path) }
+
+ it { expect(subject.matches?(request)).to be_truthy }
+ end
+ end
end
- def build_request(path)
- double(:request, params: { id: path })
+ def build_request(path, method = 'GET')
+ double(:request,
+ 'get?': (method == 'GET'),
+ params: { id: path })
end
end
diff --git a/spec/lib/constraints/project_url_constrainer_spec.rb b/spec/lib/constraints/project_url_constrainer_spec.rb
index 4f25ad88960..b6884e37aa3 100644
--- a/spec/lib/constraints/project_url_constrainer_spec.rb
+++ b/spec/lib/constraints/project_url_constrainer_spec.rb
@@ -24,9 +24,26 @@ describe ProjectUrlConstrainer, lib: true do
it { expect(subject.matches?(request)).to be_falsey }
end
end
+
+ context 'when the request matches a redirect route' do
+ let(:old_project_path) { 'old_project_path' }
+ let!(:redirect_route) { project.redirect_routes.create!(path: "#{namespace.full_path}/#{old_project_path}") }
+
+ context 'and is a GET request' do
+ let(:request) { build_request(namespace.full_path, old_project_path) }
+ it { expect(subject.matches?(request)).to be_truthy }
+ end
+
+ context 'and is NOT a GET request' do
+ let(:request) { build_request(namespace.full_path, old_project_path, 'POST') }
+ it { expect(subject.matches?(request)).to be_falsey }
+ end
+ end
end
- def build_request(namespace, project)
- double(:request, params: { namespace_id: namespace, id: project })
+ def build_request(namespace, project, method = 'GET')
+ double(:request,
+ 'get?': (method == 'GET'),
+ params: { namespace_id: namespace, id: project })
end
end
diff --git a/spec/lib/constraints/user_url_constrainer_spec.rb b/spec/lib/constraints/user_url_constrainer_spec.rb
index 207b6fe6c9e..ed69b830979 100644
--- a/spec/lib/constraints/user_url_constrainer_spec.rb
+++ b/spec/lib/constraints/user_url_constrainer_spec.rb
@@ -15,9 +15,26 @@ describe UserUrlConstrainer, lib: true do
it { expect(subject.matches?(request)).to be_falsey }
end
+
+ context 'when the request matches a redirect route' do
+ let(:old_project_path) { 'old_project_path' }
+ let!(:redirect_route) { user.namespace.redirect_routes.create!(path: 'foo') }
+
+ context 'and is a GET request' do
+ let(:request) { build_request(redirect_route.path) }
+ it { expect(subject.matches?(request)).to be_truthy }
+ end
+
+ context 'and is NOT a GET request' do
+ let(:request) { build_request(redirect_route.path, 'POST') }
+ it { expect(subject.matches?(request)).to be_falsey }
+ end
+ end
end
- def build_request(username)
- double(:request, params: { username: username })
+ def build_request(username, method = 'GET')
+ double(:request,
+ 'get?': (method == 'GET'),
+ params: { username: username })
end
end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 0abf89d060c..f451fb5017b 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -225,6 +225,7 @@ project:
- authorized_users
- project_authorizations
- route
+- redirect_routes
- statistics
- container_repositories
- uploads
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 221647d7a48..49a4132f763 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -9,6 +9,7 @@ describe Group, 'Routable' do
describe 'Associations' do
it { is_expected.to have_one(:route).dependent(:destroy) }
+ it { is_expected.to have_many(:redirect_routes).dependent(:destroy) }
end
describe 'Callbacks' do
@@ -35,10 +36,53 @@ describe Group, 'Routable' do
describe '.find_by_full_path' do
let!(:nested_group) { create(:group, parent: group) }
- it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
- it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
- it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
- it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
+ context 'without any redirect routes' do
+ it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
+ it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
+ it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
+ it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
+ end
+
+ context 'with redirect routes' do
+ let!(:group_redirect_route) { group.redirect_routes.create!(path: 'bar') }
+ let!(:nested_group_redirect_route) { nested_group.redirect_routes.create!(path: nested_group.path.sub('foo', 'bar')) }
+
+ context 'without follow_redirects option' do
+ context 'with the given path not matching any route' do
+ it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
+ end
+
+ context 'with the given path matching the canonical route' do
+ it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
+ it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
+ it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
+ end
+
+ context 'with the given path matching a redirect route' do
+ it { expect(described_class.find_by_full_path(group_redirect_route.path)).to eq(nil) }
+ it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase)).to eq(nil) }
+ it { expect(described_class.find_by_full_path(nested_group_redirect_route.path)).to eq(nil) }
+ end
+ end
+
+ context 'with follow_redirects option set to true' do
+ context 'with the given path not matching any route' do
+ it { expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to eq(nil) }
+ end
+
+ context 'with the given path matching the canonical route' do
+ it { expect(described_class.find_by_full_path(group.to_param, follow_redirects: true)).to eq(group) }
+ it { expect(described_class.find_by_full_path(group.to_param.upcase, follow_redirects: true)).to eq(group) }
+ it { expect(described_class.find_by_full_path(nested_group.to_param, follow_redirects: true)).to eq(nested_group) }
+ end
+
+ context 'with the given path matching a redirect route' do
+ it { expect(described_class.find_by_full_path(group_redirect_route.path, follow_redirects: true)).to eq(group) }
+ it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase, follow_redirects: true)).to eq(group) }
+ it { expect(described_class.find_by_full_path(nested_group_redirect_route.path, follow_redirects: true)).to eq(nested_group) }
+ end
+ end
+ end
end
describe '.where_full_path_in' do
diff --git a/spec/models/redirect_route_spec.rb b/spec/models/redirect_route_spec.rb
new file mode 100644
index 00000000000..fb72d87d94d
--- /dev/null
+++ b/spec/models/redirect_route_spec.rb
@@ -0,0 +1,16 @@
+require 'rails_helper'
+
+describe RedirectRoute, models: true do
+ let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') }
+ let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') }
+
+ describe 'relationships' do
+ it { is_expected.to belong_to(:source) }
+ end
+
+ describe 'validations' do
+ it { is_expected.to validate_presence_of(:source) }
+ it { is_expected.to validate_presence_of(:path) }
+ it { is_expected.to validate_uniqueness_of(:path) }
+ end
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 401eaac07a1..63e71f5ff2f 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -849,6 +849,65 @@ describe User, models: true do
end
end
+ describe '.find_by_full_path' do
+ let!(:user) { create(:user) }
+
+ context 'with a route matching the given path' do
+ let!(:route) { user.namespace.route }
+
+ it 'returns the user' do
+ expect(User.find_by_full_path(route.path)).to eq(user)
+ end
+
+ it 'is case-insensitive' do
+ expect(User.find_by_full_path(route.path.upcase)).to eq(user)
+ expect(User.find_by_full_path(route.path.downcase)).to eq(user)
+ end
+ end
+
+ context 'with a redirect route matching the given path' do
+ let!(:redirect_route) { user.namespace.redirect_routes.create(path: 'foo') }
+
+ context 'without the follow_redirects option' do
+ it 'returns nil' do
+ expect(User.find_by_full_path(redirect_route.path)).to eq(nil)
+ end
+ end
+
+ context 'with the follow_redirects option set to true' do
+ it 'returns the user' do
+ expect(User.find_by_full_path(redirect_route.path, follow_redirects: true)).to eq(user)
+ end
+
+ it 'is case-insensitive' do
+ expect(User.find_by_full_path(redirect_route.path.upcase, follow_redirects: true)).to eq(user)
+ expect(User.find_by_full_path(redirect_route.path.downcase, follow_redirects: true)).to eq(user)
+ end
+ end
+ end
+
+ context 'without a route or a redirect route matching the given path' do
+ context 'without the follow_redirects option' do
+ it 'returns nil' do
+ expect(User.find_by_full_path('unknown')).to eq(nil)
+ end
+ end
+ context 'with the follow_redirects option set to true' do
+ it 'returns nil' do
+ expect(User.find_by_full_path('unknown', follow_redirects: true)).to eq(nil)
+ end
+ end
+ end
+
+ context 'with a group route matching the given path' do
+ let!(:group) { create(:group, path: 'group_path') }
+
+ it 'returns nil' do
+ expect(User.find_by_full_path('group_path')).to eq(nil)
+ end
+ end
+ end
+
describe 'all_ssh_keys' do
it { is_expected.to have_many(:keys).dependent(:destroy) }
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index 163df072cf6..50e96d56191 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
describe 'project routing' do
before do
allow(Project).to receive(:find_by_full_path).and_return(false)
- allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq').and_return(true)
+ allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq', any_args).and_return(true)
end
# Shared examples for a resource inside a Project
@@ -93,13 +93,13 @@ describe 'project routing' do
end
context 'name with dot' do
- before { allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq.keys').and_return(true) }
+ before { allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq.keys', any_args).and_return(true) }
it { expect(get('/gitlab/gitlabhq.keys')).to route_to('projects#show', namespace_id: 'gitlab', id: 'gitlabhq.keys') }
end
context 'with nested group' do
- before { allow(Project).to receive(:find_by_full_path).with('gitlab/subgroup/gitlabhq').and_return(true) }
+ before { allow(Project).to receive(:find_by_full_path).with('gitlab/subgroup/gitlabhq', any_args).and_return(true) }
it { expect(get('/gitlab/subgroup/gitlabhq')).to route_to('projects#show', namespace_id: 'gitlab/subgroup', id: 'gitlabhq') }
end