From e26b63f8e8ae96c3fa62dc48c63dc10d11082be5 Mon Sep 17 00:00:00 2001 From: Ionesio Junior Date: Wed, 21 Feb 2024 14:47:19 -0300 Subject: [PATCH 1/6] Replace email_token -> smtp_username, smtp_password --- packages/grid/backend/grid/core/config.py | 5 ++--- packages/grid/backend/grid/core/node.py | 3 ++- packages/hagrid/hagrid/cli.py | 20 +++++++++++++++----- packages/syft/src/syft/node/node.py | 2 ++ 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/grid/backend/grid/core/config.py b/packages/grid/backend/grid/core/config.py index a6a32bc7036..830e67503f5 100644 --- a/packages/grid/backend/grid/core/config.py +++ b/packages/grid/backend/grid/core/config.py @@ -71,8 +71,6 @@ def sentry_dsn_can_be_blank(cls, v: str) -> Optional[str]: SMTP_TLS: bool = True SMTP_PORT: Optional[int] = None SMTP_HOST: Optional[str] = None - SMTP_USER: Optional[str] = None - SMTP_PASSWORD: Optional[str] = None EMAILS_FROM_EMAIL: Optional[EmailStr] = None EMAILS_FROM_NAME: Optional[str] = None @@ -144,7 +142,8 @@ def get_emails_enabled(cls, v: bool, values: Dict[str, Any]) -> bool: SINGLE_CONTAINER_MODE: bool = str_to_bool(os.getenv("SINGLE_CONTAINER_MODE", False)) CONSUMER_SERVICE_NAME: Optional[str] = os.getenv("CONSUMER_SERVICE_NAME") INMEMORY_WORKERS: bool = str_to_bool(os.getenv("INMEMORY_WORKERS", True)) - EMAIL_TOKEN: str = os.getenv("EMAIL_TOKEN", "") + SMTP_USERNAME: str = os.getenv("SMTP_USERNAME", "") + SMTP_PASSWORD: str = os.getenv("SMTP_PASSWORD", "") TEST_MODE: bool = ( True if os.getenv("TEST_MODE", "false").lower() == "true" else False diff --git a/packages/grid/backend/grid/core/node.py b/packages/grid/backend/grid/core/node.py index 6f6e423368b..e81583c347e 100644 --- a/packages/grid/backend/grid/core/node.py +++ b/packages/grid/backend/grid/core/node.py @@ -94,5 +94,6 @@ def seaweedfs_config() -> SeaweedFSConfig: queue_config=queue_config, migrate=True, in_memory_workers=settings.INMEMORY_WORKERS, - email_token=settings.EMAIL_TOKEN, + smtp_username=settings.SMTP_USERNAME, + smtp_password=settings.SMTP_PASSWORD, ) diff --git a/packages/hagrid/hagrid/cli.py b/packages/hagrid/hagrid/cli.py index 193e8ce92a3..6dec82af707 100644 --- a/packages/hagrid/hagrid/cli.py +++ b/packages/hagrid/hagrid/cli.py @@ -323,11 +323,18 @@ def clean(location: str) -> None: help="Container image tag to use", ) @click.option( - "--email-token", + "--smtp-username", default=None, required=False, type=str, - help="Token used to auth in email server and enable notification via emails", + help="Username used to auth in email server and enable notification via emails", +) +@click.option( + "--smtp-password", + default=None, + required=False, + type=str, + help="Password used to auth in email server and enable notification via emails", ) @click.option( "--build-src", @@ -1316,7 +1323,8 @@ def create_launch_cmd( else: parsed_kwargs["node_side_type"] = NodeSideType.HIGH_SIDE.value - parsed_kwargs["email_token"] = kwargs["email_token"] + parsed_kwargs["smtp_username"] = kwargs["smtp_username"] + parsed_kwargs["smtp_password"] = kwargs["smtp_password"] parsed_kwargs["enable_warnings"] = not kwargs["no_warnings"] @@ -2165,7 +2173,8 @@ def create_launch_docker_cmd( single_container_mode = kwargs["deployment_type"] == "single_container" in_mem_workers = kwargs.get("in_mem_workers") - email_token = kwargs.get("email_token") + smtp_username = kwargs.get("smtp_username") + smtp_password = kwargs.get("smtp_password") enable_oblv = bool(kwargs["oblv"]) print(" - NAME: " + str(snake_name)) @@ -2230,7 +2239,8 @@ def create_launch_docker_cmd( "NODE_SIDE_TYPE": kwargs["node_side_type"], "SINGLE_CONTAINER_MODE": single_container_mode, "INMEMORY_WORKERS": in_mem_workers, - "EMAIL_TOKEN": email_token, + "SMTP_USERNAME": smtp_username, + "SMTP_PASSWORD": smtp_password, } if "trace" in kwargs and kwargs["trace"] is True: diff --git a/packages/syft/src/syft/node/node.py b/packages/syft/src/syft/node/node.py index 12b7e03d4eb..c9385074012 100644 --- a/packages/syft/src/syft/node/node.py +++ b/packages/syft/src/syft/node/node.py @@ -310,6 +310,8 @@ def __init__( dev_mode: bool = False, migrate: bool = False, in_memory_workers: bool = True, + smtp_username: Optional[str] = None, + smtp_password: Optional[str] = None, email_token: Optional[str] = None, ): # 🟡 TODO 22: change our ENV variable format and default init args to make this From 62287f6b1b8a89d199edb2cc7ff74c0275d8d4e2 Mon Sep 17 00:00:00 2001 From: Julian Cardonnet Date: Thu, 22 Feb 2024 01:32:12 -0300 Subject: [PATCH 2/6] Replace email token with username/password. Enable/Disable notifications for each user. Multiple refactorings and bug fixes. --- packages/syft/src/syft/client/client.py | 6 + packages/syft/src/syft/node/node.py | 15 +- .../src/syft/service/notifier/notifier.py | 84 +++++++-- .../syft/service/notifier/notifier_service.py | 174 ++++++++++++++---- .../syft/service/notifier/notifier_stash.py | 27 ++- .../src/syft/service/notifier/smtp_client.py | 82 +++++---- .../syft/service/settings/settings_service.py | 11 +- .../src/syft/service/user/user_service.py | 5 + 8 files changed, 295 insertions(+), 109 deletions(-) diff --git a/packages/syft/src/syft/client/client.py b/packages/syft/src/syft/client/client.py index 11b4b2ab9a0..ff91ff5d5a9 100644 --- a/packages/syft/src/syft/client/client.py +++ b/packages/syft/src/syft/client/client.py @@ -684,6 +684,12 @@ def notifications(self) -> Optional[APIModule]: return self.api.services.notifications return None + @property + def notifier(self) -> Optional[APIModule]: + if self.api.has_service("notifier"): + return self.api.services.notifier + return None + @property def peers(self) -> Optional[Union[List[NodePeer], SyftError]]: if self.api.has_service("network"): diff --git a/packages/syft/src/syft/node/node.py b/packages/syft/src/syft/node/node.py index c9385074012..75fb9540829 100644 --- a/packages/syft/src/syft/node/node.py +++ b/packages/syft/src/syft/node/node.py @@ -399,18 +399,9 @@ def __init__( node=self, ) - if not email_token: - NotifierService.init_notifier( - node=self, - active=False, - email_token=None, - ) - else: - NotifierService.init_notifier( - node=self, - active=True, - email_token=email_token, - ) + NotifierService.init_notifier( + node=self, email_password=smtp_password, email_username=smtp_username + ) self.client_cache = {} if isinstance(node_type, str): diff --git a/packages/syft/src/syft/service/notifier/notifier.py b/packages/syft/src/syft/service/notifier/notifier.py index bb3822c7ef7..d625f684c18 100644 --- a/packages/syft/src/syft/service/notifier/notifier.py +++ b/packages/syft/src/syft/service/notifier/notifier.py @@ -24,28 +24,50 @@ from .notifier_enums import NOTIFIERS from .smtp_client import SMTPClient +DEFAULT_EMAIL_SERVER = "smtp.mailgun.org" + class BaseNotifier: + EMAIL_SERVER = DEFAULT_EMAIL_SERVER + def send( self, target: SyftVerifyKey, notification: Notification ) -> Union[SyftSuccess, SyftError]: - pass + return SyftError(message="Not implemented") class EmailNotifier(BaseNotifier): + smtp_client = SMTPClient + username: str + password: str server: str - token: Optional[str] - smtp_client: SMTPClient + port: int def __init__( self, - token: Optional[str], - server: str = "smtp.postmarkapp.com", + username: str, + password: str, + server: str = DEFAULT_EMAIL_SERVER, + port: int = 587, ) -> None: - self.token = token + self.username = username + self.password = password self.server = server - self.smtp_client = SMTPClient( - smtp_server=self.server, smtp_port=587, access_token=self.token + self.port = port + + @classmethod + def check_credentials( + cls, + username: str, + password: str, + server: str = DEFAULT_EMAIL_SERVER, + port: int = 587, + ) -> bool: + return cls.smtp_client.check_credentials( + server=server, + port=port, + username=username, + password=password, ) def send(self, node: AbstractNode, notification: Notification) -> Result[Ok, Err]: @@ -72,6 +94,20 @@ def send(self, node: AbstractNode, notification: Notification) -> Result[Ok, Err return Err(f"Error: unable to send email: {e}") +# @serializable() +# @dataclass +# class EmailNotifierSettings: +# """Email notifier configuration""" +# server: str +# smtp_client = SMTPClient +# username: str +# password: str +# server: str = DEFAULT_EMAIL_SERVER +# port: int = 587 +# notifier = EmailNotifier +# subscribers = set() + + @serializable() class NotifierSettings(SyftObject): __canonical_name__ = "NotifierSettings" @@ -84,7 +120,7 @@ class NotifierSettings(SyftObject): "app_enabled", ] active: bool = False - # Flag to identify which notification is enable + # Flag to identify which notification is enabled # For now, consider only the email notification # In future, Admin, must be able to have a better # control on diff notifications. @@ -100,7 +136,11 @@ class NotifierSettings(SyftObject): NOTIFIERS.APP: False, } - email_token: Optional[str] = "" + email_server: Optional[str] = DEFAULT_EMAIL_SERVER + email_port: Optional[int] = 587 + email_username: Optional[str] = "" + email_password: Optional[str] = "" + email_subscribers = set() @property def email_enabled(self) -> bool: @@ -118,6 +158,20 @@ def slack_enabled(self) -> bool: def app_enabled(self) -> bool: return self.notifiers_status[NOTIFIERS.APP] + def validate_email_credentials( + self, + username: str, + password: str, + server: Optional[str] = None, + port: Optional[int] = None, + ) -> Result[Ok, Err]: + return self.notifiers[NOTIFIERS.EMAIL].check_credentials( + server=server if server else self.email_server, + port=port if port else self.email_port, + username=username, + password=password, + ) + def send_notifications( self, node: AbstractNode, @@ -133,13 +187,13 @@ def send_notifications( return Ok("Notification sent successfully!") def select_notifiers(self, notification: Notification) -> List[BaseNotifier]: - """This method allow us to check which notification is enabled and return the - notifier object to be used to send the notification. + """ + Return a list of the notifiers enabled for the given notification" Args: notification (Notification): The notification object Returns: - List[BaseNotifier]: A list of notifier objects + List[BaseNotifier]: A list of enabled notifier objects """ notifier_objs = [] for notifier_type in notification.notifier_types: @@ -151,7 +205,9 @@ def select_notifiers(self, notification: Notification) -> List[BaseNotifier]: # If notifier is email, we need to pass the token if notifier_type == NOTIFIERS.EMAIL: notifier_objs.append( - self.notifiers[notifier_type](token=self.email_token) + self.notifiers[notifier_type]( + username=self.email_username, password=self.email_password + ) ) # If notifier is not email, we just create the notifier object # TODO: Add the other notifiers, and its auth methods diff --git a/packages/syft/src/syft/service/notifier/notifier_service.py b/packages/syft/src/syft/service/notifier/notifier_service.py index c0c97f9e457..91d1067ca9b 100644 --- a/packages/syft/src/syft/service/notifier/notifier_service.py +++ b/packages/syft/src/syft/service/notifier/notifier_service.py @@ -4,6 +4,11 @@ from typing import Optional from typing import Union +# third party +from result import Err +from result import Ok +from result import Result + # relative from ...abstract_node import AbstractNode from ...serde.serializable import serializable @@ -29,10 +34,8 @@ def __init__(self, store: DocumentStore) -> None: self.store = store self.stash = NotifierStash(store=store) - @service_method( - path="notifier.settings", name="notifier_settings", roles=ADMIN_ROLE_LEVEL - ) - def notifier_settings( # Maybe just notifier.settings + @service_method(path="notifier.settings", name="settings", roles=ADMIN_ROLE_LEVEL) + def settings( # Maybe just notifier.settings self, context: AuthedServiceContext, ) -> Union[NotifierStash, SyftError]: @@ -51,8 +54,25 @@ def notifier_settings( # Maybe just notifier.settings @service_method(path="notifier.turn_on", name="turn_on", roles=ADMIN_ROLE_LEVEL) def turn_on( - self, context: AuthedServiceContext, email_token: Optional[str] = None + self, + context: AuthedServiceContext, + email_username: Optional[str] = None, + email_password: Optional[str] = None, ) -> Union[SyftSuccess, SyftError]: + """Turn on email notifications. + + Args: + email_username (Optional[str]): Email server username. Defaults to None. + email_password (Optional[str]): Email email server password. Defaults to None. + + Returns: + Union[SyftSuccess, SyftError]: A union type representing the success or error response. + + Raises: + None + + """ + result = self.stash.get(credentials=context.credentials) # 1 - If something went wrong at db level, return the error @@ -60,20 +80,36 @@ def turn_on( return SyftError(message=result.err()) notifier = result.ok() - # 2 - If email token is not provided and notifier doesn't exist, return an error - if not email_token and not notifier.email_token: + print("[LOG] Got notifier from db") + # If no new credentials provided, check for existing ones + if not (email_username and email_password): + if not (notifier.email_username and notifier.email_password): + return SyftError( + message="Email credentials haven't been set yet." + "Username and password are required to enable email notifications." + ) + else: + print("[LOG] No new credentials provided. Using existing ones.") + email_password = notifier.email_password + email_username = notifier.email_username + print("[LOG] Validating credentials...") + + # TODO: this should be a method in NotifierSettings + validation_result = notifier.validate_email_credentials( + username=email_username, password=email_password + ) + + if validation_result.is_err(): return SyftError( - message="No valid token has been added to the domain." - + "You can add a new token via client.settings.enable_notifications(token=TOKEN)" + message=f"Error validating email credentials. \n {validation_result.err()}" ) - # 3 - Activate the notifier + notifier.email_password = email_password + notifier.email_username = email_username notifier.active = True - - # 4 - If email token is provided. - if email_token: - notifier.email_token = email_token - + print( + "[LOG] Email credentials are valid. Updating the notifier settings in the db." + ) result = self.stash.update(credentials=context.credentials, settings=notifier) if result.is_err(): return SyftError(message=result.err()) @@ -84,6 +120,11 @@ def turn_off( self, context: AuthedServiceContext, ) -> Union[SyftSuccess, SyftError]: + """ + Turn off email notifications service. + PySyft notifications will still work. + """ + result = self.stash.get(credentials=context.credentials) if result.is_err(): @@ -105,18 +146,38 @@ def activate( self, context: AuthedServiceContext, ) -> Union[SyftSuccess, SyftError]: - result = self.stash.get(credentials=context.credentials) + """ + Activate email notifications for the authenticated user. + This will only work if the domain owner has enabled notifications. + """ + + # TODO: user credentials should not be used to get the notifier settings + # TODO: WARNING THIS IS A POTENTIAL SECURITY RISK (ONLY FOR DEVELOPMENT PURPOSES) + + admin_key = self.stash.admin_verify_key() + + result = self.stash.get(credentials=admin_key) + print(result) if result.is_err(): return SyftError(message=result.err()) notifier = result.ok() - if notifier.active: + + user_key = context.credentials + + if user_key in notifier.email_subscribers: return SyftSuccess( - message="Successfully activated notifications via email." + message="Notifications are already activated for this user." ) - else: - return SyftError(message="Notifications are disabled by the domain owner.") + + notifier.email_subscribers.add(user_key) + + # TODO: user credentials should not be used to update the notifier settings + result = self.stash.update(credentials=admin_key, settings=notifier) + if result.is_err(): + return SyftError(message=result.err()) + return SyftSuccess(message="Notifications enabled successfully.") @service_method( path="notifier.deactivate", @@ -127,27 +188,46 @@ def deactivate( self, context: AuthedServiceContext, ) -> Union[SyftSuccess, SyftError]: - result = self.stash.get(credentials=context.credentials) + """Deactivate email notifications for the authenticated user + This will only work if the domain owner has enabled notifications. + """ + + # TODO: WARNING THIS IS A POTENTIAL SECURITY RISK (ONLY FOR DEVELOPMENT PURPOSES) + admin_key = self.stash.admin_verify_key() + result = self.stash.get(credentials=admin_key) if result.is_err(): return SyftError(message=result.err()) - return SyftSuccess(message="Successfully deactivated notifications via email.") + notifier = result.ok() + user_key = context.credentials + if user_key not in notifier.email_subscribers: + return SyftSuccess( + message="Notifications were already deactivated for this user." + ) + + notifier.email_subscribers.remove(user_key) + + result = self.stash.update(credentials=admin_key, settings=notifier) + if result.is_err(): + return SyftError(message=result.err()) + return SyftSuccess(message="Notifications disabled successfully.") @staticmethod def init_notifier( node: AbstractNode, - active: bool = False, - email_token: Optional[str] = None, - ) -> Union[SyftSuccess, SyftError]: - """Initialize Notifier for a Node. - If Notifier already exists, it will return the existing one. + email_username: Optional[str] = None, + email_password: Optional[str] = None, + ) -> Result[Ok, Err]: + """Initialize Notifier settings for a Node. + If settings already exist, it will use the existing one. If not, it will create a new one. Args: node: Node to initialize the notifier active: If notifier should be active - email_token: Email token to send notifications + email_username: Email username to send notifications + email_password: Email password to send notifications Raises: Exception: If something went wrong Returns: @@ -163,20 +243,44 @@ def init_notifier( # Get the notifier notifier = result.ok() # If notifier doesn't exist, create a new one + if not notifier: - notifier = NotifierSettings( - active=active, - email_token=email_token, + notifier = NotifierSettings() + notifier.active = False # Default to False + + print("New Notifier created") + print(notifier) + # TODO: this should be a method in NotifierSettings + if email_username and email_password: + validation_result = notifier.validate_email_credentials( + username=email_username, password=email_password ) - notifier_stash.set(node.signing_key.verify_key, notifier) - except Exception as e: - print("Unable to create base notifier", e) + + if validation_result.is_err(): + raise ValueError( + f"Error validating email credentials. \n {validation_result.err()}" + ) + else: + notifier.email_password = email_password + notifier.email_username = email_username + notifier.active = True + + print("After update") + print(notifier) + + notifier_stash.set(node.signing_key.verify_key, notifier) + return Ok("Notifier initialized successfully") + + except Exception: + raise Exception( + f"Error initializing notifier. \n {validation_result.err()}" + ) # This is not a public API. # This method is used by other services to dispatch notifications internally def dispatch_notification( self, node: AbstractNode, notification: Notification - ) -> Union[SyftError]: + ) -> Union[SyftSuccess, SyftError]: admin_key = node.get_service("userservice").admin_verify_key() notifier = self.stash.get(admin_key) if notifier.is_err(): diff --git a/packages/syft/src/syft/service/notifier/notifier_stash.py b/packages/syft/src/syft/service/notifier/notifier_stash.py index a97308c6ea4..ab9ecff6484 100644 --- a/packages/syft/src/syft/service/notifier/notifier_stash.py +++ b/packages/syft/src/syft/service/notifier/notifier_stash.py @@ -36,6 +36,9 @@ class NotifierStash(BaseStash): def __init__(self, store: DocumentStore) -> None: super().__init__(store=store) + def admin_verify_key(self) -> SyftVerifyKey: + return self.partition.root_verify_key + def get(self, credentials: SyftVerifyKey) -> Result[NotifierSettings, Err]: """Get Settings""" result = self.get_all(credentials) @@ -50,18 +53,22 @@ def get(self, credentials: SyftVerifyKey) -> Result[NotifierSettings, Err]: def set( self, credentials: SyftVerifyKey, settings: NotifierSettings - ) -> Result[NotifierSettings, str]: - res = self.check_type(settings, self.object_type) + ) -> Result[NotifierSettings, Err]: + result = self.check_type(settings, self.object_type) # we dont use and_then logic here as it is hard because of the order of the arguments - if res.is_err(): - return res - return super().set(credentials=credentials, obj=res.ok()) + if result.is_err(): + return Err(message=result.err()) + return super().set( + credentials=credentials, obj=result.ok() + ) # TODO check if result isInstance(Ok) def update( self, credentials: SyftVerifyKey, settings: NotifierSettings - ) -> Result[NotifierSettings, str]: - res = self.check_type(settings, self.object_type) + ) -> Result[NotifierSettings, Err]: + result = self.check_type(settings, self.object_type) # we dont use and_then logic here as it is hard because of the order of the arguments - if res.is_err(): - return res - return super().update(credentials=credentials, obj=res.ok()) + if result.is_err(): + return Err(message=result.err()) + return super().update( + credentials=credentials, obj=result.ok() + ) # TODO check if result isInstance(Ok) diff --git a/packages/syft/src/syft/service/notifier/smtp_client.py b/packages/syft/src/syft/service/notifier/smtp_client.py index 978a06f0520..dfa7be2e2a8 100644 --- a/packages/syft/src/syft/service/notifier/smtp_client.py +++ b/packages/syft/src/syft/service/notifier/smtp_client.py @@ -2,41 +2,33 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText import smtplib -from typing import List -from typing import Optional + +# third party +from result import Err +from result import Ok +from result import Result class SMTPClient: + SOCKET_TIMEOUT = 5 # seconds + def __init__( self, - smtp_server: str, - smtp_port: int, - username: Optional[str] = None, - password: Optional[str] = None, - access_token: Optional[str] = None, + server: str, + port: int, + username: str, + password: str, ) -> None: - # Should provide token or username/password but not both - if username and password and access_token: - raise ValueError( - "Either username and password or access_token must be provided, but not both" - ) - - if not (username and password) and not access_token: - raise ValueError( - "Either username and password or access_token must be provided" - ) - - if username and password: - self.username = username - self.password = password - else: - self.access_token = access_token - - self.smtp_server = smtp_server - self.smtp_port = smtp_port - - def send(self, sender: str, receiver: List[str], subject: str, body: str) -> None: - if not subject or not body or not receiver: + if not (username and password): + raise ValueError("Both username and password must be provided") + + self.username = username + self.password = password + self.server = server + self.port = port + + def send(self, sender: str, receiver: list[str], subject: str, body: str) -> None: + if not (subject and body and receiver): raise ValueError("Subject, body, and recipient email(s) are required") msg = MIMEMultipart("alternative") @@ -45,16 +37,34 @@ def send(self, sender: str, receiver: List[str], subject: str, body: str) -> Non msg["Subject"] = subject msg.attach(MIMEText(body, "plain")) - with smtplib.SMTP(self.smtp_server, self.smtp_port) as server: + with smtplib.SMTP( + self.server, self.port, timeout=self.SOCKET_TIMEOUT + ) as server: server.ehlo() if server.has_extn("STARTTLS"): server.starttls() server.ehlo() - - if self.access_token: - server.login(self.access_token, self.access_token) - elif self.username and self.password: - server.login(self.username, self.password) - + server.login(self.username, self.password) text = msg.as_string() server.sendmail(sender, ", ".join(receiver), text) + # TODO: Add error handling + + @classmethod + def check_credentials( + cls, server: str, port: int, username: str, password: str + ) -> Result[Ok, Err]: + """Check if the credentials are valid. + + Returns: + bool: True if the credentials are valid, False otherwise. + """ + try: + with smtplib.SMTP(server, port, timeout=cls.SOCKET_TIMEOUT) as smtp_server: + smtp_server.ehlo() + if smtp_server.has_extn("STARTTLS"): + smtp_server.starttls() + smtp_server.ehlo() + smtp_server.login(username, password) + return Ok("Credentials are valid.") + except Exception as e: + return Err(e) diff --git a/packages/syft/src/syft/service/settings/settings_service.py b/packages/syft/src/syft/service/settings/settings_service.py index f487e7f4feb..72790e16d04 100644 --- a/packages/syft/src/syft/service/settings/settings_service.py +++ b/packages/syft/src/syft/service/settings/settings_service.py @@ -88,10 +88,17 @@ def update( roles=ADMIN_ROLE_LEVEL, ) def enable_notifications( - self, context: AuthedServiceContext, token: Optional[str] = None + self, + context: AuthedServiceContext, + email_username: Optional[str] = None, + email_password: Optional[str] = None, ) -> Union[SyftSuccess, SyftError]: notifier_service = context.node.get_service("notifierservice") - return notifier_service.turn_on(context=context, email_token=token) + return notifier_service.turn_on( + context=context, + email_username=email_username, + email_password=email_password, + ) @service_method( path="settings.disable_notifications", diff --git a/packages/syft/src/syft/service/user/user_service.py b/packages/syft/src/syft/service/user/user_service.py index 9ad1b25da68..8bdb28bc275 100644 --- a/packages/syft/src/syft/service/user/user_service.py +++ b/packages/syft/src/syft/service/user/user_service.py @@ -359,6 +359,8 @@ def delete(self, context: AuthedServiceContext, uid: UID) -> Union[bool, SyftErr if result.is_err(): return SyftError(message=str(result.err())) + # TODO: Remove notifications for the deleted user + return result.ok() def exchange_credentials( @@ -453,6 +455,9 @@ def register( success_message = f"User '{user.name}' successfully registered!" if request_user_role in DATA_OWNER_ROLE_LEVEL: success_message += " To see users, run `[your_client].users`" + + # TODO: Add a notifications for the new user + msg = SyftSuccess(message=success_message) return (msg, user.to(UserPrivateKey)) From 5e5243f305803ae93b586be2a6c31dc7a318eabc Mon Sep 17 00:00:00 2001 From: Julian Cardonnet Date: Thu, 22 Feb 2024 13:28:33 -0300 Subject: [PATCH 3/6] Fix merge conflict --- .../src/syft/service/notifier/notifier.py | 20 ------------------- .../syft/service/notifier/notifier_service.py | 1 - .../src/syft/service/notifier/smtp_client.py | 1 - 3 files changed, 22 deletions(-) diff --git a/packages/syft/src/syft/service/notifier/notifier.py b/packages/syft/src/syft/service/notifier/notifier.py index e4da83739ad..d625f684c18 100644 --- a/packages/syft/src/syft/service/notifier/notifier.py +++ b/packages/syft/src/syft/service/notifier/notifier.py @@ -70,26 +70,6 @@ def check_credentials( password=password, ) - @staticmethod - def check_credentials( - server: str, - port: int, - token: Optional[str] = None, - username: Optional[str] = None, - password: Optional[str] = None, - ) -> bool: - if token: - return SMTPClient( - smtp_server=server, smtp_port=port, access_token=token - ).check_credentials() - else: - return SMTPClient( - smtp_server=server, - smtp_port=port, - username=username, - password=password, - ).check_credentials() - def send(self, node: AbstractNode, notification: Notification) -> Result[Ok, Err]: try: user_service = node.get_service("userservice") diff --git a/packages/syft/src/syft/service/notifier/notifier_service.py b/packages/syft/src/syft/service/notifier/notifier_service.py index 5cdabfff567..2916c3803e8 100644 --- a/packages/syft/src/syft/service/notifier/notifier_service.py +++ b/packages/syft/src/syft/service/notifier/notifier_service.py @@ -277,7 +277,6 @@ def init_notifier( f"Error initializing notifier. \n {validation_result.err()}" ) - # This is not a public API. # This method is used by other services to dispatch notifications internally def dispatch_notification( diff --git a/packages/syft/src/syft/service/notifier/smtp_client.py b/packages/syft/src/syft/service/notifier/smtp_client.py index c9e8cab58c6..dfa7be2e2a8 100644 --- a/packages/syft/src/syft/service/notifier/smtp_client.py +++ b/packages/syft/src/syft/service/notifier/smtp_client.py @@ -53,7 +53,6 @@ def send(self, sender: str, receiver: list[str], subject: str, body: str) -> Non def check_credentials( cls, server: str, port: int, username: str, password: str ) -> Result[Ok, Err]: - """Check if the credentials are valid. Returns: From 68253d4080d2b49961394f944dffa1b2a8e5a007 Mon Sep 17 00:00:00 2001 From: Ionesio Junior Date: Thu, 22 Feb 2024 15:08:40 -0300 Subject: [PATCH 4/6] Small fixes --- .../src/syft/service/notifier/notifier.py | 33 ++++++----------- .../syft/service/notifier/notifier_service.py | 37 ++++++++----------- 2 files changed, 27 insertions(+), 43 deletions(-) diff --git a/packages/syft/src/syft/service/notifier/notifier.py b/packages/syft/src/syft/service/notifier/notifier.py index d625f684c18..3fe30ed8d4b 100644 --- a/packages/syft/src/syft/service/notifier/notifier.py +++ b/packages/syft/src/syft/service/notifier/notifier.py @@ -24,7 +24,7 @@ from .notifier_enums import NOTIFIERS from .smtp_client import SMTPClient -DEFAULT_EMAIL_SERVER = "smtp.mailgun.org" +DEFAULT_EMAIL_SERVER = "smtp.postmarkapp.com" class BaseNotifier: @@ -54,6 +54,12 @@ def __init__( self.password = password self.server = server self.port = port + self.smtp_client = SMTPClient( + server=self.server, + port=self.port, + username=self.username, + password=self.password, + ) @classmethod def check_credentials( @@ -62,7 +68,7 @@ def check_credentials( password: str, server: str = DEFAULT_EMAIL_SERVER, port: int = 587, - ) -> bool: + ) -> Result[Ok, Err]: return cls.smtp_client.check_credentials( server=server, port=port, @@ -90,22 +96,10 @@ def send(self, node: AbstractNode, notification: Notification) -> Result[Ok, Err sender=sender_email, receiver=receiver_email, subject=subject, body=body ) return Ok("Email sent successfully!") - except Exception as e: - return Err(f"Error: unable to send email: {e}") - - -# @serializable() -# @dataclass -# class EmailNotifierSettings: -# """Email notifier configuration""" -# server: str -# smtp_client = SMTPClient -# username: str -# password: str -# server: str = DEFAULT_EMAIL_SERVER -# port: int = 587 -# notifier = EmailNotifier -# subscribers = set() + except Exception: + return Err( + "Some notifications failed to be delivered. Please check the health of the mailing server." + ) @serializable() @@ -115,9 +109,6 @@ class NotifierSettings(SyftObject): __repr_attrs__ = [ "active", "email_enabled", - "sms_enabled", - "slack_enabled", - "app_enabled", ] active: bool = False # Flag to identify which notification is enabled diff --git a/packages/syft/src/syft/service/notifier/notifier_service.py b/packages/syft/src/syft/service/notifier/notifier_service.py index 2916c3803e8..45fd33acdba 100644 --- a/packages/syft/src/syft/service/notifier/notifier_service.py +++ b/packages/syft/src/syft/service/notifier/notifier_service.py @@ -79,14 +79,23 @@ def turn_on( if result.is_err(): return SyftError(message=result.err()) + # 2 - If one of the credentials are set alone, return an error + if ( + email_username + and not email_password + or email_password + and not email_username + ): + return SyftError(message="You must provide both username and password") + notifier = result.ok() print("[LOG] Got notifier from db") # If no new credentials provided, check for existing ones if not (email_username and email_password): if not (notifier.email_username and notifier.email_password): return SyftError( - message="Email credentials haven't been set yet." - "Username and password are required to enable email notifications." + message="No valid token has been added to the domain." + + "You can add a pair of SMTP credentials via .settings.enable_notifications(email=<>, password=<>)" ) else: print("[LOG] No new credentials provided. Using existing ones.") @@ -94,14 +103,13 @@ def turn_on( email_username = notifier.email_username print("[LOG] Validating credentials...") - # TODO: this should be a method in NotifierSettings validation_result = notifier.validate_email_credentials( username=email_username, password=email_password ) if validation_result.is_err(): return SyftError( - message=f"Error validating email credentials. \n {validation_result.err()}" + message="Invalid SMTP credentials. Please check your username and password." ) notifier.email_password = email_password @@ -202,11 +210,6 @@ def deactivate( notifier = result.ok() user_key = context.credentials - if user_key not in notifier.email_subscribers: - return SyftSuccess( - message="Notifications were already deactivated for this user." - ) - notifier.email_subscribers.remove(user_key) result = self.stash.update(credentials=admin_key, settings=notifier) @@ -249,8 +252,6 @@ def init_notifier( notifier = NotifierSettings() notifier.active = False # Default to False - print("New Notifier created") - print(notifier) # TODO: this should be a method in NotifierSettings if email_username and email_password: validation_result = notifier.validate_email_credentials( @@ -258,25 +259,17 @@ def init_notifier( ) if validation_result.is_err(): - raise ValueError( - f"Error validating email credentials. \n {validation_result.err()}" - ) + notifier.active = False else: notifier.email_password = email_password notifier.email_username = email_username notifier.active = True - print("After update") - print(notifier) - notifier_stash.set(node.signing_key.verify_key, notifier) return Ok("Notifier initialized successfully") - except Exception: - raise Exception( - f"Error initializing notifier. \n {validation_result.err()}" - ) - + except Exception as e: + raise Exception(f"Error initializing notifier. \n {e}") # This is not a public API. # This method is used by other services to dispatch notifications internally def dispatch_notification( From daf48b0f900e96705da5d07068c4a1fca1317ff7 Mon Sep 17 00:00:00 2001 From: Ionesio Junior Date: Thu, 22 Feb 2024 15:16:25 -0300 Subject: [PATCH 5/6] Run Lint --- packages/syft/src/syft/service/notifier/notifier_service.py | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/syft/src/syft/service/notifier/notifier_service.py b/packages/syft/src/syft/service/notifier/notifier_service.py index 45fd33acdba..68a958b17f9 100644 --- a/packages/syft/src/syft/service/notifier/notifier_service.py +++ b/packages/syft/src/syft/service/notifier/notifier_service.py @@ -270,6 +270,7 @@ def init_notifier( except Exception as e: raise Exception(f"Error initializing notifier. \n {e}") + # This is not a public API. # This method is used by other services to dispatch notifications internally def dispatch_notification( From 86f5692761fdb5d9c72ff13be58045a85383ccd8 Mon Sep 17 00:00:00 2001 From: Ionesio Junior Date: Thu, 22 Feb 2024 15:20:14 -0300 Subject: [PATCH 6/6] Fix lint issue --- packages/syft/src/syft/service/notifier/notifier_service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/syft/src/syft/service/notifier/notifier_service.py b/packages/syft/src/syft/service/notifier/notifier_service.py index 68a958b17f9..7c92d1451b7 100644 --- a/packages/syft/src/syft/service/notifier/notifier_service.py +++ b/packages/syft/src/syft/service/notifier/notifier_service.py @@ -95,7 +95,8 @@ def turn_on( if not (notifier.email_username and notifier.email_password): return SyftError( message="No valid token has been added to the domain." - + "You can add a pair of SMTP credentials via .settings.enable_notifications(email=<>, password=<>)" + + "You can add a pair of SMTP credentials via " + + ".settings.enable_notifications(email=<>, password=<>)" ) else: print("[LOG] No new credentials provided. Using existing ones.")