Feat/integ tests notifications (#1597)

### Feature or Bugfix
<!-- please choose -->
- Feature

### Detail
- Integration Tests Notifications


### Relates
- #1220

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: Sofia Sazonova <[email protected]>
Co-authored-by: dlpzx <[email protected]>

backend/dataall/modules/notifications/api/mutations.py

@@ -1,5 +1,5 @@
 from dataall.base.api import gql
-from .resolvers import delete, mark_as_read
+from .resolvers import mark_as_read
 
 
 markNotificationAsRead = gql.MutationField(
@@ -10,10 +10,3 @@
     type=gql.Boolean,
     resolver=mark_as_read,
 )
-
-deleteNotification = gql.MutationField(
-    name='deleteNotification',
-    args=[gql.Argument(name='notificationUri', type=gql.String)],
-    type=gql.Boolean,
-    resolver=delete,
-)

backend/dataall/modules/notifications/api/queries.py

@@ -1,7 +1,5 @@
 from dataall.base.api import gql
 from .resolvers import (
-    count_deleted_notifications,
-    count_read_notifications,
     count_unread_notifications,
     list_my_notifications,
 )
@@ -21,17 +19,3 @@
     type=gql.Integer,
     resolver=count_unread_notifications,
 )
-
-# Not used in frontend
-countReadNotifications = gql.QueryField(
-    name='countReadNotifications',
-    type=gql.Integer,
-    resolver=count_read_notifications,
-)
-
-# Not used in frontend
-countDeletedNotifications = gql.QueryField(
-    name='countDeletedNotifications',
-    type=gql.Integer,
-    resolver=count_deleted_notifications,
-)

backend/dataall/modules/notifications/api/resolvers.py

@@ -1,19 +1,11 @@
 import logging
 
+from dataall.modules.notifications.services.notification_service import NotificationService
 from dataall.base.api.context import Context
-from dataall.base.context import get_context
 from dataall.base.db import exceptions
-from dataall.modules.notifications.db.notification_repositories import NotificationRepository
 
 log = logging.getLogger(__name__)
 
-# For simplicity there is no additional layer for the business logic of notifications as it happens with other more
-# complex modules. In the resolvers we check the input and we perform the db calls directly.
-
-
-def _session():
-    return get_context().db_engine.scoped_session()
-
 
 def _required_uri(uri):
     if not uri:
@@ -27,10 +19,7 @@ def list_my_notifications(
 ):
     if not filter:
         filter = {}
