diff options
author | Michael Kozono <mkozono@gmail.com> | 2017-05-01 23:46:30 +0300 |
---|---|---|
committer | Michael Kozono <mkozono@gmail.com> | 2017-05-05 22:11:57 +0300 |
commit | 7d02bcd2e0165a90a9f2c1edb34b064ff76afd69 (patch) | |
tree | 3b74cbbf74fca3b36effa5ea4b33d8bd29e22f73 /app | |
parent | f4a2dfb46f168d3fd7309aca8631cf680456fa82 (diff) |
Redirect from redirect routes to canonical routes
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/application_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/concerns/routable_actions.rb | 11 | ||||
-rw-r--r-- | app/controllers/groups/application_controller.rb | 21 | ||||
-rw-r--r-- | app/controllers/groups_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/application_controller.rb | 14 | ||||
-rw-r--r-- | app/controllers/users_controller.rb | 12 | ||||
-rw-r--r-- | app/models/concerns/routable.rb | 24 | ||||
-rw-r--r-- | app/models/redirect_route.rb | 10 | ||||
-rw-r--r-- | app/models/user.rb | 5 |
9 files changed, 74 insertions, 27 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 |