Merge branch 'sh-log-when-user-blocked' into 'master'

app/models/user.rb

@@ -55,6 +55,9 @@ class User < ActiveRecord::Base
   devise :lockable, :recoverable, :rememberable, :trackable,
          :validatable, :omniauthable, :confirmable, :registerable
 
+  BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \
+                    "administrator if you think this is an error.".freeze
+
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
   def update_tracked_fields!(request)
@@ -217,8 +220,7 @@ class User < ActiveRecord::Base
       end
 
       def inactive_message
-        "Your account has been blocked. Please contact your GitLab " \
-          "administrator if you think this is an error."
+        BLOCKED_MESSAGE
       end
     end
   end

app/services/system_hooks_service.rb

@@ -41,8 +41,11 @@ class SystemHooksService
     when User
       data.merge!(user_data(model))
 
-      if event == :rename
+      case event
+      when :rename
         data[:old_username] = model.username_was
+      when :failed_login
+        data[:state] = model.state
       end
     when ProjectMember
       data.merge!(project_member_data(model))

changelogs/unreleased/sh-log-when-user-blocked.yml

+---
+title: Log and send a system hook if a blocked user attempts to login
+merge_request:
+author:
+type: added

config/initializers/warden.rb

@@ -2,4 +2,8 @@ Rails.application.configure do |config|
   Warden::Manager.after_set_user do |user, auth, opts|
     Gitlab::Auth::UniqueIpsLimiter.limit_user!(user)
   end
+
+  Warden::Manager.before_failure do |env, opts|
+    Gitlab::Auth::BlockedUserTracker.log_if_user_blocked(env)
+  end
 end

doc/system_hooks/system_hooks.md

@@ -11,6 +11,7 @@ Your GitLab instance can perform HTTP POST requests on the following events:
 - `user_remove_from_team`
 - `user_create`
 - `user_destroy`
+- `user_failed_login`
 - `user_rename`
 - `key_create`
 - `key_destroy`
@@ -22,6 +23,8 @@ Your GitLab instance can perform HTTP POST requests on the following events:
 
 The triggers for most of these are self-explanatory, but `project_update` and `project_rename` deserve some clarification: `project_update` is fired any time an attribute of a project is changed (name, description, tags, etc.) *unless* the `path` attribute is also changed. In that case, a `project_rename` is triggered instead (so that, for instance, if all you care about is the repo URL, you can just listen for `project_rename`).
 
+`user_failed_login` is sent whenever a **blocked** user attempts to login and denied access.
+
 System hooks can be used, e.g. for logging or changing information in a LDAP server.
 
 > **Note:**
@@ -196,6 +199,23 @@ Please refer to `group_rename` and `user_rename` for that case.
 }
 ```
 
+**User failed login:**
+
+```json
+{
+  "event_name": "user_failed_login",
+  "created_at": "2017-10-03T06:08:48Z",
+  "updated_at": "2018-01-15T04:52:06Z",
+        "name": "John Smith",
+       "email": "user4@example.com",
+     "user_id": 26,
+    "username": "user4",
+       "state": "blocked"
+}
+```
+
+If the user is blocked via LDAP, `state` will be `ldap_blocked`.
+
 **User renamed:**
 
 ```json

lib/gitlab/auth/blocked_user_tracker.rb

+# frozen_string_literal: true
+module Gitlab
+  module Auth
+    class BlockedUserTracker
+      ACTIVE_RECORD_REQUEST_PARAMS = 'action_dispatch.request.request_parameters'
+
+      def self.log_if_user_blocked(env)
+        message = env.dig('warden.options', :message)
+
+        # Devise calls User#active_for_authentication? on the User model and then
+        # throws an exception to Warden with User#inactive_message:
+        # https://github.com/plataformatec/devise/blob/v4.2.1/lib/devise/hooks/activatable.rb#L8
+        #
+        # Since Warden doesn't pass the user record to the failure handler, we
+        # need to do a database lookup with the username. We can limit the
+        # lookups to happen when the user was blocked by checking the inactive
+        # message passed along by Warden.
+        return unless message == User::BLOCKED_MESSAGE
+
+        login = env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'user', 'login')
+
+        return unless login.present?
+
+        user = User.by_login(login)
+
+        return unless user&.blocked?
+
+        Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{env['REMOTE_ADDR']}")
+        SystemHooksService.new.execute_hooks_for(user, :failed_login)
+
+        true
+      rescue TypeError
+      end
+    end
+  end
+end

spec/lib/gitlab/auth/blocked_user_tracker_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::BlockedUserTracker do
+  set(:user) { create(:user) }
+
+  describe '.log_if_user_blocked' do
+    it 'does not log if user failed to login due to undefined reason' do
+      expect_any_instance_of(SystemHooksService).not_to receive(:execute_hooks_for)
+
+      expect(described_class.log_if_user_blocked({})).to be_nil
+    end
+
+    it 'gracefully handles malformed environment variables' do
+      env = { 'warden.options' => 'test' }
+
+      expect(described_class.log_if_user_blocked(env)).to be_nil
+    end
+
+    context 'failed login due to blocked user' do
+      let(:env) do
+        {
+          'warden.options' => { message: User::BLOCKED_MESSAGE },
+          described_class::ACTIVE_RECORD_REQUEST_PARAMS => { 'user' => { 'login' => user.username } }
+        }
+      end
+
+      subject { described_class.log_if_user_blocked(env) }
+
+      before do
+        expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login)
+      end
+
+      it 'logs a blocked user' do
+        user.block!
+
+        expect(subject).to be_truthy
+      end
+
+      it 'logs a blocked user by e-mail' do
+        user.block!
+        env[described_class::ACTIVE_RECORD_REQUEST_PARAMS]['user']['login'] = user.email
+
+        expect(subject).to be_truthy
+      end
+
+      it 'logs a LDAP blocked user' do
+        user.ldap_block!
+
+        expect(subject).to be_truthy
+      end
+    end
+  end
+end

spec/services/system_hooks_service_spec.rb

@@ -105,12 +105,25 @@ describe SystemHooksService do
         expect(data[:old_username]).to eq(user.username_was)
       end
     end
+
+    context 'user_failed_login' do
+      it 'contains state of user' do
+        user.ldap_block!
+
+        data = event_data(user, :failed_login)
+
+        expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :state)
+        expect(data[:username]).to eq(user.username)
+        expect(data[:state]).to eq('ldap_blocked')
+      end
+    end
   end
 
   context 'event names' do
     it { expect(event_name(user, :create)).to eq "user_create" }
     it { expect(event_name(user, :destroy)).to eq "user_destroy" }
     it { expect(event_name(user, :rename)).to eq 'user_rename' }
+    it { expect(event_name(user, :failed_login)).to eq 'user_failed_login' }
     it { expect(event_name(project, :create)).to eq "project_create" }
     it { expect(event_name(project, :destroy)).to eq "project_destroy" }
     it { expect(event_name(project, :rename)).to eq "project_rename" }