Commit 7d02bcd2 authored by Michael Kozono's avatar Michael Kozono

Redirect from redirect routes to canonical routes

parent f4a2dfb4
......@@ -58,7 +58,7 @@ class ApplicationController < ActionController::Base
if current_user
not_found
else
redirect_to new_user_session_path
authenticate_user!
end
end
......
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
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
......
......@@ -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]
......
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
......
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
......
......@@ -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.
......
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
......@@ -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
......
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
......@@ -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"
......
......@@ -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
......@@ -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
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
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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