Skip to content

Commit 0d187a9

Browse files
committed
Log and send a system hook if a blocked user fails to login
Closes #41633
1 parent 74f2f9b commit 0d187a9

File tree

8 files changed

+140
-4
lines changed

8 files changed

+140
-4
lines changed

app/models/user.rb

+5-3
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,10 @@ class User < ActiveRecord::Base
5353
serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize
5454

5555
devise :lockable, :recoverable, :rememberable, :trackable,
56-
:validatable, :omniauthable, :confirmable, :registerable
56+
:validatable, :omniauthable, :confirmable, :registerable
57+
58+
BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \
59+
"administrator if you think this is an error.".freeze
5760

5861
# Override Devise::Models::Trackable#update_tracked_fields!
5962
# to limit database writes to at most once every hour
@@ -217,8 +220,7 @@ def active_for_authentication?
217220
end
218221

219222
def inactive_message
220-
"Your account has been blocked. Please contact your GitLab " \
221-
"administrator if you think this is an error."
223+
BLOCKED_MESSAGE
222224
end
223225
end
224226
end

app/services/system_hooks_service.rb

+4-1
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,11 @@ def build_event_data(model, event)
4141
when User
4242
data.merge!(user_data(model))
4343

44-
if event == :rename
44+
case event
45+
when :rename
4546
data[:old_username] = model.username_was
47+
when :failed_login
48+
data[:state] = model.state
4649
end
4750
when ProjectMember
4851
data.merge!(project_member_data(model))
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
title: Log and send a system hook if a blocked user attempts to login
3+
merge_request:
4+
author:
5+
type: added

config/initializers/warden.rb

+4
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,8 @@
22
Warden::Manager.after_set_user do |user, auth, opts|
33
Gitlab::Auth::UniqueIpsLimiter.limit_user!(user)
44
end
5+
6+
Warden::Manager.before_failure do |env, opts|
7+
Gitlab::Auth::BlockedUserTracker.log_if_user_blocked(env)
8+
end
59
end

doc/system_hooks/system_hooks.md

+20
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ Your GitLab instance can perform HTTP POST requests on the following events:
1111
- `user_remove_from_team`
1212
- `user_create`
1313
- `user_destroy`
14+
- `user_failed_login`
1415
- `user_rename`
1516
- `key_create`
1617
- `key_destroy`
@@ -22,6 +23,8 @@ Your GitLab instance can perform HTTP POST requests on the following events:
2223

2324
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`).
2425

26+
`user_failed_login` is sent whenever a **blocked** user attempts to login and denied access.
27+
2528
System hooks can be used, e.g. for logging or changing information in a LDAP server.
2629

2730
> **Note:**
@@ -196,6 +199,23 @@ Please refer to `group_rename` and `user_rename` for that case.
196199
}
197200
```
198201

202+
**User failed login:**
203+
204+
```json
205+
{
206+
"event_name": "user_failed_login",
207+
"created_at": "2017-10-03T06:08:48Z",
208+
"updated_at": "2018-01-15T04:52:06Z",
209+
"name": "John Smith",
210+
"email": "[email protected]",
211+
"user_id": 26,
212+
"username": "user4",
213+
"state": "blocked"
214+
}
215+
```
216+
217+
If the user is blocked via LDAP, `state` will be `ldap_blocked`.
218+
199219
**User renamed:**
200220

201221
```json
+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# frozen_string_literal: true
2+
module Gitlab
3+
module Auth
4+
class BlockedUserTracker
5+
ACTIVE_RECORD_REQUEST_PARAMS = 'action_dispatch.request.request_parameters'
6+
7+
def self.log_if_user_blocked(env)
8+
message = env.dig('warden.options', :message)
9+
10+
# Devise calls User#active_for_authentication? on the User model and then
11+
# throws an exception to Warden with User#inactive_message:
12+
# https://github.com/plataformatec/devise/blob/v4.2.1/lib/devise/hooks/activatable.rb#L8
13+
#
14+
# Since Warden doesn't pass the user record to the failure handler, we
15+
# need to do a database lookup with the username. We can limit the
16+
# lookups to happen when the user was blocked by checking the inactive
17+
# message passed along by Warden.
18+
return unless message == User::BLOCKED_MESSAGE
19+
20+
login = env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'user', 'login')
21+
22+
return unless login.present?
23+
24+
user = User.by_login(login)
25+
26+
return unless user&.blocked?
27+
28+
Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{env['REMOTE_ADDR']}")
29+
SystemHooksService.new.execute_hooks_for(user, :failed_login)
30+
31+
true
32+
rescue TypeError
33+
end
34+
end
35+
end
36+
end
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
require 'spec_helper'
2+
3+
describe Gitlab::Auth::BlockedUserTracker do
4+
set(:user) { create(:user) }
5+
6+
describe '.log_if_user_blocked' do
7+
it 'does not log if user failed to login due to undefined reason' do
8+
expect_any_instance_of(SystemHooksService).not_to receive(:execute_hooks_for)
9+
10+
expect(described_class.log_if_user_blocked({})).to be_nil
11+
end
12+
13+
it 'gracefully handles malformed environment variables' do
14+
env = { 'warden.options' => 'test' }
15+
16+
expect(described_class.log_if_user_blocked(env)).to be_nil
17+
end
18+
19+
context 'failed login due to blocked user' do
20+
let(:env) do
21+
{
22+
'warden.options' => { message: User::BLOCKED_MESSAGE },
23+
described_class::ACTIVE_RECORD_REQUEST_PARAMS => { 'user' => { 'login' => user.username } }
24+
}
25+
end
26+
27+
subject { described_class.log_if_user_blocked(env) }
28+
29+
before do
30+
expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login)
31+
end
32+
33+
it 'logs a blocked user' do
34+
user.block!
35+
36+
expect(subject).to be_truthy
37+
end
38+
39+
it 'logs a blocked user by e-mail' do
40+
user.block!
41+
env[described_class::ACTIVE_RECORD_REQUEST_PARAMS]['user']['login'] = user.email
42+
43+
expect(subject).to be_truthy
44+
end
45+
46+
it 'logs a LDAP blocked user' do
47+
user.ldap_block!
48+
49+
expect(subject).to be_truthy
50+
end
51+
end
52+
end
53+
end

spec/services/system_hooks_service_spec.rb

+13
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,25 @@
105105
expect(data[:old_username]).to eq(user.username_was)
106106
end
107107
end
108+
109+
context 'user_failed_login' do
110+
it 'contains state of user' do
111+
user.ldap_block!
112+
113+
data = event_data(user, :failed_login)
114+
115+
expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :state)
116+
expect(data[:username]).to eq(user.username)
117+
expect(data[:state]).to eq('ldap_blocked')
118+
end
119+
end
108120
end
109121

110122
context 'event names' do
111123
it { expect(event_name(user, :create)).to eq "user_create" }
112124
it { expect(event_name(user, :destroy)).to eq "user_destroy" }
113125
it { expect(event_name(user, :rename)).to eq 'user_rename' }
126+
it { expect(event_name(user, :failed_login)).to eq 'user_failed_login' }
114127
it { expect(event_name(project, :create)).to eq "project_create" }
115128
it { expect(event_name(project, :destroy)).to eq "project_destroy" }
116129
it { expect(event_name(project, :rename)).to eq "project_rename" }

0 commit comments

Comments
 (0)