Merge branch 'security-332605-deactivated-user' into 'master'

Prevent GraphQL API access by deactivated users

See merge request gitlab-org/security/gitlab!1517

app/controllers/graphql_controller.rb

@@ -20,8 +20,10 @@ class GraphqlController < ApplicationController
   # around in GraphiQL.
   protect_from_forgery with: :null_session, only: :execute
 
-  before_action :authorize_access_api!
+  # must come first: current_user is set up here
   before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
+
+  before_action :authorize_access_api!
   before_action :set_user_last_activity
   before_action :track_vs_code_usage
   before_action :disable_query_limiting
@@ -151,7 +153,9 @@ class GraphqlController < ApplicationController
   end
 
   def authorize_access_api!
-    access_denied!("API not accessible for user.") unless can?(current_user, :access_api)
+    return if can?(current_user, :access_api)
+
+    render_error('API not accessible for user', status: :forbidden)
   end
 
   # Overridden from the ApplicationController to make the response look like

spec/controllers/graphql_controller_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe GraphqlController do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
-      it 'returns access denied template when user cannot access API' do
+      it 'returns forbidden when user cannot access API' do
         # User cannot access API in a couple of cases
         # * When user is internal(like ghost users)
         # * When user is blocked
@@ -54,7 +54,9 @@ RSpec.describe GraphqlController do
         post :execute
 
         expect(response).to have_gitlab_http_status(:forbidden)
-        expect(response).to render_template('errors/access_denied')
+        expect(json_response).to include(
+          'errors' => include(a_hash_including('message' => /API not accessible/))
+        )
       end
 
       it 'updates the users last_activity_on field' do

spec/policies/global_policy_spec.rb

@@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do
       it { is_expected.not_to be_allowed(:access_api) }
     end
 
+    context 'with a deactivated user' do
+      before do
+        current_user.deactivate!
+      end
+
+      it { is_expected.not_to be_allowed(:access_api) }
+    end
+
     context 'user with expired password' do
       before do
         current_user.update!(password_expires_at: 2.minutes.ago)

spec/requests/api/graphql_spec.rb

@@ -243,11 +243,9 @@ RSpec.describe 'GraphQL' do
     context 'with token authentication' do
       let(:token) { create(:personal_access_token) }
 
-      before do
+      it 'authenticates users with a PAT' 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)
@@ -258,6 +256,14 @@ RSpec.describe 'GraphQL' do
         expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world")
       end
 
+      it 'prevents access by deactived users' do
+        token.user.deactivate!
+
+        post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token })
+
+        expect(graphql_errors).to include({ 'message' => /API not accessible/ })
+      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])