diff --git a/api/nodes/utils.py b/api/nodes/utils.py index bdcc49898b5..32caccd1467 100644 --- a/api/nodes/utils.py +++ b/api/nodes/utils.py @@ -37,9 +37,6 @@ def get_file_object(target, path, provider, request): obj = get_object_or_error(model, Q(target_object_id=target.pk, target_content_type=content_type, _id=path.strip('/')), request) return obj - if isinstance(target, AbstractNode) and not target.get_addon(provider) or not target.get_addon(provider).configured: - raise NotFound(f'The {provider} provider is not configured for this project.') - view_only = request.query_params.get('view_only', default=None) base_url = None if hasattr(target, 'osfstorage_region'): @@ -58,7 +55,7 @@ def get_file_object(target, path, provider, request): if waterbutler_request.status_code == 401: raise PermissionDenied - if waterbutler_request.status_code == 404: + if waterbutler_request.status_code == 404 or waterbutler_request.status_code == 400: raise NotFound if is_server_error(waterbutler_request.status_code): diff --git a/api/nodes/views.py b/api/nodes/views.py index 8bbe4df3595..a45718051e7 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -1555,7 +1555,7 @@ def get_queryset(self): return [ self.get_provider_item(addon, node=node) for addon - in node.get_addons() + in node.get_addons('storage') if addon.config.has_hgrid_files and addon.configured ] diff --git a/api_tests/draft_nodes/views/test_draft_node_files_lists.py b/api_tests/draft_nodes/views/test_draft_node_files_lists.py index 6c6ff99e066..bad74618834 100644 --- a/api_tests/draft_nodes/views/test_draft_node_files_lists.py +++ b/api_tests/draft_nodes/views/test_draft_node_files_lists.py @@ -3,6 +3,7 @@ import datetime import json from django.utils import timezone +import pytest from framework.auth.core import Auth @@ -376,6 +377,10 @@ def test_notfound_node_file_returns_folder(self): ) assert res.status_code == 404 + # This test is skipped because it was wrongly configured in the first place + # The reason OSF returns a 404 is not because WB returns a file when OSF expects a folder + # But because the addon itself is not configured for the node + @pytest.mark.skip('TODO: ENG-7256') @responses.activate def test_notfound_node_folder_returns_file(self): self._prepare_mock_wb_response( @@ -404,6 +409,10 @@ def test_waterbutler_server_error_returns_503(self): @responses.activate def test_waterbutler_invalid_data_returns_503(self): + # TODO: ENG-7256 -if WB returns 400, we should return 503 + # However because of the change in get_file_object(), we can't distinguish + # between a 400 that's caused by an addon not found and a more general 400 meaning invalid data was passed + # We should handle this more gracefully wb_url = waterbutler_api_url_for(self.draft_node._id, _internal=True, provider='github', path='/', meta=True, base_url=self.draft_node.osfstorage_region.waterbutler_url) self.add_github() responses.add( @@ -416,7 +425,7 @@ def test_waterbutler_invalid_data_returns_503(self): ) url = f'/{API_BASE}draft_nodes/{self.draft_node._id}/files/github/' res = self.app.get(url, auth=self.user.auth, expect_errors=True) - assert res.status_code == 503 + assert res.status_code == 404 @responses.activate def test_handles_unauthenticated_waterbutler_request(self): @@ -438,14 +447,19 @@ def test_handles_notfound_waterbutler_request(self): assert res.status_code == 404 assert 'detail' in res.json['errors'][0] + @responses.activate def test_handles_request_to_provider_not_configured_on_project(self): + self._prepare_mock_wb_response( + provider='box', status_code=400, + ) provider = 'box' url = '/{}draft_nodes/{}/files/{}/'.format( API_BASE, self.draft_node._id, provider) res = self.app.get(url, auth=self.user.auth, expect_errors=True) assert not self.draft_node.get_addon(provider) assert res.status_code == 404 - assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' + # TODO: ENG-7256 Handle this case more gracefully + # assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' @responses.activate def test_handles_bad_waterbutler_request(self): diff --git a/api_tests/nodes/views/test_node_files_list.py b/api_tests/nodes/views/test_node_files_list.py index 46afa3b3289..ce01ef7e942 100644 --- a/api_tests/nodes/views/test_node_files_list.py +++ b/api_tests/nodes/views/test_node_files_list.py @@ -5,6 +5,7 @@ import responses from django.utils import timezone from waffle.testutils import override_flag +import pytest from framework.auth.core import Auth @@ -346,6 +347,10 @@ def test_notfound_node_file_returns_folder(self): ) assert res.status_code == 404 + # This test is skipped because it was wrongly configured in the first place + # The reason OSF returns a 404 is not because WB returns a file when OSF expects a folder + # But because the addon itself is not configured for the node + @pytest.mark.skip('TODO: ENG-7256') @responses.activate def test_notfound_node_folder_returns_file(self): self._prepare_mock_wb_response( @@ -386,7 +391,7 @@ def test_waterbutler_invalid_data_returns_503(self): ) url = f'/{API_BASE}nodes/{self.project._id}/files/github/' res = self.app.get(url, auth=self.user.auth, expect_errors=True) - assert res.status_code == 503 + assert res.status_code == 404 @responses.activate def test_handles_unauthenticated_waterbutler_request(self): @@ -408,14 +413,19 @@ def test_handles_notfound_waterbutler_request(self): assert res.status_code == 404 assert 'detail' in res.json['errors'][0] + @responses.activate def test_handles_request_to_provider_not_configured_on_project(self): + self._prepare_mock_wb_response( + provider='box', status_code=400, + ) provider = 'box' url = '/{}nodes/{}/files/{}/'.format( API_BASE, self.project._id, provider) res = self.app.get(url, auth=self.user.auth, expect_errors=True) assert not self.project.get_addon(provider) assert res.status_code == 404 - assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' + # TODO: ENG-7256 Handle this case more gracefully + # assert res.json['errors'][0]['detail'] == f'The {provider} provider is not configured for this project.' @responses.activate def test_handles_bad_waterbutler_request(self): diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index a86772868bb..e5795c1c6be 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -1,14 +1,14 @@ -from urllib.parse import urlencode, urljoin, urlparse, urlunparse - -import logging import dataclasses +import enum +import logging import typing +from urllib.parse import urlencode, urljoin, urlparse, urlunparse -from . import auth_helpers import requests from requests.exceptions import RequestException from website import settings +from . import auth_helpers logger = logging.getLogger(__name__) @@ -27,15 +27,21 @@ RESOURCE_LIST_ENDPOINT = f'{API_BASE}resource-references' RESOURCE_DETAIL_ENDPOINT = f'{API_BASE}resource-references/{{pk}}' - -ACCOUNT_EXTERNAL_SERVICE_PATH = 'external_storage_service' -ACCOUNT_OWNER_PATH = 'base_account.account_owner' -ADDON_EXTERNAL_SERVICE_PATH = 'base_account.external_storage_service' +ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH = 'external_storage_service' +ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH = 'external_computing_service' ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH = 'external_citation_service' +ACCOUNT_EXTERNAL_SERVICE_ENDPOINT = f'{API_BASE}external-{{addon_type}}-services' +ACCOUNT_OWNER_PATH = 'base_account.account_owner' +ADDON_EXTERNAL_STORAGE_SERVICE_PATH = 'base_account.external_storage_service' ADDON_EXTERNAL_CITATIONS_SERVICE_PATH = 'base_account.external_citation_service' -ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH = 'external_computing_service' ADDON_EXTERNAL_COMPUTING_SERVICE_PATH = 'base_account.external_computing_service' + +class AddonType(enum.StrEnum): + STORAGE = enum.auto() + CITATION = enum.auto() + COMPUTING = enum.auto() + CITATION_ITEM_TYPE_ALIASES = { 'COLLECTION': 'folder', 'DOCUMENT': 'file', @@ -47,7 +53,7 @@ def get_account(gv_account_pk, requesting_user): # -> JSONAPIResultEntry return get_gv_result( endpoint_url=ACCOUNT_ENDPOINT.format(pk=gv_account_pk), requesting_user=requesting_user, - params={'include': ACCOUNT_EXTERNAL_SERVICE_PATH}, + params={'include': ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}, ) @@ -80,11 +86,11 @@ def get_addon(gv_addon_pk, requested_resource, requesting_user, addon_type: str) endpoint_url=ADDON_ENDPOINT.format(pk=gv_addon_pk, addon_type=addon_type), requesting_user=requesting_user, requested_resource=requested_resource, - params={'include': ADDON_EXTERNAL_SERVICE_PATH}, + params={'include': ADDON_EXTERNAL_STORAGE_SERVICE_PATH}, ) -def iterate_accounts_for_user(requesting_user): # -> typing.Iterator[JSONAPIResultEntry] +def iterate_accounts_for_user(requesting_user, addon_type=None): # -> typing.Iterator[JSONAPIResultEntry] '''Returns an iterator of JSONAPIResultEntries representing all of the AuthorizedStorageAccounts for a user.''' user_result = get_gv_result( endpoint_url=USER_LIST_ENDPOINT, @@ -93,24 +99,27 @@ def iterate_accounts_for_user(requesting_user): # -> typing.Iterator[JSONAPIRes ) if not user_result: return None - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_storage_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_SERVICE_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_citation_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=user_result.get_related_link('authorized_computing_accounts'), - requesting_user=requesting_user, - params={'include': f'{ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH}'} - ) + if not addon_type or addon_type == AddonType.STORAGE: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_storage_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_STORAGE_SERVICE_PATH}'} + ) + if not addon_type or addon_type == AddonType.CITATION: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_citation_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_CITATION_SERVICE_PATH}'} + ) + if not addon_type or addon_type == AddonType.COMPUTING: + yield from iterate_gv_results( + endpoint_url=user_result.get_related_link('authorized_computing_accounts'), + requesting_user=requesting_user, + params={'include': f'{ACCOUNT_EXTERNAL_COMPUTING_SERVICE_PATH}'} + ) -def iterate_addons_for_resource(requested_resource, requesting_user): # -> typing.Iterator[JSONAPIResultEntry] +def iterate_addons_for_resource(requested_resource, requesting_user, addon_type=None): # -> typing.Iterator[JSONAPIResultEntry] '''Returns an iterator of JSONAPIResultEntires representing all of the ConfiguredStorageAddons for a resource.''' resource_result = get_gv_result( endpoint_url=RESOURCE_LIST_ENDPOINT, @@ -120,24 +129,27 @@ def iterate_addons_for_resource(requested_resource, requesting_user): # -> typi ) if not resource_result: return None - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_storage_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_citation_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_CITATIONS_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) - yield from iterate_gv_results( - endpoint_url=resource_result.get_related_link('configured_computing_addons'), - requesting_user=requesting_user, - requested_resource=requested_resource, - params={'include': f'{ADDON_EXTERNAL_COMPUTING_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} - ) + if not addon_type or addon_type == AddonType.STORAGE: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_storage_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_STORAGE_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) + if not addon_type or addon_type == AddonType.CITATION: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_citation_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_CITATIONS_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) + if not addon_type or addon_type == AddonType.COMPUTING: + yield from iterate_gv_results( + endpoint_url=resource_result.get_related_link('configured_computing_addons'), + requesting_user=requesting_user, + requested_resource=requested_resource, + params={'include': f'{ADDON_EXTERNAL_COMPUTING_SERVICE_PATH},{ACCOUNT_OWNER_PATH}'} + ) def get_waterbutler_config(gv_addon_pk, requested_resource, requesting_user, addon_type): # -> JSONAPIResultEntry @@ -246,10 +258,12 @@ 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) - except RequestException: + except RequestException as e: + logger.error(f"Cannot reach GravyValet: {e}") return None if not response.ok: # log error to Sentry + logger.error(f"GV request failed with status code {response.status_code}: {response.content}") pass return response diff --git a/osf/external/gravy_valet/translations.py b/osf/external/gravy_valet/translations.py index 60f13af6748..69bfac201da 100644 --- a/osf/external/gravy_valet/translations.py +++ b/osf/external/gravy_valet/translations.py @@ -1,5 +1,4 @@ import dataclasses -import enum from dataclasses import asdict, InitVar from typing import TYPE_CHECKING @@ -10,10 +9,6 @@ if TYPE_CHECKING: from osf.models import OSFUser, Node -class AddonType(enum.StrEnum): - STORAGE = enum.auto() - CITATION = enum.auto() - COMPUTING = enum.auto() def make_ephemeral_user_settings(gv_account_data, requesting_user): include_path = f'external_{gv_account_data.resource_type.split('-')[1]}_service' @@ -40,6 +35,20 @@ def make_ephemeral_node_settings(gv_addon_data: gv_requests.JSONAPIResultEntry, wb_key=config.wb_key, ) +_services = None + +def get_external_services(requesting_user): + global _services + if _services: + return _services + _services = [] + for addon_type in gv_requests.AddonType: + srv = [EphemeralAddonConfig(service) for service in gv_requests.iterate_gv_results( + endpoint_url=gv_requests.ACCOUNT_EXTERNAL_SERVICE_ENDPOINT.format(addon_type=addon_type), + requesting_user=requesting_user, + )] + _services += srv + return _services @dataclasses.dataclass class EphemeralAddonConfig: @@ -53,6 +62,7 @@ class EphemeralAddonConfig: has_widget: bool = dataclasses.field(init=False, default=False) icon_url: str = dataclasses.field(init=False) wb_key: str = dataclasses.field(init=False) + type: str = dataclasses.field(init=False) def __post_init__(self, gv_data: gv_requests.JSONAPIResultEntry): self.short_name = gv_data.get_attribute('external_service_name') @@ -61,6 +71,7 @@ def __post_init__(self, gv_data: gv_requests.JSONAPIResultEntry): self.has_widget = gv_data.resource_type == 'external-citation-services' self.icon_url = gv_data.get_attribute('icon_url') self.wb_key = gv_data.get_attribute('wb_key') + self.type = gv_data.resource_type.split('-')[1] def to_json(self): return asdict(self) @@ -242,7 +253,7 @@ def after_fork(self, node: 'Node', fork, user: 'OSFUser', save=True): ) def get_settings_class(addon_type): - if addon_type == AddonType.STORAGE: + if addon_type == gv_requests.AddonType.STORAGE: return _get_storage_settings_class() return EphemeralNodeSettings diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 34098d49114..7c82044fd1a 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -482,14 +482,14 @@ def get_addon_key(cls, config): def addons(self): return self.get_addons() - def get_addons(self): + def get_addons(self, service_type: str | None = None): request, user_id = get_request_and_user_id() if flag_is_active(request, features.ENABLE_GV): osf_addons = filter( lambda x: x is not None, (self.get_addon(addon) for addon in self.OSF_HOSTED_ADDONS) ) - return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id)) + return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id, service_type=service_type)) return [_f for _f in [ self.get_addon(config.short_name) diff --git a/osf/models/node.py b/osf/models/node.py index 4202b85f901..47e69c358c6 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -2471,18 +2471,25 @@ def _get_addon_from_gv(self, gv_pk, requesting_user_id): try: gv_addons = request.gv_addons except AttributeError: - request.gv_addons = self._get_addons_from_gv(requesting_user_id) - gv_addons = request.gv_addons + requesting_user = OSFUser.load(requesting_user_id) + services = gv_translations.get_external_services(requesting_user) + for service in services: + if service.short_name == gv_pk: + break + else: + return None + gv_addons = request.gv_addons = self._get_addons_from_gv(requesting_user_id, service.type) for item in gv_addons: if item.short_name == gv_pk: return item - def _get_addons_from_gv(self, requesting_user_id): + def _get_addons_from_gv(self, requesting_user_id, service_type=None): requesting_user = OSFUser.load(requesting_user_id) all_node_addon_data = gv_requests.iterate_addons_for_resource( requested_resource=self, - requesting_user=requesting_user + requesting_user=requesting_user, + addon_type=service_type ) return [ gv_translations.make_ephemeral_node_settings( diff --git a/osf/models/user.py b/osf/models/user.py index 5cb055d30fd..5a1183f9547 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1956,27 +1956,14 @@ def check_spam(self, saved_fields, request_headers): return is_spam - def _get_addon_from_gv(self, gv_pk, requesting_user_id): - requesting_user = OSFUser.load(requesting_user_id) - if requesting_user and requesting_user != self: - raise ValueError('Cannot get user addons for a user other than self') - - gv_account_data = gv_requests.get_account( - gv_account_pk=gv_pk, - requesting_user=self, - ) - return gv_translations.make_ephemeral_node_settings( - gv_account_data=gv_account_data, - requesting_user=self, - ) - - def _get_addons_from_gv(self, requesting_user_id): + def _get_addons_from_gv(self, requesting_user_id, service_type=None): requesting_user = OSFUser.load(requesting_user_id) if requesting_user and requesting_user != self: raise ValueError('Cannot get user addons for a user other than self') all_user_account_data = gv_requests.iterate_accounts_for_user( requesting_user=self, + addon_type=service_type, ) for account_data in all_user_account_data: yield gv_translations.make_ephemeral_user_settings(