diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2018-02-02 22:12:28 +0800 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2018-02-07 22:45:02 +0800 |
commit | bbfce29ba8d75df5344dae34dc472dfb3b3acf4b (patch) | |
tree | 852a3e8b99d48385106fe24e8bdb36c92ee794b7 | |
parent | d4d564c8e7d2cbc3e6742475a793ba0f630167e3 (diff) | |
download | gitlab-ce-bbfce29ba8d75df5344dae34dc472dfb3b3acf4b.tar.gz |
Use a controller to hold request values
So that we don't need to hold env after the request.
This makes it much harder to test, especially Rails session is
acting weirdly, so we need `dig('flash', 'flashes', 'alert')`
to dig the actual flash value.
-rw-r--r-- | lib/gitlab/middleware/read_only.rb | 125 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/read_only_spec.rb | 23 |
2 files changed, 87 insertions, 61 deletions
diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index a68c6c3d15c..b7649ea01db 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -5,86 +5,95 @@ module Gitlab APPLICATION_JSON = 'application/json'.freeze API_VERSIONS = (3..4) - def initialize(app) - @app = app - @whitelisted = internal_routes - end - - def call(env) - @env = env - @route_hash = nil + class Controller + def initialize(app, env) + @app = app + @env = env + end - if disallowed_request? && Gitlab::Database.read_only? - Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation') - error_message = 'You cannot do writing operations on a read-only GitLab instance' + def call + if disallowed_request? && Gitlab::Database.read_only? + Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation') + error_message = 'You cannot do writing operations on a read-only GitLab instance' - if json_request? - return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] - else - rack_flash.alert = error_message - rack_session['flash'] = rack_flash.to_session_value + if json_request? + return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] + else + rack_flash.alert = error_message + rack_session['flash'] = rack_flash.to_session_value - return [301, { 'Location' => last_visited_url }, []] + return [301, { 'Location' => last_visited_url }, []] + end end + + @app.call(@env) end - @app.call(env).tap { @env = nil } - end + private - private + def disallowed_request? + DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && + !whitelisted_routes + end - def internal_routes - API_VERSIONS.flat_map { |version| "api/v#{version}/internal" } - end + def json_request? + request.media_type == APPLICATION_JSON + end - def disallowed_request? - DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && !whitelisted_routes - end + def rack_flash + @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) + end - def json_request? - request.media_type == APPLICATION_JSON - end + def rack_session + @env['rack.session'] + end - def rack_flash - @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) - end + def request + @env['rack.request'] ||= Rack::Request.new(@env) + end - def rack_session - @env['rack.session'] - end + def last_visited_url + @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url + end - def request - @env['rack.request'] ||= Rack::Request.new(@env) - end + def route_hash + @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} + end - def last_visited_url - @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url - end + def whitelisted_routes + grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route + end - def route_hash - @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} - end + def sidekiq_route + request.path.start_with?('/admin/sidekiq') + end - def whitelisted_routes - grack_route || @whitelisted.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route - end + def grack_route + # Calling route_hash may be expensive. Only do it if we think there's a possible match + return false unless request.path.end_with?('.git/git-upload-pack') - def sidekiq_route - request.path.start_with?('/admin/sidekiq') - end + route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack' + end + + def lfs_route + # Calling route_hash may be expensive. Only do it if we think there's a possible match + return false unless request.path.end_with?('/info/lfs/objects/batch') - def grack_route - # Calling route_hash may be expensive. Only do it if we think there's a possible match - return false unless request.path.end_with?('.git/git-upload-pack') + route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch' + end + end - route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack' + def self.internal_routes + @internal_routes ||= + API_VERSIONS.map { |version| "api/v#{version}/internal" } end - def lfs_route - # Calling route_hash may be expensive. Only do it if we think there's a possible match - return false unless request.path.end_with?('/info/lfs/objects/batch') + def initialize(app) + @app = app + end - route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch' + def call(env) + Controller.new(@app, env).call end end end diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb index 07ba11b93a3..b3c85142b82 100644 --- a/spec/lib/gitlab/middleware/read_only_spec.rb +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -11,8 +11,10 @@ describe Gitlab::Middleware::ReadOnly do RSpec::Matchers.define :disallow_request do match do |middleware| - flash = middleware.send(:rack_flash) - flash['alert'] && flash['alert'].include?('You cannot do writing operations') + alert = middleware.env['rack.session'].to_hash + .dig('flash', 'flashes', 'alert') + + alert&.include?('You cannot do writing operations') end end @@ -34,7 +36,22 @@ describe Gitlab::Middleware::ReadOnly do rack.to_app end - subject { described_class.new(fake_app) } + let(:observe_env) do + Module.new do + attr_reader :env + + def call(env) + @env = env + super + end + end + end + + subject do + app = described_class.new(fake_app) + app.extend(observe_env) + app + end let(:request) { Rack::MockRequest.new(rack_stack) } |