#389 Add support for multiple LDAP queries
Merged a month ago by vmaljulin. Opened a month ago by vmaljulin.
vmaljulin/waiverdb RHELWF-1187  into  master

file modified
+12 -12
@@ -12,12 +12,12 @@ 

      r = client.get('/api/v1.0/metrics')

  

      assert r.status_code == 200

-     assert len([l for l in r.get_data(as_text=True).splitlines()

-                 if l.startswith('# TYPE messaging_')

-                 and l.endswith(' counter')]) == 4

-     assert len([l for l in r.get_data(as_text=True).splitlines()

-                 if l.startswith('# TYPE db_')

-                 and l.endswith(' counter')]) == 4

+     assert len([line for line in r.get_data(as_text=True).splitlines()

+                 if line.startswith('# TYPE messaging_')

+                 and line.endswith(' counter')]) == 4

+     assert len([line for line in r.get_data(as_text=True).splitlines()

+                 if line.startswith('# TYPE db_')

+                 and line.endswith(' counter')]) == 4

  

  

  def test_standalone_metrics_server_disabled_by_default():
@@ -31,9 +31,9 @@ 

  

      r = requests.get('http://127.0.0.1:10040/metrics')

  

-     assert len([l for l in r.text.splitlines()

-                 if l.startswith('# TYPE messaging_')

-                 and l.endswith(' counter')]) == 4

-     assert len([l for l in r.text.splitlines()

-                 if l.startswith('# TYPE db_')

-                 and l.endswith(' counter')]) == 4

+     assert len([line for line in r.text.splitlines()

+                 if line.startswith('# TYPE messaging_')

+                 and line.endswith(' counter')]) == 4

+     assert len([line for line in r.text.splitlines()

+                 if line.startswith('# TYPE db_')

+                 and line.endswith(' counter')]) == 4

file modified
+6 -2
@@ -334,8 +334,12 @@ 

              return True

  

          ldap_host = current_app.config.get('LDAP_HOST')

-         ldap_base = current_app.config.get('LDAP_BASE')

-         return verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_base)

+         ldap_searches = current_app.config.get('LDAP_SEARCHES')

+         if not ldap_searches:

+             ldap_base = current_app.config.get('LDAP_BASE')

+             ldap_search_string = current_app.config.get('LDAP_SEARCH_STRING', '(memberUid={user})')

+             ldap_searches = [{'BASE': ldap_base, 'SEARCH_STRING': ldap_search_string}]

+         return verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_searches)

  

      def _create_waiver(self, args, user):

          proxied_by = None

file modified
+30 -19
@@ -12,28 +12,27 @@ 

  log = logging.getLogger(__name__)

  

  

- def get_group_membership(user, ldap_host, ldap_base):

+ def get_group_membership(ldap, user, con, ldap_search):

      try:

-         import ldap

-     except ImportError:

-         raise InternalServerError(('If PERMISSION_MAPPING is defined, '

-                                    'python-ldap needs to be installed.'))

- 

-     try:

-         con = ldap.initialize(ldap_host)

-         results = con.search_s(ldap_base, ldap.SCOPE_SUBTREE, f'(memberUid={user})', ['cn'])

+         results = con.search_s(

+             ldap_search['BASE'], ldap.SCOPE_SUBTREE,

+             ldap_search.get('SEARCH_STRING', '(memberUid={user})').format(user), ['cn']

+         )

          return [group[1]['cn'][0].decode('utf-8') for group in results]

-     except ldap.LDAPError:

-         log.exception('Some error occurred initializing the LDAP connection.')

-         raise Unauthorized('Some error occurred initializing the LDAP connection.')

+     except KeyError:

+         log.exception('LDAP_SEARCHES parameter should contain the BASE key')

+         raise InternalServerError('LDAP_SEARCHES parameter should contain the BASE key')

      except ldap.SERVER_DOWN:

          log.exception('The LDAP server is not reachable.')

          raise BadGateway('The LDAP server is not reachable.')

+     except ldap.LDAPError:

+         log.exception('Some error occurred initializing the LDAP connection.')

+         raise Unauthorized('Some error occurred initializing the LDAP connection.')

  

  

- def verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_base):

-     if not (ldap_host and ldap_base):

-         raise InternalServerError(('LDAP_HOST and LDAP_BASE also need to be defined '

+ def verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_searches):

+     if not (ldap_host and ldap_searches):

+         raise InternalServerError(('LDAP_HOST and LDAP_SEARCHES also need to be defined '

                                     'if PERMISSION_MAPPING is defined.'))

  

      allowed_groups = []
@@ -45,12 +44,24 @@ 

                  return True

              allowed_groups += permission['groups']

  

-     group_membership = get_group_membership(user, ldap_host, ldap_base)

+     try:

+         import ldap

+     except ImportError:

+         raise InternalServerError(('If PERMISSION_MAPPING is defined, '

+                                    'python-ldap needs to be installed.'))

+ 

+     con = ldap.initialize(ldap_host)

+     group_membership = set()

+ 

+     for cur_ldap_search in ldap_searches:

+         group_membership.update(

+             get_group_membership(ldap, user, con, cur_ldap_search)

+         )

+         if group_membership & set(allowed_groups):

+             return True

+ 

      if not group_membership:

          raise Unauthorized(f'Couldn\'t find user {user} in LDAP')

  

-     if set(group_membership) & set(allowed_groups):

-         return True

- 

      raise Unauthorized(('You are not authorized to submit a waiver '

                          f'for the test case {testcase}'))

no initial comment

There is always one host. I don't see a reason why multiple hosts would be needed.

It would be also faster if ldap.initialize(ldap_host) is called only once in waiverdb request.

Better to have a single new option with both base and filter values. E.g.:

LDAP_SEARCHES = [
    {
        "base": "ou=Groups,dc=example,dc=com",
        "filter": "(memberUid={user})"},
    },
    # ...
]

You would just need to handle the backwards compatibility elsewhere.

If you check the set(group_membership) & set(allowed_groups) here, you can avoid doing any following LDAP queries.

1 new commit added

  • fixup! Rename the variable in test_monitor.py according to PEP8
a month ago

Pull-Request has been merged by vmaljulin

a month ago