Revert "Dev: #1344 Separate read and write engine bindings & Authenti…

…catedServiceInfo instead of "return query.one()" (#1395)" (#1418)

This reverts commit 908faed, which causes migration failures.

.github/workflows/tests.yaml

@@ -52,7 +52,6 @@ jobs:
           NOTIFY_ENVIRONMENT: test
           RETRY_SQS_URL: RETRY_SQS_URL
           SQLALCHEMY_DATABASE_URI: postgresql://postgres:postgres@localhost/test_notification_api
-          SQLALCHEMY_DATABASE_URI_READ: postgresql://postgres:postgres@localhost/test_notification_api
           TIMEOUT: 10
           VA_PROFILE_DOMAIN: int.vaprofile.va.gov
           VETEXT_API_AUTH_SSM_PATH: test

app/__init__.py

@@ -97,12 +97,8 @@ def create_app(application, worker_id=None):
     if notify_environment == "test":
         assert worker_id is not None
         application.config["SQLALCHEMY_DATABASE_URI"] += f"_{worker_id}"
-        # set read-db to be the same as write/default instance for testing
-        application.config["SQLALCHEMY_BINDS"] = {"read-db": application.config["SQLALCHEMY_DATABASE_URI"]}
         assert "test_notification_api" in application.config["SQLALCHEMY_DATABASE_URI"], \
             "Don't run tests against the main database."
-        assert "test_notification_api" in application.config["SQLALCHEMY_DATABASE_URI_READ"], \
-            "Don't run tests against the main database."
 
     application.config["NOTIFY_APP_NAME"] = application.name
     init_app(application)

app/cloudfoundry_config.py

@@ -15,7 +15,6 @@ def extract_cloudfoundry_config():
 def set_config_env_vars(vcap_services):
     # Postgres config
     os.environ['SQLALCHEMY_DATABASE_URI'] = vcap_services['postgres'][0]['credentials']['uri']
-    os.environ['SQLALCHEMY_DATABASE_URI_READ'] = vcap_services['postgres'][0]['credentials']['uri']
 
     vcap_application = json.loads(os.environ['VCAP_APPLICATION'])
     os.environ['NOTIFY_ENVIRONMENT'] = vcap_application['space_name']

app/config.py

@@ -108,7 +108,6 @@ class Config(object):
 
     # DB conection string
     SQLALCHEMY_DATABASE_URI = os.getenv('SQLALCHEMY_DATABASE_URI')
-    SQLALCHEMY_DATABASE_URI_READ = os.getenv('SQLALCHEMY_DATABASE_URI_READ')
 
     # MMG API Key
     MMG_API_KEY = os.getenv('MMG_API_KEY')
@@ -515,10 +514,6 @@ class Development(Config):
         "SQLALCHEMY_DATABASE_URI",
         'postgresql://postgres@localhost/notification_api')
 
-    SQLALCHEMY_BINDS = {"read-db": os.getenv(
-        "SQLALCHEMY_DATABASE_URI_READ",
-        'postgresql://postgres@localhost/notification_api')}
-
     ANTIVIRUS_ENABLED = os.getenv('ANTIVIRUS_ENABLED') == '1'
 
     for queue in QueueNames.all_queues():
@@ -548,9 +543,6 @@ class Test(Development):
         'SQLALCHEMY_DATABASE_URI',
         'postgresql://postgres@localhost/test_notification_api'
     )
