Automatic merge of gitlab-org/gitlab-ce master

app/controllers/graphql_controller.rb

@@ -3,9 +3,16 @@
 class GraphqlController < ApplicationController
   # Unauthenticated users have access to the API for public data
   skip_before_action :authenticate_user!
-  prepend_before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
+
+  # Allow missing CSRF tokens, this would mean that if a CSRF is invalid or missing,
+  # the user won't be authenticated but can proceed as an anonymous user.
+  #
+  # If a CSRF is valid, the user is authenticated. This makes it easier to play
+  # around in GraphiQL.
+  protect_from_forgery with: :null_session, only: :execute
 
   before_action :check_graphql_feature_flag!
+  before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
 
   def execute
     variables = Gitlab::Graphql::Variables.new(params[:variables]).to_h

changelogs/unreleased/bvl-graphql-csrf.yml

+---
+title: Allow GraphQL requests without CSRF token
+merge_request: 25719
+author:
+type: fixed

spec/controllers/graphql_controller_spec.rb

-require 'spec_helper'
-
-describe GraphqlController do
-  describe 'execute' do
-    let(:user) { nil }
-
-    before do
-      sign_in(user) if user
-
-      run_test_query!
-    end
-
-    subject { query_response }
-
-    context 'graphql is disabled by feature flag' do
-      let(:user) { nil }
-
-      before do
-        stub_feature_flags(graphql: false)
-      end
-
-      it 'returns 404' do
-        run_test_query!
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-    end
-
-    context 'signed out' do
-      let(:user) { nil }
-
-      it 'runs the query with current_user: nil' do
-        is_expected.to eq('echo' => 'nil says: test success')
-      end
-    end
-
-    context 'signed in' do
-      let(:user) { create(:user, username: 'Simon') }
-
-      it 'runs the query with current_user set' do
-        is_expected.to eq('echo' => '"Simon" says: test success')
-      end
-    end
-
-    context 'invalid variables' do
-      it 'returns an error' do
-        run_test_query!(variables: "This is not JSON")
-
-        expect(response).to have_gitlab_http_status(422)
-        expect(json_response['errors'].first['message']).not_to be_nil
-      end
-    end
-  end
-
-  context 'token authentication' do
-    before do
-      stub_authentication_activity_metrics(debug: false)
-    end
-
-    let(:user) { create(:user, username: 'Simon') }
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
-
-    context "when the 'personal_access_token' param is populated with the personal access token" do
-      it 'logs the user in' do
-        expect(authentication_metrics)
-          .to increment(:user_authenticated_counter)
-                .and increment(:user_session_override_counter)
-                       .and increment(:user_sessionless_authentication_counter)
-
-        run_test_query!(private_token: personal_access_token.token)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(query_response).to eq('echo' => '"Simon" says: test success')
-      end
-    end
-
-    context 'when the personal access token has no api scope' do
-      it 'does not log the user in' do
-        personal_access_token.update(scopes: [:read_user])
-
-        run_test_query!(private_token: personal_access_token.token)
-
-        expect(response).to have_gitlab_http_status(200)
-
-        expect(query_response).to eq('echo' => 'nil says: test success')
-      end
-    end
-
-    context 'without token' do
-      it 'shows public data' do
-        run_test_query!
-
-        expect(query_response).to eq('echo' => 'nil says: test success')
-      end
-    end
-  end
-
-  # Chosen to exercise all the moving parts in GraphqlController#execute
-  def run_test_query!(variables: { 'text' => 'test success' }, private_token: nil)
-    query = <<~QUERY
-      query Echo($text: String) {
-        echo(text: $text)
-      }
-    QUERY
-
-    post :execute, params: { query: query, operationName: 'Echo', variables: variables, private_token: private_token }
-  end
-
-  def query_response
-    json_response['data']
-  end
-end

spec/requests/api/graphql_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe 'GraphQL' do
+  include GraphqlHelpers
+
+  let(:query) { graphql_query_for('echo', 'text' => 'Hello world' ) }
+
+  context 'graphql is disabled by feature flag' do
+    before do
+      stub_feature_flags(graphql: false)
+    end
+
+    it 'does not generate a route for GraphQL' do
+      expect { post_graphql(query) }.to raise_error(ActionController::RoutingError)
+    end
+  end
+
+  context 'invalid variables' do
+    it 'returns an error' do
+      post_graphql(query, variables: "This is not JSON")
+
+      expect(response).to have_gitlab_http_status(422)
+      expect(json_response['errors'].first['message']).not_to be_nil
+    end
+  end
+
+  context 'authentication', :allow_forgery_protection do
+    let(:user) { create(:user) }
+
+    it 'allows access to public data without authentication' do
+      post_graphql(query)
+
+      expect(graphql_data['echo']).to eq('nil says: Hello world')
+    end
+
+    it 'does not authenticate a user with an invalid CSRF' do
+      login_as(user)
+
+      post_graphql(query, headers: { 'X-CSRF-Token' => 'invalid' })
+
+      expect(graphql_data['echo']).to eq('nil says: Hello world')
+    end
+
+    it 'authenticates a user with a valid session token' do
+      # Create a session to get a CSRF token from
+      login_as(user)
+      get('/')
+
+      post '/api/graphql', params: { query: query }, headers: { 'X-CSRF-Token' => response.session['_csrf_token'] }
+
+      expect(graphql_data['echo']).to eq("\"#{user.username}\" says: Hello world")
+    end
+
+    context 'token authentication' do
+      let(:token) { create(:personal_access_token) }
+
+      before do
+        stub_authentication_activity_metrics(debug: false)
+      end
+
+      it 'Authenticates users with a PAT' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+          .and increment(:user_session_override_counter)
+          .and increment(:user_sessionless_authentication_counter)
+
+        post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token })
+
+        expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world")
+      end
+
+      context 'when the personal access token has no api scope' do
+        it 'does not log the user in' do
+          token.update(scopes: [:read_user])
+
+          post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token })
+
+          expect(response).to have_gitlab_http_status(200)
+
+          expect(graphql_data['echo']).to eq('nil says: Hello world')
+        end
+      end
+    end
+  end
+end

spec/support/helpers/graphql_helpers.rb

@@ -79,12 +79,21 @@ module GraphqlHelpers
     attributes = attributes_to_graphql(attributes)
     attributes = "(#{attributes})" if attributes.present?
     <<~QUERY
-      #{name}#{attributes} {
-        #{fields}
-      }
+      #{name}#{attributes}
+      #{wrap_fields(fields)}
     QUERY
   end
 
+  def wrap_fields(fields)
+    return unless fields.strip.present?
+
+    <<~FIELDS
+    {
+      #{fields}
+    }
+    FIELDS
+  end
+
   def all_graphql_fields_for(class_name, parent_types = Set.new)
     type = GitlabSchema.types[class_name.to_s]
     return "" unless type
@@ -116,8 +125,8 @@ module GraphqlHelpers
     end.join(", ")
   end
 
-  def post_graphql(query, current_user: nil, variables: nil)
-    post api('/', current_user, version: 'graphql'), params: { query: query, variables: variables }
+  def post_graphql(query, current_user: nil, variables: nil, headers: {})
+    post api('/', current_user, version: 'graphql'), params: { query: query, variables: variables }, headers: headers
   end
 
   def post_graphql_mutation(mutation, current_user: nil)