[ENG-11228] 6. Fix Registration File Links Pointing to Source Project During Moderation

osf/management/commands/force_archive.py

@@ -45,6 +45,7 @@
 from api.waffle.utils import flag_is_active
 from scripts import utils as script_utils
 from website.archiver import ARCHIVER_SUCCESS
+from website.archiver import utils as archiver_utils
 from website.settings import ARCHIVE_TIMEOUT_TIMEDELTA, ARCHIVE_PROVIDER, COOKIE_NAME, EXTERNAL_REQUEST_TIMEOUT
 from website.files.utils import attach_versions
 
@@ -432,6 +433,10 @@ def archive(registration, *args, permissible_addons=DEFAULT_PERMISSIBLE_ADDONS,
                 assert reg.archiving, f'{reg._id}: Must be `archiving` for WB to copy'
                 perform_wb_copy(reg, node_settings, *args, **kwargs)
 
+    # The archive_success task rewrites these links, but it never runs for a
+    # force-archived registration, without this call the files stay on the private project.
+    archiver_utils.migrate_file_metadata(registration.root)
+
 def archive_registrations(*args, **kwargs):
     for reg in deepcopy(VERIFIED):
         archive(reg, *args, *kwargs)

osf_tests/test_archiver.py

@@ -21,12 +21,15 @@
 from website.archiver.tasks import *   # noqa: F403
 
 from osf.models import Guid, RegistrationSchema, Registration, NotificationTypeEnum
+from osf.models.schema_response import SchemaResponse
+from osf.utils.workflows import ApprovalStates
 from osf.models.archive import ArchiveTarget, ArchiveJob
 from osf.models.base import generate_object_id
 from osf.utils.migrations import map_schema_to_schemablocks
 from addons.base.models import BaseStorageAddon
 from api.base.utils import waterbutler_api_url_for
 
+from osf.management.commands import force_archive as force_archive_command
 from osf_tests import factories
 from tests.base import OsfTestCase, fake
 from tests import utils as test_utils
@@ -351,6 +354,19 @@ def generate_metadata(file_trees, selected_files, node_index):
     }
     return dict(**uploader_types, **other_questions)
 
+def generate_file_input_metadata(node, files):
+    return {
+        ('q_' + file_info['name']): {
+            'extra': [{
+                'sha256': file_info['extra']['hashes']['sha256'],
+                'viewUrl': f"/project/{node._id}/files/osfstorage{file_info['path']}",
+                'selectedFileName': file_info['name'],
+                'nodeId': node._id
+            }]
+        }
+        for file_info in files
+    }
+
 class ArchiverTestCase(OsfTestCase):
 
     def setUp(self):
@@ -773,16 +789,7 @@ def test_archive_success_different_name_same_sha(self):
         file_tree['children'] = [fake_file, fake_file2]
 
         node = factories.NodeFactory(creator=self.user)
-        data = {
-            ('q_' + fake_file['name']): {
-                'extra': [{
-                    'sha256': fake_file['extra']['hashes']['sha256'],
-                    'viewUrl': f"/project/{node._id}/files/osfstorage{fake_file['path']}",
-                    'selectedFileName': fake_file['name'],
-                    'nodeId': node._id
-                }]
-            }
-        }
+        data = generate_file_input_metadata(node, [fake_file])
         schema = generate_schema_from_data(data)
         draft_registration = factories.DraftRegistrationFactory(registration_schema=schema, branched_from=node, registration_metadata=data)
         with test_utils.mock_archive(node, schema=schema, draft_registration=draft_registration, autocomplete=True, autoapprove=True) as registration:
@@ -801,16 +808,7 @@ def test_archive_failure_different_name_same_sha(self):
         file_tree['children'] = [fake_file2]
 
         node = factories.NodeFactory(creator=self.user)
-        data = {
-            ('q_' + fake_file['name']): {
-                'extra': [{
-                    'sha256': fake_file['extra']['hashes']['sha256'],
-                    'viewUrl': f"/project/{node._id}/files/osfstorage{fake_file['path']}",
-                    'selectedFileName': fake_file['name'],
-                    'nodeId': node._id
-                }]
-            }
-        }
+        data = generate_file_input_metadata(node, [fake_file])
         schema = generate_schema_from_data(data)
         draft = factories.DraftRegistrationFactory(branched_from=node, registration_schema=schema, registration_metadata=data)
         with test_utils.mock_archive(node, schema=schema, draft_registration=draft, autocomplete=True, autoapprove=True) as registration:
@@ -830,16 +828,7 @@ def test_archive_success_same_file_in_component(self):
         node = factories.NodeFactory(creator=self.user)
         child = factories.NodeFactory(creator=self.user, parent=node)
 
