LDAP stopped working after update (master branch), PR provided

After performing bench update trying to login via LDAP results in freezing login window with constant Verifying... message. Previous update was on May 31st (apparently Frappe v11.1.32).

Current version:
ERPNext: v11.1.46 (master)
Frappe Framework: v11.1.41 (master)

screen

Before last update it was working fine.
Except that similar freeze was seen for one specific user.

BUG FILLED

UPDATE

Frappe stack trace on LDAP login attempt:

Request Error
Traceback (most recent call last):
  File "/home/frappe/frappe-bench/apps/frappe/frappe/app.py", line 61, in application
    response = frappe.handler.handle()
  File "/home/frappe/frappe-bench/apps/frappe/frappe/handler.py", line 21, in handle
    data = execute_cmd(cmd)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/handler.py", line 56, in execute_cmd
    return frappe.call(method, **frappe.form_dict)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/__init__.py", line 1036, in call
    return fn(*args, **newargs)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 206, in login
    user = ldap.authenticate(frappe.as_unicode(args.usr), frappe.as_unicode(args.pwd))
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 172, in authenticate
    return self.create_or_update_user(self.convert_ldap_entry_to_dict(user), groups=groups)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 123, in create_or_update_user
    self.sync_roles(user, groups)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 89, in sync_roles
    lower_groups = [g.lower() for g in additional_groups]
TypeError: 'NoneType' object is not iterable
[ERROR] 2019-07-12 15:51:19,926 | /home/frappe/frappe-bench/apps/frappe/frappe/utils/error.py:
Could not take error snapshot: startswith first arg must be str or a tuple of str, not bytes
Traceback (most recent call last):
  File "/home/frappe/frappe-bench/apps/frappe/frappe/app.py", line 61, in application
    response = frappe.handler.handle()
  File "/home/frappe/frappe-bench/apps/frappe/frappe/handler.py", line 21, in handle
    data = execute_cmd(cmd)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/handler.py", line 56, in execute_cmd
    return frappe.call(method, **frappe.form_dict)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/__init__.py", line 1036, in call
    return fn(*args, **newargs)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 206, in login
    user = ldap.authenticate(frappe.as_unicode(args.usr), frappe.as_unicode(args.pwd))
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 172, in authenticate
    return self.create_or_update_user(self.convert_ldap_entry_to_dict(user), groups=groups)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 123, in create_or_update_user
    self.sync_roles(user, groups)
  File "/home/frappe/frappe-bench/apps/frappe/frappe/integrations/doctype/ldap_settings/ldap_settings.py", line 89, in sync_roles
    lower_groups = [g.lower() for g in additional_groups]
TypeError: 'NoneType' object is not iterable

BUG

additional_groups is not tested for being None befor being iterated. See here:

So replacing this

def sync_roles(self, user, additional_groups=None):

with this

def sync_roles(self, user, additional_groups=[]):

should fix it.

There are three places where group var should be changed to [] for it to work.

diff --git a/frappe/integrations/doctype/ldap_settings/ldap_settings.py b/frappe/integrations/doctype/ldap_settings/ldap_settings.py
index cfe2a26..1c9fd0c 100644
--- a/frappe/integrations/doctype/ldap_settings/ldap_settings.py
+++ b/frappe/integrations/doctype/ldap_settings/ldap_settings.py
@@ -78,7 +78,7 @@ class LDAPSettings(Document):
                        setattr(user, key, value)
                user.save(ignore_permissions=True)

-       def sync_roles(self, user, additional_groups=None):
+       def sync_roles(self, user, additional_groups=[]):

                current_roles = set([d.role for d in user.get("roles")])

@@ -99,7 +99,7 @@ class LDAPSettings(Document):

                user.remove_roles(*roles_to_remove)

-       def create_or_update_user(self, user_data, groups=None):
+       def create_or_update_user(self, user_data, groups=[]):
                user = None
                if frappe.db.exists("User", user_data['email']):
                        user = frappe.get_doc("User", user_data['email'])
@@ -160,7 +160,7 @@ class LDAPSettings(Document):
                        # only try and connect as the user, once we have their fqdn entry.
                        self.connect_to_ldap(base_dn=user.entry_dn, password=password)

-                       groups = None
+                       groups = []
                        if self.ldap_group_field:
                                groups = getattr(user, self.ldap_group_field).values

Are you guys going to make the PR?

I’ll make one.

1 Like

PULL REQUEST

@rmeyer, apparently codacy doesn’t like function argument to have empty array as default value. Fixed with None check, should be fine now.

1 Like