From 557d13209f9aa5be95d0186e8ef9d74a4ac9ec07 Mon Sep 17 00:00:00 2001 From: Danyal-Faheem Date: Tue, 23 Apr 2024 11:07:05 +0500 Subject: [PATCH 1/3] fix: use usage_key instead of block_id as location identifier This is because block_id is maintained across course export/import Both the courses try to access the same data on storage Manipulating data in one course will overwrite the data for the other one, breaking it --- openedxscorm/scormxblock.py | 47 +++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/openedxscorm/scormxblock.py b/openedxscorm/scormxblock.py index b7af42f..cebdc35 100644 --- a/openedxscorm/scormxblock.py +++ b/openedxscorm/scormxblock.py @@ -8,6 +8,7 @@ import mimetypes import urllib +from django.core.files import File from django.core.files.base import ContentFile from django.core.files.storage import default_storage from django.db.models import Q @@ -167,8 +168,24 @@ def scorm_storage(xblock): scope=Scope.settings, ) + # To save the value of package_path across course exports or unit/subsection duplicates + package_file_path = String(scope=Scope.settings, default="") + has_author_view = True + @property + def package_path(self): + """ + Get file path of storage. + """ + return ( + "{loc.org}/{loc.course}/{loc.block_type}/{loc.block_id}/{sha1}{ext}" + ).format( + loc=self.location, + sha1=self.package_meta["sha1"], + ext=os.path.splitext(self.package_meta["name"])[1], + ) + def render_template(self, template_path, context): template_str = self.resource_string(template_path) template = Template(template_str) @@ -194,6 +211,8 @@ def author_view(self, context=None): return self.student_view(context=context) def student_view(self, context=None): + if not self.storage.exists(self.extract_folder_base_path) and self.package_file_path: + self.extract_package(self.storage.open(self.package_file_path)) student_context = { "index_page_url": urllib.parse.unquote(self.index_page_url), "completion_status": self.lesson_status, @@ -300,6 +319,17 @@ def studio_submit(self, request, _suffix): package_file = request.params["file"].file self.update_package_meta(package_file) + # First, save scorm file in the storage for later use + # We only download this file and use it in case storage does not exist + # i.e, the unuzipped scorm data has been deleted or the path has changed + if self.storage.exists(self.package_path): + logger.info('Removing previously uploaded "%s"', self.package_path) + self.storage.delete(self.package_path) + self.storage.save(self.package_path, File(package_file)) + logger.info('Scorm "%s" file stored at "%s"', package_file, self.package_path) + # save the package path so that it is maintained across course imports + self.package_file_path = self.package_path + # Clean storage folder, if it already exists self.clean_storage() @@ -411,6 +441,23 @@ def extract_folder_path(self): def extract_folder_base_path(self): """ Path to the folder where packages will be extracted. + Compute hash of the unique block usage_id and use that as our directory name. + """ + # For backwards compatibility, we return the old path if the directory exists + if self.storage.exists(self.extract_old_folder_base_path): + return self.extract_old_folder_base_path + sha1 = hashlib.sha1() + sha1.update(str(self.scope_ids.usage_id).encode()) + hashed_usage_id = sha1.hexdigest() + return os.path.join(self.scorm_location(), hashed_usage_id) + + @property + def extract_old_folder_base_path(self): + """ + Deprecated Path to the folder where packages will be extracted. + Deprecated as the block_id was shared by courses when exporting and importing + them, thus causing storage conflicts. + Only keeping this here for backwards compatibility. """ return os.path.join(self.scorm_location(), self.location.block_id) From 89d57a644a8a64155ba931313e426cfa9bf32030 Mon Sep 17 00:00:00 2001 From: Danyal-Faheem Date: Tue, 23 Apr 2024 11:08:10 +0500 Subject: [PATCH 2/3] fix: use usage_key instead of block_id as location identifier This is because block_id is maintained across course export/import Both the courses try to access the same data on storage Manipulating data in one course will overwrite the data for the other one, breaking it --- openedxscorm/scormxblock.py | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/openedxscorm/scormxblock.py b/openedxscorm/scormxblock.py index cebdc35..5395d92 100644 --- a/openedxscorm/scormxblock.py +++ b/openedxscorm/scormxblock.py @@ -8,7 +8,6 @@ import mimetypes import urllib -from django.core.files import File from django.core.files.base import ContentFile from django.core.files.storage import default_storage from django.db.models import Q @@ -168,24 +167,8 @@ def scorm_storage(xblock): scope=Scope.settings, ) - # To save the value of package_path across course exports or unit/subsection duplicates - package_file_path = String(scope=Scope.settings, default="") - has_author_view = True - @property - def package_path(self): - """ - Get file path of storage. - """ - return ( - "{loc.org}/{loc.course}/{loc.block_type}/{loc.block_id}/{sha1}{ext}" - ).format( - loc=self.location, - sha1=self.package_meta["sha1"], - ext=os.path.splitext(self.package_meta["name"])[1], - ) - def render_template(self, template_path, context): template_str = self.resource_string(template_path) template = Template(template_str) @@ -211,8 +194,6 @@ def author_view(self, context=None): return self.student_view(context=context) def student_view(self, context=None): - if not self.storage.exists(self.extract_folder_base_path) and self.package_file_path: - self.extract_package(self.storage.open(self.package_file_path)) student_context = { "index_page_url": urllib.parse.unquote(self.index_page_url), "completion_status": self.lesson_status, @@ -319,17 +300,6 @@ def studio_submit(self, request, _suffix): package_file = request.params["file"].file self.update_package_meta(package_file) - # First, save scorm file in the storage for later use - # We only download this file and use it in case storage does not exist - # i.e, the unuzipped scorm data has been deleted or the path has changed - if self.storage.exists(self.package_path): - logger.info('Removing previously uploaded "%s"', self.package_path) - self.storage.delete(self.package_path) - self.storage.save(self.package_path, File(package_file)) - logger.info('Scorm "%s" file stored at "%s"', package_file, self.package_path) - # save the package path so that it is maintained across course imports - self.package_file_path = self.package_path - # Clean storage folder, if it already exists self.clean_storage() From b19fb921722f990963f192263ae8395b3ce696d6 Mon Sep 17 00:00:00 2001 From: Danyal-Faheem Date: Tue, 23 Apr 2024 12:25:07 +0500 Subject: [PATCH 3/3] docs: add changelog entry --- ...23_122156_danyal.faheem_fix_course_import_duplicate_path.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/20240423_122156_danyal.faheem_fix_course_import_duplicate_path.md diff --git a/changelog.d/20240423_122156_danyal.faheem_fix_course_import_duplicate_path.md b/changelog.d/20240423_122156_danyal.faheem_fix_course_import_duplicate_path.md new file mode 100644 index 0000000..3f97391 --- /dev/null +++ b/changelog.d/20240423_122156_danyal.faheem_fix_course_import_duplicate_path.md @@ -0,0 +1,3 @@ +- [Bugfix] Prevent overwriting of exported course scorm data by imported course. (by @Danyal-Faheem) + - Use usage_key instead of block_id as the location identifier for scorm data as it is unique across course imports. + - This change will not take effect for previously created scorm modules. \ No newline at end of file