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.

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

Prevent duplicate unsubscribe requests #4304

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/dao/unsubscribe_request_dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def create_unsubscribe_request_dao(unsubscribe_data):


def get_unsubscribe_request_by_notification_id_dao(notification_id):
return UnsubscribeRequest.query.filter_by(notification_id=notification_id).one()
return UnsubscribeRequest.query.filter_by(notification_id=notification_id).one_or_none()


def get_unsubscribe_requests_statistics_dao(service_id):
Expand Down
25 changes: 25 additions & 0 deletions app/one_click_unsubscribe/rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
from app.dao.unsubscribe_request_dao import (
create_unsubscribe_request_dao,
get_unbatched_unsubscribe_requests_dao,
get_unsubscribe_request_by_notification_id_dao,
get_unsubscribe_request_report_by_id_dao,
get_unsubscribe_request_reports_dao,
)
from app.errors import InvalidRequest, register_errors
Expand All @@ -30,6 +32,10 @@ def one_click_unsubscribe(notification_id, token):
errors = {"unsubscribe request": "This is not a valid unsubscribe link."}
raise InvalidRequest(errors, status_code=404) from e

# Check if the unsubscribe request is a duplicate
if is_duplicate_unsubscribe_request(notification_id):
return jsonify(result="success", message="Unsubscribe successful"), 200

if notification := get_notification_by_id(notification_id):
unsubscribe_data = get_unsubscribe_request_data(notification, email_address)

Expand Down Expand Up @@ -69,3 +75,22 @@ def create_unsubscribe_request_reports_summary(service_id):
UnsubscribeRequestReport.serialize_unbatched_requests(unbatched_unsubscribe_requests)
] + unsubscribe_request_reports
return unsubscribe_request_reports


def is_duplicate_unsubscribe_request(notification_id):
"""
A duplicate unsubscribe request is being defined as an unsubscribe_request that has
the same notification_id of a previously received unsubscribe request that has not yet been processed
by the service that initiated the notification.
"""
unsubscribe_request = get_unsubscribe_request_by_notification_id_dao(notification_id)

if not unsubscribe_request:
return False

report_id = unsubscribe_request.unsubscribe_request_report_id

if report_id and get_unsubscribe_request_report_by_id_dao(report_id).processed_by_service_at:
return False

return True
36 changes: 36 additions & 0 deletions tests/app/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
User,
WebauthnCredential,
)
from app.utils import midnight_n_days_ago


def create_user(*, mobile_number="+447700900986", email=None, state="active", id_=None, name="Test User", **kwargs):
Expand Down Expand Up @@ -1322,3 +1323,38 @@ def create_unsubscribe_request_report(
)
create_unsubscribe_request_reports_dao(report)
return report


def create_unsubscribe_request_and_return_the_notification_id(
sample_service, sample_template, is_batched, processed_by_service
):
notification = create_notification(
template=sample_template,
to_field="example@example.com",
sent_at=datetime.now() - timedelta(days=4),
)

unsubscribe_request_report_id = None
if is_batched:
# Create processed unsubscribe request report
unsubscribe_request_report = create_unsubscribe_request_report(
sample_service,
earliest_timestamp=midnight_n_days_ago(4),
latest_timestamp=midnight_n_days_ago(2),
processed_by_service_at=midnight_n_days_ago(1) if processed_by_service else None,
)
unsubscribe_request_report_id = unsubscribe_request_report.id

# Create an unsubscribe request
create_unsubscribe_request_dao(
{
"notification_id": notification.id,
"template_id": notification.template_id,
"template_version": notification.template_version,
"service_id": sample_service.id,
"email_address": notification.to,
"created_at": datetime.now(),
"unsubscribe_request_report_id": unsubscribe_request_report_id,
}
)
return notification.id
43 changes: 42 additions & 1 deletion tests/app/one_click_unsubscribe/test_one_click_unsubscribe.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import uuid
from unittest.mock import call

import pytest
from flask import current_app
from notifications_utils.url_safe_token import generate_token

from app.constants import EMAIL_TYPE
from app.dao.templates_dao import dao_update_template
from app.dao.unsubscribe_request_dao import get_unsubscribe_request_by_notification_id_dao
from tests.app.db import create_notification, create_template
from app.models import UnsubscribeRequest
from app.one_click_unsubscribe.rest import is_duplicate_unsubscribe_request
from tests.app.db import create_notification, create_template, create_unsubscribe_request_and_return_the_notification_id


def unsubscribe_url_post(client, notification_id, token):
Expand Down Expand Up @@ -36,6 +39,21 @@ def test_valid_one_click_unsubscribe_url(mocker, client, sample_email_notificati
]


def test_duplicate_unsubscribe_requests(mocker, client, sample_email_notification):
token = generate_token(
sample_email_notification.to, current_app.config["SECRET_KEY"], current_app.config["DANGEROUS_SALT"]
)
# first unsubscribe request
unsubscribe_url_post(client, sample_email_notification.id, token)
# duplicate unsubscribe request
unsubscribe_url_post(client, sample_email_notification.id, token)

result = UnsubscribeRequest.query.filter_by(notification_id=sample_email_notification.id).all()

# The duplicate unsubscribe request shouldn't be added to the unsubscribe_request table
assert len(result) == 1


def test_unsubscribe_request_object_refers_to_correct_template_version_after_template_updated(client, sample_service):
test_template = create_template(
sample_service,
Expand Down Expand Up @@ -119,3 +137,26 @@ def test_invalid_one_click_unsubscribe_url_notification_id(client, sample_email_
response_json_data = response.get_json()
assert response.status_code == 404
assert response_json_data["message"] == {"unsubscribe request": "This is not a valid unsubscribe link."}


@pytest.mark.parametrize(
"create_previous_unsubscribe_request,is_batched, processed_by_service, expected_result",
[(False, False, False, False), (True, True, True, False), (True, False, False, True), (True, True, False, True)],
)
def test_is_duplicate_unsubscribe_request(
sample_service,
sample_email_template,
create_previous_unsubscribe_request,
is_batched,
processed_by_service,
expected_result,
):
if create_previous_unsubscribe_request:
notification_id = create_unsubscribe_request_and_return_the_notification_id(
sample_service, sample_email_template, is_batched, processed_by_service
)
else:
notification_id = uuid.uuid4()

result = is_duplicate_unsubscribe_request(notification_id)
assert result == expected_result