From 138030190a229ce998ba8c08c1a747de43b7e21c Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Tue, 3 Jul 2018 20:54:10 -0700 Subject: [PATCH] Avoid silently changing settings; raise ImproperlyConfigured instead If a setting is considered invalid, raise an exception instead of silently changing it to some other value. Avoids confusion and forces the library user to be explicit. For example, setting S3 location to a value with a leading slash previously stripped it. Instead, now instruct the user to use a valid value without the leading slash. --- storages/backends/gcloud.py | 5 +++-- storages/backends/s3boto.py | 5 +++-- storages/backends/s3boto3.py | 5 +++-- storages/utils.py | 12 ++++++++++++ tests/test_gcloud.py | 9 +++++++++ tests/test_gs.py | 9 +++++++++ tests/test_s3boto.py | 9 +++++++++ tests/test_s3boto3.py | 9 +++++++++ 8 files changed, 57 insertions(+), 6 deletions(-) diff --git a/storages/backends/gcloud.py b/storages/backends/gcloud.py index 1d98d1c8e..5bb14ade0 100644 --- a/storages/backends/gcloud.py +++ b/storages/backends/gcloud.py @@ -8,7 +8,7 @@ from django.utils.deconstruct import deconstructible from django.utils.encoding import force_bytes, smart_str -from storages.utils import clean_name, safe_join, setting +from storages.utils import check_location, clean_name, safe_join, setting try: from google.cloud.storage.client import Client @@ -101,7 +101,8 @@ def __init__(self, **settings): if hasattr(self, name): setattr(self, name, value) - self.location = (self.location or '').lstrip('/') + check_location(self) + self._bucket = None self._client = None diff --git a/storages/backends/s3boto.py b/storages/backends/s3boto.py index 3fd4e6be0..73f200584 100644 --- a/storages/backends/s3boto.py +++ b/storages/backends/s3boto.py @@ -14,7 +14,7 @@ ) from django.utils.six import BytesIO -from storages.utils import clean_name, safe_join, setting +from storages.utils import check_location, clean_name, safe_join, setting try: from boto import __version__ as boto_version @@ -235,7 +235,8 @@ def __init__(self, acl=None, bucket=None, **settings): if bucket is not None: self.bucket_name = bucket - self.location = (self.location or '').lstrip('/') + check_location(self) + # Backward-compatibility: given the anteriority of the SECURE_URL setting # we fall back to https if specified in order to avoid the construction # of unsecure urls. diff --git a/storages/backends/s3boto3.py b/storages/backends/s3boto3.py index be3ec425d..d6a7a50c0 100644 --- a/storages/backends/s3boto3.py +++ b/storages/backends/s3boto3.py @@ -16,7 +16,7 @@ from django.utils.six.moves.urllib import parse as urlparse from django.utils.timezone import is_naive, localtime -from storages.utils import safe_join, setting +from storages.utils import check_location, safe_join, setting try: import boto3.session @@ -237,7 +237,8 @@ def __init__(self, acl=None, bucket=None, **settings): if bucket is not None: self.bucket_name = bucket - self.location = (self.location or '').lstrip('/') + check_location(self) + # Backward-compatibility: given the anteriority of the SECURE_URL setting # we fall back to https if specified in order to avoid the construction # of unsecure urls. diff --git a/storages/utils.py b/storages/utils.py index 566aa5127..199a8fc7d 100644 --- a/storages/utils.py +++ b/storages/utils.py @@ -80,3 +80,15 @@ def safe_join(base, *paths): ' component') return final_path.lstrip('/') + + +def check_location(storage): + if storage.location.startswith('/'): + correct = storage.location.lstrip('/') + raise ImproperlyConfigured( + "%s.location cannot begin with a leading slash. Found '%s'. Use '%s' instead." % ( + storage.__class__.__name__, + storage.location, + correct, + ) + ) diff --git a/tests/test_gcloud.py b/tests/test_gcloud.py index 416e15aa0..a3e85fb6d 100644 --- a/tests/test_gcloud.py +++ b/tests/test_gcloud.py @@ -8,6 +8,7 @@ import datetime import mimetypes +from django.core.exceptions import ImproperlyConfigured from django.core.files.base import ContentFile from django.test import TestCase from django.utils import timezone @@ -359,3 +360,11 @@ def test_cache_control(self): bucket = self.storage.client.get_bucket(self.bucket_name) blob = bucket.get_blob(filename) self.assertEqual(blob.cache_control, cache_control) + + def test_location_leading_slash(self): + msg = ( + "GoogleCloudStorage.location cannot begin with a leading slash. " + "Found '/'. Use '' instead." + ) + with self.assertRaises(ImproperlyConfigured, msg=msg): + gcloud.GoogleCloudStorage(location='/') diff --git a/tests/test_gs.py b/tests/test_gs.py index 48ad71e78..56df5b090 100644 --- a/tests/test_gs.py +++ b/tests/test_gs.py @@ -1,3 +1,4 @@ +from django.core.exceptions import ImproperlyConfigured from django.core.files.base import ContentFile from django.test import TestCase @@ -30,3 +31,11 @@ def test_gs_gzip(self): policy=self.storage.default_acl, rewind=True, ) + + def test_location_leading_slash(self): + msg = ( + "GSBotoStorage.location cannot begin with a leading slash. " + "Found '/'. Use '' instead." + ) + with self.assertRaises(ImproperlyConfigured, msg=msg): + gs.GSBotoStorage(location='/') diff --git a/tests/test_s3boto.py b/tests/test_s3boto.py index 4d069de8d..58a129567 100644 --- a/tests/test_s3boto.py +++ b/tests/test_s3boto.py @@ -9,6 +9,7 @@ from boto.exception import S3ResponseError from boto.s3.key import Key from boto.utils import ISO8601, parse_ts +from django.core.exceptions import ImproperlyConfigured from django.core.files.base import ContentFile from django.test import TestCase from django.utils import timezone as tz @@ -343,3 +344,11 @@ def upload_part_from_file(fp, part_num, *args, **kwargs): f.close() assert content.size == sum(file_part_size) + + def test_location_leading_slash(self): + msg = ( + "S3BotoStorage.location cannot begin with a leading slash. " + "Found '/'. Use '' instead." + ) + with self.assertRaises(ImproperlyConfigured, msg=msg): + s3boto.S3BotoStorage(location='/') diff --git a/tests/test_s3boto3.py b/tests/test_s3boto3.py index d4dfeac56..1281e68eb 100644 --- a/tests/test_s3boto3.py +++ b/tests/test_s3boto3.py @@ -8,6 +8,7 @@ from botocore.exceptions import ClientError from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.core.files.base import ContentFile from django.test import TestCase from django.utils.six.moves.urllib import parse as urlparse @@ -509,3 +510,11 @@ def test_file_write_after_exceeding_5mb(self): 'PartNumber': 2 } ]}) + + def test_location_leading_slash(self): + msg = ( + "S3Boto3Storage.location cannot begin with a leading slash. " + "Found '/'. Use '' instead." + ) + with self.assertRaises(ImproperlyConfigured, msg=msg): + s3boto3.S3Boto3Storage(location='/')