-    with _session() as session:
-        return NotificationRepository.paginated_notifications(
-            session=session, username=get_context().username, groups=get_context().groups, filter=filter
-        )
+    return NotificationService.list_my_notifications(filter=filter)
 
 
 def mark_as_read(
@@ -39,32 +28,8 @@ def mark_as_read(
     notificationUri: str = None,
 ):
     _required_uri(notificationUri)
-    with _session() as session:
-        return NotificationRepository.read_notification(session=session, notificationUri=notificationUri)
+    return NotificationService.mark_as_read(notificationUri=notificationUri)
 
 
 def count_unread_notifications(context: Context, source):
-    with _session() as session:
-        return NotificationRepository.count_unread_notifications(
-            session=session, username=get_context().username, groups=get_context().groups
-        )
-
-
-def count_deleted_notifications(context: Context, source):
-    with _session() as session:
-        return NotificationRepository.count_deleted_notifications(
-            session=session, username=get_context().username, groups=get_context().groups
-        )
-
-
-def count_read_notifications(context: Context, source):
-    with _session() as session:
-        return NotificationRepository.count_read_notifications(
-            session=session, username=get_context().username, groups=get_context().groups
-        )
-
-
-def delete(context: Context, source, notificationUri):
-    _required_uri(notificationUri)
-    with _session() as session:
-        return NotificationRepository.delete_notification(session=session, notificationUri=notificationUri)
+    return NotificationService.count_unread_notifications()

backend/dataall/modules/notifications/db/notification_repositories.py

@@ -1,33 +1,7 @@
-from datetime import datetime
-
 from sqlalchemy import func, and_, or_
 
 from dataall.modules.notifications.db import notification_models as models
-from dataall.base.db import paginate, exceptions
-from dataall.base.context import get_context
-from functools import wraps
-
-
-class NotificationAccess:
-    @staticmethod
-    def is_recipient(f):
-        @wraps(f)
-        def wrapper(*args, **kwds):
-            uri = kwds.get('notificationUri')
-            if not uri:
-                raise KeyError(f"{f.__name__} doesn't have parameter uri.")
-            context = get_context()
-            with context.db_engine.scoped_session() as session:
-                notification = session.query(models.Notification).get(uri)
-                if notification and (notification.recipient in context.groups + [context.username]):
-                    return f(*args, **kwds)
-                else:
-                    raise exceptions.UnauthorizedOperation(
-                        action='UPDATE NOTIFICATION',
-                        message=f'User {context.username} is not the recipient user/group of the notification {uri}',
-                    )
-
-        return wrapper
+from dataall.base.db import paginate
 
 
 class NotificationRepository:
@@ -91,39 +65,12 @@ def count_unread_notifications(session, username, groups):
         return int(count)
 
     @staticmethod
-    def count_read_notifications(session, username, groups):
-        count = (
-            session.query(func.count(models.Notification.notificationUri))
-            .filter(or_(models.Notification.recipient == username, models.Notification.recipient.in_(groups)))
-            .filter(models.Notification.is_read == True)
-            .filter(models.Notification.deleted.is_(None))
-            .scalar()
-        )
-        return int(count)
-
-    @staticmethod
-    def count_deleted_notifications(session, username, groups):
-        count = (
-            session.query(func.count(models.Notification.notificationUri))
-            .filter(or_(models.Notification.recipient == username, models.Notification.recipient.in_(groups)))
-            .filter(models.Notification.deleted.isnot(None))
-            .scalar()
-        )
-        return int(count)
-
-    @staticmethod
-    @NotificationAccess.is_recipient
     def read_notification(session, notificationUri):
         notification = session.query(models.Notification).get(notificationUri)
         notification.is_read = True
         session.commit()
         return True
 
     @staticmethod
-    @NotificationAccess.is_recipient
-    def delete_notification(session, notificationUri):
-        notification = session.query(models.Notification).get(notificationUri)
-        if notification:
-            notification.deleted = datetime.now()
-            session.commit()
-        return True
+    def get_notification(session, uri):
+        return session.query(models.Notification).get(uri)

backend/dataall/modules/notifications/services/notification_service.py

@@ -0,0 +1,71 @@
+"""
+A service layer for Notifications
+"""
+
+import logging
+from dataall.base.db import exceptions
+
+from dataall.base.context import get_context
+from dataall.modules.notifications.db import notification_models as models
+from functools import wraps
+
+from dataall.modules.notifications.db.notification_repositories import NotificationRepository
+
+logger = logging.getLogger(__name__)
+
+
+class NotificationAccess:
+    @staticmethod
+    def check_recipient(uri):
+        context = get_context()
+        with context.db_engine.scoped_session() as session:
+            notification = NotificationRepository.get_notification(session=session, uri=uri)
+            return notification and (notification.recipient in context.groups + [context.username])
+
+    @staticmethod
+    def is_recipient(f):
+        @wraps(f)
+        def wrapper(*args, **kwds):
+            uri = kwds.get('notificationUri')
+            if not uri:
+                raise KeyError(f"{f.__name__} doesn't have parameter uri.")
+
+            if NotificationAccess.check_recipient(uri):
+                return f(*args, **kwds)
+            else:
+                raise exceptions.UnauthorizedOperation(
+                    action='UPDATE NOTIFICATION',
+                    message=f'User {get_context().username} is not the recipient user/group of the notification {uri}',
+                )
+
+        return wrapper
+
+
+class NotificationService:
+    """
+    Encapsulate the logic of interactions with notifications.
+    """
+
+    @staticmethod
+    def list_my_notifications(filter: dict = {}):
+        """List existed user notifications. Filters only required notifications by the filter param"""
+        context = get_context()
+
+        with context.db_engine.scoped_session() as session:
+            return NotificationRepository.paginated_notifications(
+                session=session, username=context.username, groups=context.groups, filter=filter
+            )
+
+    @staticmethod
+    @NotificationAccess.is_recipient
+    def mark_as_read(notificationUri: str):
+        with get_context().db_engine.scoped_session() as session:
+            return NotificationRepository.read_notification(session=session, notificationUri=notificationUri)
+
+    @staticmethod
+    def count_unread_notifications():
+        context = get_context()
+        with context.db_engine.scoped_session() as session:
+            return NotificationRepository.count_unread_notifications(
+                session=session, username=context.username, groups=context.groups
+            )

frontend/src/services/graphql/Notification/index.js

@@ -1,6 +1,3 @@
-export * from './archiveNotification';
-export * from './countDeletedNotifications';
-export * from './countReadNotifications';
 export * from './countUnreadNotifications';
 export * from './listNotifications';
 export * from './markAsRead';

tests/permissions.py

@@ -159,6 +159,8 @@ class TestData:
     glossary_owner_perm: bool = False
     mf_owner_ignore: IgnoreReason = IgnoreReason.NOTREQUIRED
     mf_owner_perm: bool = False
+    notification_recipient_ignore: IgnoreReason = IgnoreReason.NOTREQUIRED
+    notification_recipient_perm: bool = False
 
     def get_perm(self, _type: str) -> str:
         return getattr(self, f'{_type}_perm')
@@ -496,9 +498,6 @@ def __post_init__(self):
         tenant_perm=MANAGE_METADATA_FORMS, resource_ignore=IgnoreReason.USERLIMITED, mf_owner_perm=True
     ),
     field_id('Mutation', 'deleteNetwork'): TestData(tenant_perm=MANAGE_ENVIRONMENTS, resource_perm=DELETE_NETWORK),
