diff --git a/admin_tests/nodes/test_views.py b/admin_tests/nodes/test_views.py index 2c29b1af2c6..1c5f7e59abb 100644 --- a/admin_tests/nodes/test_views.py +++ b/admin_tests/nodes/test_views.py @@ -52,6 +52,7 @@ from tests.base import AdminTestCase from osf_tests.factories import ( + SubjectFactory, UserFactory, AuthUserFactory, ProjectFactory, @@ -619,6 +620,7 @@ def setUp(self): provider=RegistrationProviderFactory(reviews_workflow='pre-moderation'), creator=self.user ) + pre_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr3) @@ -632,6 +634,7 @@ def setUp(self): provider=RegistrationProviderFactory(reviews_workflow='post-moderation'), creator=self.user ) + post_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr3) @@ -644,6 +647,7 @@ def setUp(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr3) @@ -843,6 +847,7 @@ def test_revert_node_based_registration(self): self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr3) + pre_moderation_draft.subjects.add(SubjectFactory()) pre_moderation_draft.register(auth=self.auth, save=True) pre_moderation_registration = pre_moderation_draft.registered_node @@ -863,6 +868,7 @@ def test_can_revert_embargo_registration_to_draft(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self.no_moderation_draft.register(auth=self.auth, save=True) self.registration = self.no_moderation_draft.registered_node @@ -891,6 +897,7 @@ def test_embargo_is_reset_after_reversion(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self.no_moderation_draft.register(auth=self.auth, save=True) self.registration = self.no_moderation_draft.registered_node diff --git a/api/registrations/views.py b/api/registrations/views.py index 67705b5eace..72b216d923c 100644 --- a/api/registrations/views.py +++ b/api/registrations/views.py @@ -6,7 +6,7 @@ from addons.base.views import DOWNLOAD_ACTIONS from website.archiver import signals, ARCHIVER_NETWORK_ERROR, ARCHIVER_SUCCESS, ARCHIVER_FAILURE -from website.project import signals as project_signals +from website.archiver.tasks import archive_callback from osf.models import Registration, OSFUser, RegistrationProvider, OutcomeArtifact, CedarMetadataRecord from osf.models.spam import SpamStatus @@ -1114,7 +1114,7 @@ def update(self, request, *args, **kwargs): src_provider, ARCHIVER_SUCCESS, ) - project_signals.archive_callback.send(registration) + archive_callback(registration._id) return Response(status=status.HTTP_200_OK) except HTTPError as e: registration.archive_status = ARCHIVER_NETWORK_ERROR diff --git a/api/share/utils.py b/api/share/utils.py index d9ffd388771..14a739e5631 100644 --- a/api/share/utils.py +++ b/api/share/utils.py @@ -170,6 +170,8 @@ def _schedule_cedar_record_updates(guid_instance): acks_late=True, max_retries=4, retry_backoff=True, + soft_time_limit=settings.SHARE_UPDATE_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SHARE_UPDATE_TASK_HARD_TIME_LIMIT, ) def task__update_share(self, guid: str, is_backfill=False, osfmap_partition_name='MAIN'): """ @@ -278,6 +280,7 @@ def pls_send_trove_record(osf_item, *, is_backfill: bool, osfmap_partition: Osfm **_shtrove_auth_headers(osf_item), }, data=ensure_bytes(_serialized_record), + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) @@ -288,6 +291,7 @@ def pls_delete_trove_record(osf_item, osfmap_partition: OsfmapPartition): 'record_identifier': _shtrove_record_identifier(osf_item, osfmap_partition), }, headers=_shtrove_auth_headers(osf_item), + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) diff --git a/api_tests/draft_nodes/views/test_draft_node_detail.py b/api_tests/draft_nodes/views/test_draft_node_detail.py index 777d77a948c..8b3d21accb6 100644 --- a/api_tests/draft_nodes/views/test_draft_node_detail.py +++ b/api_tests/draft_nodes/views/test_draft_node_detail.py @@ -5,7 +5,8 @@ from osf_tests.factories import ( DraftRegistrationFactory, AuthUserFactory, - ProjectFactory + ProjectFactory, + SubjectFactory ) @@ -23,6 +24,7 @@ def user_two(self): def test_detail_response(self, app, user, user_two): draft_reg = DraftRegistrationFactory(initiator=user) draft_reg.add_contributor(user_two) + draft_reg.subjects.add(SubjectFactory()) draft_reg.save() draft_node = draft_reg.branched_from diff --git a/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py b/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py index 4e982ad0275..1279df4da5c 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py +++ b/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py @@ -22,6 +22,19 @@ def resource(self, user_admin_contrib, user_write_contrib, user_read_contrib): def url(self, resource): return f'/{API_BASE}draft_registrations/{resource._id}/relationships/subjects/' + def test_update_subjects_empty_payload(self, app, user_admin_contrib, resource, url, subject): + # override test as draft registration must have at least one subject to be registered + resource.subjects.add(subject) + + payload = { + 'data': [] + } + + res = app.patch_json_api(url, payload, auth=user_admin_contrib.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Registration must have at least one subject to be registered' + assert resource.subjects.count() == 1 + # Overwrites SubjectsRelationshipMixin def test_update_subjects_relationship_permissions(self, app, user_write_contrib, user_read_contrib, user_non_contrib, resource, url, payload): diff --git a/api_tests/registrations/views/test_registration_list.py b/api_tests/registrations/views/test_registration_list.py index 26418574c6c..92ac11eea9e 100644 --- a/api_tests/registrations/views/test_registration_list.py +++ b/api_tests/registrations/views/test_registration_list.py @@ -637,7 +637,7 @@ def project_public_excluded_sibling(self, project_public): @pytest.fixture() def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( + draft_registration = DraftRegistrationFactory( initiator=user, registration_schema=schema, branched_from=project_public, @@ -646,6 +646,8 @@ def draft_registration(self, user, project_public, schema): 'item33': {'value': 'success'} } ) + draft_registration.subjects.add(SubjectFactory()) + return draft_registration @pytest.fixture() def url_registrations(self, project_public): @@ -1396,6 +1398,7 @@ def test_assert_file_validity_on_registration_metadata( registration_schema=prereg_schema, branched_from=project_public ) + prereg_draft_registration.subjects.add(SubjectFactory()) prereg_registration_responses['q11.uploader'] = [] @@ -1598,6 +1601,7 @@ def test_need_admin_perms_on_draft( # User is an admin contributor on draft registration but not on node draft_registration = DraftRegistrationFactory(creator=user_two, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) draft_registration.add_contributor(user, permissions.ADMIN) draft_registration.branched_from.add_contributor(user, permissions.WRITE) payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id @@ -1611,6 +1615,7 @@ def test_need_admin_perms_on_draft( # User is admin on draft and node draft_registration = DraftRegistrationFactory(creator=user, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) assert draft_registration.branched_from.is_admin_contributor(user) is True assert draft_registration.has_permission(user, permissions.ADMIN) is True payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id @@ -1622,6 +1627,7 @@ def test_need_admin_perms_on_draft( # User is an admin contributor on node but not on draft registration draft_registration = DraftRegistrationFactory(creator=user_two, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) draft_registration.add_contributor(user, permissions.WRITE) draft_registration.branched_from.add_contributor(user, permissions.ADMIN) payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id diff --git a/api_tests/registrations/views/test_registration_relationship_subjects.py b/api_tests/registrations/views/test_registration_relationship_subjects.py index 442ed39cc1a..d356d436352 100644 --- a/api_tests/registrations/views/test_registration_relationship_subjects.py +++ b/api_tests/registrations/views/test_registration_relationship_subjects.py @@ -21,3 +21,16 @@ def resource(self, user_admin_contrib, user_write_contrib, user_read_contrib): @pytest.fixture() def url(self, resource): return f'/{API_BASE}registrations/{resource._id}/relationships/subjects/' + + def test_update_subjects_empty_payload(self, app, user_admin_contrib, resource, url, subject): + # override test as registration must have at least one subject to be registered + resource.subjects.add(subject) + + payload = { + 'data': [] + } + + res = app.patch_json_api(url, payload, auth=user_admin_contrib.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Registration must have at least one subject to be registered' + assert resource.subjects.count() == 1 diff --git a/api_tests/share/test_request_timeout.py b/api_tests/share/test_request_timeout.py new file mode 100644 index 00000000000..ff7f2670482 --- /dev/null +++ b/api_tests/share/test_request_timeout.py @@ -0,0 +1,36 @@ +from unittest import mock + +import pytest + +from api.share import utils as share_utils +from osf.metadata.osf_gathering import OsfmapPartition +from osf_tests.factories import ProjectFactory +from website import settings + + +@pytest.mark.django_db +class TestShareRequestTimeout: + + @pytest.fixture() + def public_node(self): + return ProjectFactory(is_public=True) + + def test_delete_trove_record_passes_timeout(self, public_node): + with mock.patch.object(share_utils.requests, 'delete') as mock_delete: + share_utils.pls_delete_trove_record(public_node, osfmap_partition=OsfmapPartition.MAIN) + assert mock_delete.call_args.kwargs['timeout'] == settings.EXTERNAL_REQUEST_TIMEOUT + + def test_send_trove_record_passes_timeout(self, public_node): + fake_serializer = mock.Mock(mediatype='text/turtle') + fake_serializer.serialize.return_value = b'' + with ( + mock.patch.object(share_utils, 'pls_get_magic_metadata_basket'), + mock.patch.object(share_utils, 'get_metadata_serializer', return_value=fake_serializer), + mock.patch.object(share_utils.requests, 'post') as mock_post, + ): + share_utils.pls_send_trove_record( + public_node, + is_backfill=False, + osfmap_partition=OsfmapPartition.MAIN, + ) + assert mock_post.call_args.kwargs['timeout'] == settings.EXTERNAL_REQUEST_TIMEOUT diff --git a/framework/sessions/__init__.py b/framework/sessions/__init__.py index 72939c1ec5d..ee52e2a4543 100644 --- a/framework/sessions/__init__.py +++ b/framework/sessions/__init__.py @@ -4,13 +4,11 @@ from urllib.parse import urlparse, parse_qs, urlunparse, urlencode from django.apps import apps -from django.utils import timezone from django.conf import settings as django_conf_settings import itsdangerous from flask import request, g from furl import furl -from framework.celery_tasks.handlers import enqueue_task from framework.flask import redirect from osf.utils.fields import ensure_str from osf.exceptions import InvalidCookieOrSessionError @@ -213,7 +211,7 @@ def before_request(): cookie = request.cookies.get(settings.COOKIE_NAME) if cookie: try: - user_session = flask_get_session_from_cookie(cookie) + flask_get_session_from_cookie(cookie) except InvalidCookieOrSessionError: # If invalid session/cookie happens, only remove the invalid cookie and redirect to the same request. # This ensures users landing on the page/link they previously clicked. @@ -226,17 +224,6 @@ def before_request(): response = redirect(redirect_url) response.delete_cookie(settings.COOKIE_NAME, domain=settings.OSF_COOKIE_DOMAIN) return response - # Case 1: anonymous session that is used for first time external (e.g. ORCiD) login only - if user_session.get('auth_user_external_first_login', False) is True: - return - # Case 2: session without authenticated user - user_id = user_session.get('auth_user_id', None) - if not user_id: - return - # Case 3: authenticated session with user - # Update date last login when making non-api requests - from framework.auth.tasks import update_user_from_activity - enqueue_task(update_user_from_activity.s(user_id, timezone.now().timestamp(), cas_login=False)) def after_request(response): diff --git a/osf/external/askismet/client.py b/osf/external/askismet/client.py index db57b1d3cfa..eaf726dd740 100644 --- a/osf/external/askismet/client.py +++ b/osf/external/askismet/client.py @@ -38,7 +38,8 @@ def _is_apikey_valid(self): 'key': self.apikey, 'blog': self.website }, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) self._apikey_is_valid = res.text == 'valid' return self._is_apikey_valid() @@ -108,7 +109,8 @@ def submit_spam(self, user_ip, user_agent, **kwargs): res = requests.post( f'{self.API_PROTOCOL}{self.apikey}.{self.API_HOST}/1.1/submit-spam', data=data, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if res.status_code != requests.codes.ok: raise AkismetClientError(reason=res.text) @@ -129,7 +131,8 @@ def submit_ham(self, user_ip, user_agent, **kwargs): res = requests.post( f'{self.API_PROTOCOL}{self.apikey}.{self.API_HOST}/1.1/submit-ham', data=data, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if res.status_code != requests.codes.ok: raise AkismetClientError(reason=res.text) diff --git a/osf/external/askismet/tasks.py b/osf/external/askismet/tasks.py index f77516204b1..083f4bb86d3 100644 --- a/osf/external/askismet/tasks.py +++ b/osf/external/askismet/tasks.py @@ -1,9 +1,13 @@ from framework.celery_tasks import app as celery_app from osf.external.askismet.client import AkismetClient +from website import settings -@celery_app.task() +@celery_app.task( + soft_time_limit=settings.SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SPAM_SUBMIT_TASK_HARD_TIME_LIMIT, +) def submit_spam(guid): from osf.models import Guid resource = Guid.load(guid).referent @@ -20,7 +24,10 @@ def submit_spam(guid): ) -@celery_app.task() +@celery_app.task( + soft_time_limit=settings.SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SPAM_SUBMIT_TASK_HARD_TIME_LIMIT, +) def submit_ham(guid): from osf.models import Guid resource = Guid.load(guid).referent diff --git a/osf/external/cedar/client.py b/osf/external/cedar/client.py index e748d7ccc9c..130be44b171 100644 --- a/osf/external/cedar/client.py +++ b/osf/external/cedar/client.py @@ -19,7 +19,7 @@ class CedarClient: def retrieve_all_template_ids(self): url = f'{self.host}folders/{self.home_folder_id}/contents/?resource_types=template' try: - r = requests.get(url, headers=self.headers) + r = requests.get(url, headers=self.headers, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) r.raise_for_status() except RequestException: raise CedarClientRequestError( @@ -36,7 +36,7 @@ def retrieve_all_template_ids(self): def retrieve_template_by_id(self, template_id): url = f'{self.host}templates/{quote_plus(template_id)}' try: - r = requests.get(url, headers=self.headers) + r = requests.get(url, headers=self.headers, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) r.raise_for_status() except RequestException: raise CedarClientRequestError(reason=f'Fail to complete Cedar API request: template_id={template_id}') diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index a4f7d37694b..48096ef8429 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -283,7 +283,7 @@ def _make_gv_request( ) assert not (request_method == 'GET' and json_data is not None) try: - response = requests.request(url=endpoint_url, headers=auth_headers, params=params, method=request_method, json=json_data) + response = requests.request(url=endpoint_url, headers=auth_headers, params=params, method=request_method, json=json_data, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) except RequestException as e: logger.error(f"Cannot reach GravyValet: {e}") return None diff --git a/osf/external/oopspam/client.py b/osf/external/oopspam/client.py index 0abdfdd021f..151a102ec37 100644 --- a/osf/external/oopspam/client.py +++ b/osf/external/oopspam/client.py @@ -34,7 +34,8 @@ def check_content(self, user_ip, content, **kwargs): 'content-type': 'application/json', 'x-rapidapi-key': self.apikey, 'x-rapidapi-host': 'oopspam.p.rapidapi.com' - } + }, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if response.status_code != requests.codes.ok: diff --git a/osf/external/spam/tasks.py b/osf/external/spam/tasks.py index 4cb4c7784b5..d67b21912dc 100644 --- a/osf/external/spam/tasks.py +++ b/osf/external/spam/tasks.py @@ -24,6 +24,7 @@ def reclassify_domain_references(notable_domain_id, current_note, previous_note) from osf.models.notable_domain import DomainReference, NotableDomain domain = NotableDomain.load(notable_domain_id) references = DomainReference.objects.filter(domain=domain) + referrers = [] with transaction.atomic(): for item in references: item.is_triaged = current_note != NotableDomain.Note.UNKNOWN @@ -37,7 +38,11 @@ def reclassify_domain_references(notable_domain_id, current_note, previous_note) if not item.referrer.spam_data.get('domains') and not item.referrer.spam_data.get('who_flagged'): item.referrer.unspam(save=False) item.save() - item.referrer.save() + referrers.append(item.referrer) + # Reindexing triggers SHARE/search HTTP calls; run it after the transaction + # commits so those requests never hold row locks. + for referrer in referrers: + referrer.save() def _check_resource_for_domains(resource, content): diff --git a/osf/management/commands/force_archive.py b/osf/management/commands/force_archive.py index 7b6d18b6c65..4a8d04feb28 100644 --- a/osf/management/commands/force_archive.py +++ b/osf/management/commands/force_archive.py @@ -45,7 +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.settings import ARCHIVE_TIMEOUT_TIMEDELTA, ARCHIVE_PROVIDER, COOKIE_NAME +from website.settings import ARCHIVE_TIMEOUT_TIMEDELTA, ARCHIVE_PROVIDER, COOKIE_NAME, EXTERNAL_REQUEST_TIMEOUT from website.files.utils import attach_versions logger = logging.getLogger(__name__) @@ -174,7 +174,7 @@ def perform_wb_copy(reg, node_settings, delete_collisions=False, skip_collisions 'provider': ARCHIVE_PROVIDER, } url = waterbutler_api_url_for(src._id, node_settings.short_name, _internal=True, base_url=src.osfstorage_region.waterbutler_url, **params) - res = requests.post(url, data=json.dumps(data), cookies={COOKIE_NAME: cookie}) + res = requests.post(url, data=json.dumps(data), cookies={COOKIE_NAME: cookie}, timeout=EXTERNAL_REQUEST_TIMEOUT) if res.status_code not in (http_status.HTTP_200_OK, http_status.HTTP_201_CREATED, http_status.HTTP_202_ACCEPTED): http_exception = HTTPError(res.status_code) sentry.log_exception(http_exception) diff --git a/osf/metrics/reporters/preprint_count.py b/osf/metrics/reporters/preprint_count.py index 6cafa063c62..99dbede560f 100644 --- a/osf/metrics/reporters/preprint_count.py +++ b/osf/metrics/reporters/preprint_count.py @@ -47,7 +47,11 @@ def report(self, date): for preprint_provider in PreprintProvider.objects.all(): elastic_query = get_elastic_query(date, preprint_provider) - resp = requests.post(f'{settings.SHARE_URL}api/v2/search/creativeworks/_search', json=elastic_query).json() + resp = requests.post( + f'{settings.SHARE_URL}api/v2/search/creativeworks/_search', + json=elastic_query, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, + ).json() yield DailyPreprintSummaryReport( cycle_coverage=cycle_coverage_date(date), diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 692075ef4c6..2f741f147bf 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -21,6 +21,7 @@ from framework.exceptions import PermissionsError from osf.exceptions import ( InvalidTriggerError, + NodeStateError, ValidationValueError, UserStateError, UserNotAffiliatedError, @@ -1184,6 +1185,10 @@ def set_subjects(self, new_subjects, auth, add_log=True, **kwargs): :return: None """ + from osf.models import Registration, DraftRegistration + if isinstance(self, (Registration, DraftRegistration)) and not new_subjects: + raise NodeStateError('Registration must have at least one subject to be registered') + if auth: self.check_subject_perms(auth, **kwargs) self.assert_subject_format(new_subjects, expect_list=True, error_msg='Expecting list of lists.') @@ -1217,6 +1222,10 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, **k :return: None """ + from osf.models import Registration, DraftRegistration + if isinstance(self, (Registration, DraftRegistration)) and not subjects_list: + raise NodeStateError('Registration must have at least one subject to be registered') + self.check_subject_perms(auth, **kwargs) self.assert_subject_format(subjects_list, expect_list=True, error_msg='Expecting a list of subjects.') if subjects_list: diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 568a1a575ed..5cf311b94bf 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1456,6 +1456,9 @@ def register(self, auth, save=False, child_ids=None, manual_guid=None): if not self.title: raise NodeStateError('Draft Registration must have title to be registered') + if not self.subjects.exists(): + raise NodeStateError('Registration must have at least one subject to be registered') + # Create the registration registration = node.register_node( schema=self.registration_schema, diff --git a/osf/models/user.py b/osf/models/user.py index 5739813cd48..4f69a30724e 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1068,7 +1068,10 @@ def save(self, *args, **kwargs): dirty_fields = self.get_dirty_fields(check_relationship=True) ret = super().save(*args, **kwargs) # must save BEFORE spam check, as user needs guid. - if has_domain and not was_creating: + if has_domain and self.is_hammy: + self.flag_spam() + + if has_domain and not was_creating and not self.is_hammy: self.confirm_spam() if set(self.SPAM_USER_PROFILE_FIELDS.keys()).intersection(dirty_fields): @@ -1482,7 +1485,8 @@ def confirm_email(self, token, merge=False): return True def confirm_spam(self, domains=None, save=True, train_spam_services=False, skip_resources_spam=False): - self.deactivate_account() + if not self.is_hammy: + self.deactivate_account() super().confirm_spam(domains=domains, save=save, train_spam_services=train_spam_services) if skip_resources_spam: diff --git a/osf/models/validators.py b/osf/models/validators.py index c5acafd3ea7..7fe745685b8 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -1,4 +1,5 @@ import re +from urllib.parse import urlsplit import waffle from jsonschema import ValidationError as JsonSchemaValidationError, SchemaError, Draft7Validator, validate, validators @@ -114,9 +115,18 @@ def validate_email(value): def has_domain_in_user_fields_for_names(user): - name_content = ' '.join([getattr(user, field) or '' for field in user.DOMAIN_VALIDATION_FIELDS]) - return True if DOMAIN_REGEX.search(name_content) else False - + name_content = ' '.join(getattr(user, field) or '' for field in user.DOMAIN_VALIDATION_FIELDS).strip() + for match in DOMAIN_REGEX.finditer(name_content): + candidate = match.group(0).strip() + if looks_like_url(candidate): + return True + return False + +def looks_like_url(candidate): + if candidate.startswith('www.'): + candidate = f'https://{candidate}' + parsed = urlsplit(candidate) + return bool(parsed.scheme and parsed.netloc) def validate_subject_hierarchy_length(parent): from osf.models import Subject diff --git a/osf_tests/test_archiver.py b/osf_tests/test_archiver.py index 94c8b4f2827..4d6c0ac5be5 100644 --- a/osf_tests/test_archiver.py +++ b/osf_tests/test_archiver.py @@ -526,7 +526,7 @@ def test_archiver_task_load_archive_job_final_failure_logs_context(self, mock_lo ) mock_log_exception.assert_called_once() - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_pass(self, mock_archive_addon): settings.MAX_ARCHIVE_SIZE = 1024 ** 3 with mock.patch.object(BaseStorageAddon, '_get_file_tree') as mock_file_tree: @@ -545,8 +545,8 @@ def test_archive_node_fail(self): with pytest.raises(ArchiverSizeExceeded): # Note: Requires task_eager_propagates = True in celery archive_node.apply(args=(results, self.archive_job._id)) - @mock.patch('website.project.signals.archive_callback.send') - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_callback.si') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_does_not_archive_empty_addons(self, mock_archive_addon, mock_send): with mock.patch('osf.models.mixins.AddonModelMixin.get_addon') as mock_get_addon: mock_addon = MockAddon() @@ -566,7 +566,7 @@ def empty_file_tree(user, version): assert mock_send.called @use_fake_addons - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_no_archive_size_limit(self, mock_archive_addon): settings.MAX_ARCHIVE_SIZE = 100 self.archive_job.initiator.add_system_tag(NO_ARCHIVE_LIMIT) @@ -580,23 +580,64 @@ def test_archive_node_no_archive_size_limit(self, mock_archive_addon): job_pk=self.archive_job._id, ) - @mock.patch('website.archiver.tasks.make_copy_request.delay') - def test_archive_addon(self, mock_make_copy_request): - archive_addon('osfstorage', self.archive_job._id) + def test_archive_addon(self): + params = archive_addon('osfstorage', self.archive_job._id) assert self.archive_job.get_target('osfstorage').status == ARCHIVER_INITIATED cookie = self.user.get_or_create_cookie() - mock_make_copy_request.assert_called_with( + assert params['addon_short_name'] == 'osfstorage' + assert params['url'] == f'{settings.WATERBUTLER_URL}/v1/resources/{self.src._id}/providers/osfstorage/?cookie={cookie.decode()}' + assert params['data'] == { + 'action': 'copy', + 'path': '/', + 'rename': 'Archive of OSF Storage', + 'resource': self.archive_job.info()[1]._id, + 'provider': 'osfstorage', + } + + @mock.patch('website.archiver.tasks.archive_callback.delay') + def test_archive_addon_does_not_trigger_callback_immediately(self, mock_archive_callback): + archive_addon('osfstorage', self.archive_job._id) + + mock_archive_callback.assert_not_called() + + @mock.patch('website.archiver.tasks.handlers.enqueue_task') + @mock.patch('website.archiver.tasks.archive_callback.si') + @mock.patch('website.archiver.tasks.make_copy_request.s') + @mock.patch('website.archiver.tasks.celery.chain') + @mock.patch('website.archiver.tasks.celery.group') + @mock.patch('website.archiver.tasks.archive_addon.si') + def test_archive_node_only_enqueues_addon_work_before_callback( + self, + mock_archive_addon, + mock_group, + mock_chain, + mock_make_copy_request_s, + mock_archive_callback, + mock_enqueue_task, + ): + settings.MAX_ARCHIVE_SIZE = 1024 ** 3 + with mock.patch.object(BaseStorageAddon, '_get_file_tree') as mock_file_tree: + mock_file_tree.return_value = FILE_TREE + results = [stat_addon(addon, self.archive_job._id) for addon in ['osfstorage']] + + archive_node(results, self.archive_job._id) + + mock_archive_addon.assert_called_once_with( + addon_short_name='osfstorage', job_pk=self.archive_job._id, - url=f'{settings.WATERBUTLER_URL}/v1/resources/{self.src._id}/providers/osfstorage/?cookie={cookie.decode()}', - data={ - 'action': 'copy', - 'path': '/', - 'rename': 'Archive of OSF Storage', - 'resource': self.archive_job.info()[1]._id, - 'provider': 'osfstorage', - } ) + self.assertEqual(mock_chain.call_count, 2) + mock_chain.assert_any_call([ + mock_archive_addon.return_value, + mock_make_copy_request_s.return_value, + ]) + mock_group.assert_called_once_with([mock_chain.return_value]) + mock_chain.assert_any_call([ + mock_group.return_value, + mock_archive_callback.return_value, + ]) + mock_enqueue_task.assert_called_once_with(mock_chain.return_value) @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links') def test_archive_success(self): @@ -993,7 +1034,7 @@ def test_archive_callback_pending(self, mock_delay): ) self.dst.archive_job.save() with mock.patch('website.archiver.utils.handle_archive_fail') as mock_fail: - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) assert not mock_fail.called assert mock_delay.called @@ -1001,7 +1042,7 @@ def test_archive_callback_pending(self, mock_delay): def test_archive_callback_done_success(self, mock_archive_success): self.dst.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) self.dst.archive_job.save() - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) @mock.patch('website.archiver.tasks.archive_success.delay') def test_archive_callback_done_embargoed(self, mock_archive_success): @@ -1015,13 +1056,13 @@ def test_archive_callback_done_embargoed(self, mock_archive_success): self.dst.embargo_registration(self.user, end_date) self.dst.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) self.dst.save() - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) def test_archive_callback_done_errors(self): self.dst.archive_job.update_target('osfstorage', ARCHIVER_FAILURE) self.dst.archive_job.save() with mock.patch('website.archiver.utils.handle_archive_fail') as mock_fail: - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) call_args = mock_fail.call_args[0] assert call_args[0] == ARCHIVER_UNCAUGHT_ERROR assert call_args[1] == self.src @@ -1037,7 +1078,7 @@ def test_archive_callback_updates_archiving_state_when_done(self): child = reg.nodes[0] child.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) child.save() - listeners.archive_callback(child) + archive_callback(child._id) assert not child.archiving def test_archive_tree_finished_d1(self): @@ -1105,13 +1146,13 @@ def test_archive_callback_on_tree_sends_only_one_email(self, mock_archive_succes node.archive_job.update_target('osfstorage', ARCHIVER_INITIATED) rchild.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) rchild.save() - listeners.archive_callback(rchild) + archive_callback(rchild._id) reg.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) reg.save() - listeners.archive_callback(reg) + archive_callback(reg._id) rchild2.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) rchild2.save() - listeners.archive_callback(rchild2) + archive_callback(rchild2._id) class TestArchiverScripts(ArchiverTestCase): @@ -1178,7 +1219,7 @@ def test_archiving_nodes_added_to_search_on_archive_success_if_public(self, mock mock.patch('osf.models.ArchiveJob.archive_tree_finished', mock.Mock(return_value=True)), mock.patch('osf.models.ArchiveJob.success', mock.PropertyMock(return_value=True)) ) as (mock_finished, mock_success): - listeners.archive_callback(reg) + archive_callback(reg._id) assert mock_update_search.call_count == 1 @pytest.mark.enable_search @@ -1192,7 +1233,7 @@ def test_archiving_nodes_not_added_to_search_on_archive_failure(self, mock_delet mock.patch('osf.models.archive.ArchiveJob.archive_tree_finished', mock.Mock(return_value=True)), mock.patch('osf.models.archive.ArchiveJob.success', mock.PropertyMock(return_value=False)) ) as (mock_finished, mock_success): - listeners.archive_callback(reg) + archive_callback(reg._id) @mock.patch('osf.models.AbstractNode.update_search') def test_archiving_nodes_not_added_to_search_on_archive_incomplete(self, mock_update_search): @@ -1200,7 +1241,7 @@ def test_archiving_nodes_not_added_to_search_on_archive_incomplete(self, mock_up reg = factories.RegistrationFactory(project=proj) reg.save() with mock.patch('osf.models.ArchiveJob.archive_tree_finished', mock.Mock(return_value=False)): - listeners.archive_callback(reg) + archive_callback(reg._id) assert not mock_update_search.called diff --git a/osf_tests/test_draft_registration.py b/osf_tests/test_draft_registration.py index 7d6938814eb..f4b2feb3c1d 100644 --- a/osf_tests/test_draft_registration.py +++ b/osf_tests/test_draft_registration.py @@ -59,12 +59,26 @@ def test_factory(self): assert draft.registration_schema == schema assert draft.registration_metadata == data + @mock.patch('website.settings.ENABLE_ARCHIVER', False) + def test_no_subjects_register(self): + user = factories.UserFactory() + auth = Auth(user) + project = factories.ProjectFactory(creator=user) + draft = factories.DraftRegistrationFactory(branched_from=project) + try: + draft.register(auth) + except NodeStateError as e: + assert str(e) == 'Registration must have at least one subject to be registered' + else: + assert False, 'Expected NodeStateError must be raised' + @mock.patch('website.settings.ENABLE_ARCHIVER', False) def test_register(self): user = factories.UserFactory() auth = Auth(user) project = factories.ProjectFactory(creator=user) draft = factories.DraftRegistrationFactory(branched_from=project) + draft.subjects.add(factories.SubjectFactory()) assert not draft.registered_node draft.register(auth) assert draft.registered_node diff --git a/osf_tests/test_validators.py b/osf_tests/test_validators.py index dc561389356..6d02aaa38d3 100644 --- a/osf_tests/test_validators.py +++ b/osf_tests/test_validators.py @@ -1,11 +1,21 @@ import pytest from osf.exceptions import ValidationValueError -from osf.models import validators +from types import SimpleNamespace +from osf.models import validators, OSFUser +from osf.models.validators import has_domain_in_user_fields_for_names from osf_tests.factories import SubjectFactory + # Ported from tests/framework/test_mongo.py +def mock_user(user_data): + user = SimpleNamespace() + user.DOMAIN_VALIDATION_FIELDS = OSFUser.DOMAIN_VALIDATION_FIELDS + for field in user.DOMAIN_VALIDATION_FIELDS: + setattr(user, field, user_data.get(field, '')) + return user + def test_string_required_passes_with_string(): assert validators.string_required('Hi!') is True @@ -50,3 +60,134 @@ def test_validate_expand_subject_hierarchy(): subject_list = [fruit._id, '12345_bad_id'] with pytest.raises(ValidationValueError): validators.expand_subject_hierarchy(subject_list) + + +@pytest.mark.parametrize( + 'user_data', + [ + { + 'fullname': 'Judith Sarah Preuss, M.Sc.', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'Sarah', + 'suffix': 'M.Sc.', + }, + { + 'fullname': 'J.H. van Hateren', + 'given_name': 'J.H.', + 'family_name': 'van Hateren', + 'middle_names': '', + }, + { + 'fullname': 'Sami-Egil Ahonen', + 'given_name': 'Sami-Egil', + 'family_name': 'Ahonen', + 'middle_names': '', + }, + { + 'fullname': 'Giovanni Luca Ciampaglia', + 'given_name': 'Giovanni Luca', + 'family_name': 'Ciampaglia', + 'middle_names': '', + }, + { + 'fullname': 'Joseph P.R.O. Orgel', + 'given_name': 'Joseph', + 'family_name': 'Orgel', + 'middle_names': 'P.R.O.', + }, + { + 'fullname': 'Andrew Daoust', + 'given_name': 'Andrew', + 'family_name': 'Daoust', + 'middle_names': '', + }, + { + 'fullname': 'Aidan G.C. Wright', + 'given_name': 'Aidan', + 'family_name': 'Wright', + 'middle_names': 'G.C.', + }, + { + 'fullname': 'Guillermo Perez Algorta', + 'given_name': 'Guillermo', + 'family_name': 'Perez Algorta', + 'middle_names': '', + }, + { + 'fullname': 'Sarah Wojkowski, MSc.PT, PhD.', + 'given_name': 'Sarah', + 'family_name': 'Wojkowski', + 'middle_names': 'MSc.PT', + }, + { + 'fullname': 'Brockmann, L.C. (Leon)', + 'given_name': 'Leon', + 'family_name': 'Brockmann', + 'middle_names': 'L.C.', + }, + { + 'fullname': 'Gragnolati, G.M. (Gaia Mariavittoria)', + 'given_name': 'Gaia', + 'family_name': 'Gragnolati', + 'middle_names': 'G.M.', + }, + { + 'fullname': 'F.H. Leeuwis', + 'given_name': 'F.H.', + 'family_name': 'Leeuwis', + 'middle_names': '', + }, + { + 'fullname': 'Grauss, S.E. (Sophie)', + 'given_name': 'Sophie', + 'family_name': 'Grauss', + 'middle_names': 'S.E.', + }, + { + 'fullname': 'Sandhya N.Sathesh', + 'given_name': 'Sandhya', + 'family_name': 'N.Sathesh', + 'middle_names': '', + }, + { + 'fullname': 'John Doe', + 'given_name': 'John', + 'family_name': 'Doe', + 'middle_names': '', + } + ] +) +def test_has_domain_in_user_fields(user_data): + user = mock_user(user_data) + assert has_domain_in_user_fields_for_names(user) is False + + +@pytest.mark.parametrize( + 'user_data', + [ + { + 'fullname': 'Judith Sarah', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'https://www.google.com', + 'suffix': 'M.Sc.', + }, + { + 'fullname': 'J.H. van Hateren', + 'given_name': 'J.H.', + 'family_name': 'https://google.com', + 'middle_names': '', + }, + { + 'fullname': 'Judith Sarah', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'www.google.com', + 'suffix': 'M.Sc.', + }, + ] +) +def test_has_domain_in_user_fields_fail(user_data): + user = mock_user(user_data) + assert has_domain_in_user_fields_for_names(user) is True diff --git a/osf_tests/users/test_last_login_date.py b/osf_tests/users/test_last_login_date.py deleted file mode 100644 index 33eebc17e1e..00000000000 --- a/osf_tests/users/test_last_login_date.py +++ /dev/null @@ -1,82 +0,0 @@ -from unittest import mock -import pytz -import pytest -import itsdangerous -from datetime import datetime, timedelta -from importlib import import_module - -from django.utils import timezone -from django.conf import settings as django_conf_settings - -from website import settings - -from osf_tests.factories import ( - AuthUserFactory, -) -from tests.base import OsfTestCase -from tests.utils import run_celery_tasks - -SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore - -@pytest.mark.django_db -@pytest.mark.enable_enqueue_task -class TestUserLastLoginDate(OsfTestCase): - - def setUp(self): - super().setUp() - - self.user = AuthUserFactory() - - self.session = SessionStore() - self.session['auth_user_id'] = self.user._id - self.session['auth_user_username'] = self.user.username - self.session.create() - self.cookie = itsdangerous.Signer(settings.SECRET_KEY).sign(self.session.session_key).decode() - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_from_none(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - assert self.user.date_last_login is None - - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - assert self.user.date_last_login == now - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_below_threshold(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - self.user.date_last_login = now - self.user.save() - - # Time is mocked one second below the last login date threshold, so it should not change. - mock_time.return_value = now + (settings.DATE_LAST_LOGIN_THROTTLE_DELTA - timedelta(seconds=1)) - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - # date_last_login is unchanged - assert self.user.date_last_login == now - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_above_threshold(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - self.user.date_last_login = now - self.user.save() - - # Time is mocked one second below the last login date threshold, so it should not change. - new_time = now + (settings.DATE_LAST_LOGIN_THROTTLE_DELTA + timedelta(seconds=1)) - mock_time.return_value = new_time - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - # date_last_login is changed! - assert self.user.date_last_login == new_time diff --git a/osf_tests/utils.py b/osf_tests/utils.py index adb00482168..0d93d1421c7 100644 --- a/osf_tests/utils.py +++ b/osf_tests/utils.py @@ -10,7 +10,8 @@ import blinker from website.signals import ALL_SIGNALS from website.archiver import ARCHIVER_SUCCESS -from website.archiver import listeners as archiver_listeners +from website.archiver.tasks import archive_callback + from osf.models import ( Sanction, @@ -154,7 +155,7 @@ def mock_archive(project, schema=None, auth=None, data=None, parent=None, sanction = registration.sanction mock.patch.object(root_job, 'archive_tree_finished', mock.Mock(return_value=True)), mock.patch('website.archiver.tasks.archive_success.delay', mock.Mock()) - archiver_listeners.archive_callback(registration) + archive_callback(registration._id) if autoapprove: sanction = registration.sanction diff --git a/tests/test_registrations/base.py b/tests/test_registrations/base.py index 60d8b855808..bc8447b39a3 100644 --- a/tests/test_registrations/base.py +++ b/tests/test_registrations/base.py @@ -9,7 +9,7 @@ from osf.models import RegistrationSchema from tests.base import OsfTestCase -from osf_tests.factories import AuthUserFactory, ProjectFactory, DraftRegistrationFactory +from osf_tests.factories import AuthUserFactory, ProjectFactory, DraftRegistrationFactory, SubjectFactory class RegistrationsTestBase(OsfTestCase): def setUp(self): @@ -37,6 +37,7 @@ def setUp(self): 'summary': {'value': 'Some airy'} } ) + self.draft.subjects.add(SubjectFactory()) current_month = timezone.now().strftime('%B') current_year = timezone.now().strftime('%Y') diff --git a/tests/test_registrations/test_embargoes.py b/tests/test_registrations/test_embargoes.py index c58de3c450e..49e1da79073 100644 --- a/tests/test_registrations/test_embargoes.py +++ b/tests/test_registrations/test_embargoes.py @@ -14,7 +14,7 @@ from tests.base import fake, OsfTestCase from osf_tests.factories import ( AuthUserFactory, EmbargoFactory, NodeFactory, ProjectFactory, - RegistrationFactory, UserFactory, UnconfirmedUserFactory, DraftRegistrationFactory, + RegistrationFactory, SubjectFactory, UserFactory, UnconfirmedUserFactory, DraftRegistrationFactory, EmbargoTerminationApprovalFactory ) from tests import utils diff --git a/tests/utils.py b/tests/utils.py index 9f0aba2f4cf..5a97433821b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -16,11 +16,11 @@ from framework.auth import Auth from framework.celery_tasks.handlers import celery_teardown_request from osf.email import _render_email_html -from osf_tests.factories import DraftRegistrationFactory +from osf_tests.factories import DraftRegistrationFactory, SubjectFactory from osf.models import Sanction, NotificationType, Notification from tests.base import get_default_metaschema from website.archiver import ARCHIVER_SUCCESS -from website.archiver import listeners as archiver_listeners +from website.archiver.tasks import archive_callback from website import settings as website_settings from osf import features @@ -136,8 +136,10 @@ def mock_archive(project, schema=None, auth=None, draft_registration=None, paren draft_registration = draft_registration or DraftRegistrationFactory( branched_from=project, registration_schema=schema ) + draft_registration.subjects.add(SubjectFactory()) with mock.patch('framework.celery_tasks.handlers.enqueue_task'): + draft_registration.subjects.add(SubjectFactory()) registration = draft_registration.register(auth=auth, save=True) if embargo: @@ -155,7 +157,7 @@ def mock_archive(project, schema=None, auth=None, draft_registration=None, paren # Ensure patches actually apply: with mock.patch.object(root_job, 'archive_tree_finished', mock.Mock(return_value=True)), \ mock.patch('website.archiver.tasks.archive_success.delay', mock.Mock()): - archiver_listeners.archive_callback(registration) + archive_callback(registration._id) if autoapprove: sanction = registration.root.sanction diff --git a/website/archiver/listeners.py b/website/archiver/listeners.py index d0f8f3b3c03..a9e562291da 100644 --- a/website/archiver/listeners.py +++ b/website/archiver/listeners.py @@ -3,9 +3,6 @@ from framework.celery_tasks import handlers from website.archiver import utils as archiver_utils -from website.archiver import ( - ARCHIVER_UNCAUGHT_ERROR, -) from website.archiver import signals as archiver_signals from website.project import signals as project_signals @@ -30,33 +27,6 @@ def after_register(src, dst, user): ) -@project_signals.archive_callback.connect -def archive_callback(dst): - """Blinker listener for updates to the archive task. When the tree of ArchiveJob - instances is complete, proceed to send success or failure mails - - :param dst: registration Node - """ - root = dst.root - root_job = root.archive_job - if not root_job.archive_tree_finished(): - return - if root_job.sent: - return - if root_job.success: - # Prevent circular import with app.py - from website.archiver import tasks - tasks.archive_success.delay(dst_pk=root._id, job_pk=root_job._id) - else: - archiver_utils.handle_archive_fail( - ARCHIVER_UNCAUGHT_ERROR, - root.registered_from, - root, - root.registered_user, - dst.archive_job.target_info(), - ) - - @archiver_signals.archive_fail.connect def archive_fail(dst, errors): reason = dst.archive_status diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index 7608b5c02c6..fad30d6e98b 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -10,7 +10,7 @@ from django.utils import timezone from datetime import timedelta -from framework.celery_tasks import app as celery_app +from framework.celery_tasks import app as celery_app, handlers from framework.celery_tasks.utils import logged from framework.exceptions import HTTPError from framework import sentry @@ -30,11 +30,11 @@ ) from website.archiver import utils from website.archiver.utils import normalize_unicode_filenames +from website.archiver import utils as archiver_utils from website.archiver import signals as archiver_signals from scripts.check_manual_restart_approval import delayed_manual_restart_approval -from website.project import signals as project_signals from website import settings from website.app import init_addons @@ -254,22 +254,35 @@ def stat_addon(self, addon_short_name, job_pk): default_retry_delay=60, ) @logged('make_copy_request') -def make_copy_request(self, job_pk, url, data): +def make_copy_request(self, params, job_pk): """Make the copy request to the WaterButler API and handle successful and failed responses + :param params: dict returned by archive_addon containing addon_short_name, url, and data :param job_pk: primary key of ArchiveJob - :param url: URL to send request to - :param data: of setting to send in POST to WaterButler API :return: None """ create_app_context() job = self.load_archive_job(job_pk) src, dst, user = job.info() + addon_short_name = params['addon_short_name'] + url = params['url'] + data = params['data'] logger.info(f"Sending copy request for addon: {data['provider']} on node: {dst._id}") cookie = furl(url).query.params.get('cookie') - res = requests.post(url, data=json.dumps(data), cookies={settings.COOKIE_NAME: cookie}) + try: + res = requests.post( + url, + data=json.dumps(data), + cookies={settings.COOKIE_NAME: cookie}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, + ) + except requests.RequestException as exc: + job.update_target(addon_short_name, ARCHIVER_FAILURE, errors=[str(exc)]) + raise + if res.status_code not in (http_status.HTTP_200_OK, http_status.HTTP_201_CREATED, http_status.HTTP_202_ACCEPTED): + job.update_target(addon_short_name, ARCHIVER_FAILURE, errors=[res.text or f'WaterButler request failed with status {res.status_code}']) raise HTTPError(res.status_code) def make_waterbutler_payload(dst_id, rename): @@ -324,7 +337,11 @@ def archive_addon(self, addon_short_name, job_pk): rename = f'{folder_name_nfd}{rename_suffix}' url = waterbutler_api_url_for(src._id, addon_short_name, _internal=True, base_url=src.osfstorage_region.waterbutler_url, **params) data = make_waterbutler_payload(dst._id, rename) - make_copy_request.delay(job_pk=job_pk, url=url, data=data) + return { + 'addon_short_name': addon_short_name, + 'url': url, + 'data': data, + } @celery_app.task( bind=True, @@ -361,15 +378,38 @@ def archive_node(self, stat_results, job_pk): if not stat_result.targets: job.status = ARCHIVER_SUCCESS job.save() + + addon_tasks = [] for result in stat_result.targets: if not result['num_files']: job.update_target(result['target_name'], ARCHIVER_SUCCESS) else: - archive_addon.delay( - addon_short_name=result['target_name'], - job_pk=job_pk + addon_tasks.append( + celery.chain( + [ + archive_addon.si( + addon_short_name=result['target_name'], + job_pk=job_pk, + ), + make_copy_request.s( + job_pk=job_pk, + ), + ] + ) ) - project_signals.archive_callback.send(dst) + + if not addon_tasks: + handlers.enqueue_task(archive_callback.si(dst_id=dst._id)) + return + + handlers.enqueue_task( + celery.chain( + [ + celery.group(addon_tasks), + archive_callback.si(dst_id=dst._id), + ] + ) + ) def archive(job_pk): @@ -488,3 +528,30 @@ def force_archive(self, registration_id, permissible_addons, allow_unconfigured= sentry.log_message(f'Archive task failed for {registration_id}: {exc}') sentry.log_exception(exc) return f'{exc.__class__.__name__}: {str(exc)}' + + +@celery_app.task +@logged('archive_callback') +def archive_callback(dst_id): + """Blinker task for updates to the archive task. When the tree of ArchiveJob + instances is complete, proceed to send success or failure mails + + :param dst: registration Node + """ + dst = Registration.load(dst_id) + root = dst.root + root_job = root.archive_job + if not root_job.archive_tree_finished(): + return + if root_job.sent: + return + if root_job.success: + archive_success.delay(dst_pk=root._id, job_pk=root_job._id) + else: + archiver_utils.handle_archive_fail( + ARCHIVER_UNCAUGHT_ERROR, + root.registered_from, + root, + root.registered_user, + dst.archive_job.target_info(), + ) diff --git a/website/identifiers/clients/crossref.py b/website/identifiers/clients/crossref.py index 8f496ce363b..f7e20a618c2 100644 --- a/website/identifiers/clients/crossref.py +++ b/website/identifiers/clients/crossref.py @@ -263,6 +263,7 @@ def create_identifier(self, preprint, category, include_relation=True): fname=f'{preprint._id}.xml' ), files={'file': (f'{preprint._id}.xml', metadata)}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) if response.status_code == 429: raise CrossRefRateLimitError(response.text) @@ -291,6 +292,7 @@ def bulk_create(self, metadata, filename): fname=f'{filename}.xml' ), files={'file': (f'{filename}.xml', metadata)}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) logger.info('Sent a bulk update of metadata to CrossRef') diff --git a/website/project/signals.py b/website/project/signals.py index a9b1e0e735a..5306f7e4e7f 100644 --- a/website/project/signals.py +++ b/website/project/signals.py @@ -9,5 +9,3 @@ node_deleted = signals.signal('node-deleted') after_create_registration = signals.signal('post-create-registration') - -archive_callback = signals.signal('archive-callback') diff --git a/website/settings/defaults.py b/website/settings/defaults.py index da607c2afb3..026187948a5 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -375,6 +375,13 @@ def parent_dir(path): SHARE_URL = 'https://share.osf.io/' SHARE_API_TOKEN = None # Required to send project updates to SHARE +EXTERNAL_REQUEST_TIMEOUT = (10, 30) # (connect, read) timeout for outbound requests to external services + +SHARE_UPDATE_TASK_SOFT_TIME_LIMIT = 90 +SHARE_UPDATE_TASK_HARD_TIME_LIMIT = 120 +SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT = 60 +SPAM_SUBMIT_TASK_HARD_TIME_LIMIT = 90 + CAS_SERVER_URL = 'http://localhost:8080' MFR_SERVER_URL = 'http://localhost:7778'