Merge pull request #18 from alan-turing-institute/fix-missing-library

Use ldap3 instead of python-ldap

.github/workflows/publish_docker.yaml

@@ -18,24 +18,24 @@ jobs:
       contents: read
     steps:
       - name: Check out the repo
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Log in to the Container registry
-        uses: docker/login-action@v2
+        uses: docker/login-action@v3
         with:
           registry: ghcr.io
           username: ${{ github.actor }}
           password: ${{ secrets.GITHUB_TOKEN }}
 
       - name: Extract metadata (tags, labels) for Docker
         id: meta
-        uses: docker/metadata-action@v4
+        uses: docker/metadata-action@v5
         with:
           images: |
             ghcr.io/${{ github.repository }}
 
       - name: Build and push Docker images
-        uses: docker/build-push-action@v4
+        uses: docker/build-push-action@v6
         with:
           push: true
           tags: ${{ steps.meta.outputs.tags }}

guacamole_user_sync/ldap/ldap_client.py

@@ -1,14 +1,19 @@
 import logging
+from typing import cast
 
-import ldap
-from ldap.asyncsearch import List as AsyncSearchList
-from ldap.ldapobject import LDAPObject
+from ldap3 import ALL, ALL_ATTRIBUTES, Connection, Server
+from ldap3.abstract.entry import Entry
+from ldap3.core.exceptions import (
+    LDAPBindError,
+    LDAPException,
+    LDAPSessionTerminatedByServerError,
+    LDAPSocketOpenError,
+)
 
 from guacamole_user_sync.models import (
     LDAPError,
     LDAPGroup,
     LDAPQuery,
-    LDAPSearchResult,
     LDAPUser,
 )
 
@@ -22,85 +27,93 @@ def __init__(
         self,
         hostname: str,
         *,
+        auto_bind: bool = True,
         bind_dn: str | None = None,
         bind_password: str | None = None,
     ) -> None:
-        self.cnxn: LDAPObject | None = None
+        self.auto_bind = auto_bind
         self.bind_dn = bind_dn
         self.bind_password = bind_password
-        self.hostname = hostname
+        self.server = Server(hostname, get_info=ALL)
 
-    def connect(self) -> LDAPObject:
-        if not self.cnxn:
-            logger.info("Initialising connection to LDAP host at %s", self.hostname)
-            self.cnxn = ldap.initialize(f"ldap://{self.hostname}")
-            if self.bind_dn:
-                try:
-                    self.cnxn.simple_bind_s(self.bind_dn, self.bind_password)
-                except ldap.INVALID_CREDENTIALS as exc:
-                    logger.warning("Connection credentials were incorrect.")
-                    raise LDAPError from exc
-        return self.cnxn
+    @staticmethod
+    def as_list(ldap_entry: str | list[str] | None) -> list[str]:
+        if isinstance(ldap_entry, list):
+            return ldap_entry
+        if ldap_entry is None:
+            return []
+        if isinstance(ldap_entry, str):
+            return [ldap_entry]
+        msg = f"Unexpected input {ldap_entry} of type {type(ldap_entry)}"
+        raise ValueError(msg)
+
+    def connect(self) -> Connection:
+        logger.info("Initialising connection to LDAP host at %s", self.server.host)
+        try:
+            return Connection(
+                self.server,
+                user=self.bind_dn,
+                password=self.bind_password,
+                auto_bind=self.auto_bind,
+            )
+        except LDAPSocketOpenError as exc:
+            msg = "Server could not be reached."
+            logger.exception(msg, exc_info=exc)
+            raise LDAPError(msg) from exc
+        except LDAPBindError as exc:
+            msg = "Connection credentials were incorrect."
+            logger.exception(msg, exc_info=exc)
+            raise LDAPError(msg) from exc
+        except LDAPException as exc:
+            msg = f"Unexpected LDAP exception of type {type(exc)}."
+            logger.exception(msg, exc_info=exc)
+            raise LDAPError(msg) from exc
 
     def search_groups(self, query: LDAPQuery) -> list[LDAPGroup]:
         output = []
-        for result in self.search(query):
-            attr_dict = result[1][1]
+        for entry in self.search(query):
             output.append(
                 LDAPGroup(
-                    member_of=[
-                        group.decode("utf-8") for group in attr_dict["memberOf"]
-                    ],
-                    member_uid=[
-                        group.decode("utf-8") for group in attr_dict["memberUid"]
-                    ],
-                    name=attr_dict[query.id_attr][0].decode("utf-8"),
+                    member_of=self.as_list(entry.memberOf.value),
+                    member_uid=self.as_list(entry.memberUid.value),
+                    name=getattr(entry, query.id_attr).value,
                 ),
             )
+            logger.debug("Found LDAP group %s", output[-1])
         logger.debug("Loaded %s LDAP groups", len(output))
         return output
 
     def search_users(self, query: LDAPQuery) -> list[LDAPUser]:
         output = []
