feat: generalize proxy assets for use with every storage (#91)

* feat: generalize proxy assets for use with every storage
We do this so that custom storage does not need to be defined for Scorm
The default storage configured for Open edX should work out of the box with scorm now
This also fixes an issue of X-Frame-Options on the LMS when running in development mode
closes #87

Co-authored-by: Tim McCormack <[email protected]>

---------

Co-authored-by: Tim McCormack <[email protected]>

README.rst

@@ -72,7 +72,9 @@ By default, SCORM modules will be accessible at "/scorm/" urls and static assets
 Custom storage backends
 ~~~~~~~~~~~~~~~~~~~~~~~
 
-By default, static assets are stored in the default Django storage backend. To override this behaviour, you should define a custom storage function. This function must take the xblock instance as its first and only argument.
+By default, static assets are stored in the default Open edX storage backend.
+
+To override this behaviour, you should define a custom storage function. This function must take the xblock instance as its first and only argument.
 For instance, you can store assets in different directories depending on the XBlock organization with
 
 .. code-block:: python
@@ -102,13 +104,14 @@ This should be added both to the LMS and the CMS settings. Instead of a function
         "STORAGE_FUNC": "my.custom.storage.module.get_scorm_storage_function",
     }
 
-Note that the SCORM XBlock comes with S3 storage support out of the box. See the following section:
+Note that the SCORM XBlock comes with extended S3 storage support out of the box. See the following section:
 
 S3 storage
 ~~~~~~~~~~
 
-The SCORM XBlock may be configured to proxy static SCORM assets stored in either public or private S3 buckets.
-To configure S3 storage, add the following to your LMS and CMS settings
+The SCORM XBlock will serve static assets from S3 if it is configured as the default storage for Open edX.
+
+However, to configure S3 storage specific to scorm xblock, add the following to your LMS and CMS settings
 
 .. code-block:: python
 
@@ -118,7 +121,7 @@ To configure S3 storage, add the following to your LMS and CMS settings
 
 You may define the following additional settings in ``XBLOCK_SETTINGS["ScormXBlock"]``:
 
-* ``S3_BUCKET_NAME`` (default: ``AWS_STORAGE_BUCKET_NAME``): to store SCORM assets in a specific bucket.
+* ``S3_BUCKET_NAME`` (default: ``AWS_STORAGE_BUCKET_NAME``): to store SCORM assets in a specific bucket separate from the rest of your Open edX assets.
 * ``S3_QUERY_AUTH`` (default: ``True``): boolean flag (``True`` or ``False``) for query string authentication in S3 urls. If your bucket is public, set this value to ``False``. But be aware that in such case your SCORM assets will be publicly available to everyone.
 * ``S3_EXPIRES_IN`` (default: 604800): time duration (in seconds) for the presigned URLs to stay valid. The default is one week.
 

openedxscorm/scormxblock.py

@@ -264,13 +264,12 @@ def assets_proxy(self, request, suffix):
         Response object containing the content of the requested file with the appropriate content type.
         """
         file_name = os.path.basename(suffix)
-        signed_url = self.storage.url(suffix)
-        if request.query_string:
-            signed_url = "&".join([signed_url, request.query_string])
+        file_path = self.find_file_path(file_name)        
         file_type, _ = mimetypes.guess_type(file_name)
-        with urllib.request.urlopen(signed_url) as response:
+        with self.storage.open(file_path) as response:
             file_content = response.read()
 
+
         return Response(file_content, content_type=file_type)
 
     def studio_view(self, context=None):
@@ -417,16 +416,20 @@ def extract_package(self, package_file):
     def index_page_url(self):
         if not self.package_meta or not self.index_page_path:
             return ""
-        folder = self.extract_folder_path
         if self.storage.exists(
             os.path.join(self.extract_folder_base_path, self.clean_path(self.index_page_path))
         ):
             # For backward-compatibility, we must handle the case when the xblock data
             # is stored in the base folder.
-            folder = self.extract_folder_base_path
-            logger.warning("Serving SCORM content from old-style path: %s", folder)
+            logger.warning("Serving SCORM content from old-style path: %s", self.extract_folder_base_path)
+
+        return f"{self.proxy_base_url}/{self.index_page_path}"
 
-        return self.storage.url(os.path.join(folder, self.index_page_path))
+    @property
+    def proxy_base_url(self):
+        return self.runtime.handler_url(
+                self, "assets_proxy"
+            ).rstrip("?/")
 
     @property
     def extract_folder_path(self):
@@ -689,11 +692,7 @@ def find_titles_recursively(self, item, prefix, root):
                 f"{prefix}resources/{prefix}resource[@identifier='{item_identifier}']"
             )
             # Attach the storage path with the file path
-            resource_link = urllib.parse.unquote(
-                self.storage.url(
-                    os.path.join(self.extract_folder_path, resource.get("href"))
-                )
-            )
+            resource_link = f"{self.proxy_base_url}/{resource.get('href')}"
         if not children:
             return [(sanitized_title, resource_link)]
         child_titles = []
@@ -765,7 +764,7 @@ def find_file_path(self, filename):
         Search recursively in the extracted folder for a given file. Path of the first
         found file will be returned. Raise a ScormError if file cannot be found.
         """
-        path = self.get_file_path(filename, self.extract_folder_path)
+        path = self.get_file_path(filename, self.extract_folder_path) or self.get_file_path(filename, self.extract_folder_base_path)
         if path is None:
             raise ScormError(f"Invalid package: could not find '{filename}' file")
         return path

openedxscorm/storage.py

@@ -25,31 +25,6 @@ def __init__(
             querystring_expire=querystring_expire,
         )
 
-    def url(self, name, parameters=None, expire=None):
-        """
-        Override url method of S3Boto3Storage
-        """
-        if not self.querystring_auth:
-            # No need to use assets proxy when authentication is disabled
-            return super().url(name, parameters=parameters, expire=expire)
-
-        if name.startswith(self.xblock.extract_folder_path):
-            # Proxy assets serving through the `assets_proxy` view. This case should
-            # only ever happen when we attempt to serve the index page from the
-            # index_page_url method.
-            proxy_base_url = self.xblock.runtime.handler_url(
-                self.xblock, "assets_proxy"
-            ).rstrip("?/")
-            # Note that we serve the index page here.
-            return f"{proxy_base_url}/{self.xblock.index_page_path}"
-
-        # This branch is executed when the `url` method is called from `assets_proxy`
-        return super().url(
-            os.path.join(self.xblock.extract_folder_path, name),
-            parameters=parameters,
-            expire=expire,
-        )
-
 
 def s3(xblock):
     """