FIX: Don't redirect XHR/JSON requests when login is required (#15093)

When redirecting to login, we store a destination_url cookie, which the user is then redirected to after login. We never want the user to be redirected to a JSON URL. Instead, we should return a 403 in these situations.

This should also be much less confusing for API consumers - a 403 is a better representation than a 302.

app/controllers/application_controller.rb

@@ -819,7 +819,11 @@ class ApplicationController < ActionController::Base
 
     if !current_user && SiteSetting.login_required?
       flash.keep
-      redirect_to_login
+      if (request.format && request.format.json?) || request.xhr? || !request.get?
+        ensure_logged_in
+      else
+        redirect_to_login
+      end
       return
     end
 

spec/integration/api_keys_spec.rb

@@ -45,7 +45,7 @@ describe 'api keys' do
 
     # Confirm not allowed for json
     get "/latest.json?api_key=#{api_key.key}&api_username=#{user.username.downcase}"
-    expect(response.status).to eq(302)
+    expect(response.status).to eq(403)
   end
 
   context "with a plugin registered filter" do
@@ -96,7 +96,7 @@ describe 'user api keys' do
 
     # Confirm not allowed for json
     get "/latest.json?user_api_key=#{user_api_key.key}"
-    expect(response.status).to eq(302)
+    expect(response.status).to eq(403)
   end
 
   it "can restrict scopes by parameters" do

spec/requests/application_controller_spec.rb

@@ -120,6 +120,14 @@ RSpec.describe ApplicationController do
       expect(response.body).not_to include("data-authentication-data=")
       expect(response.headers["Set-Cookie"]).to include("authentication_data=;") # Delete cookie
     end
+
+    it "returns a 403 for json requests" do
+      get '/latest'
+      expect(response.status).to eq(302)
+
+      get '/latest.json'
+      expect(response.status).to eq(403)
+    end
   end
 
   describe '#redirect_to_second_factor_if_required' do

spec/requests/topics_controller_spec.rb

@@ -1928,19 +1928,21 @@ RSpec.describe TopicsController do
       let!(:nonexistent_topic_id) { Topic.last.id + 10000 }
       fab!(:secure_accessible_topic) { Fabricate(:topic, category: accessible_category) }
 
-      shared_examples "various scenarios" do |expected|
+      shared_examples "various scenarios" do |expected, request_json: false|
         expected.each do |key, value|
           it "returns #{value} for #{key}" do
             slug = key == :nonexistent ? "garbage-slug" : send(key.to_s).slug
             topic_id = key == :nonexistent ? nonexistent_topic_id : send(key.to_s).id
-            get "/t/#{slug}/#{topic_id}.json"
+            format = request_json ? ".json" : ""
+            get "/t/#{slug}/#{topic_id}#{format}"
             expect(response.status).to eq(value)
           end
         end
 
         expected_slug_response = expected[:secure_topic] == 200 ? 301 : expected[:secure_topic]
         it "will return a #{expected_slug_response} when requesting a secure topic by slug" do
-          get "/t/#{secure_topic.slug}"
+          format = request_json ? ".json" : ""
+          get "/t/#{secure_topic.slug}#{format}"
           expect(response.status).to eq(expected_slug_response)
         end
       end
@@ -1981,6 +1983,23 @@ RSpec.describe TopicsController do
           include_examples "various scenarios", expected
         end
 
+        context 'anonymous with login required, requesting json' do
+          before do
+            SiteSetting.login_required = true
+          end
+          expected = {
+            normal_topic: 403,
+            secure_topic: 403,
+            private_topic: 403,
+            deleted_topic: 403,
+            deleted_secure_topic: 403,
+            deleted_private_topic: 403,
+            nonexistent: 403,
+            secure_accessible_topic: 403
+          }
+          include_examples "various scenarios", expected, request_json: true
+        end
+
         context 'normal user' do
           before do
             sign_in(user)
@@ -2070,7 +2089,7 @@ RSpec.describe TopicsController do
             nonexistent: 404,
             secure_accessible_topic: 403
           }
-          include_examples "various scenarios", expected
+          include_examples "various scenarios", expected, request_json: true
         end
 
         context 'anonymous with login required' do
@@ -2105,7 +2124,7 @@ RSpec.describe TopicsController do
             nonexistent: 404,
             secure_accessible_topic: 403
           }
-          include_examples "various scenarios", expected
+          include_examples "various scenarios", expected, request_json: true
         end
 
         context 'allowed user' do
@@ -2395,12 +2414,16 @@ RSpec.describe TopicsController do
       context 'and the user is not logged in' do
         let(:api_key) { Fabricate(:api_key, user: topic.user) }
 
-        it 'redirects to the login page' do
-          get "/t/#{topic.slug}/#{topic.id}.json"
-
+        it 'redirects browsers to the login page' do
+          get "/t/#{topic.slug}/#{topic.id}"
           expect(response).to redirect_to login_path
         end
 
+        it 'raises a 403 for json requests' do
+          get "/t/#{topic.slug}/#{topic.id}.json"
+          expect(response.status).to eq(403)
+        end
+
         it 'shows the topic if valid api key is provided' do
           get "/t/#{topic.slug}/#{topic.id}.json", headers: { "HTTP_API_KEY" => api_key.key }