-        for result in self.search(query):
-            attr_dict = result[1][1]
+        for entry in self.search(query):
             output.append(
                 LDAPUser(
-                    display_name=attr_dict["displayName"][0].decode("utf-8"),
-                    member_of=[
-                        group.decode("utf-8") for group in attr_dict["memberOf"]
-                    ],
-                    name=attr_dict[query.id_attr][0].decode("utf-8"),
-                    uid=attr_dict["uid"][0].decode("utf-8"),
+                    display_name=entry.displayName.value,
+                    member_of=self.as_list(entry.memberOf.value),
+                    name=getattr(entry, query.id_attr).value,
+                    uid=entry.uid.value,
                 ),
             )
+            logger.debug("Found LDAP user %s", output[-1])
         logger.debug("Loaded %s LDAP users", len(output))
         return output
 
-    def search(self, query: LDAPQuery) -> LDAPSearchResult:
-        results: LDAPSearchResult = []
+    def search(self, query: LDAPQuery) -> list[Entry]:
         logger.info("Querying LDAP host with:")
         logger.info("... base DN: %s", query.base_dn)
         logger.info("... filter: %s", query.filter)
-        searcher = AsyncSearchList(self.connect())
         try:
-            searcher.startSearch(
-                query.base_dn,
-                ldap.SCOPE_SUBTREE,
-                query.filter,
-            )
-            if searcher.processResults() != 0:
-                logger.warning("Only partial results received.")
-        except ldap.NO_SUCH_OBJECT as exc:
-            logger.warning("Server returned no results.")
-            raise LDAPError from exc
-        except ldap.SERVER_DOWN as exc:
-            logger.warning("Server could not be reached.")
-            raise LDAPError from exc
-        except ldap.SIZELIMIT_EXCEEDED as exc:
-            logger.warning("Server-side size limit exceeded.")
-            raise LDAPError from exc
+            connection = self.connect()
+            connection.search(query.base_dn, query.filter, attributes=ALL_ATTRIBUTES)
+        except LDAPSessionTerminatedByServerError as exc:
+            msg = "Server terminated LDAP request."
+            logger.exception(msg, exc_info=exc)
+            raise LDAPError(msg) from exc
+        except LDAPException as exc:
+            msg = f"Unexpected LDAP exception of type {type(exc)}."
+            logger.exception(msg, exc_info=exc)
+            raise LDAPError(msg) from exc
         else:
-            results = searcher.allResults
+            results = cast(list[Entry], connection.entries)
             logger.debug("Server returned %s results.", len(results))
             return results

guacamole_user_sync/models/__init__.py

@@ -3,14 +3,11 @@
 from .ldap_objects import LDAPGroup, LDAPUser
 from .ldap_query import LDAPQuery
 
-LDAPSearchResult = list[tuple[int, tuple[str, dict[str, list[bytes]]]]]
-
 __all__ = [
     "GuacamoleUserDetails",
     "LDAPError",
     "LDAPGroup",
     "LDAPQuery",
-    "LDAPSearchResult",
     "LDAPUser",
     "PostgreSQLError",
 ]

guacamole_user_sync/postgresql/orm.py

@@ -23,7 +23,9 @@ class GuacamoleEntity(GuacamoleBase):
 
     entity_id: Mapped[int] = mapped_column(Integer, primary_key=True)
     name: Mapped[str] = mapped_column(String(128))
-    type: Mapped[GuacamoleEntityType] = mapped_column(Enum(GuacamoleEntityType))
+    type: Mapped[GuacamoleEntityType] = mapped_column(
+        Enum(GuacamoleEntityType, name="guacamole_entity_type"),
+    )
 
 
 class GuacamoleUser(GuacamoleBase):

guacamole_user_sync/postgresql/postgresql_client.py

@@ -77,7 +77,8 @@ def assign_users_to_groups(
                     )
                 )
                 logger.debug(
-                    "-> entity_id: %s; user_group_id: %s",
+                    "Group '%s' has entity_id: %s and user_group_id: %s",
+                    group.name,
                     group_entity_id,
                     user_group_id,
                 )
@@ -88,6 +89,11 @@ def assign_users_to_groups(
                 )
                 continue
             # Get the user_entity_id for each user belonging to this group
+            logger.debug(
+                "Group '%s' has %s member(s).",
+                group.name,
+                len(group.member_uid),
+            )
             for user_uid in group.member_uid:
                 try:
                     user = next(filter(lambda u: u.uid == user_uid, users))
@@ -105,7 +111,7 @@ def assign_users_to_groups(
                     )
                     logger.debug(
                         "... group member '%s' has entity_id '%s'",
-                        user,
+                        user.name,
                         user_entity_id,
                     )
                 except StopIteration:

pyproject.toml