-    SQLALCHEMY_BINDS = {"read-db": os.getenv(
-        "SQLALCHEMY_DATABASE_URI_READ",
-        'postgresql://postgres@localhost/notification_api')}
 
     CELERY_SETTINGS = {
         'broker_url': 'you-forgot-to-mock-celery-in-your-tests://'

app/dao/api_key_dao.py

@@ -17,8 +17,7 @@
 def save_model_api_key(api_key):
     if not api_key.id:
         api_key.id = uuid.uuid4()  # must be set now so version history model can use same id
-    if not api_key.secret:
-        api_key.secret = uuid.uuid4()
+    api_key.secret = uuid.uuid4()
     db.session.add(api_key)
 
 

app/dao/dao_utils.py

@@ -2,8 +2,6 @@
 from app import db
 from app.history_meta import create_history
 from functools import wraps
-from contextlib import contextmanager
-from sqlalchemy.orm import scoped_session, sessionmaker
 
 
 def transactional(func):
@@ -81,26 +79,3 @@ def record_version(*args, **kwargs):
 
 def dao_rollback():
     db.session.rollback()
-
-
-@contextmanager
-def get_reader_session():
-    """
-    This context manager is used to abstract the connection to the read-only database engine
-    in order to execute read queries. By using a scoped session, it ensures that the session
-    is thread-local and can be reused if needed. It ensures proper handling of the session's
-    lifecycle by closing it when the context is exited.
-
-    Yields:
-        session (scoped_session): A session connected to the read-only database engine.
-
-    Example Usage:
-        with get_reader_session() as session:
-            result = session.query(Service).filter_by(id=service_id).one()
-    """
-    engine = db.engines['read-db']
-    session = scoped_session(sessionmaker(bind=engine))
-    try:
-        yield session
-    finally:
-        session.close()

app/dao/services_dao.py

@@ -1,18 +1,15 @@
 import uuid
 from datetime import date, datetime, timedelta
-from app.service.service_data import ServiceData, ServiceDataException
 
 from notifications_utils.statsd_decorators import statsd
 from notifications_utils.timezones import convert_utc_to_local_timezone
 from sqlalchemy.sql.expression import asc, case, and_, func
 from sqlalchemy.orm import joinedload
-from sqlalchemy.orm.exc import NoResultFound, MultipleResultsFound
 from flask import current_app
 
 from app import db
 from app.dao.date_util import get_current_financial_year
 from app.dao.dao_utils import (
-    get_reader_session,
     transactional,
     version_class,
     VersionOptions,
@@ -197,29 +194,16 @@ def dao_fetch_service_by_inbound_number(number):
 
 
 def dao_fetch_service_by_id_with_api_keys(service_id, only_active=False):
-    with get_reader_session() as session:
-        query = session.query(Service).filter_by(
-            id=service_id
-        ).options(
-            joinedload('api_keys')
-        )
+    query = Service.query.filter_by(
+        id=service_id
+    ).options(
+        joinedload('api_keys')
+    )
 
-        if only_active:
-            query = query.filter(Service.active)
-
-        try:
-            result = query.one()
-            # instead of returning the whole model attached to read-db engine
-            # extract needed properties and return object that can be
-            # serialized for caching
-            return ServiceData(result)
-        except (ServiceDataException, NoResultFound, MultipleResultsFound) as err:
-            # we handle this failure in the parent
-            current_app.logger.error("Could not find unique service with ID %s", service_id)
-            raise NoResultFound(err)
-        except Exception:
-            # we handle this failure in the parent
-            raise
+    if only_active:
+        query = query.filter(Service.active)
+
+    return query.one()
 
 
 def dao_fetch_all_services_by_user(user_id, only_active=False):

app/models.py

@@ -34,7 +34,7 @@
 from sqlalchemy.ext.associationproxy import association_proxy
 from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.ext.hybrid import hybrid_property
-from sqlalchemy.orm.collections import attribute_mapped_collection, InstrumentedList
+from sqlalchemy.orm.collections import attribute_mapped_collection
 
 
 SMS_TYPE = 'sms'
@@ -448,20 +448,8 @@ def get_default_letter_contact(self):
         default_letter_contact = [x for x in self.letter_contacts if x.is_default]
         return default_letter_contact[0].contact_block if default_letter_contact else None
 
-    def has_permissions(self, permissions_to_check_for):
-        if isinstance(permissions_to_check_for, InstrumentedList):
-            _permissions_to_check_for = [p.permission for p in permissions_to_check_for]
-        elif not isinstance(permissions_to_check_for, list):
-            _permissions_to_check_for = [permissions_to_check_for]
-        else:
-            _permissions_to_check_for = permissions_to_check_for
-
-        if isinstance(self.permissions, InstrumentedList):
-            _permissions = [p.permission for p in self.permissions]
-        else:
-            _permissions = self.permissions
-
-        return frozenset(_permissions_to_check_for).issubset(frozenset(_permissions))
+    def has_permission(self, permission):
+        return permission in [p.permission for p in self.permissions]
 
     def serialize_for_org_dashboard(self):
         return {

app/notifications/process_notifications.py

@@ -96,6 +96,7 @@ def persist_notification(
         template_version=template_version,
         to=recipient,
         service_id=service.id,
+        service=service,
         personalisation=personalisation,
         notification_type=notification_type,
         api_key_id=api_key_id,

app/notifications/receive_notifications.py

@@ -223,7 +223,7 @@ def fetch_potential_service(inbound_number: str, provider_name: str) -> Service:
         current_app.logger.error(message)
         raise NoSuitableServiceForInboundSms(message)
 
-    elif not service.has_permissions([INBOUND_SMS_TYPE, SMS_TYPE]):
+    elif not has_inbound_sms_permissions(service.permissions):
         statsd_client.incr(f"inbound.{provider_name}.failed")
         message = f'Service "{service.id}" does not allow inbound SMS'
         current_app.logger.error(message)
@@ -233,6 +233,11 @@ def fetch_potential_service(inbound_number: str, provider_name: str) -> Service:
         return service
 
 
+def has_inbound_sms_permissions(permissions):
+    str_permissions = [p.permission for p in permissions]
+    return set([INBOUND_SMS_TYPE, SMS_TYPE]).issubset(set(str_permissions))
+
+
 def strip_leading_forty_four(number):
     if number.startswith('44'):
         return number.replace('44', '0', 1)

app/notifications/rest.py

@@ -31,7 +31,8 @@
 from app.notifications.validators import (
     check_template_is_for_notification_type,
     check_template_is_active,
-    check_rate_limiting
+    check_rate_limiting,
+    service_has_permission
 )
 from app.schemas import (
     email_notification_schema,
@@ -121,7 +122,7 @@ def send_notification(notification_type):
     template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {}))
 
     _service_allowed_to_send_to(notification_form, authenticated_service)
-    if not authenticated_service.has_permissions(notification_type):
+    if not service_has_permission(notification_type, authenticated_service.permissions):
         raise InvalidRequest(
             {'service': ["Cannot send {}".format(get_public_notify_type_text(notification_type, plural=True))]},
             status_code=400
@@ -186,9 +187,10 @@ def get_notification_return_data(notification_id, notification, template):
 
 
 def _service_can_send_internationally(service, number):
-    phone_info = get_international_phone_info(number)
+    international_phone_info = get_international_phone_info(number)
 
-    if phone_info.international and not service.has_permissions(INTERNATIONAL_SMS_TYPE):
+    if international_phone_info.international and \
+            INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
         raise InvalidRequest(
             {'to': ["Cannot send to international mobile numbers"]},
             status_code=400

app/notifications/validators.py

@@ -23,6 +23,7 @@
 from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError
 from app import redis_store
 from app.notifications.process_notifications import create_content_for_notification
+from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id
 
@@ -107,12 +108,16 @@ def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_
         raise BadRequestError(message=message)
 
 
-# TODO #1410 clean up and remove
 def service_has_permission(notify_type, permissions):
     return notify_type in [p.permission for p in permissions]
 
 
-# TODO #1410 clean up and remove
+def check_service_has_permission(notify_type, permissions):
+    if not service_has_permission(notify_type, permissions):
+        raise BadRequestError(message="Service is not allowed to send {}".format(
+            get_public_notify_type_text(notify_type, plural=True)))
+
+
 def check_service_can_schedule_notification(permissions, scheduled_for):
     if scheduled_for:
         if not service_has_permission(SCHEDULE_NOTIFICATIONS, permissions):
@@ -126,14 +131,15 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type,
     service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_recipients)
 
     if notification_type == SMS_TYPE:
-        phone_info = get_international_phone_info(send_to)
+        international_phone_info = get_international_phone_info(send_to)
 
-        if phone_info.international and not service.has_permissions(INTERNATIONAL_SMS_TYPE):
+        if international_phone_info.international and \
+                INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
             raise BadRequestError(message="Cannot send to international mobile numbers")
 
         return validate_and_format_phone_number(
             number=send_to,
-            international=phone_info.international
+            international=international_phone_info.international
         )
     elif notification_type == EMAIL_TYPE:
         return validate_and_format_email_address(email_address=send_to)

app/service/send_notification.py

@@ -8,6 +8,7 @@
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_sms_sender_dao import dao_get_service_sms_sender_by_id
 from app.notifications.validators import (
+    check_service_has_permission,
     check_service_over_daily_message_limit,
     validate_and_format_recipient,
     validate_template
@@ -34,7 +35,6 @@
     move_uploaded_pdf_to_letters_bucket,
 )
 from app.v2.errors import BadRequestError
-from app.utils import get_public_notify_type_text
 
 
 def validate_created_by(service, created_by_id):
@@ -135,14 +135,8 @@ def get_reply_to_text(notification_type, sender_id, service, template):
 def send_pdf_letter_notification(service_id, post_data):
     service = dao_fetch_service_by_id(service_id)
 
-    if not service.has_permissions(LETTER_TYPE):
-        raise BadRequestError(message="Service is not allowed to send {}".format(
-            get_public_notify_type_text(LETTER_TYPE, plural=True)))
-
-    if not service.has_permissions(UPLOAD_LETTERS):
-        raise BadRequestError(message="Service is not allowed to send {}".format(
-            get_public_notify_type_text(UPLOAD_LETTERS, plural=True)))
-
+    check_service_has_permission(LETTER_TYPE, service.permissions)
+    check_service_has_permission(UPLOAD_LETTERS, service.permissions)
     check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service)
     validate_created_by(service, post_data['created_by'])