-        data = {
-            ('q_' + selected['name']): {
-                'extra': [{
-                    'sha256': selected['extra']['hashes']['sha256'],
-                    'viewUrl': f"/project/{child._id}/files/osfstorage{selected['path']}",
-                    'selectedFileName': selected['name'],
-                    'nodeId': child._id
-                }]
-            }
-        }
+        data = generate_file_input_metadata(child, [selected])
         schema = generate_schema_from_data(data)
         draft_registration = factories.DraftRegistrationFactory(registration_schema=schema, branched_from=node, registration_metadata=data)
         with test_utils.mock_archive(node, schema=schema, draft_registration=draft_registration, autocomplete=True, autoapprove=True) as registration:
@@ -852,6 +841,103 @@ def test_archive_success_same_file_in_component(self):
                 for key, question in registration.registered_meta[schema._id].items():
                     assert child_reg._id in question['extra'][0]['viewUrl']
 
+    @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links')
+    def test_archive_success_is_idempotent(self):
+        file_tree = file_tree_factory(0, 0, 0)
+        fake_file = file_factory()
+        file_tree['children'] = [fake_file]
+        node = factories.NodeFactory(creator=self.user)
+        data = generate_file_input_metadata(node, [fake_file])
+        schema = generate_schema_from_data(data)
+        draft = factories.DraftRegistrationFactory(branched_from=node, registration_schema=schema, registration_metadata=data)
+        with test_utils.mock_archive(node, schema=schema, draft_registration=draft, autocomplete=True, autoapprove=True) as registration:
+            with mock.patch.object(BaseStorageAddon, '_get_file_tree', mock.Mock(return_value=file_tree)):
+                job = factories.ArchiveJobFactory(initiator=registration.creator)
+                with capture_notifications():
+                    archive_success(registration._id, job._id)
+                registration.reload()
+                first_pass = [q['extra'][0]['viewUrl'] for q in registration.registered_meta[schema._id].values()]
+                archive_success(registration._id, job._id)
+        registration.refresh_from_db()
+        for key, question in registration.registered_meta[schema._id].items():
+            assert node._id not in question['extra'][0]['viewUrl']
+            assert registration._id in question['extra'][0]['viewUrl']
+        assert [q['extra'][0]['viewUrl'] for q in registration.registered_meta[schema._id].values()] == first_pass
+
+    @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links')
+    def test_archive_success_retry_sees_files_archived_since_the_first_attempt(self):
+        copied_file = file_factory()
+        slow_file = file_factory()
+        partial_tree = file_tree_factory(0, 0, 0)
+        partial_tree['children'] = [copied_file]
+        complete_tree = file_tree_factory(0, 0, 0)
+        complete_tree['children'] = [copied_file, slow_file]
+        node = factories.NodeFactory(creator=self.user)
+        data = generate_file_input_metadata(node, (copied_file, slow_file))
+        schema = generate_schema_from_data(data)
+        draft = factories.DraftRegistrationFactory(branched_from=node, registration_schema=schema, registration_metadata=data)
+        with test_utils.mock_archive(node, schema=schema, draft_registration=draft, autocomplete=True, autoapprove=True) as registration:
+            job = factories.ArchiveJobFactory(initiator=registration.creator)
+            with mock.patch.object(BaseStorageAddon, '_get_file_tree', mock.Mock(return_value=partial_tree)):
+                with pytest.raises(ArchivedFileNotFound):
+                    archive_success(registration._id, job._id)
+            with mock.patch.object(BaseStorageAddon, '_get_file_tree', mock.Mock(return_value=complete_tree)):
+                with capture_notifications():
+                    archive_success(registration._id, job._id)
+        registration.refresh_from_db()
+        assert len(registration.registered_meta[schema._id]) == 2
+        for key, question in registration.registered_meta[schema._id].items():
+            assert node._id not in question['extra'][0]['viewUrl']
+            assert registration._id in question['extra'][0]['viewUrl']
+
+    @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links')
+    def test_force_archive_rewrites_file_references(self):
+        file_tree = file_tree_factory(0, 0, 0)
+        fake_file = file_factory()
+        file_tree['children'] = [fake_file]
+        node = factories.NodeFactory(creator=self.user)
+        data = generate_file_input_metadata(node, [fake_file])
+        schema = generate_schema_from_data(data)
+        draft = factories.DraftRegistrationFactory(branched_from=node, registration_schema=schema, registration_metadata=data)
+        with test_utils.mock_archive(node, schema=schema, draft_registration=draft, autocomplete=True, autoapprove=True) as registration:
+            # The file links still point at the project here. Force-archive has to
+            # rewrite them itself, because archive_success never runs for it.
+            for key, question in registration.registered_meta[schema._id].items():
+                assert node._id in question['extra'][0]['viewUrl']
+            with mock.patch.object(BaseStorageAddon, '_get_file_tree', mock.Mock(return_value=file_tree)):
+                assert force_archive_command.verify(registration)
+                force_archive_command.archive(registration)
+        registration.refresh_from_db()
+        for key, question in registration.registered_meta[schema._id].items():
+            assert node._id not in question['extra'][0]['viewUrl']
+            assert registration._id in question['extra'][0]['viewUrl']
+
+    @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links')
+    def test_force_archive_skips_migration_for_registration_with_revision(self):
+        file_tree = file_tree_factory(0, 0, 0)
+        fake_file = file_factory()
+        file_tree['children'] = [fake_file]
+        node = factories.NodeFactory(creator=self.user)
+        data = generate_file_input_metadata(node, [fake_file])
+        schema = generate_schema_from_data(data)
+        draft = factories.DraftRegistrationFactory(branched_from=node, registration_schema=schema, registration_metadata=data)
+        with test_utils.mock_archive(node, schema=schema, draft_registration=draft, autocomplete=True, autoapprove=True) as registration:
+            # An accepted and then updated registration has more than one schema response,
+            # which the rewrite cannot target. It must not fail the force-archive.
+            initial = registration.schema_responses.get()
+            initial.approvals_state_machine.set_state(ApprovalStates.APPROVED)
+            initial.save()
+            with capture_notifications():
+                SchemaResponse.create_from_previous_response(
+                    initiator=registration.creator, previous_response=initial
+                )
+            with mock.patch.object(BaseStorageAddon, '_get_file_tree', mock.Mock(return_value=file_tree)):
+                force_archive_command.archive(registration)
+        # The rewrite was skipped, so the references are left exactly as they were
+        registration.refresh_from_db()
+        for key, question in registration.registered_meta[schema._id].items():
+            assert node._id in question['extra'][0]['viewUrl']
+
 
 class TestArchiverUtils(ArchiverTestCase):
 

