Fix/addon query

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):

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
         ]

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):

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):

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
 

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

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)