From abe7dab3147ed497e4ee68e83f6b3a80876e5d3e Mon Sep 17 00:00:00 2001 From: TheByronHimes Date: Mon, 12 Aug 2024 13:18:28 +0000 Subject: [PATCH 1/4] Update config so credentials are optional --- config_schema.json | 4 ++-- src/ns/adapters/outbound/smtp_client.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config_schema.json b/config_schema.json index 4aeb4f5..9aa09ae 100644 --- a/config_schema.json +++ b/config_schema.json @@ -97,11 +97,13 @@ "type": "integer" }, "login_user": { + "default": "", "description": "The login username or email", "title": "Login User", "type": "string" }, "login_password": { + "default": "", "description": "The login password", "format": "password", "title": "Login Password", @@ -199,8 +201,6 @@ "from_address", "smtp_host", "smtp_port", - "login_user", - "login_password", "notification_event_topic", "notification_event_type", "kafka_servers" diff --git a/src/ns/adapters/outbound/smtp_client.py b/src/ns/adapters/outbound/smtp_client.py index 73b1d2e..71c3448 100644 --- a/src/ns/adapters/outbound/smtp_client.py +++ b/src/ns/adapters/outbound/smtp_client.py @@ -38,8 +38,8 @@ class SmtpClientConfig(BaseSettings): smtp_port: int = Field( default=..., description="The port for the mail server connection" ) - login_user: str = Field(default=..., description="The login username or email") - login_password: SecretStr = Field(default=..., description="The login password") + login_user: str = Field(default="", description="The login username or email") + login_password: SecretStr = Field(default="", description="The login password") use_starttls: bool = Field( default=True, description="Boolean flag indicating the use of STARTTLS" ) From 7aade678b2dc1372cdf4a4a4db2379694176f8eb Mon Sep 17 00:00:00 2001 From: TheByronHimes Date: Mon, 12 Aug 2024 14:11:40 +0000 Subject: [PATCH 2/4] Move server connection into cm function --- src/ns/adapters/outbound/smtp_client.py | 38 ++++++++++------- tests/test_basic.py | 56 ++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/src/ns/adapters/outbound/smtp_client.py b/src/ns/adapters/outbound/smtp_client.py index 71c3448..5ff9094 100644 --- a/src/ns/adapters/outbound/smtp_client.py +++ b/src/ns/adapters/outbound/smtp_client.py @@ -17,9 +17,11 @@ """Contains the smtp client adapter""" import logging -import smtplib import ssl +from collections.abc import Generator +from contextlib import contextmanager from email.message import EmailMessage +from smtplib import SMTP, SMTPAuthenticationError, SMTPException from pydantic import Field, SecretStr from pydantic_settings import BaseSettings @@ -52,27 +54,35 @@ def __init__(self, *, config: SmtpClientConfig): """Assign config, which should contain all needed info""" self._config = config + @contextmanager + def get_connection(self) -> Generator[SMTP, None, None]: + """Establish a connection to the SMTP server""" + with SMTP(self._config.smtp_host, self._config.smtp_port) as server: + yield server + def send_email_message(self, message: EmailMessage): """Send an email message. - Creates an ssl security context if configured, then log in with the configured - credentials and send the provided email message. + Creates an ssl security context if configured, then logs in with the configured + credentials and sends the provided email message. """ try: - with smtplib.SMTP(self._config.smtp_host, self._config.smtp_port) as server: + with self.get_connection() as server: if self._config.use_starttls: # create ssl security context per Python's Security considerations context = ssl.create_default_context() server.starttls(context=context) - try: - server.login( - self._config.login_user, - self._config.login_password.get_secret_value(), - ) - except smtplib.SMTPAuthenticationError as err: - login_error = self.FailedLoginError() - log.critical(login_error) - raise login_error from err + + if self._config.login_user or self._config.login_password: + try: + server.login( + self._config.login_user, + self._config.login_password.get_secret_value(), + ) + except SMTPAuthenticationError as err: + login_error = self.FailedLoginError() + log.critical(login_error) + raise login_error from err # check for a connection if server.noop()[0] != 250: @@ -81,7 +91,7 @@ def send_email_message(self, message: EmailMessage): raise connection_error server.send_message(msg=message) - except smtplib.SMTPException as exc: + except SMTPException as exc: error = self.GeneralSmtpException(error_info=exc.args[0]) log.error(error, extra={"error_info": exc.args[0]}) raise error from exc diff --git a/tests/test_basic.py b/tests/test_basic.py index f458406..afc5f0c 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -17,13 +17,17 @@ import html import json +import smtplib +from contextlib import contextmanager +from email.message import EmailMessage from hashlib import sha256 from typing import cast +from unittest.mock import Mock import pytest from hexkit.correlation import correlation_id_var -from ns.adapters.outbound.smtp_client import SmtpClient +from ns.adapters.outbound.smtp_client import SmtpClient, SmtpClientConfig from ns.core.models import NotificationRecord from ns.core.notifier import Notifier from tests.fixtures.joint import JointFixture @@ -54,10 +58,7 @@ def correlation_id_fixture(): correlation_id_var.reset(token) -@pytest.mark.parametrize( - "notification_details", - [sample_notification], -) +@pytest.mark.parametrize("notification_details", [sample_notification]) async def test_email_construction( joint_fixture: JointFixture, notification_details, @@ -97,8 +98,51 @@ async def test_email_construction( assert html_content.strip() == expected_html +@pytest.mark.parametrize( + "username", ["bob@bobswebsite.com", ""], ids=["WithUsername", "NoUsername"] +) +@pytest.mark.parametrize( + "password", ["passw0rd", ""], ids=["WithPassword", "NoPassword"] +) +async def test_anonymous_smtp(username: str, password: str): + """Verify that authentication is only used when credentials are configured.""" + config = SmtpClientConfig( + smtp_host="127.0.0.1", + smtp_port=587, + login_user=username, + login_password=password, # type: ignore + ) + + mock_server = Mock(spec=smtplib.SMTP) + mock_server.noop.side_effect = lambda: (250, b"") + + smtp_client = SmtpClient(config=config) + + @contextmanager + def get_mock_server(): + yield mock_server + + smtp_client.get_connection = get_mock_server # type: ignore [method-assign] + + message = EmailMessage() + message["To"] = "to@example.com" + message["Subject"] = "Test" + message["From"] = "from@example.com" + + smtp_client.send_email_message(message) + + # Verify that 'login' is only called when credentials are set + if username or password: + mock_server.login.assert_called() + else: + mock_server.login.assert_not_called() + + # Verify that 'send_message' is called regardless + mock_server.send_message.assert_called() + + async def test_failed_authentication(joint_fixture: JointFixture): - """Change login credentials so authentication fails.""" + """Test that we raise the expected error when auth fails on a secured SMTP server.""" # Cast notifier type joint_fixture.notifier = cast(Notifier, joint_fixture.notifier) From 0b2cc279b023cbc9130a04e368ad9d9ed5edb607 Mon Sep 17 00:00:00 2001 From: TheByronHimes Date: Mon, 12 Aug 2024 14:13:59 +0000 Subject: [PATCH 3/4] Bump version from 1.1.3 -> 1.2.0 --- .pyproject_generation/pyproject_custom.toml | 2 +- README.md | 10 +++++----- pyproject.toml | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.pyproject_generation/pyproject_custom.toml b/.pyproject_generation/pyproject_custom.toml index 376caac..040f098 100644 --- a/.pyproject_generation/pyproject_custom.toml +++ b/.pyproject_generation/pyproject_custom.toml @@ -1,6 +1,6 @@ [project] name = "ns" -version = "1.1.3" +version = "1.2.0" description = "The Notification Service (NS) handles notification kafka events." dependencies = [ "typer>=0.12", diff --git a/README.md b/README.md index 7d27535..1b2b7ee 100644 --- a/README.md +++ b/README.md @@ -33,13 +33,13 @@ We recommend using the provided Docker container. A pre-build version is available at [docker hub](https://hub.docker.com/repository/docker/ghga/notification-service): ```bash -docker pull ghga/notification-service:1.1.3 +docker pull ghga/notification-service:1.2.0 ``` Or you can build the container yourself from the [`./Dockerfile`](./Dockerfile): ```bash # Execute in the repo's root dir: -docker build -t ghga/notification-service:1.1.3 . +docker build -t ghga/notification-service:1.2.0 . ``` For production-ready deployment, we recommend using Kubernetes, however, @@ -47,7 +47,7 @@ for simple use cases, you could execute the service using docker on a single server: ```bash # The entrypoint is preconfigured: -docker run -p 8080:8080 ghga/notification-service:1.1.3 --help +docker run -p 8080:8080 ghga/notification-service:1.2.0 --help ``` If you prefer not to use containers, you may install the service from source: @@ -131,9 +131,9 @@ The service requires the following configuration parameters: - **`smtp_port`** *(integer)*: The port for the mail server connection. -- **`login_user`** *(string)*: The login username or email. +- **`login_user`** *(string)*: The login username or email. Default: `""`. -- **`login_password`** *(string, format: password)*: The login password. +- **`login_password`** *(string, format: password)*: The login password. Default: `""`. - **`use_starttls`** *(boolean)*: Boolean flag indicating the use of STARTTLS. Default: `true`. diff --git a/pyproject.toml b/pyproject.toml index 534b7bb..9547c47 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,7 +21,7 @@ classifiers = [ "Intended Audience :: Developers", ] name = "ns" -version = "1.1.3" +version = "1.2.0" description = "The Notification Service (NS) handles notification kafka events." dependencies = [ "typer>=0.12", From ee7bc08526bc7b7d039a9b6e55d828a87605ea08 Mon Sep 17 00:00:00 2001 From: TheByronHimes Date: Mon, 12 Aug 2024 15:03:05 +0000 Subject: [PATCH 4/4] Allow login values to be None --- README.md | 16 +++++++++-- config_schema.json | 30 +++++++++++++++------ src/ns/adapters/outbound/smtp_client.py | 30 ++++++++++++++++++--- tests/fixtures/server.py | 10 +++---- tests/test_basic.py | 35 ++++++++++++++++++++----- 5 files changed, 96 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 1b2b7ee..a389b08 100644 --- a/README.md +++ b/README.md @@ -131,9 +131,21 @@ The service requires the following configuration parameters: - **`smtp_port`** *(integer)*: The port for the mail server connection. -- **`login_user`** *(string)*: The login username or email. Default: `""`. +- **`login_user`**: The login username or email. Default: `null`. -- **`login_password`** *(string, format: password)*: The login password. Default: `""`. + - **Any of** + + - *string* + + - *null* + +- **`login_password`**: The login password. Default: `null`. + + - **Any of** + + - *string, format: password* + + - *null* - **`use_starttls`** *(boolean)*: Boolean flag indicating the use of STARTTLS. Default: `true`. diff --git a/config_schema.json b/config_schema.json index 9aa09ae..8666967 100644 --- a/config_schema.json +++ b/config_schema.json @@ -97,18 +97,32 @@ "type": "integer" }, "login_user": { - "default": "", + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "default": null, "description": "The login username or email", - "title": "Login User", - "type": "string" + "title": "Login User" }, "login_password": { - "default": "", + "anyOf": [ + { + "format": "password", + "type": "string", + "writeOnly": true + }, + { + "type": "null" + } + ], + "default": null, "description": "The login password", - "format": "password", - "title": "Login Password", - "type": "string", - "writeOnly": true + "title": "Login Password" }, "use_starttls": { "default": true, diff --git a/src/ns/adapters/outbound/smtp_client.py b/src/ns/adapters/outbound/smtp_client.py index 5ff9094..326d6a5 100644 --- a/src/ns/adapters/outbound/smtp_client.py +++ b/src/ns/adapters/outbound/smtp_client.py @@ -22,8 +22,9 @@ from contextlib import contextmanager from email.message import EmailMessage from smtplib import SMTP, SMTPAuthenticationError, SMTPException +from typing import Self -from pydantic import Field, SecretStr +from pydantic import Field, SecretStr, model_validator from pydantic_settings import BaseSettings from ns.ports.outbound.smtp_client import SmtpClientPort @@ -40,12 +41,28 @@ class SmtpClientConfig(BaseSettings): smtp_port: int = Field( default=..., description="The port for the mail server connection" ) - login_user: str = Field(default="", description="The login username or email") - login_password: SecretStr = Field(default="", description="The login password") + login_user: str | None = Field( + default=None, description="The login username or email" + ) + login_password: SecretStr | None = Field( + default=None, description="The login password" + ) use_starttls: bool = Field( default=True, description="Boolean flag indicating the use of STARTTLS" ) + @model_validator(mode="after") + def validate_prefix(self) -> Self: + """Enforce setting both the user/pwd or leaving both unconfigured (`None`)""" + msg = "Login_user and login_password must be configured together or not at all." + nones = [ + _ + for _ in filter(lambda x: x is None, [self.login_user, self.login_password]) + ] + if nones and len(nones) == 1: + raise ValueError(msg) + return self + class SmtpClient(SmtpClientPort): """Concrete implementation of an SmtpClientPort""" @@ -65,6 +82,8 @@ def send_email_message(self, message: EmailMessage): Creates an ssl security context if configured, then logs in with the configured credentials and sends the provided email message. + In the case that username and password are `None`, authentication will not be + performed. """ try: with self.get_connection() as server: @@ -73,7 +92,10 @@ def send_email_message(self, message: EmailMessage): context = ssl.create_default_context() server.starttls(context=context) - if self._config.login_user or self._config.login_password: + if ( + self._config.login_user is not None + and self._config.login_password is not None + ): try: server.login( self._config.login_user, diff --git a/tests/fixtures/server.py b/tests/fixtures/server.py index fe97e21..4abfb00 100644 --- a/tests/fixtures/server.py +++ b/tests/fixtures/server.py @@ -29,7 +29,7 @@ class Authenticator: """Basic authenticator so we can test error handling for failed authentication""" - def __init__(self, user: str, password: str): + def __init__(self, user: str | None, password: str | None): self._user = user self._password = password @@ -38,10 +38,9 @@ def __call__(self, server, session, envelope, mechanism, auth_data): login = str(auth_data.login, encoding="utf-8") password = str(auth_data.password, encoding="utf-8") - if login == self._user and password == self._password: - return AuthResult(success=True) + authenticated = login == self._user and password == self._password - return AuthResult(success=False, handled=False) + return AuthResult(success=authenticated, handled=False) class CustomHandler(Sink): @@ -111,7 +110,8 @@ def __init__(self, *, config: Config): """Assign config""" self._config = config self.login = self._config.login_user - self.password = self._config.login_password.get_secret_value() + password = self._config.login_password + self.password = password.get_secret_value() if password else None def _record_email( self, *, expected_email: EmailMessage, controller: Controller diff --git a/tests/test_basic.py b/tests/test_basic.py index afc5f0c..c3ee703 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -98,13 +98,36 @@ async def test_email_construction( assert html_content.strip() == expected_html +async def test_bad_login_config(): + """Verify that we get an error if credentials are misconfigured""" + with pytest.raises(ValueError): + SmtpClientConfig( + smtp_host="127.0.0.1", + smtp_port=587, + login_user="test", + login_password=None, + ) + + with pytest.raises(ValueError): + SmtpClientConfig( + smtp_host="127.0.0.1", + smtp_port=587, + login_user=None, + login_password="test", # type: ignore + ) + + @pytest.mark.parametrize( - "username", ["bob@bobswebsite.com", ""], ids=["WithUsername", "NoUsername"] -) -@pytest.mark.parametrize( - "password", ["passw0rd", ""], ids=["WithPassword", "NoPassword"] + "username, password", + [ + ("bob@bobswebsite.com", "passw0rd"), + ("bob@bobswebsite.com", ""), + ("", "passw0rd"), + (None, None), + ], + ids=["WithUsernameAndPassword", "BlankPassword", "BlankUsername", "NoAuth"], ) -async def test_anonymous_smtp(username: str, password: str): +async def test_smtp_authentication(username: str | None, password: str | None): """Verify that authentication is only used when credentials are configured.""" config = SmtpClientConfig( smtp_host="127.0.0.1", @@ -132,7 +155,7 @@ def get_mock_server(): smtp_client.send_email_message(message) # Verify that 'login' is only called when credentials are set - if username or password: + if username is not None and password is not None: mock_server.login.assert_called() else: mock_server.login.assert_not_called()