From eef3103dfe570b8264e5fd5815e7414cb739b766 Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Tue, 28 May 2024 18:05:51 +0500 Subject: [PATCH 1/6] added predefined format for subscribe url --- videos/constants.py | 2 ++ videos/views.py | 11 +++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/videos/constants.py b/videos/constants.py index 0f39e1e79..dfa4de452 100644 --- a/videos/constants.py +++ b/videos/constants.py @@ -14,6 +14,8 @@ PDF_FORMAT_ID = 46 WEBVTT_FORMAT_ID = 51 +TRANSCODE_JOB_SUBSCRIPTION_URL = "https://sns.{AWS_REGION}.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:{AWS_REGION}:{AWS_ACCOUNT_ID}:MediaConvertJobAlert&Token={TOKEN}" + class VideoStatus: """Simple class for possible VideoFile statuses""" diff --git a/videos/views.py b/videos/views.py index a79becb14..8a711d94b 100644 --- a/videos/views.py +++ b/videos/views.py @@ -15,7 +15,7 @@ from rest_framework.response import Response from videos.api import update_video_job -from videos.constants import VideoStatus +from videos.constants import TRANSCODE_JOB_SUBSCRIPTION_URL, VideoStatus from videos.models import Video, VideoJob from videos.tasks import update_transcripts_for_video @@ -39,7 +39,14 @@ def post( # Confirm the subscription if settings.AWS_ACCOUNT_ID not in message.get("TopicArn", ""): raise PermissionDenied - requests.get(message.get("SubscribeURL"), timeout=60) + + token = message.get("Token", "") + subscribe_url = TRANSCODE_JOB_SUBSCRIPTION_URL.format( + AWS_REGION=settings.AWS_REGION, + AWS_ACCOUNT_ID=settings.AWS_ACCOUNT_ID, + TOKEN=token, + ) + requests.get(subscribe_url, timeout=60) else: if message.get("account", "") != settings.AWS_ACCOUNT_ID: raise PermissionDenied From feb49144dca680e27edd45ea83463c711b5a1103 Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Thu, 30 May 2024 07:43:51 +0500 Subject: [PATCH 2/6] added unit tests and token validation --- videos/constants.py | 1 + videos/exceptions.py | 12 ++++++++++++ videos/utils.py | 11 +++++++++++ videos/utils_test.py | 13 +++++++++++++ videos/views.py | 13 +++++++------ videos/views_test.py | 20 ++++++++++++++++++++ 6 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 videos/exceptions.py diff --git a/videos/constants.py b/videos/constants.py index dfa4de452..b68799bdd 100644 --- a/videos/constants.py +++ b/videos/constants.py @@ -15,6 +15,7 @@ WEBVTT_FORMAT_ID = 51 TRANSCODE_JOB_SUBSCRIPTION_URL = "https://sns.{AWS_REGION}.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:{AWS_REGION}:{AWS_ACCOUNT_ID}:MediaConvertJobAlert&Token={TOKEN}" +EMPTY_TOKEN_MSG = "Token Cannot be empty!" # noqa: S105 class VideoStatus: diff --git a/videos/exceptions.py b/videos/exceptions.py new file mode 100644 index 000000000..196718dc1 --- /dev/null +++ b/videos/exceptions.py @@ -0,0 +1,12 @@ +"""Videos Exceptions""" + +from rest_framework import status +from rest_framework.exceptions import APIException + + +class BadRequest(APIException): + """Exception for Invalid request data""" + + status_code = status.HTTP_400_BAD_REQUEST + default_detail = "Bad request." + default_code = "bad_request" diff --git a/videos/utils.py b/videos/utils.py index 23bbcea7e..f134ef15e 100644 --- a/videos/utils.py +++ b/videos/utils.py @@ -11,6 +11,7 @@ from main.s3_utils import get_boto3_resource from main.utils import get_dirpath_and_filename, get_file_extension, uuid_string +from videos.constants import TRANSCODE_JOB_SUBSCRIPTION_URL from videos.models import WebsiteContent from websites.models import Website, WebsiteStarter @@ -111,3 +112,13 @@ def update_metadata(source_obj, new_uid, new_s3_path): new_metadata["uid"] = new_uid new_metadata["file"] = str(new_s3_path).lstrip("/") return new_metadata + + +def get_subscribe_url(token: str) -> str: + """Get a sns subscribe url""" + + return TRANSCODE_JOB_SUBSCRIPTION_URL.format( + AWS_REGION=settings.AWS_REGION, + AWS_ACCOUNT_ID=settings.AWS_ACCOUNT_ID, + TOKEN=token, + ) diff --git a/videos/utils_test.py b/videos/utils_test.py index 56c2b9b46..fd2eb6b99 100644 --- a/videos/utils_test.py +++ b/videos/utils_test.py @@ -10,6 +10,7 @@ create_new_content, generate_s3_path, get_content_dirpath, + get_subscribe_url, update_metadata, ) from websites.factories import ( @@ -99,3 +100,15 @@ def test_create_new_content(mocker): mock_copy_obj_s3.assert_called_once_with(source_content, destination_course) mock_get_dirpath_and_filename.assert_called_once_with("new_s3_loc") mock_uuid_string.assert_called_once() + + +def test_get_subscribe_url(mocker): + """Test for get_subscribe_url method""" + + mocker.patch("django.conf.settings.AWS_REGION", "us-east-1") + mocker.patch("django.conf.settings.AWS_ACCOUNT_ID", "1234567890") + + assert ( + get_subscribe_url("fake-token") + == "https://sns.us-east-1.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:us-east-1:1234567890:MediaConvertJobAlert&Token=fake-token" + ) diff --git a/videos/views.py b/videos/views.py index 8a711d94b..1dbf1b18c 100644 --- a/videos/views.py +++ b/videos/views.py @@ -15,9 +15,11 @@ from rest_framework.response import Response from videos.api import update_video_job -from videos.constants import TRANSCODE_JOB_SUBSCRIPTION_URL, VideoStatus +from videos.constants import EMPTY_TOKEN_MSG, VideoStatus +from videos.exceptions import BadRequest from videos.models import Video, VideoJob from videos.tasks import update_transcripts_for_video +from videos.utils import get_subscribe_url log = logging.getLogger() @@ -41,11 +43,10 @@ def post( raise PermissionDenied token = message.get("Token", "") - subscribe_url = TRANSCODE_JOB_SUBSCRIPTION_URL.format( - AWS_REGION=settings.AWS_REGION, - AWS_ACCOUNT_ID=settings.AWS_ACCOUNT_ID, - TOKEN=token, - ) + if not token: + raise BadRequest(EMPTY_TOKEN_MSG) + + subscribe_url = get_subscribe_url(token) requests.get(subscribe_url, timeout=60) else: if message.get("account", "") != settings.AWS_ACCOUNT_ID: diff --git a/videos/views_test.py b/videos/views_test.py index 06a121dba..dde6f12ac 100644 --- a/videos/views_test.py +++ b/videos/views_test.py @@ -132,6 +132,26 @@ def test_transcode_jobs_subscribe_denied(settings, mocker, drf_client): mock_get.assert_not_called() +def test_transcode_jobs_subscribe_bad_request(settings, mocker, drf_client): + """TranscodeJobView should deny a subscription request if token is invalid""" + mock_get = mocker.patch("videos.views.requests.get") + with open( # noqa: PTH123 + f"{TEST_VIDEOS_WEBHOOK_PATH}/subscribe.json", encoding="utf-8" + ) as infile: + data = json.loads( + infile.read() + .replace("AWS_ACCOUNT_ID", settings.AWS_ACCOUNT_ID) + .replace("AWS_REGION", settings.AWS_REGION) + ) + + # mock token + data["Token"] = "" + + response = drf_client.post(reverse("transcode_jobs"), data=data) + assert response.status_code == 400 + mock_get.assert_not_called() + + @pytest.mark.parametrize("callback_key", [None, "callback_key", "different_key"]) @pytest.mark.parametrize("video_status", ["submitted_for_transcription", "complete"]) def test_transcript_job(mocker, video_status, callback_key, drf_client, settings): From 52f902f3f2b4296e3a95b19ae3cddf56907d8421 Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Fri, 31 May 2024 01:16:47 +0500 Subject: [PATCH 3/6] renamed variable to remove linter warning --- videos/constants.py | 2 +- videos/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/videos/constants.py b/videos/constants.py index b68799bdd..f62ff2788 100644 --- a/videos/constants.py +++ b/videos/constants.py @@ -15,7 +15,7 @@ WEBVTT_FORMAT_ID = 51 TRANSCODE_JOB_SUBSCRIPTION_URL = "https://sns.{AWS_REGION}.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:{AWS_REGION}:{AWS_ACCOUNT_ID}:MediaConvertJobAlert&Token={TOKEN}" -EMPTY_TOKEN_MSG = "Token Cannot be empty!" # noqa: S105 +BAD_REQUEST_MSG = "Token Cannot be empty!" class VideoStatus: diff --git a/videos/views.py b/videos/views.py index 1dbf1b18c..f04244561 100644 --- a/videos/views.py +++ b/videos/views.py @@ -15,7 +15,7 @@ from rest_framework.response import Response from videos.api import update_video_job -from videos.constants import EMPTY_TOKEN_MSG, VideoStatus +from videos.constants import BAD_REQUEST_MSG, VideoStatus from videos.exceptions import BadRequest from videos.models import Video, VideoJob from videos.tasks import update_transcripts_for_video @@ -44,7 +44,7 @@ def post( token = message.get("Token", "") if not token: - raise BadRequest(EMPTY_TOKEN_MSG) + raise BadRequest(BAD_REQUEST_MSG) subscribe_url = get_subscribe_url(token) requests.get(subscribe_url, timeout=60) From fe70f63d9646f338cca208abee6963d3869cd522 Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Fri, 31 May 2024 01:19:54 +0500 Subject: [PATCH 4/6] replaced `open()` with `Path.open()` to remove linter warning --- videos/views_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/videos/views_test.py b/videos/views_test.py index dde6f12ac..719f2be52 100644 --- a/videos/views_test.py +++ b/videos/views_test.py @@ -1,6 +1,7 @@ """Tests for videos.views""" import json +from pathlib import Path from types import SimpleNamespace import pytest @@ -135,8 +136,8 @@ def test_transcode_jobs_subscribe_denied(settings, mocker, drf_client): def test_transcode_jobs_subscribe_bad_request(settings, mocker, drf_client): """TranscodeJobView should deny a subscription request if token is invalid""" mock_get = mocker.patch("videos.views.requests.get") - with open( # noqa: PTH123 - f"{TEST_VIDEOS_WEBHOOK_PATH}/subscribe.json", encoding="utf-8" + with Path(f"{TEST_VIDEOS_WEBHOOK_PATH}/subscribe.json").open( + encoding="utf-8" ) as infile: data = json.loads( infile.read() From 39b96c49d20c3bf59e2e52e868841536c70c8f3e Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Fri, 31 May 2024 02:52:20 +0500 Subject: [PATCH 5/6] resolved change requests --- videos/constants.py | 2 +- videos/exceptions.py | 2 +- videos/utils_test.py | 2 +- videos/views_test.py | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/videos/constants.py b/videos/constants.py index f62ff2788..45bb164ce 100644 --- a/videos/constants.py +++ b/videos/constants.py @@ -15,7 +15,7 @@ WEBVTT_FORMAT_ID = 51 TRANSCODE_JOB_SUBSCRIPTION_URL = "https://sns.{AWS_REGION}.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:{AWS_REGION}:{AWS_ACCOUNT_ID}:MediaConvertJobAlert&Token={TOKEN}" -BAD_REQUEST_MSG = "Token Cannot be empty!" +BAD_REQUEST_MSG = "Token cannot be empty!" class VideoStatus: diff --git a/videos/exceptions.py b/videos/exceptions.py index 196718dc1..cedf83fb4 100644 --- a/videos/exceptions.py +++ b/videos/exceptions.py @@ -5,7 +5,7 @@ class BadRequest(APIException): - """Exception for Invalid request data""" + """Exception for invalid request data""" status_code = status.HTTP_400_BAD_REQUEST default_detail = "Bad request." diff --git a/videos/utils_test.py b/videos/utils_test.py index fd2eb6b99..82abf99b3 100644 --- a/videos/utils_test.py +++ b/videos/utils_test.py @@ -103,7 +103,7 @@ def test_create_new_content(mocker): def test_get_subscribe_url(mocker): - """Test for get_subscribe_url method""" + """Test get_subscribe_url to format ConfirmSubscription url correctly""" mocker.patch("django.conf.settings.AWS_REGION", "us-east-1") mocker.patch("django.conf.settings.AWS_ACCOUNT_ID", "1234567890") diff --git a/videos/views_test.py b/videos/views_test.py index 719f2be52..cdb4f9d11 100644 --- a/videos/views_test.py +++ b/videos/views_test.py @@ -7,6 +7,7 @@ import pytest from django.http.response import HttpResponse from django.urls import reverse +from rest_framework.status import HTTP_400_BAD_REQUEST from gdrive_sync.factories import DriveFileFactory from users.factories import UserFactory @@ -102,7 +103,7 @@ def test_transcode_jobs_wrong_account(drf_client): def test_transcode_jobs_subscribe(settings, mocker, drf_client): - """TranscodeJobView should confirm a subcsription request""" + """TranscodeJobView should confirm a subscription request""" mock_get = mocker.patch("videos.views.requests.get") with open( # noqa: PTH123 f"{TEST_VIDEOS_WEBHOOK_PATH}/subscribe.json", encoding="utf-8" @@ -149,7 +150,7 @@ def test_transcode_jobs_subscribe_bad_request(settings, mocker, drf_client): data["Token"] = "" response = drf_client.post(reverse("transcode_jobs"), data=data) - assert response.status_code == 400 + assert response.status_code == HTTP_400_BAD_REQUEST mock_get.assert_not_called() From a6d5146fe4617585347d59f67d2ab8b4e35f8019 Mon Sep 17 00:00:00 2001 From: Umar Hassan Date: Fri, 31 May 2024 02:57:01 +0500 Subject: [PATCH 6/6] meeting change request --- videos/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/videos/utils.py b/videos/utils.py index f134ef15e..6a69565c4 100644 --- a/videos/utils.py +++ b/videos/utils.py @@ -115,7 +115,7 @@ def update_metadata(source_obj, new_uid, new_s3_path): def get_subscribe_url(token: str) -> str: - """Get a sns subscribe url""" + """Get a SNS subscribe url""" return TRANSCODE_JOB_SUBSCRIPTION_URL.format( AWS_REGION=settings.AWS_REGION,