website/archiver/utils.py

@@ -1,4 +1,5 @@
 import functools
+import logging
 import unicodedata
 
 from collections import defaultdict
@@ -16,6 +17,8 @@
 )
 from website.settings import MAX_ARCHIVE_SIZE
 
+logger = logging.getLogger(__name__)
+
 FILE_HTML_LINK_TEMPLATE = settings.DOMAIN + 'project/{registration_guid}/files/osfstorage/{file_id}'
 FILE_DOWNLOAD_LINK_TEMPLATE = settings.DOMAIN + 'download/{file_id}'
 
@@ -234,6 +237,9 @@ def wrapper(node):
             file_tree = osf_storage._get_file_tree(user=OSFUser.load(list(node.admin_contributor_or_group_member_ids)[0]))
             cache[node._id] = _do_get_file_map(file_tree)
         return func(node, cache[node._id])
+    # cache is created once at import, so every task in the worker shares it.
+    # it must not be seen a previous task's file tree reset it through clear_cache.
+    wrapper.clear_cache = cache.clear
     return wrapper
 
 @_memoize_get_file_map
@@ -267,6 +273,15 @@ def get_title_for_question(schema, qid):
 
 
 def migrate_file_metadata(dst):
+    '''
+    Point the registration's file responses at its own archived copies.
+    Registration copies the project's files in the background, so those responses are
+    firstly saved pointing at the originals on registered_from, usually a private
+    project. Moderators can open the registration but not that project, so until this
+    runs they get a 403 on every file. Called by the archive_success task,
+    and by force-archive, where that task never runs. Safe to run twice:
+    a reference that was already rewritten resolves to the same value.
+    '''
     if dst.root_id != dst.id:
         return
 
@@ -276,7 +291,15 @@ def migrate_file_metadata(dst):
     ).values_list('registration_response_key', flat=True)
     if not file_input_qids:
         return
-
+    # The rewrite below calls schema_responses.get(), which raises MultipleObjectsReturned
+    # for several objects. Skip it to not fail the archive.
+    response_count = dst.schema_responses.count()
+    if response_count != 1:
+        logger.warning(f'{dst._id}: skipping file metadata migration, {response_count} schema responses')
+        return
+    # get_file_map caches per worker process, not per task, so a retry would reuse the
+    # possible incomplete file tree and the failed run produced and fail the same way.
+    get_file_map.clear_cache()
     file_response_keys_by_hash = _get_file_response_hashes(dst, file_input_qids)
     updated_file_responses = _get_updated_file_references(dst, file_response_keys_by_hash)
 
@@ -330,8 +353,12 @@ def _get_updated_file_references(registration, file_response_keys_by_hash):
                 # Handle the case where the same file exists in multiple components
                 original_response = _get_response_entry_for_hash(original_responses, qid, file_sha)
                 normalized_original_file_name = normalize_unicode_filenames(original_response['file_name'])[0]
+                original_html_url = original_response['file_urls']['html']
+                # On the first run this url holds source_project_id. On a second run it
+                # already holds archived_node_id. Match both, so the file is found both times.
                 if (
-                    source_project_id in original_response['file_urls']['html']
+                    (source_project_id in original_html_url
+                     or archived_node_id in original_html_url)
                     and response_value['file_name'] == normalized_original_file_name
                 ):
                     updated_file_responses[qid].append(response_value)