Merge pull request #8525 from OpenMined/feature/user_notifications

Add email notification setting to User

packages/syft/src/syft/service/notifier/notifier.py

@@ -77,9 +77,14 @@ def send(
         try:
             user_service = context.node.get_service("userservice")
 
-            receiver_email = user_service.get_by_verify_key(
-                notification.to_user_verify_key
-            ).email
+            receiver = user_service.get_by_verify_key(notification.to_user_verify_key)
+
+            if not receiver.notifications_enabled[NOTIFIERS.EMAIL]:
+                return Ok(
+                    "Email notifications are disabled for this user."
+                )  # TODO: Should we return an error here?
+
+            receiver_email = receiver.email
 
             subject = notification.email_template.email_title(
                 notification, context=context
@@ -191,7 +196,7 @@ def select_notifiers(self, notification: Notification) -> List[BaseNotifier]:
                 self.notifiers_status[notifier_type]
                 and self.notifiers[notifier_type] is not None
             ):
-                # If notifier is email, we need to pass the token
+                # If notifier is email, we need to pass the parameters
                 if notifier_type == NOTIFIERS.EMAIL:
                     notifier_objs.append(
                         self.notifiers[notifier_type](

packages/syft/src/syft/service/notifier/notifier_service.py

@@ -5,6 +5,7 @@
 from typing import Union
 
 # third party
+from pydantic import EmailStr
 from result import Err
 from result import Ok
 from result import Result
@@ -109,11 +110,6 @@ def turn_on(
             username=email_username, password=email_password
         )
 
-        if not email_sender and not notifier.email_sender:
-            return SyftError(
-                message="You must provide a sender email address to enable notifications."
-            )
-
         if validation_result.is_err():
             return SyftError(
                 message="Invalid SMTP credentials. Please check your username and password."
@@ -122,7 +118,19 @@ def turn_on(
         notifier.email_password = email_password
         notifier.email_username = email_username
 
+        # Email sender verification
+        if not email_sender and not notifier.email_sender:
+            return SyftError(
+                message="You must provide a sender email address to enable notifications."
+            )
+
         if email_sender:
+            try:
+                EmailStr.validate(email_sender)
+            except ValueError:
+                return SyftError(
+                    message="Invalid sender email address. Please check your email address."
+                )
             notifier.email_sender = email_sender
 
         notifier.active = True
@@ -170,34 +178,8 @@ def activate(
         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()
-
-        user_key = context.credentials
-
-        if user_key in notifier.email_subscribers:
-            return SyftSuccess(
-                message="Notifications are already activated for this user."
-            )
-
-        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.")
+        user_service = context.node.get_service("userservice")
+        return user_service.enable_notifications(context)
 
     @service_method(
         path="notifier.deactivate",
@@ -211,22 +193,8 @@ def deactivate(
         """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())
-
-        notifier = result.ok()
-        user_key = context.credentials
-        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.")
+        user_service = context.node.get_service("userservice")
+        return user_service.disable_notifications(context)
 
     @staticmethod
     def init_notifier(

packages/syft/src/syft/service/notifier/notifier_stash.py

@@ -29,24 +29,25 @@ class NotifierStash(BaseStash):
         name=NotifierSettings.__canonical_name__, object_type=NotifierSettings
     )
 
-    # Usual Stash Implementation
-    # The only diff is: We need to be sure that
-    # this will act as a Singleton (as SettingsStash).
-
     def __init__(self, store: DocumentStore) -> None:
         super().__init__(store=store)
 
     def admin_verify_key(self) -> SyftVerifyKey:
         return self.partition.root_verify_key
 
+    # TODO: should this method behave like a singleton?
     def get(self, credentials: SyftVerifyKey) -> Result[NotifierSettings, Err]:
         """Get Settings"""
         result = self.get_all(credentials)
         if result.is_ok():
             settings = result.ok()
             if len(settings) == 0:
-                return Ok(None)
-            result = settings[0]
+                return Ok(
+                    None
+                )  # TODO: Stash shouldn't be empty after init. Return Err instead?
+            result = settings[
+                0
+            ]  # TODO: Should we check if theres more than one? => Report corruption
             return Ok(result)
         else:
             return Err(message=result.err())