From 83750a48fd1daadf345efc680c481061ad385ec5 Mon Sep 17 00:00:00 2001
From: Rob van der Linde <robvdl@gmail.com>
Date: Sat, 9 Mar 2024 10:41:56 +1300
Subject: [PATCH] login: bubble up LdbError so that it can be handled better

Step one is to change connect_samdb, also placing host as the first argument for consistency.

connect_samdb now expects host, username and password to be strings and won't do checking itself anymore if they are present.

It will no longer catch LdbError in connect_samdb either and just let it raise instead. This is so that it can be handled further up the chain.

In get_samdb, which provides the request.samdb property, we keep the return type as Optional[SamDB], but only return None if there are no credentials in the session at all (it raises KeyError).

If the credentials in the session worked before, but now no longer work, for example server is gone, just let it raise LdbError in get_samdb because we need to know at this point when that happens rather than return None (which means not authenticated).
---
 src/sambal/client.py   | 74 ++++++++++++++++++++++++------------------
 src/sambal/security.py | 18 ++++++----
 2 files changed, 53 insertions(+), 39 deletions(-)

diff --git a/src/sambal/client.py b/src/sambal/client.py
index 63e763f..273c111 100644
--- a/src/sambal/client.py
+++ b/src/sambal/client.py
@@ -1,52 +1,62 @@
 from typing import Optional
 
-from ldb import LdbError
 from samba.auth import system_session
 from samba.credentials import Credentials
 from samba.param import LoadParm
 from samba.samdb import SamDB
 
 
-def connect_samdb(username, password, host, realm=None) -> Optional[SamDB]:
-    """Connect to Samba or Windows host and return SamDB on success."""
-    if host and username and password:
-        if host.startswith(("ldap://", "ldaps://")):
-            url = host
-        else:
-            url = f"ldap://{host}"
+def connect_samdb(host, username, password, realm=None) -> SamDB:
+    """Connect to Samba or Windows host and return SamDB on success.
 
-        lp = LoadParm()
-        lp.load_default()
+    :param host: Host name or URL
+    :param username: Account name
+    :param password: Account password
+    :param realm: Optional realm
+    :raises LdbError: on failure, caller should handle error.
+    """
+    if host.startswith(("ldap://", "ldaps://")):
+        url = host
+    else:
+        url = f"ldap://{host}"
+
+    lp = LoadParm()
+    lp.load_default()
 
-        creds = Credentials()
-        creds.set_username(username)
-        creds.set_password(password)
+    creds = Credentials()
+    creds.set_username(username)
+    creds.set_password(password)
 
-        if realm:
-            creds.set_realm(realm)
+    if realm:
+        creds.set_realm(realm)
 
-        try:
-            return SamDB(
-                url=url,
-                session_info=system_session(),
-                credentials=creds,
-                lp=lp,
-            )
-        except LdbError:
-            return None
+    return SamDB(
+        url=url,
+        session_info=system_session(),
+        credentials=creds,
+        lp=lp,
+    )
 
 
 def get_samdb(request) -> Optional[SamDB]:
     """Returns a SamDB connection to be used via the request.samdb property.
 
+    Fetch credentials out of the session after user has logged in.
+    All keys except for realm must be present.
+
+    For this to be secure the session MUST be a backend session only,
+    with a password on Redis and a unique session secret different to the
+    authtkt cookie secret.
+
     :param request: Pyramid request object
     :return: SamDB or None if no credentials in session
+    :raises LdbError: On connection error or if the credentials no longer work
     """
-    # Fetch credentials out of the session after user logs in.
-    # For this to be secure the session MUST be a backend session only.
-    username = request.session.get("samba.username")
-    password = request.session.get("samba.password")
-    host = request.session.get("samba.host")
-    realm = request.session.get("samba.realm")
-
-    return connect_samdb(username, password, host, realm)
+    try:
+        host = request.session["samba.host"]
+        username = request.session["samba.username"]
+        password = request.session["samba.password"]
+        realm = request.session.get("samba.realm")
+        return connect_samdb(host, username, password, realm)
+    except KeyError:
+        return None
diff --git a/src/sambal/security.py b/src/sambal/security.py
index 24a9ed9..2cdb474 100644
--- a/src/sambal/security.py
+++ b/src/sambal/security.py
@@ -1,5 +1,6 @@
 from typing import Optional
 
+from ldb import LdbError
 from pyramid.authentication import AuthTktCookieHelper
 from pyramid.interfaces import ISecurityPolicy
 from pyramid.security import forget, remember
@@ -41,13 +42,16 @@ def forget(self, request, **kwargs):
 
 def login(request, username, password, host, realm):
     """Log into server and put credentials in session on success only."""
-    samdb = connect_samdb(username, password, host, realm)
-    if samdb and (user_sid := samdb.connecting_user_sid):
-        request.session["samba.username"] = username
-        request.session["samba.password"] = password
-        request.session["samba.host"] = host
-        request.session["samba.realm"] = realm
-        return remember(request, user_sid)
+    try:
+        samdb = connect_samdb(host, username, password, realm)
+        if user_sid := samdb.connecting_user_sid:
+            request.session["samba.username"] = username
+            request.session["samba.password"] = password
+            request.session["samba.host"] = host
+            request.session["samba.realm"] = realm
+            return remember(request, user_sid)
+    except LdbError:
+        pass
 
 
 def logout(request):