From f27a5afd4677f30eabb265c012330981c5597c1f Mon Sep 17 00:00:00 2001 From: groovecoder Date: Tue, 7 May 2024 11:57:58 -0500 Subject: [PATCH 1/4] for MPP-3802: add timeouts to requests --- api/authentication.py | 2 +- api/views/privaterelay.py | 2 +- emails/utils.py | 2 +- phones/iq_utils.py | 1 + privaterelay/apps.py | 3 ++- pyproject.toml | 1 - 6 files changed, 6 insertions(+), 5 deletions(-) diff --git a/api/authentication.py b/api/authentication.py index b3dfe6b405..52947538e2 100644 --- a/api/authentication.py +++ b/api/authentication.py @@ -29,7 +29,7 @@ def get_cache_key(token): def introspect_token(token: str) -> dict[str, Any]: try: - fxa_resp = requests.post(INTROSPECT_TOKEN_URL, json={"token": token}) + fxa_resp = requests.post(INTROSPECT_TOKEN_URL, json={"token": token}, timeout=1) except Exception as exc: logger.error( "Could not introspect token with FXA.", diff --git a/api/views/privaterelay.py b/api/views/privaterelay.py index f100402c5f..0e8f5e73a7 100644 --- a/api/views/privaterelay.py +++ b/api/views/privaterelay.py @@ -330,7 +330,7 @@ def terms_accepted_user(request): except SocialAccount.DoesNotExist: # User does not exist, create a new Relay user fxa_profile_resp = requests.get( - FXA_PROFILE_URL, headers={"Authorization": f"Bearer {token}"} + FXA_PROFILE_URL, headers={"Authorization": f"Bearer {token}"}, timeout=1 ) if not (fxa_profile_resp.ok and fxa_profile_resp.content): logger.error( diff --git a/emails/utils.py b/emails/utils.py index 63ad3585a8..8c2d7a763e 100644 --- a/emails/utils.py +++ b/emails/utils.py @@ -85,7 +85,7 @@ def get_trackers(level): def download_trackers(repo_url, category="Email"): # email tracker lists from shavar-prod-list as per agreed use under license: - resp = requests.get(repo_url) + resp = requests.get(repo_url, timeout=10) json_resp = resp.json() formatted_trackers = json_resp["categories"][category] trackers = [] diff --git a/phones/iq_utils.py b/phones/iq_utils.py index 3651c4611e..d70e5111f3 100644 --- a/phones/iq_utils.py +++ b/phones/iq_utils.py @@ -18,6 +18,7 @@ def send_iq_sms(to_num: str, from_num: str, text: str) -> None: settings.IQ_PUBLISH_MESSAGE_URL, headers={"Authorization": f"Bearer {settings.IQ_OUTBOUND_API_KEY}"}, json=json_body, + timeout=5, ) if resp.status_code < 200 or resp.status_code > 299: raise exceptions.ValidationError(json.loads(resp.content.decode())) diff --git a/privaterelay/apps.py b/privaterelay/apps.py index 2fc476d8ae..7166d6416f 100644 --- a/privaterelay/apps.py +++ b/privaterelay/apps.py @@ -92,7 +92,8 @@ def ready(self) -> None: @cached_property def fxa_verifying_keys(self) -> list[dict[str, Any]]: resp = requests.get( - "{}/jwks".format(settings.SOCIALACCOUNT_PROVIDERS["fxa"]["OAUTH_ENDPOINT"]) + "{}/jwks".format(settings.SOCIALACCOUNT_PROVIDERS["fxa"]["OAUTH_ENDPOINT"]), + timeout=10, ) if resp.status_code == 200: keys: list[dict[str, Any]] = resp.json()["keys"] diff --git a/pyproject.toml b/pyproject.toml index b0b32a5271..1110f2245c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -100,7 +100,6 @@ testpaths = [ ignore = [ # TODO MPP-3802: Enable more bandit security checks "S101", # https://docs.astral.sh/ruff/rules/assert/ - "S113", # https://docs.astral.sh/ruff/rules/request-without-timeout/ ] select = [ "S", # flake8-bandit From c6a71b5dd73fedb3ec1bd3fd66b79667d5873437 Mon Sep 17 00:00:00 2001 From: groovecoder Date: Tue, 7 May 2024 13:29:02 -0500 Subject: [PATCH 2/4] for MPP-3802: refactor assert statements into if/raise checks --- api/exceptions.py | 15 ++++++---- api/serializers/emails.py | 3 +- api/views/__init__.py | 6 ++-- api/views/emails.py | 15 ++++++---- api/views/phones.py | 19 ++++++++---- emails/apps.py | 3 +- .../command_from_django_settings.py | 12 +++++--- .../commands/process_emails_from_sqs.py | 6 ++-- .../commands/send_welcome_emails.py | 9 ++++-- emails/models.py | 27 +++++++++++------ emails/utils.py | 18 ++++++++---- emails/views.py | 29 +++++++++++++------ phones/apps.py | 12 ++++++-- phones/models.py | 12 ++++++-- privaterelay/allauth.py | 5 +++- privaterelay/apps.py | 4 ++- privaterelay/cleaners.py | 16 +++++++--- privaterelay/glean_interface.py | 5 +++- .../management/commands/cleanup_data.py | 3 +- privaterelay/plans.py | 18 ++++++++---- privaterelay/settings.py | 7 +++-- privaterelay/storage.py | 3 +- privaterelay/views.py | 12 +++++--- 23 files changed, 182 insertions(+), 77 deletions(-) diff --git a/api/exceptions.py b/api/exceptions.py index fcc21f1fc8..891281b3fe 100644 --- a/api/exceptions.py +++ b/api/exceptions.py @@ -58,13 +58,17 @@ def __init__( self, detail: _APIExceptionInput = None, code: str | None = None ) -> None: """Check that derived classes have set the required data.""" - assert isinstance(self.default_code, str) - assert isinstance(self.status_code, int) + if not isinstance(self.default_code, str): + raise TypeError("default_code must be type str") + if not isinstance(self.status_code, int): + raise TypeError("self.status_code must be type int") if hasattr(self, "default_detail_template"): context = self.error_context() - assert context + if not context: + raise ValueError("error_context is required") self.default_detail = self.default_detail_template.format(**context) - assert isinstance(self.default_detail, str) + if not isinstance(self.default_detail, str): + raise TypeError("self.default_detail must be type str") super().__init__(detail, code) def error_context(self) -> ErrorContextType: @@ -76,7 +80,8 @@ def error_data(self) -> ErrorData: # For RelayAPIException classes, this is the default_code and is a string error_code = self.get_codes() - assert isinstance(error_code, str) + if not isinstance(error_code, str): + raise TypeError("error_code must be type str") # Build the Fluent error ID ftl_id_sub = "api-error-" diff --git a/api/serializers/emails.py b/api/serializers/emails.py index a6b415ba9e..093931792b 100644 --- a/api/serializers/emails.py +++ b/api/serializers/emails.py @@ -12,7 +12,8 @@ class PremiumValidatorsMixin: def validate_block_list_emails(self, value): if not value: return value - assert hasattr(self, "context") + if not hasattr(self, "context"): + raise AttributeError("self must have attribute context") user = self.context["request"].user prefetch_related_objects([user], "socialaccount_set", "profile") if not user.profile.has_premium: diff --git a/api/views/__init__.py b/api/views/__init__.py index 5a79daff1d..67ce6c2959 100644 --- a/api/views/__init__.py +++ b/api/views/__init__.py @@ -12,8 +12,10 @@ class SaveToRequestUser: """ModelViewSet mixin for creating object for the authenticated user.""" def perform_create(self, serializer): - assert hasattr(self, "request") - assert hasattr(self.request, "user") + if not hasattr(self, "request"): + raise AttributeError("self must have request attribute.") + if not hasattr(self.request, "user"): + raise AttributeError("self.request must have user attribute.") serializer.save(user=self.request.user) diff --git a/api/views/emails.py b/api/views/emails.py index 1d65e2eb2a..1f6b51473d 100644 --- a/api/views/emails.py +++ b/api/views/emails.py @@ -74,7 +74,8 @@ class Meta: class AddressViewSet(Generic[_Address], SaveToRequestUser, ModelViewSet): def perform_create(self, serializer: BaseSerializer[_Address]) -> None: super().perform_create(serializer) - assert serializer.instance + if not serializer.instance: + raise ValueError("serializer.instance must be truthy value.") glean_logger().log_email_mask_created( request=self.request, mask=serializer.instance, @@ -82,7 +83,8 @@ def perform_create(self, serializer: BaseSerializer[_Address]) -> None: ) def perform_update(self, serializer: BaseSerializer[_Address]) -> None: - assert serializer.instance is not None + if not serializer.instance: + raise ValueError("serializer.instance must be truthy value.") old_description = serializer.instance.description super().perform_update(serializer) new_description = serializer.instance.description @@ -200,10 +202,13 @@ def first_forwarded_email(request): return Response(f"{mask} does not exist for user.", status=HTTP_404_NOT_FOUND) profile = user.profile app_config = apps.get_app_config("emails") - assert isinstance(app_config, EmailsConfig) + if not isinstance(app_config, EmailsConfig): + raise TypeError("app_config must be type EmailsConfig") ses_client = app_config.ses_client - assert ses_client - assert settings.RELAY_FROM_ADDRESS + if not ses_client: + raise ValueError("ses_client must be truthy value.") + if not settings.RELAY_FROM_ADDRESS: + raise ValueError("settings.RELAY_FROM_ADDRESS must have a value.") with django_ftl.override(profile.language): translated_subject = ftl_bundle.format("forwarded-email-hero-header") first_forwarded_email_html = render_to_string( diff --git a/api/views/phones.py b/api/views/phones.py index 94577c27a9..9b5cb3376c 100644 --- a/api/views/phones.py +++ b/api/views/phones.py @@ -1316,9 +1316,15 @@ class RelaySMSException(Exception): def __init__(self, critical=False, *args, **kwargs): self.critical = critical - assert ( + if not ( self.default_detail is not None and self.default_detail_template is None - ) or (self.default_detail is None and self.default_detail_template is not None) + ) and not ( + self.default_detail is None and self.default_detail_template is not None + ): + raise ValueError( + "One and only one of default_detail or " + "default_detail_template must be None." + ) super().__init__(*args, **kwargs) @property @@ -1326,7 +1332,8 @@ def detail(self): if self.default_detail: return self.default_detail else: - assert self.default_detail_template is not None + if self.default_detail_template is None: + raise ValueError("self.default_detail_template must not be None.") return self.default_detail_template.format(**self.error_context()) def get_codes(self): @@ -1438,14 +1445,16 @@ def _prepare_sms_reply( if match and not match.contacts and match.match_type == "full": raise FullNumberMatchesNoSenders(full_number=match.detected) if match and len(match.contacts) > 1: - assert match.match_type == "short" + if not match.match_type == "short": + raise ValueError("match.match_type must be 'short'.") raise MultipleNumberMatches(short_prefix=match.detected) # Determine the destination number destination_number: str | None = None if match: # Use the sender matched by the prefix - assert len(match.contacts) == 1 + if not len(match.contacts) == 1: + raise ValueError("len(match.contacts) must be 1.") destination_number = match.contacts[0].inbound_number else: # No prefix, default to last sender if any diff --git a/emails/apps.py b/emails/apps.py index d974e9a831..f8051fe671 100644 --- a/emails/apps.py +++ b/emails/apps.py @@ -65,7 +65,8 @@ def ready(self): def emails_config() -> EmailsConfig: emails_config = apps.get_app_config("emails") - assert isinstance(emails_config, EmailsConfig) + if not isinstance(emails_config, EmailsConfig): + raise TypeError("emails_config must be type EmailsConfig") return emails_config diff --git a/emails/management/command_from_django_settings.py b/emails/management/command_from_django_settings.py index 9753e6e417..fd77e058bb 100644 --- a/emails/management/command_from_django_settings.py +++ b/emails/management/command_from_django_settings.py @@ -37,7 +37,8 @@ def create_parser(self, prog_name, subcommand, **kwargs): * Add the Django settings and their values to the command help * Override the verbosity from an environment variable """ - assert self.settings_to_locals + if not self.settings_to_locals: + raise ValueError("self.settings_to_locals must be truthy value.") epilog_lines = [ ( "Parameters are read from Django settings and the related environment" @@ -65,15 +66,18 @@ def create_parser(self, prog_name, subcommand, **kwargs): epilog = "\n".join(epilog_lines) parser = super().create_parser(prog_name, subcommand, epilog=epilog, **kwargs) - assert parser.formatter_class == DjangoHelpFormatter + if parser.formatter_class != DjangoHelpFormatter: + raise TypeError("parser.formatter_class must be DjangoHelpFormatter") parser.formatter_class = RawDescriptionDjangoHelpFormatter - assert verbosity_override is not None + if verbosity_override is None: + raise ValueError("verbosity_override must not be None.") parser.set_defaults(verbosity=verbosity_override) return parser def init_from_settings(self, verbosity): """Initialize local variables from settings""" - assert self.settings_to_locals + if not self.settings_to_locals: + raise ValueError("self.settings_to_locals must be truthy value.") for setting_key, local_name, help_str, validator in self.settings_to_locals: value = getattr(settings, setting_key) if not validator(value): diff --git a/emails/management/commands/process_emails_from_sqs.py b/emails/management/commands/process_emails_from_sqs.py index abf058c639..f7054cb295 100644 --- a/emails/management/commands/process_emails_from_sqs.py +++ b/emails/management/commands/process_emails_from_sqs.py @@ -152,8 +152,10 @@ def init_locals(self): def create_client(self): """Create the SQS client.""" - assert self.aws_region - assert self.sqs_url + if not self.aws_region: + raise ValueError("self.aws_region must be truthy value.") + if not self.sqs_url: + raise ValueError("self.sqs_url must be truthy value.") sqs_client = boto3.resource("sqs", region_name=self.aws_region) return sqs_client.Queue(self.sqs_url) diff --git a/emails/management/commands/send_welcome_emails.py b/emails/management/commands/send_welcome_emails.py index 85bc926413..2ec2437451 100644 --- a/emails/management/commands/send_welcome_emails.py +++ b/emails/management/commands/send_welcome_emails.py @@ -40,10 +40,13 @@ def _ses_message_props(data: str) -> ContentTypeDef: def send_welcome_email(profile: Profile) -> None: user = profile.user app_config = apps.get_app_config("emails") - assert isinstance(app_config, EmailsConfig) + if not isinstance(app_config, EmailsConfig): + raise TypeError("app_config must be type EmailsConfig") ses_client = app_config.ses_client - assert ses_client - assert settings.RELAY_FROM_ADDRESS + if not ses_client: + raise ValueError("ses_client must be truthy value") + if not settings.RELAY_FROM_ADDRESS: + raise ValueError("settings.RELAY_FROM_ADDRESS must be truthy value.") with django_ftl.override(profile.language): translated_subject = ftl_bundle.format("first-time-user-email-welcome") try: diff --git a/emails/models.py b/emails/models.py index a9f17e00b2..40b9d5fcf3 100644 --- a/emails/models.py +++ b/emails/models.py @@ -266,13 +266,16 @@ def next_email_try(self) -> datetime: return datetime.now(UTC) if bounce_type == "soft": - assert self.last_soft_bounce + if not self.last_soft_bounce: + raise ValueError("self.last_soft_bounce must be truthy value.") return self.last_soft_bounce + timedelta( days=settings.SOFT_BOUNCE_ALLOWED_DAYS ) - assert bounce_type == "hard" - assert self.last_hard_bounce + if bounce_type != "hard": + raise ValueError("bounce_type must be either 'soft' or 'hard'") + if not self.last_hard_bounce: + raise ValueError("self.last_hard_bounce must be truthy value.") return self.last_hard_bounce + timedelta(days=settings.HARD_BOUNCE_ALLOWED_DAYS) @property @@ -293,7 +296,8 @@ def fxa(self) -> SocialAccount | None: # Note: we are NOT using .filter() here because it invalidates # any profile instances that were queried with prefetch_related, which # we use in at least the profile view to minimize queries - assert hasattr(self.user, "socialaccount_set") + if not hasattr(self.user, "socialaccount_set"): + raise AttributeError("self.user must have socialaccount_set attribute") for sa in self.user.socialaccount_set.all(): if sa.provider == "fxa": return sa @@ -310,7 +314,8 @@ def display_name(self) -> str | None: @property def custom_domain(self) -> str: - assert self.subdomain + if not self.subdomain: + raise ValueError("self.subdomain must be truthy value.") return f"@{self.subdomain}.{settings.MOZMAIL_DOMAIN}" @property @@ -844,7 +849,8 @@ def full_address(self) -> str: @property def metrics_id(self) -> str: - assert self.id + if not self.id: + raise ValueError("self.id must be truthy value.") # Prefix with 'R' for RelayAddress, since there may be a DomainAddress with the # same row ID return f"R{self.id}" @@ -1002,7 +1008,8 @@ def make_domain_address( # DomainAlias will be a feature address = address_default() # Only check for bad words if randomly generated - assert isinstance(address, str) + if not isinstance(address, str): + raise TypeError("address must be type str") first_emailed_at = datetime.now(UTC) if made_via_email else None domain_address = DomainAddress.objects.create( @@ -1052,7 +1059,8 @@ def full_address(self) -> str: @property def metrics_id(self) -> str: - assert self.id + if not self.id: + raise ValueError("self.id must be truthy value.") # Prefix with 'D' for DomainAddress, since there may be a RelayAddress with the # same row ID return f"D{self.id}" @@ -1083,7 +1091,8 @@ def owner_has_premium(self): def increment_num_replied(self): address = self.relay_address or self.domain_address - assert address + if not address: + raise ValueError("address must be truthy value") address.num_replied += 1 address.last_used_at = datetime.now(UTC) address.save(update_fields=["num_replied", "last_used_at"]) diff --git a/emails/utils.py b/emails/utils.py index 8c2d7a763e..54a00cc5cb 100644 --- a/emails/utils.py +++ b/emails/utils.py @@ -198,7 +198,8 @@ def _get_hero_img_src(lang_code): if major_lang in avail_l10n_image_codes: img_locale = major_lang - assert settings.SITE_ORIGIN + if not settings.SITE_ORIGIN: + raise ValueError("settings.SITE_ORIGIN must have a value") return ( settings.SITE_ORIGIN + f"/static/images/email-images/first-time-user/hero-image-{img_locale}.png" @@ -229,8 +230,11 @@ def ses_send_raw_email( destination_address: str, message: EmailMessage, ) -> SendRawEmailResponseTypeDef: - assert (client := ses_client()) is not None - assert settings.AWS_SES_CONFIGSET + client = ses_client() + if client is None: + raise ValueError("client must have a value") + if not settings.AWS_SES_CONFIGSET: + raise ValueError("settings.AWS_SES_CONFIGSET must have a value") data = message.as_string() try: @@ -393,7 +397,9 @@ def _get_bucket_and_key_from_s3_json(message_json): @time_if_enabled("s3_get_message_content") def get_message_content_from_s3(bucket, object_key): if bucket and object_key: - assert (client := s3_client()) is not None + client = s3_client() + if client is None: + raise ValueError("client must not be None") streamed_s3_object = client.get_object(Bucket=bucket, Key=object_key).get( "Body" ) @@ -405,7 +411,9 @@ def remove_message_from_s3(bucket, object_key): if bucket is None or object_key is None: return False try: - assert (client := s3_client()) is not None + client = s3_client() + if client is None: + raise ValueError("client must not be None") response = client.delete_object(Bucket=bucket, Key=object_key) return response.get("DeleteMarker") except ClientError as e: diff --git a/emails/views.py b/emails/views.py index 0f268f6f73..c8672ac50b 100644 --- a/emails/views.py +++ b/emails/views.py @@ -202,13 +202,15 @@ def wrapped_email_test(request: HttpRequest) -> HttpResponse: if "language" in request.GET: language = request.GET["language"] else: - assert user_profile is not None + if user_profile is None: + raise ValueError("user_profile must not be None") language = user_profile.language if "has_premium" in request.GET: has_premium = strtobool(request.GET["has_premium"]) else: - assert user_profile is not None + if user_profile is None: + raise ValueError("user_profile must not be None") has_premium = user_profile.has_premium if "num_level_one_email_trackers_removed" in request.GET: @@ -335,7 +337,8 @@ def _store_reply_record( if isinstance(address, DomainAddress): reply_create_args["domain_address"] = address else: - assert isinstance(address, RelayAddress) + if not isinstance(address, RelayAddress): + raise TypeError("address must be type RelayAddress") reply_create_args["relay_address"] = address Reply.objects.create(**reply_create_args) return mail @@ -492,7 +495,10 @@ def _sns_message(message_json: AWS_SNSMessageJSON) -> HttpResponse: return _handle_bounce(message_json) if notification_type == "Complaint" or event_type == "Complaint": return _handle_complaint(message_json) - assert notification_type == "Received" and event_type is None + if notification_type != "Received": + raise ValueError('notification_type must be "Received"') + if event_type is not None: + raise ValueError("event_type must be None") return _handle_received(message_json) @@ -928,7 +934,8 @@ def _convert_to_forwarded_email( # python/typeshed issue 2418 # The Python 3.2 default was Message, 3.6 uses policy.message_factory, and # policy.default.message_factory is EmailMessage - assert isinstance(email, EmailMessage) + if not isinstance(email, EmailMessage): + raise TypeError("email must be type EmailMessage") # Replace headers in the original email header_issues = _replace_headers(email, headers) @@ -939,7 +946,8 @@ def _convert_to_forwarded_email( has_text = False if text_body: has_text = True - assert isinstance(text_body, EmailMessage) + if not isinstance(text_body, EmailMessage): + raise TypeError("text_body must be type EmailMessage") text_content = text_body.get_content() new_text_content = _convert_text_content(text_content, to_address) text_body.set_content(new_text_content) @@ -950,7 +958,8 @@ def _convert_to_forwarded_email( has_html = False if html_body: has_html = True - assert isinstance(html_body, EmailMessage) + if not isinstance(html_body, EmailMessage): + raise TypeError("html_body must be type EmailMessage") html_content = html_body.get_content() new_content, level_one_trackers_removed = _convert_html_content( html_content, @@ -974,7 +983,8 @@ def _convert_to_forwarded_email( sample_trackers, remove_level_one_trackers, ) - assert isinstance(text_body, EmailMessage) + if not isinstance(text_body, EmailMessage): + raise TypeError("text_body must be type EmailMessage") try: text_body.add_alternative(new_content, subtype="html") except TypeError as e: @@ -1303,7 +1313,8 @@ def _handle_reply( return HttpResponse("Cannot fetch the message content from S3", status=503) email = message_from_bytes(email_bytes, policy=relay_policy) - assert isinstance(email, EmailMessage) + if not isinstance(email, EmailMessage): + raise TypeError("email must be type EmailMessage") # Convert to a reply email # TODO: Issue #1747 - Remove wrapper / prefix in replies diff --git a/phones/apps.py b/phones/apps.py index e35115bfd5..1216310d87 100644 --- a/phones/apps.py +++ b/phones/apps.py @@ -27,7 +27,8 @@ def twiml_app(self) -> InstanceResource: instance = self.twilio_client.applications( settings.TWILIO_SMS_APPLICATION_SID ).fetch() - assert isinstance(instance, InstanceResource) + if not isinstance(instance, InstanceResource): + raise TypeError("instance must be type InstanceResource") return instance @cached_property @@ -47,10 +48,15 @@ def twilio_validator(self) -> RequestValidator: def phones_config() -> PhonesConfig: phones_config = apps.get_app_config("phones") - assert isinstance(phones_config, PhonesConfig) + if not isinstance(phones_config, PhonesConfig): + raise TypeError("phones_config must be type PhonesConfig") return phones_config def twilio_client() -> Client: - assert not settings.PHONES_NO_CLIENT_CALLS_IN_TEST + if settings.PHONES_NO_CLIENT_CALLS_IN_TEST: + raise ValueError( + "settings.PHONES_NO_CLIENT_CALLS_IN_TEST must be False when " + "calling twilio_client()" + ) return phones_config().twilio_client diff --git a/phones/models.py b/phones/models.py index 530aca3827..8960c45e7d 100644 --- a/phones/models.py +++ b/phones/models.py @@ -328,7 +328,11 @@ def append(self, item: str) -> None: def register_with_messaging_service(client: Client, number_sid: str) -> None: """Register a Twilio US phone number with a Messaging Service.""" - assert settings.TWILIO_MESSAGING_SERVICE_SID + if not settings.TWILIO_MESSAGING_SERVICE_SID: + raise ValueError( + "settings.TWILIO_MESSAGING_SERVICE_SID must contain a value when calling " + "register_with_messaging_service" + ) closed_sids = CachedList("twilio_messaging_service_closed") @@ -386,7 +390,11 @@ def relaynumber_post_save(sender, instance, created, **kwargs): def send_welcome_message(user, relay_number): real_phone = RealPhone.objects.get(user=user) - assert settings.SITE_ORIGIN + if not settings.SITE_ORIGIN: + raise ValueError( + "settings.SITE_ORIGIN must contain a value when calling " + "send_welcome_message" + ) media_url = settings.SITE_ORIGIN + reverse( "vCard", kwargs={"lookup_key": relay_number.vcard_lookup_key} ) diff --git a/privaterelay/allauth.py b/privaterelay/allauth.py index df69ed6167..0d72246bcc 100644 --- a/privaterelay/allauth.py +++ b/privaterelay/allauth.py @@ -17,7 +17,10 @@ def get_login_redirect_url(self, request): """ Redirect to dashboard, preserving utm params from FXA. """ - assert request.user.is_authenticated + if not request.user.is_authenticated: + raise ValueError( + "request.user must be authenticated when calling get_login_redirect_url" + ) url = "/accounts/profile/?" utm_params = {k: v for k, v in request.GET.items() if k.startswith("utm")} url += urlencode(utm_params) diff --git a/privaterelay/apps.py b/privaterelay/apps.py index 7166d6416f..6ed1f5dfd4 100644 --- a/privaterelay/apps.py +++ b/privaterelay/apps.py @@ -81,7 +81,9 @@ def ready(self) -> None: import privaterelay.signals - assert privaterelay.signals # Suppress "imported but unused" warnings + assert ( # noqa S101 + privaterelay.signals + ) # Suppress "imported but unused" warnings try: del self.fxa_verifying_keys # Clear cache diff --git a/privaterelay/cleaners.py b/privaterelay/cleaners.py index a2fc1341ab..80bd936d54 100644 --- a/privaterelay/cleaners.py +++ b/privaterelay/cleaners.py @@ -29,15 +29,22 @@ def __init__(self): def counts(self) -> Counts: """Get relevant counts for data issues and prepare to clean if possible.""" if self._counts is None: - assert self._cleanup_data is None + if self._cleanup_data is not None: + raise ValueError( + "self.cleanup_data should be None when self._counts is None" + ) self._counts, self._cleanup_data = self._get_counts_and_data() return self._counts @property def cleanup_data(self) -> CleanupData: """Get data needed to clean data issues.""" - assert self.counts # Populate self._cleanup_data if not populated - assert self._cleanup_data + if not self.counts: + raise ValueError("self.counts must have a value when calling cleanup_data.") + if not self._cleanup_data: + raise ValueError( + "self._cleanup_data must have a value when calling cleanup_data." + ) return self._cleanup_data def issues(self) -> int: @@ -72,7 +79,8 @@ def markdown_report(self) -> str: @staticmethod def _as_percent(part: int, whole: int) -> str: """Return value followed by percent of whole, like '5 ( 30.0%)'""" - assert whole > 0 + if not whole > 0: + raise ValueError("whole must be greater than 0 when calling _as_percent") len_whole = len(str(whole)) return f"{part:{len_whole}d} ({part / whole:6.1%})" diff --git a/privaterelay/glean_interface.py b/privaterelay/glean_interface.py index 365a291f8e..b8f32ad528 100644 --- a/privaterelay/glean_interface.py +++ b/privaterelay/glean_interface.py @@ -137,7 +137,10 @@ def __init__( app_display_version: str, channel: RELAY_CHANNEL_NAME, ): - assert settings.GLEAN_EVENT_MOZLOG_TYPE == GLEAN_EVENT_MOZLOG_TYPE + if not settings.GLEAN_EVENT_MOZLOG_TYPE == GLEAN_EVENT_MOZLOG_TYPE: + raise ValueError( + "settings.GLEAN_EVENT_MOZLOG_TYPE must equal GLEAN_EVENT_MOZLOG_TYPE" + ) self._logger = getLogger(GLEAN_EVENT_MOZLOG_TYPE) super().__init__( application_id=application_id, diff --git a/privaterelay/management/commands/cleanup_data.py b/privaterelay/management/commands/cleanup_data.py index bdb89505aa..600d5fea34 100644 --- a/privaterelay/management/commands/cleanup_data.py +++ b/privaterelay/management/commands/cleanup_data.py @@ -57,7 +57,8 @@ def create_parser(self, prog_name, subcommand, **kwargs): epilog = "\n".join(epilog_lines) parser = super().create_parser(prog_name, subcommand, epilog=epilog, **kwargs) - assert parser.formatter_class == DjangoHelpFormatter + if not parser.formatter_class == DjangoHelpFormatter: + raise TypeError("parser.formatter_class must be type DjangoHelpFormatter") parser.formatter_class = RawDescriptionDjangoHelpFormatter return parser diff --git a/privaterelay/plans.py b/privaterelay/plans.py index 1afa521bdf..7ea2bdc2fd 100644 --- a/privaterelay/plans.py +++ b/privaterelay/plans.py @@ -557,15 +557,18 @@ def _cached_country_language_mapping( mapping: PlanCountryLangMapping = {} for relay_country in relay_maps.get("by_country", []): - assert relay_country not in mapping + if relay_country in mapping: + raise ValueError("relay_country should not be in mapping.") mapping[relay_country] = {"*": _get_stripe_prices(relay_country, stripe_data)} for relay_country, override in relay_maps.get("by_country_override", {}).items(): - assert relay_country not in mapping + if relay_country in mapping: + raise ValueError("relay_country should not be in mapping.") mapping[relay_country] = {"*": _get_stripe_prices(override, stripe_data)} for relay_country, languages in relay_maps.get("by_country_and_lang", {}).items(): - assert relay_country not in mapping + if relay_country in mapping: + raise ValueError("relay_country should not be in mapping.") mapping[relay_country] = { lang: _get_stripe_prices(stripe_country, stripe_data) for lang, stripe_country in languages.items() @@ -586,16 +589,19 @@ def _get_stripe_prices( # mypy thinks stripe_details _could_ be _StripeYearlyPriceDetails, # so extra asserts are needed to make mypy happy. monthly_id = str(stripe_details.get("monthly_id")) - assert monthly_id.startswith("price_") + if not monthly_id.startswith("price_"): + raise ValueError("monthly_id must start with 'price_'") price = prices.get("monthly", 0.0) - assert price and isinstance(price, float) + if not isinstance(price, float): + raise TypeError("price must be of type float.") period_to_details["monthly"] = { "id": monthly_id, "currency": currency, "price": price, } yearly_id = stripe_details["yearly_id"] - assert yearly_id.startswith("price_") + if not yearly_id.startswith("price_"): + raise ValueError("yearly_id must start with 'price_'") period_to_details["yearly"] = { "id": yearly_id, "currency": currency, diff --git a/privaterelay/settings.py b/privaterelay/settings.py index 0bb0b12940..32cd15c48b 100644 --- a/privaterelay/settings.py +++ b/privaterelay/settings.py @@ -40,7 +40,7 @@ # https://github.com/jazzband/django-silk import silk - assert silk # Suppress "imported but unused" warning + assert silk # Suppress "imported but unused" warning # noqa S101 HAS_SILK = True except ImportError: @@ -119,7 +119,10 @@ ] _ACCOUNT_CONNECT_SRC = [FXA_BASE_ORIGIN] else: - assert FXA_BASE_ORIGIN == "https://accounts.stage.mozaws.net" + if not FXA_BASE_ORIGIN == "https://accounts.stage.mozaws.net": + raise ValueError( + "FXA_BASE_ORIGIN must be either https://accounts.firefox.com or https://accounts.stage.mozaws.net" + ) _AVATAR_IMG_SRC = [ "mozillausercontent.com", "https://profile.stage.mozaws.net", diff --git a/privaterelay/storage.py b/privaterelay/storage.py index f81c271250..b8cf8861d9 100644 --- a/privaterelay/storage.py +++ b/privaterelay/storage.py @@ -37,7 +37,8 @@ def hashed_name( return name else: new_name = super().hashed_name(name, content, filename) - assert isinstance(new_name, str) + if not isinstance(new_name, str): + raise TypeError("new_name must be type str") return new_name def url_converter( diff --git a/privaterelay/views.py b/privaterelay/views.py index 5b41a2dc05..7778f9d733 100644 --- a/privaterelay/views.py +++ b/privaterelay/views.py @@ -168,7 +168,8 @@ def _parse_jwt_from_request(request: HttpRequest) -> str: def fxa_verifying_keys(reload: bool = False) -> list[dict[str, Any]]: """Get list of FxA verifying (public) keys.""" private_relay_config = apps.get_app_config("privaterelay") - assert isinstance(private_relay_config, PrivateRelayConfig) + if not isinstance(private_relay_config, PrivateRelayConfig): + raise TypeError("private_relay_config must be PrivateRelayConfig") if reload: private_relay_config.ready() return private_relay_config.fxa_verifying_keys @@ -177,7 +178,8 @@ def fxa_verifying_keys(reload: bool = False) -> list[dict[str, Any]]: def fxa_social_app(reload: bool = False) -> SocialApp: """Get FxA SocialApp from app config or DB.""" private_relay_config = apps.get_app_config("privaterelay") - assert isinstance(private_relay_config, PrivateRelayConfig) + if not isinstance(private_relay_config, PrivateRelayConfig): + raise TypeError("private_relay_config must be PrivateRelayConfig") if reload: private_relay_config.ready() return private_relay_config.fxa_social_app @@ -222,11 +224,13 @@ def _verify_jwt_with_fxa_key( social_app = fxa_social_app() if not social_app: raise Exception("FXA SocialApp is not available.") - assert isinstance(social_app, SocialApp) + if not isinstance(social_app, SocialApp): + raise TypeError("social_app must be SocialApp") for verifying_key in verifying_keys: if verifying_key["alg"] == "RS256": public_key = jwt.algorithms.RSAAlgorithm.from_jwk(json.dumps(verifying_key)) - assert isinstance(public_key, RSAPublicKey) + if not isinstance(public_key, RSAPublicKey): + raise TypeError("public_key must be RSAPublicKey") try: security_event = jwt.decode( req_jwt, From 0117e2e332f29c2dd8f9e18f1246428778edf28b Mon Sep 17 00:00:00 2001 From: groovecoder Date: Wed, 8 May 2024 11:25:55 -0500 Subject: [PATCH 3/4] for MPP-3802: stop ignoring bandit security checks --- pyproject.toml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 1110f2245c..7b988b5aad 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -97,10 +97,6 @@ testpaths = [ ] [tool.ruff.lint] -ignore = [ - # TODO MPP-3802: Enable more bandit security checks - "S101", # https://docs.astral.sh/ruff/rules/assert/ -] select = [ "S", # flake8-bandit "E", # pycodestyle errors From b14d6a6589eda7ed35cf4eaf72fd60bbe6d8a2d6 Mon Sep 17 00:00:00 2001 From: groovecoder Date: Thu, 9 May 2024 11:01:54 -0500 Subject: [PATCH 4/4] for MPP-3802: add FXA_REQUESTS_TIMEOUT_SECONDS and use noqa: F401 --- api/authentication.py | 6 +++++- api/views/privaterelay.py | 4 +++- privaterelay/apps.py | 6 +----- privaterelay/settings.py | 5 ++--- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/api/authentication.py b/api/authentication.py index 52947538e2..203ef7660c 100644 --- a/api/authentication.py +++ b/api/authentication.py @@ -29,7 +29,11 @@ def get_cache_key(token): def introspect_token(token: str) -> dict[str, Any]: try: - fxa_resp = requests.post(INTROSPECT_TOKEN_URL, json={"token": token}, timeout=1) + fxa_resp = requests.post( + INTROSPECT_TOKEN_URL, + json={"token": token}, + timeout=settings.FXA_REQUESTS_TIMEOUT_SECONDS, + ) except Exception as exc: logger.error( "Could not introspect token with FXA.", diff --git a/api/views/privaterelay.py b/api/views/privaterelay.py index 0e8f5e73a7..df8bd7ff96 100644 --- a/api/views/privaterelay.py +++ b/api/views/privaterelay.py @@ -330,7 +330,9 @@ def terms_accepted_user(request): except SocialAccount.DoesNotExist: # User does not exist, create a new Relay user fxa_profile_resp = requests.get( - FXA_PROFILE_URL, headers={"Authorization": f"Bearer {token}"}, timeout=1 + FXA_PROFILE_URL, + headers={"Authorization": f"Bearer {token}"}, + timeout=settings.FXA_REQUESTS_TIMEOUT_SECONDS, ) if not (fxa_profile_resp.ok and fxa_profile_resp.content): logger.error( diff --git a/privaterelay/apps.py b/privaterelay/apps.py index 6ed1f5dfd4..642cfc57d1 100644 --- a/privaterelay/apps.py +++ b/privaterelay/apps.py @@ -79,11 +79,7 @@ def ready(self) -> None: f" with key file: {gcp_key_json_path}" ) - import privaterelay.signals - - assert ( # noqa S101 - privaterelay.signals - ) # Suppress "imported but unused" warnings + import privaterelay.signals # noqa: F401 (imported but unused warning) try: del self.fxa_verifying_keys # Clear cache diff --git a/privaterelay/settings.py b/privaterelay/settings.py index 32cd15c48b..96b8929182 100644 --- a/privaterelay/settings.py +++ b/privaterelay/settings.py @@ -38,9 +38,7 @@ try: # Silk is a live profiling and inspection tool for the Django framework # https://github.com/jazzband/django-silk - import silk - - assert silk # Suppress "imported but unused" warning # noqa S101 + import silk # noqa: F401 HAS_SILK = True except ImportError: @@ -598,6 +596,7 @@ def set_index_cache_control_headers( ACCOUNT_PRESERVE_USERNAME_CASING = False ACCOUNT_USERNAME_REQUIRED = False +FXA_REQUESTS_TIMEOUT_SECONDS = config("FXA_REQUESTS_TIMEOUT_SECONDS", 1, cast=int) FXA_SETTINGS_URL = config("FXA_SETTINGS_URL", f"{FXA_BASE_ORIGIN}/settings") FXA_SUBSCRIPTIONS_URL = config( "FXA_SUBSCRIPTIONS_URL", f"{FXA_BASE_ORIGIN}/subscriptions"