-    field_id('Mutation', 'deleteNotification'): TestData(
-        tenant_ignore=IgnoreReason.APPSUPPORT, resource_ignore=IgnoreReason.APPSUPPORT
-    ),
     field_id('Mutation', 'deleteOmicsRun'): TestData(tenant_perm=MANAGE_OMICS_RUNS, resource_perm=DELETE_OMICS_RUN),
     field_id('Mutation', 'deleteRedshiftConnection'): TestData(
         tenant_perm=MANAGE_REDSHIFT_CONNECTIONS, resource_perm=DELETE_REDSHIFT_CONNECTION
@@ -544,7 +543,7 @@ def __post_init__(self):
         tenant_perm=MANAGE_ORGANIZATIONS, resource_perm=INVITE_ORGANIZATION_GROUP
     ),
     field_id('Mutation', 'markNotificationAsRead'): TestData(
-        tenant_ignore=IgnoreReason.APPSUPPORT, resource_ignore=IgnoreReason.APPSUPPORT
+        tenant_ignore=IgnoreReason.APPSUPPORT, resource_ignore=IgnoreReason.CUSTOM, notification_recipient_perm=True
     ),
     field_id('Mutation', 'postFeedMessage'): TestData(
         tenant_ignore=IgnoreReason.APPSUPPORT, resource_perm=TARGET_TYPE_PERM
@@ -701,12 +700,6 @@ def __post_init__(self):
     field_id('Permission', 'type'): TestData(
         resource_ignore=IgnoreReason.INTRAMODULE, tenant_ignore=IgnoreReason.NOTREQUIRED
     ),
-    field_id('Query', 'countDeletedNotifications'): TestData(
-        resource_ignore=IgnoreReason.USERLIMITED, tenant_ignore=IgnoreReason.USERLIMITED
-    ),
-    field_id('Query', 'countReadNotifications'): TestData(
-        resource_ignore=IgnoreReason.USERLIMITED, tenant_ignore=IgnoreReason.USERLIMITED
-    ),
     field_id('Query', 'countUnreadNotifications'): TestData(
         resource_ignore=IgnoreReason.USERLIMITED, tenant_ignore=IgnoreReason.USERLIMITED
     ),

tests/test_permissions.py

@@ -66,8 +66,11 @@ def setup_Mutation_upVote(mocker, **kwargs):
 
 
 @pytest.mark.parametrize('field', ALL_PARAMS)
-@pytest.mark.parametrize('perm_type', ['resource', 'tenant', 'tenant_admin', 'glossary_owner', 'mf_owner'])
+@pytest.mark.parametrize(
+    'perm_type', ['resource', 'tenant', 'tenant_admin', 'glossary_owner', 'mf_owner', 'notification_recipient']
+)
 @patch('dataall.base.context._request_storage')
+@patch('dataall.modules.notifications.services.notification_service.NotificationAccess.check_recipient')
 @patch('dataall.modules.metadata_forms.services.metadata_form_access_service.MetadataFormAccessService.is_owner')
 @patch('dataall.modules.catalog.services.glossaries_service.GlossariesResourceAccess.check_owner')
 @patch('dataall.core.permissions.services.resource_policy_service.ResourcePolicyService.check_user_resource_permission')
@@ -89,6 +92,7 @@ def test_permissions(
     mock_check_resource,
     mock_check_glossary_owner,
     mock_check_mf_owner,
+    mock_check_notification_recipient,
     mock_storage,
     field,
     perm_type,
@@ -139,7 +143,7 @@ def test_permissions(
             tenant_name=ANY,
             permission_name=perm,
         )
-    elif perm_type in ['tenant_admin', 'glossary_owner', 'mf_owner']:
+    elif perm_type in ['tenant_admin', 'glossary_owner', 'mf_owner', 'notification_recipient']:
         locals()[f'mock_check_{perm_type}'].assert_called()  # nosemgrep
     else:
         raise ValueError(f'unknown permission type {perm_type}')

tests_new/integration_tests/conftest.py

@@ -17,6 +17,7 @@
     'integration_tests.core.environment.global_conftest',
     'integration_tests.modules.s3_datasets.global_conftest',
     'integration_tests.modules.redshift_datasets.global_conftest',
+    'integration_tests.modules.shares.s3_datasets_shares.global_conftest',
 ]