Merge branch 'patch/geo-readonly-json-errors' into 'master'

Geo: ReadOnly middleware improvements

We should handle API requests (JSON) in a different way we handle normal requests.

Current implementation always redirect to root and display error messages using `flash[:alert]`.
This MR adds `{ message: "..." }` type of response when request is under `application/json` content_type.

This is a small patch/enhancement for Geo (#76)

See merge request !274

lib/gitlab/middleware/readonly_geo.rb

@@ -3,6 +3,7 @@ module Gitlab
     class ReadonlyGeo
       DISALLOWED_METHODS = %w(POST PATCH PUT DELETE)
       WHITELISTED = %w(api/v3/internal api/v3/geo/refresh_projects api/v3/geo/refresh_wikis)
+      APPLICATION_JSON = 'application/json'
 
       def initialize(app)
         @app = app
@@ -13,11 +14,16 @@ module Gitlab
 
         if disallowed_request? && Gitlab::Geo.secondary?
           Rails.logger.debug('Gitlab Geo: preventing possible non readonly operation')
+          error_message = 'You cannot do writing operations on a secondary Gitlab Geo instance'
 
-          rack_flash.alert = 'You cannot do writing operations on a secondary Gitlab Geo instance'
-          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)
@@ -29,6 +35,10 @@ module Gitlab
         DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && !whitelisted_routes
       end
 
+      def json_request?
+        request.media_type == APPLICATION_JSON
+      end
+
       def rack_flash
         @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session)
       end

spec/lib/gitlab/middleware/readonly_geo_spec.rb

@@ -16,6 +16,13 @@ describe Gitlab::Middleware::ReadonlyGeo, lib: true do
     end
   end
 
+  RSpec::Matchers.define :disallow_request_in_json do
+    match do |response|
+      json_response = JSON.parse(response.body)
+      response.body.include?('You cannot do writing operations') && json_response.key?('message')
+    end
+  end
+
   let(:rack_stack) do
     rack = Rack::Builder.new do
       use ActionDispatch::Session::CacheStore
@@ -28,10 +35,10 @@ describe Gitlab::Middleware::ReadonlyGeo, lib: true do
   end
 
   subject { described_class.new(fake_app) }
-  let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
   let(:request) { @request ||= Rack::MockRequest.new(rack_stack) }
 
-  context 'when in secondary Gitlab Geo node' do
+  context 'normal requests to a secondary Gitlab Geo' do
+    let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
     before(:each) { allow(Gitlab::Geo).to receive(:secondary?) { true } }
 
     it 'expects PATCH requests to be disallowed' do
@@ -62,11 +69,43 @@ describe Gitlab::Middleware::ReadonlyGeo, lib: true do
       expect(subject).to disallow_request
     end
 
-    it 'expects DELETE request to logout to be allowed' do
-      response = request.delete('/users/sign_out')
+    context 'whitelisted requests' do
+      it 'expects DELETE request to logout to be allowed' do
+        response = request.delete('/users/sign_out')
+
+        expect(response).not_to be_a_redirect
+        expect(subject).not_to disallow_request
+      end
+    end
+  end
+
+  context 'json requests to a secondary Geo node' do
+    let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } }
+    let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } }
+    before(:each) { allow(Gitlab::Geo).to receive(:secondary?) { true } }
+
+    it 'expects PATCH requests to be disallowed' do
+      response = request.patch('/test_request', content_json)
+
+      expect(response).to disallow_request_in_json
+    end
+
+    it 'expects PUT requests to be disallowed' do
+      response = request.put('/test_request', content_json)
+
+      expect(response).to disallow_request_in_json
+    end
+
+    it 'expects POST requests to be disallowed' do
+      response = request.post('/test_request', content_json)
+
+      expect(response).to disallow_request_in_json
+    end
+
+    it 'expects DELETE requests to be disallowed' do
+      response = request.delete('/test_request', content_json)
 
-      expect(response).not_to be_a_redirect
-      expect(subject).not_to disallow_request
+      expect(response).to disallow_request_in_json
     end
   end
 end