Commit 4d64a32c authored by Douwe Maan's avatar Douwe Maan

Merge branch 'feature/ldap-sync-edgecases' into 'master'

LDAP Sync blocked user edgecases

Allow GitLab admins to block otherwise valid GitLab LDAP users
(https://gitlab.com/gitlab-org/gitlab-ce/issues/3462)

Based on the discussion on the original issue, we are going to differentiate "normal" block operations to the ldap automatic ones in order to make some decisions when its one or the other.

Expected behavior:

- [x] "ldap_blocked" users respond to both `blocked?` and `ldap_blocked?`
- [x] "ldap_blocked" users can't be unblocked by the Admin UI
- [x] "ldap_blocked" users can't be unblocked by the API
- [x] Block operations that are originated from LDAP synchronization will flag user as "ldap_blocked"
- [x] Only "ldap_blocked" users will be automatically unblocked by LDAP synchronization
- [x] When LDAP identity is removed, we should convert `ldap_blocked` into `blocked`
 
Mockup for the Admin UI with both "ldap_blocked" and normal "blocked" users:
![image](/uploads/4f56fc17b73cb2c9e2a154a22e7ad291/image.png)

There will be another MR for the EE version.

See merge request !2242
parents cda96354 dd6fc01f
...@@ -131,6 +131,12 @@ ...@@ -131,6 +131,12 @@
&:last-child { &:last-child {
margin-right: 0px; margin-right: 0px;
} }
&.btn-xs {
margin-right: 3px;
}
}
&.disabled {
pointer-events: auto !important;
} }
} }
......
...@@ -26,6 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController ...@@ -26,6 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController
def update def update
if @identity.update_attributes(identity_params) if @identity.update_attributes(identity_params)
RepairLdapBlockedUserService.new(@user).execute
redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.' redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.'
else else
render :edit render :edit
...@@ -34,6 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController ...@@ -34,6 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController
def destroy def destroy
if @identity.destroy if @identity.destroy
RepairLdapBlockedUserService.new(@user).execute
redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.' redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.'
else else
redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.' redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.'
......
...@@ -40,7 +40,9 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -40,7 +40,9 @@ class Admin::UsersController < Admin::ApplicationController
end end
def unblock def unblock
if user.activate if user.ldap_blocked?
redirect_back_or_admin_user(alert: "This user cannot be unlocked manually from GitLab")
elsif user.activate
redirect_back_or_admin_user(notice: "Successfully unblocked") redirect_back_or_admin_user(notice: "Successfully unblocked")
else else
redirect_back_or_admin_user(alert: "Error occurred. User was not unblocked") redirect_back_or_admin_user(alert: "Error occurred. User was not unblocked")
......
...@@ -18,4 +18,8 @@ class Identity < ActiveRecord::Base ...@@ -18,4 +18,8 @@ class Identity < ActiveRecord::Base
validates :provider, presence: true validates :provider, presence: true
validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider }
validates :user_id, uniqueness: { scope: :provider } validates :user_id, uniqueness: { scope: :provider }
def ldap?
provider.starts_with?('ldap')
end
end end
...@@ -196,10 +196,22 @@ class User < ActiveRecord::Base ...@@ -196,10 +196,22 @@ class User < ActiveRecord::Base
state_machine :state, initial: :active do state_machine :state, initial: :active do
event :block do event :block do
transition active: :blocked transition active: :blocked
transition ldap_blocked: :blocked
end
event :ldap_block do
transition active: :ldap_blocked
end end
event :activate do event :activate do
transition blocked: :active transition blocked: :active
transition ldap_blocked: :active
end
state :blocked, :ldap_blocked do
def blocked?
true
end
end end
end end
...@@ -207,7 +219,7 @@ class User < ActiveRecord::Base ...@@ -207,7 +219,7 @@ class User < ActiveRecord::Base
# Scopes # Scopes
scope :admins, -> { where(admin: true) } scope :admins, -> { where(admin: true) }
scope :blocked, -> { with_state(:blocked) } scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
scope :active, -> { with_state(:active) } scope :active, -> { with_state(:active) }
scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all } scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all }
scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') } scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') }
......
class RepairLdapBlockedUserService
attr_accessor :user
def initialize(user)
@user = user
end
def execute
user.block if ldap_hard_blocked?
end
private
def ldap_hard_blocked?
user.ldap_blocked? && !user.ldap_user?
end
end
...@@ -88,14 +88,19 @@ ...@@ -88,14 +88,19 @@
%i.fa.fa-envelope %i.fa.fa-envelope
= mail_to user.email, user.email, class: 'light' = mail_to user.email, user.email, class: 'light'
&nbsp; &nbsp;
= link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-xs" .pull-right
- unless user == current_user = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: 'btn-grouped btn btn-xs'
- if user.blocked? - unless user == current_user
= link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success" - if user.ldap_blocked?
- else = link_to '#', title: 'Cannot unblock LDAP blocked users', data: {toggle: 'tooltip'}, class: 'btn-grouped btn btn-xs btn-success disabled' do
= link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs btn-warning" %i.fa.fa-lock
- if user.access_locked? Unblock
= link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success", data: { confirm: 'Are you sure?' } - elsif user.blocked?
- if user.can_be_removed? = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success'
= link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: "btn btn-xs btn-remove" - else
= link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: 'btn-grouped btn btn-xs btn-warning'
- if user.access_locked?
= link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success', data: { confirm: 'Are you sure?' }
- if user.can_be_removed?
= link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: 'btn-grouped btn btn-xs btn-remove'
= paginate @users, theme: "gitlab" = paginate @users, theme: "gitlab"
...@@ -558,7 +558,8 @@ Parameters: ...@@ -558,7 +558,8 @@ Parameters:
- `uid` (required) - id of specified user - `uid` (required) - id of specified user
Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
`403 Forbidden` when trying to block an already blocked user by LDAP synchronization.
## Unblock user ## Unblock user
...@@ -572,4 +573,5 @@ Parameters: ...@@ -572,4 +573,5 @@ Parameters:
- `uid` (required) - id of specified user - `uid` (required) - id of specified user
Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
`403 Forbidden` when trying to unblock a user blocked by LDAP synchronization.
...@@ -284,10 +284,12 @@ module API ...@@ -284,10 +284,12 @@ module API
authenticated_as_admin! authenticated_as_admin!
user = User.find_by(id: params[:id]) user = User.find_by(id: params[:id])
if user if !user
not_found!('User')
elsif !user.ldap_blocked?
user.block user.block
else else
not_found!('User') forbidden!('LDAP blocked users cannot be modified by the API')
end end
end end
...@@ -299,10 +301,12 @@ module API ...@@ -299,10 +301,12 @@ module API
authenticated_as_admin! authenticated_as_admin!
user = User.find_by(id: params[:id]) user = User.find_by(id: params[:id])
if user if !user
user.activate
else
not_found!('User') not_found!('User')
elsif user.ldap_blocked?
forbidden!('LDAP blocked users cannot be unblocked by the API')
else
user.activate
end end
end end
end end
......
...@@ -37,15 +37,15 @@ module Gitlab ...@@ -37,15 +37,15 @@ module Gitlab
# Block user in GitLab if he/she was blocked in AD # Block user in GitLab if he/she was blocked in AD
if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter) if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter)
user.block user.ldap_block
false false
else else
user.activate if user.blocked? && !ldap_config.block_auto_created_users user.activate if user.ldap_blocked?
true true
end end
else else
# Block the user if they no longer exist in LDAP/AD # Block the user if they no longer exist in LDAP/AD
user.block user.ldap_block
false false
end end
rescue rescue
......
require 'spec_helper'
describe Admin::IdentitiesController do
let(:admin) { create(:admin) }
before { sign_in(admin) }
describe 'UPDATE identity' do
let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') }
it 'repairs ldap blocks' do
expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute)
put :update, user_id: user.username, id: user.ldap_identity.id, identity: { provider: 'twitter' }
end
end
describe 'DELETE identity' do
let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') }
it 'repairs ldap blocks' do
expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute)
delete :destroy, user_id: user.username, id: user.ldap_identity.id
end
end
end
...@@ -34,17 +34,34 @@ describe Admin::UsersController do ...@@ -34,17 +34,34 @@ describe Admin::UsersController do
end end
describe 'PUT unblock/:id' do describe 'PUT unblock/:id' do
let(:user) { create(:user) } context 'ldap blocked users' do
let(:user) { create(:omniauth_user, provider: 'ldapmain') }
before do
user.block before do
user.ldap_block
end
it 'will not unblock user' do
put :unblock, id: user.username
user.reload
expect(user.blocked?).to be_truthy
expect(flash[:alert]).to eq 'This user cannot be unlocked manually from GitLab'
end
end end
it 'unblocks user' do context 'manually blocked users' do
put :unblock, id: user.username let(:user) { create(:user) }
user.reload
expect(user.blocked?).to be_falsey before do
expect(flash[:notice]).to eq 'Successfully unblocked' user.block
end
it 'unblocks user' do
put :unblock, id: user.username
user.reload
expect(user.blocked?).to be_falsey
expect(flash[:notice]).to eq 'Successfully unblocked'
end
end end
end end
......
...@@ -13,64 +13,58 @@ describe Gitlab::LDAP::Access, lib: true do ...@@ -13,64 +13,58 @@ describe Gitlab::LDAP::Access, lib: true do
end end
it { is_expected.to be_falsey } it { is_expected.to be_falsey }
it 'should block user in GitLab' do it 'should block user in GitLab' do
access.allowed? access.allowed?
expect(user).to be_blocked expect(user).to be_blocked
expect(user).to be_ldap_blocked
end end
end end
context 'when the user is found' do context 'when the user is found' do
before do before do
allow(Gitlab::LDAP::Person). allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
to receive(:find_by_dn).and_return(:ldap_user)
end end
context 'and the user is disabled via active directory' do context 'and the user is disabled via active directory' do
before do before do
allow(Gitlab::LDAP::Person). allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(true)
to receive(:disabled_via_active_directory?).and_return(true)
end end
it { is_expected.to be_falsey } it { is_expected.to be_falsey }
it "should block user in GitLab" do it 'should block user in GitLab' do
access.allowed? access.allowed?
expect(user).to be_blocked expect(user).to be_blocked
expect(user).to be_ldap_blocked
end end
end end
context 'and has no disabled flag in active diretory' do context 'and has no disabled flag in active diretory' do
before do before do
user.block allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false)
allow(Gitlab::LDAP::Person).
to receive(:disabled_via_active_directory?).and_return(false)
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
context 'when auto-created users are blocked' do context 'when auto-created users are blocked' do
before do before do
allow_any_instance_of(Gitlab::LDAP::Config). user.block
to receive(:block_auto_created_users).and_return(true)
end end
it "does not unblock user in GitLab" do it 'does not unblock user in GitLab' do
access.allowed? access.allowed?
expect(user).to be_blocked expect(user).to be_blocked
expect(user).not_to be_ldap_blocked # this block is handled by omniauth not by our internal logic
end end
end end
context "when auto-created users are not blocked" do context 'when auto-created users are not blocked' do
before do before do
allow_any_instance_of(Gitlab::LDAP::Config). user.ldap_block
to receive(:block_auto_created_users).and_return(false)
end end
it "should unblock user in GitLab" do it 'should unblock user in GitLab' do
access.allowed? access.allowed?
expect(user).not_to be_blocked expect(user).not_to be_blocked
end end
...@@ -80,8 +74,7 @@ describe Gitlab::LDAP::Access, lib: true do ...@@ -80,8 +74,7 @@ describe Gitlab::LDAP::Access, lib: true do
context 'without ActiveDirectory enabled' do context 'without ActiveDirectory enabled' do
before do before do
allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true) allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
allow_any_instance_of(Gitlab::LDAP::Config). allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false)
to receive(:active_directory).and_return(false)
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
......
# == Schema Information
#
# Table name: identities
#
# id :integer not null, primary key
# extern_uid :string(255)
# provider :string(255)
# user_id :integer
# created_at :datetime
# updated_at :datetime
#
require 'spec_helper'
RSpec.describe Identity, models: true do
describe 'relations' do
it { is_expected.to belong_to(:user) }
end
describe 'fields' do
it { is_expected.to respond_to(:provider) }
it { is_expected.to respond_to(:extern_uid) }
end
describe '#is_ldap?' do
let(:ldap_identity) { create(:identity, provider: 'ldapmain') }
let(:other_identity) { create(:identity, provider: 'twitter') }
it 'returns true if it is a ldap identity' do
expect(ldap_identity.ldap?).to be_truthy
end
it 'returns false if it is not a ldap identity' do
expect(other_identity.ldap?).to be_falsey
end
end
end
...@@ -569,27 +569,39 @@ describe User, models: true do ...@@ -569,27 +569,39 @@ describe User, models: true do
end end
end end
describe :ldap_user? do context 'ldap synchronized user' do
it "is true if provider name starts with ldap" do describe :ldap_user? do
user = create(:omniauth_user, provider: 'ldapmain') it 'is true if provider name starts with ldap' do
expect( user.ldap_user? ).to be_truthy user = create(:omniauth_user, provider: 'ldapmain')
end expect(user.ldap_user?).to be_truthy
end
it "is false for other providers" do it 'is false for other providers' do
user = create(:omniauth_user, provider: 'other-provider') user = create(:omniauth_user, provider: 'other-provider')
expect( user.ldap_user? ).to be_falsey expect(user.ldap_user?).to be_falsey
end
it 'is false if no extern_uid is provided' do
user = create(:omniauth_user, extern_uid: nil)
expect(user.ldap_user?).to be_falsey
end
end end
it "is false if no extern_uid is provided" do describe :ldap_identity do
user = create(:omniauth_user, extern_uid: nil) it 'returns ldap identity' do
expect( user.ldap_user? ).to be_falsey user = create :omniauth_user
expect(user.ldap_identity.provider).not_to be_empty
end
end end
end
describe :ldap_identity do describe '#ldap_block' do
it "returns ldap identity" do let(:user) { create(:omniauth_user, provider: 'ldapmain', name: 'John Smith') }
user = create :omniauth_user
expect(user.ldap_identity.provider).not_to be_empty it 'blocks user flaging the action caming from ldap' do
user.ldap_block
expect(user.blocked?).to be_truthy
expect(user.ldap_blocked?).to be_truthy
end
end end
end end
......
...@@ -8,6 +8,8 @@ describe API::API, api: true do ...@@ -8,6 +8,8 @@ describe API::API, api: true do
let(:key) { create(:key, user: user) } let(:key) { create(:key, user: user) }
let(:email) { create(:email, user: user) } let(:email) { create(:email, user: user) }
let(:omniauth_user) { create(:omniauth_user) } let(:omniauth_user) { create(:omniauth_user) }
let(:ldap_user) { create(:omniauth_user, provider: 'ldapmain') }
let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
describe "GET /users" do describe "GET /users" do
context "when unauthenticated" do context "when unauthenticated" do
...@@ -783,6 +785,12 @@ describe API::API, api: true do ...@@ -783,6 +785,12 @@ describe API::API, api: true do
expect(user.reload.state).to eq('blocked') expect(user.reload.state).to eq('blocked')
end end
it 'should not re-block ldap blocked users' do
put api("/users/#{ldap_blocked_user.id}/block", admin)
expect(response.status).to eq(403)
expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
end
it 'should not be available for non admin users' do it 'should not be available for non admin users' do
put api("/users/#{user.id}/block", user) put api("/users/#{user.id}/block", user)
expect(response.status).to eq(403) expect(response.status).to eq(403)
...@@ -797,7 +805,9 @@ describe API::API, api: true do ...@@ -797,7 +805,9 @@ describe API::API, api: true do
end end
describe 'PUT /user/:id/unblock' do describe 'PUT /user/:id/unblock' do
let(:blocked_user) { create(:user, state: 'blocked') }
before { admin } before { admin }
it 'should unblock existing user' do it 'should unblock existing user' do
put api("/users/#{user.id}/unblock", admin) put api("/users/#{user.id}/unblock", admin)
expect(response.status).to eq(200) expect(response.status).to eq(200)
...@@ -805,12 +815,15 @@ describe API::API, api: true do ...@@ -805,12 +815,15 @@ describe API::API, api: true do
end end
it 'should unblock a blocked user' do it 'should unblock a blocked user' do
put api("/users/#{user.id}/block", admin) put api("/users/#{blocked_user.id}/unblock", admin)
expect(response.status).to eq(200)
expect(user.reload.state).to eq('blocked')
put api("/users/#{user.id}/unblock", admin)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(user.reload.state).to eq('active') expect(blocked_user.reload.state).to eq('active')
end
it 'should not unblock ldap blocked users' do
put api("/users/#{ldap_blocked_user.id}/unblock", admin)
expect(response.status).to eq(403)
expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
end end
it 'should not be available for non admin users' do it 'should not be available for non admin users' do
......
require 'spec_helper'
describe RepairLdapBlockedUserService, services: true do
let(:user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
let(:identity) { user.ldap_identity }
subject(:service) { RepairLdapBlockedUserService.new(user) }
describe '#execute' do
it 'change to normal block after destroying last ldap identity' do
identity.destroy
service.execute
expect(user.reload).not_to be_ldap_blocked
end
it 'change to normal block after changing last ldap identity to another provider' do
identity.update_attribute(:provider, 'twitter')
service.execute
expect(user.reload).not_to be_ldap_blocked
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment