Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delayed signing of notification from processing to the redis saving #2463

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Delayed signing of notification from processing to the redis saving #2463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions app/annotations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,45 @@
from functools import wraps

# from flask import current_app
from inspect import signature

from app import signer_notification
from app.encryption import SignedNotification, SignedNotifications

def sign_param(func):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a great idea but I can't tell if it works just by looking at the code. Could you add some instructions for how to test if this is working?

"""
A decorator that signs parameters annotated with `PendingNotification` or `VerifiedNotification`
before passing them to the decorated function.
This decorator inspects the function's signature to find parameters annotated with
`PendingNotification` or `VerifiedNotification`. It then uses `signer_notification.sign`
to sign these parameters and replaces the original values with the signed values before
calling the decorated function.
Args:
func (Callable): The function to be decorated.
Returns:
Callable: The wrapped function with signed parameters.
"""

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.queue import QueueMessage

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
app.queue
Loading
begins an import cycle.

sig = signature(func)

# Find the parameter annotated with VerifyAndSign
bound_args = sig.bind(*args, **kwargs)
bound_args.apply_defaults()

for param_name, param in sig.parameters.items():
if issubclass(param.annotation, QueueMessage):
unsigned: QueueMessage = bound_args.arguments[param_name] # type: ignore
signed_param = signer_notification.sign(unsigned.to_dict())
# Replace the signed value with the verified value
bound_args.arguments[param_name] = signed_param

# Call the decorated function with the signed value
result = func(*bound_args.args, **bound_args.kwargs)
return result

return wrapper


def unsign_params(func):
Expand All @@ -22,6 +57,9 @@

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.types import SignedNotification, SignedNotifications

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
app.types
Loading
begins an import cycle.

sig = signature(func)

# Find the parameter annotated with VerifyAndSign
Expand Down Expand Up @@ -59,6 +97,8 @@

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification

# Call the decorated function with the verified value
result = func(*args, **kwargs)

Expand Down
81 changes: 43 additions & 38 deletions app/celery/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
from app.dao.services_dao import dao_fetch_service_by_id
from app.dao.templates_dao import dao_get_template_by_id
from app.email_limit_utils import fetch_todays_email_count
from app.encryption import SignedNotification
from app.exceptions import DVLAException
from app.models import (
BULK,
Expand All @@ -84,7 +83,7 @@
send_notification_to_queue,
)
from app.sms_fragment_utils import fetch_todays_requested_sms_count
from app.types import VerifiedNotification
from app.types import SignedNotification, VerifiedNotification
from app.utils import get_csv_max_rows, get_delivery_queue_for_template, get_fiscal_year
from app.v2.errors import (
LiveServiceTooManyRequestsError,
Expand Down Expand Up @@ -289,23 +288,25 @@ def save_smss(self, service_id: Optional[str], signed_notifications: List[Signed
else:
reply_to_text = service.get_default_sms_sender() # type: ignore

notification: VerifiedNotification = {
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": SMS_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
notification = VerifiedNotification.from_dict(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! having a from_dict method is a much better way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename notification -> verified_notification to make this consistent with the code below?

{
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": SMS_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
)

verified_notifications.append(notification)
notification_id_queue[notification_id] = notification.get("queue") # type: ignore
Expand Down Expand Up @@ -404,26 +405,29 @@ def save_emails(self, _service_id: Optional[str], signed_notifications: List[Sig
else:
reply_to_text = service.get_default_reply_to_email_address()

notification: VerifiedNotification = {
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": EMAIL_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
verified_notification = VerifiedNotification.from_dict(
{
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": EMAIL_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
"queue": _notification.get("queue", None),
}
)

verified_notifications.append(notification)
notification_id_queue[notification_id] = notification.get("queue") # type: ignore
verified_notifications.append(verified_notification)
notification_id_queue[notification_id] = verified_notification.queue # type: ignore
process_type = template.process_type

try:
Expand Down Expand Up @@ -750,6 +754,7 @@ def send_notify_no_reply(self, data):
template = dao_get_template_by_id(current_app.config["NO_REPLY_TEMPLATE_ID"])

try:
# TODO: replace dict creation with VerifiedNotification.from_dict
data_to_send = [
dict(
template_id=template.id,
Expand Down
34 changes: 5 additions & 29 deletions app/encryption.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,7 @@
from typing import Any, List, NewType, Optional, TypedDict, cast
from typing import Any, List, cast

from flask_bcrypt import check_password_hash, generate_password_hash
from itsdangerous import URLSafeSerializer
from typing_extensions import NotRequired # type: ignore

SignedNotification = NewType("SignedNotification", str)
SignedNotifications = NewType("SignedNotifications", List[SignedNotification])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved into the types module.


class NotificationDictToSign(TypedDict):
# todo: remove duplicate keys
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved into the types module and became PendingNotification to represent the early lifecycle stage of the notification processing (when it hits the API and prior to be saved into the DB).

# todo: remove all NotRequired and decide if key should be there or not
id: NotRequired[str]
template: str # actually template_id
service_id: NotRequired[str]
template_version: int
to: str # recipient
reply_to_text: NotRequired[str]
personalisation: Optional[dict]
simulated: NotRequired[bool]
api_key: str
key_type: str # should be ApiKeyType but I can't import that here
client_reference: Optional[str]
queue: Optional[str]
sender_id: Optional[str]
job: NotRequired[str] # actually job_id
row_number: Optional[Any] # should this be int or str?


class CryptoSigner:
Expand All @@ -42,22 +18,22 @@ def init_app(self, app: Any, secret_key: str | List[str], salt: str) -> None:
self.serializer = URLSafeSerializer(secret_key)
self.salt = salt

def sign(self, to_sign: str | NotificationDictToSign) -> str | bytes:
def sign(self, to_sign: str) -> str | bytes:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncoupling the notification model from the CryptoSigner. It will only have a str to sign when that is called.

"""Sign a string or dict with the class secret key and salt.

Args:
to_sign (str | NotificationDictToSign): The string or dict to sign.
to_sign (str): The string or dict to sign.

Returns:
str | bytes: The signed string or bytes.
"""
return self.serializer.dumps(to_sign, salt=self.salt)

def sign_with_all_keys(self, to_sign: str | NotificationDictToSign) -> List[str | bytes]:
def sign_with_all_keys(self, to_sign: str) -> List[str | bytes]:
"""Sign a string or dict with all the individual keys in the class secret key list, and the class salt.

Args:
to_sign (str | NotificationDictToSign): The string or dict to sign.
to_sign (str): The string or dict to sign.

Returns:
List[str | bytes]: A list of signed values.
Expand Down
80 changes: 43 additions & 37 deletions app/notifications/process_notifications.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
)
from notifications_utils.timezones import convert_local_timezone_to_utc

from app import redis_store
from app import models, redis_store
from app.celery import provider_tasks
from app.celery.letters_pdf_tasks import create_letters_pdf
from app.config import QueueNames
Expand Down Expand Up @@ -295,70 +295,76 @@ def send_notification_to_queue(notification, research_mode, queue=None):
)


def persist_notifications(notifications: List[VerifiedNotification]) -> List[Notification]:
def persist_notifications(verifiedNotifications: List[VerifiedNotification]) -> List[Notification]:
"""
Persist Notifications takes a list of json objects and creates a list of Notifications
that gets bulk inserted into the DB.
"""

lofnotifications = []

for notification in notifications:
notification_created_at = notification.get("created_at") or datetime.utcnow()
notification_id = notification.get("notification_id", uuid.uuid4())
notification_recipient = notification.get("recipient") or notification.get("to")
service_id = notification.get("service").id if notification.get("service") else None # type: ignore
for verifiedNotification in verifiedNotifications:
notification_created_at = verifiedNotification.created_at or datetime.utcnow()
notification_id = verifiedNotification.notification_id or uuid.uuid4()
notification_recipient = verifiedNotification.recipient or verifiedNotification.to
service_id = verifiedNotification.service.id if verifiedNotification.service else None # type: ignore
# todo: potential bug. notification_obj is being created using some keys that don't exist on notification
# reference, created_by_id, status, billable_units aren't keys on notification at this point
notification_obj = Notification(
id=notification_id,
template_id=notification.get("template_id"),
template_version=notification.get("template_version"),
template_id=verifiedNotification.template_id,
template_version=verifiedNotification.template_version,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a data class so it has proper fields instead of dictionary entries.

to=notification_recipient,
service_id=service_id,
personalisation=notification.get("personalisation"),
notification_type=notification.get("notification_type"),
api_key_id=notification.get("api_key_id"),
key_type=notification.get("key_type"),
personalisation=verifiedNotification.personalisation,
notification_type=verifiedNotification.notification_type,
api_key_id=verifiedNotification.api_key_id,
key_type=verifiedNotification.key_type,
created_at=notification_created_at,
job_id=notification.get("job_id"),
job_row_number=notification.get("job_row_number"),
client_reference=notification.get("client_reference"),
reference=notification.get("reference"), # type: ignore
created_by_id=notification.get("created_by_id"), # type: ignore
status=notification.get("status"), # type: ignore
reply_to_text=notification.get("reply_to_text"),
billable_units=notification.get("billable_units"), # type: ignore
job_id=verifiedNotification.job_id,
job_row_number=verifiedNotification.job_row_number,
client_reference=verifiedNotification.client_reference,
# REVIEW: We can remove these ones if possible, as these will be set later in the process:
# reference: this is the provider's reference and will be set on sending time
# created_by_id: this is the user who created the notification and will be set on sending time, used by one off or admin UI uploads
# reference=verifiedNotification.reference, # type: ignore
# created_by_id=verifiedNotification.created_by_id, # type: ignore
# billable_units=verifiedNotification.billable_units, # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove these:

  • reference: a notification will only have a reference once SES or Pinpoint is called. The exception is letters where a random reference will be generated but this function does not handle these, not GCNotify at large.
  • created_by_id: Not set when received by the API AFAIK. Other code paths are used by the one-off send or bulk send, where the created_by_id might be set.
  • billable_units: most likely not set at this stage yet.

status=NOTIFICATION_CREATED, # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not receive a status from the notification, and should we? We can safely say the notification is created at this point, prior to save it into the database. I checked the other code paths for the persist_notification function (the one off save -- this one is for multiple) and it sets the notification to created status by default.

reply_to_text=verifiedNotification.reply_to_text,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reply_to_text field can be set via the API by providing the email_reply_to_id JSON field in the POST request. So that should be present in the PendingNotification | VerifiedNotification type and passed down the stream to the database.

)
template = dao_get_template_by_id(notification_obj.template_id, notification_obj.template_version, use_cache=True)
service = dao_fetch_service_by_id(service_id, use_cache=True)
notification_obj.queue_name = choose_queue(
notification=notification_obj, research_mode=service.research_mode, queue=get_delivery_queue_for_template(template)
)

if notification.get("notification_type") == SMS_TYPE:
formatted_recipient = validate_and_format_phone_number(notification_recipient, international=True)
recipient_info = get_international_phone_info(formatted_recipient)
notification_obj.normalised_to = formatted_recipient
notification_obj.international = recipient_info.international
notification_obj.phone_prefix = recipient_info.country_prefix
notification_obj.rate_multiplier = recipient_info.billable_units
elif notification.get("notification_type") == EMAIL_TYPE:
notification_obj.normalised_to = format_email_address(notification_recipient)
elif notification.get("notification_type") == LETTER_TYPE:
notification_obj.postage = notification.get("postage") or notification.get("template_postage") # type: ignore
match verifiedNotification.notification_type:
case models.SMS_TYPE:
formatted_recipient = validate_and_format_phone_number(notification_recipient, international=True)
recipient_info = get_international_phone_info(formatted_recipient)
notification_obj.normalised_to = formatted_recipient
notification_obj.international = recipient_info.international
notification_obj.phone_prefix = recipient_info.country_prefix
notification_obj.rate_multiplier = recipient_info.billable_units
case models.EMAIL_TYPE:
notification_obj.normalised_to = format_email_address(notification_recipient)
# case models.LETTER_TYPE:
# notification_obj.postage = verifiedNotification.postage # or verifiedNotification.template_postage
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah.. I thought this is so useless and annoying..

case _:
current_app.logger.debug(f"Notification type {verifiedNotification.notification_type} not handled")

lofnotifications.append(notification_obj)
if notification.get("key_type") != KEY_TYPE_TEST:
service_id = notification.get("service").id # type: ignore
if verifiedNotification.key_type != KEY_TYPE_TEST:
service_id = verifiedNotification.service.id # type: ignore
if redis_store.get(redis.daily_limit_cache_key(service_id)):
redis_store.incr(redis.daily_limit_cache_key(service_id))

current_app.logger.info(
"{} {} created at {}".format(
notification.get("notification_type"),
notification.get("notification_id"),
notification.get("notification_created_at"), # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this just a bug? I don't think this key was actually there.

verifiedNotification.notification_type,
verifiedNotification.notification_id,
verifiedNotification.created_at,
)
)
bulk_insert_notifications(lofnotifications)
Expand Down
Loading
Loading