From 591881ccf64a0cb060ea1086715fe470ce497c27 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@znuny.com>
Date: Thu, 27 Apr 2017 10:49:03 +0200
Subject: [PATCH] Working on issue #350 - issuecomment-295252402 - Users lock
 theirselfs out if they provide no role mapping.

---
 lib/import/ldap/user.rb           | 49 +++++++++++++++++++++++++++----
 spec/lib/import/ldap/user_spec.rb | 15 +++++++++-
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/lib/import/ldap/user.rb b/lib/import/ldap/user.rb
index 7d2376ed7..a71415aea 100644
--- a/lib/import/ldap/user.rb
+++ b/lib/import/ldap/user.rb
@@ -31,7 +31,6 @@ module Import
 
       def create_or_update(resource, *args)
         return if skip?(resource)
-        resource[:role_ids] = role_ids(resource)
         result = super(resource, *args)
 
         ldap_log(
@@ -58,14 +57,49 @@ module Import
         return @signup_role_ids if !dn
         # check if roles are mapped for the found dn
         roles = @dn_roles[ dn.downcase ]
-        # use signup roles if no mapped roles were found
-        return @signup_role_ids if !roles
         # return found roles
-        roles
+        return roles if roles
+        # return signup roles if there is a role mapping in general
+        # this makes the LDAP the leading source of roles
+        return @signup_role_ids if @dn_roles.present?
+
+        # special case: there is no LDAP role mapping configured
+        #
+        # if there is no role mapping in general the signup roles
+        # should only get used for newly created users (see .create method)
+        # otherwise users might overwrite their local assigned
+        # roles with less privileged roles and lock theirselfs out
+        #
+        # see issue 350#issuecomment-295252402
+        #
+        # this method only gets called from the .updated? method
+        # so we won't return any roles which will keep the current
+        # role assignment for the User
+        # the signup roles will be taken in this case direcly in
+        # the .create method
+        nil
       end
 
       def updated?(resource, *_args)
-        super
+
+        # this is needed for the special case described in the role_ids method
+        #
+        # assign roles only if there are any found in the mapping
+        # otherwise keep those stored to the User
+        update_roles        = role_ids(resource)
+        resource[:role_ids] = update_roles if update_roles
+
+        user_found = super
+
+        # in case a User was found and we had no roles
+        # to set/update we have to note the currently
+        # assigned roles so that our action check won't
+        # falsly detect changes to the User
+        if user_found && update_roles.blank?
+          resource[:role_ids] = @resource.role_ids
+        end
+
+        user_found
       rescue => e
         ldap_log(
           action:   "update -> #{resource[:login]}",
@@ -122,6 +156,11 @@ module Import
       end
 
       def create(resource, *_args)
+
+        # this is needed for the special case described in the role_ids method
+        #
+        # in case we have no role IDs yet we have to fall back to the signup roles
+        resource[:role_ids] ||= @signup_role_ids
         super
       rescue => e
         ldap_log(
diff --git a/spec/lib/import/ldap/user_spec.rb b/spec/lib/import/ldap/user_spec.rb
index 69f46ad22..f112b4ecd 100644
--- a/spec/lib/import/ldap/user_spec.rb
+++ b/spec/lib/import/ldap/user_spec.rb
@@ -99,7 +99,12 @@ RSpec.describe Import::Ldap::User do
   context 'update' do
 
     before(:each) do
-      user = create(:user, login: uid)
+      user = create(:user,
+                    login:    uid,
+                    role_ids: [
+                      Role.find_by(name: 'Agent').id,
+                      Role.find_by(name: 'Admin').id
+                    ])
 
       ExternalSync.create(
         source:    'Ldap::User',
@@ -119,6 +124,14 @@ RSpec.describe Import::Ldap::User do
       }
     end
 
+    it "doesn't change roles if no role mapping is configured" do
+      expect do
+        described_class.new(user_entry, ldap_config, {}, signup_role_ids)
+      end.to not_change {
+        User.last.role_ids
+      }
+    end
+
     it 'creates an HTTP Log entry' do
       expect do
         described_class.new(user_entry, ldap_config, user_roles, signup_role_ids)