@@ -21,8 +21,8 @@ classifiers = [
     "Programming Language :: Python :: Implementation :: PyPy",
 ]
 dependencies = [
+    "ldap3==2.9.1",
     "psycopg==3.2.1",
-    "python-ldap==3.4.4",
     "SQLAlchemy==2.0.32",
     "sqlparse==0.5.1",
 ]
@@ -92,7 +92,7 @@ strict = true                        # enable all optional error checking flags
 
 [[tool.mypy.overrides]]
 module = [
-    "ldap.*",
+    "ldap3.*",
     "pytest.*",
     "sqlalchemy.*",
     "sqlparse.*",

tests/conftest.py

@@ -2,14 +2,16 @@
 
 import pytest
 
-from guacamole_user_sync.models import LDAPGroup, LDAPQuery, LDAPSearchResult, LDAPUser
+from guacamole_user_sync.models import LDAPGroup, LDAPQuery, LDAPUser
 from guacamole_user_sync.postgresql.orm import (
     GuacamoleEntity,
     GuacamoleEntityType,
     GuacamoleUser,
     GuacamoleUserGroup,
 )
 
+from .mocks import MockLDAPGroupEntry, MockLDAPUserEntry
+
 
 @pytest.fixture
 def ldap_model_groups_fixture() -> list[LDAPGroup]:
@@ -69,70 +71,45 @@ def ldap_query_users_fixture() -> LDAPQuery:
 
 
 @pytest.fixture
-def ldap_response_groups_fixture() -> LDAPSearchResult:
+def ldap_response_groups_fixture() -> list[MockLDAPGroupEntry]:
     return [
-        (
-            0,
-            (
-                "CN=plaintiffs,OU=groups,DC=rome,DC=la",
-                {
-                    "cn": [b"plaintiffs"],
-                    "memberOf": [],
-                    "memberUid": [b"aulus.agerius"],
-                },
-            ),
-        ),
-        (
-            1,
-            (
-                "CN=defendants,OU=groups,DC=rome,DC=la",
-                {
-                    "cn": [b"defendants"],
-                    "memberOf": [],
-                    "memberUid": [b"numerius.negidius"],
-                },
-            ),
-        ),
-        (
-            2,
-            (
-                "CN=everyone,OU=groups,DC=rome,DC=la",
-                {
-                    "cn": [b"everyone"],
-                    "memberOf": [],
-                    "memberUid": [b"aulus.agerius", b"numerius.negidius"],
-                },
-            ),
+        MockLDAPGroupEntry(
+            dn="CN=plaintiffs,OU=groups,DC=rome,DC=la",
+            cn="plaintiffs",
+            memberOf=[],
+            memberUid=["aulus.agerius"],
+        ),
+        MockLDAPGroupEntry(
+            dn="CN=defendants,OU=groups,DC=rome,DC=la",
+            cn="defendants",
+            memberOf=[],
+            memberUid=["numerius.negidius"],
+        ),
+        MockLDAPGroupEntry(
+            dn="CN=everyone,OU=groups,DC=rome,DC=la",
+            cn="everyone",
+            memberOf=[],
+            memberUid=["aulus.agerius", "numerius.negidius"],
         ),
     ]
 
 
 @pytest.fixture
-def ldap_response_users_fixture() -> LDAPSearchResult:
+def ldap_response_users_fixture() -> list[MockLDAPUserEntry]:
     return [
-        (
-            0,
-            (
-                "CN=aulus.agerius,OU=users,DC=rome,DC=la",
-                {
-                    "displayName": [b"Aulus Agerius"],
-                    "memberOf": [b"CN=plaintiffs,OU=groups,DC=rome,DC=la"],
-                    "uid": [b"aulus.agerius"],
-                    "userName": [b"[email protected]"],
-                },
-            ),
-        ),
-        (
-            1,
-            (
-                "CN=numerius.negidius,OU=users,DC=rome,DC=la",
-                {
-                    "displayName": [b"Numerius Negidius"],
-                    "memberOf": [b"CN=defendants,OU=groups,DC=rome,DC=la"],
-                    "uid": [b"numerius.negidius"],
-                    "userName": [b"[email protected]"],
-                },
-            ),
+        MockLDAPUserEntry(
+            dn="CN=aulus.agerius,OU=users,DC=rome,DC=la",
+            displayName="Aulus Agerius",
+            memberOf=["CN=plaintiffs,OU=groups,DC=rome,DC=la"],
+            uid="aulus.agerius",
+            userName="[email protected]",
+        ),
+        MockLDAPUserEntry(
+            dn="CN=numerius.negidius,OU=users,DC=rome,DC=la",
+            displayName="Numerius Negidius",
+            memberOf=["CN=defendants,OU=groups,DC=rome,DC=la"],
+            uid="numerius.negidius",
+            userName="[email protected]",
         ),
     ]