From 7b39349f4d49af8cd9269cb639a1128ec0ddb263 Mon Sep 17 00:00:00 2001 From: Rob van der Linde Date: Sat, 9 Mar 2024 13:56:09 +1300 Subject: [PATCH 1/4] ruff: add lint-fix and format from ruff to Makefile --- Makefile | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Makefile b/Makefile index ac1cfff..fc03a32 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,14 @@ build : lint : @ruff check +.PHONY : lint-fix +lint-fix : + @ruff check --fix + +.PHONY : format +format : + @ruff format + .PHONY : test test : @pytest From 46a544bf631c9a63ca5085e205db778903c8335c Mon Sep 17 00:00:00 2001 From: Rob van der Linde Date: Sat, 9 Mar 2024 13:57:07 +1300 Subject: [PATCH 2/4] ruff: run make format and make lint-fix --- src/sambal/forms/__init__.py | 4 +--- src/sambal/tweens/__init__.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/sambal/forms/__init__.py b/src/sambal/forms/__init__.py index 3d238fc..8e73c3d 100644 --- a/src/sambal/forms/__init__.py +++ b/src/sambal/forms/__init__.py @@ -1,5 +1,3 @@ from .login import LoginForm -__all__ = ( - "LoginForm", -) +__all__ = ("LoginForm",) diff --git a/src/sambal/tweens/__init__.py b/src/sambal/tweens/__init__.py index 18453ef..ea25bff 100644 --- a/src/sambal/tweens/__init__.py +++ b/src/sambal/tweens/__init__.py @@ -1,5 +1,3 @@ from .headers import SecurityHeaders -__all__ = ( - "SecurityHeaders", -) +__all__ = ("SecurityHeaders",) From 83750a48fd1daadf345efc680c481061ad385ec5 Mon Sep 17 00:00:00 2001 From: Rob van der Linde Date: Sat, 9 Mar 2024 10:41:56 +1300 Subject: [PATCH 3/4] 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): From 351d8372aba8473bb2ebe25150a7b83e1da92a4f Mon Sep 17 00:00:00 2001 From: Rob van der Linde Date: Sat, 9 Mar 2024 13:54:01 +1300 Subject: [PATCH 4/4] login: bubble up LdbError to auth view Step two bubble it all the way upto the login view in views/auth.py and handle the message returned from the LdbError exception. --- src/sambal/security.py | 26 +++++++++++++------------- src/sambal/views/auth.py | 14 ++++++++++---- tests/test_login.py | 2 +- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/sambal/security.py b/src/sambal/security.py index 2cdb474..7949d10 100644 --- a/src/sambal/security.py +++ b/src/sambal/security.py @@ -1,6 +1,5 @@ from typing import Optional -from ldb import LdbError from pyramid.authentication import AuthTktCookieHelper from pyramid.interfaces import ISecurityPolicy from pyramid.security import forget, remember @@ -40,18 +39,19 @@ def forget(self, request, **kwargs): return self.authtkt.forget(request, **kwargs) -def login(request, username, password, host, realm): - """Log into server and put credentials in session on success only.""" - 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 login(request, host, username, password, realm): + """Log into server and put credentials in session on success only. + + :raises LdbError: If the login failed or host is incorrect + """ + 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) def logout(request): diff --git a/src/sambal/views/auth.py b/src/sambal/views/auth.py index ec930e4..84080e0 100644 --- a/src/sambal/views/auth.py +++ b/src/sambal/views/auth.py @@ -1,3 +1,4 @@ +from ldb import LdbError from pyramid.httpexceptions import HTTPFound from pyramid.view import forbidden_view_config, view_config @@ -21,10 +22,15 @@ def login(request): host = form.host.data realm = form.realm.data - if headers := request.login(username, password, host, realm): - return HTTPFound(location=return_url, headers=headers) - else: - request.session.flash("Login to host failed", queue="error") + try: + if headers := request.login(host, username, password, realm): + return HTTPFound(location=return_url, headers=headers) + else: + request.session.flash("Login failed", queue="error") + + except LdbError as e: + msg = e.args[1] + request.session.flash(f"Login failed: {msg}", queue="error") else: form = LoginForm() diff --git a/tests/test_login.py b/tests/test_login.py index fa4ed43..aad391d 100644 --- a/tests/test_login.py +++ b/tests/test_login.py @@ -61,7 +61,7 @@ def test_login_invalid_credentials(testapp, settings): } response = testapp.post("/login/", login_form, status=200) - assert "Login to host failed" in response.text + assert "Login failed" in response.text def test_login_required(testapp):