Merge branch 'develop' into upstream/b-and-i-25-01

CHANGELOG

@@ -2,6 +2,10 @@
 
 We follow the CalVer (https://calver.org/) versioning scheme: YY.MINOR.MICRO.
 
+25.03.0 (2025-02-24)
+====================
+- Addons and GV - BE Release
+
 25.02.0 (2025-01-27)
 ====================
 - DOI Versioning - BE Release

addons/base/views.py

@@ -58,6 +58,7 @@
 )
 from osf.metrics import PreprintView, PreprintDownload
 from osf.utils import permissions
+from osf.external.gravy_valet import request_helpers
 from website.profile.utils import get_profile_image_url
 from website.project import decorators
 from website.project.decorators import must_be_contributor_or_public, must_be_valid_project, check_contributor_auth
@@ -227,15 +228,25 @@ def get_auth(auth, **kwargs):
     _check_resource_permissions(resource, auth, action)
 
     provider_name = waterbutler_data['provider']
+    waterbutler_settings = None
+    waterbutler_credentials = None
     file_version = file_node = None
-    if provider_name == 'osfstorage':
+    if provider_name == 'osfstorage' or (not flag_is_active(request, features.ENABLE_GV)):
         file_version, file_node = _get_osfstorage_file_version_and_node(
             file_path=waterbutler_data.get('path'), file_version_id=waterbutler_data.get('version')
         )
-
-    waterbutler_settings, waterbutler_credentials = _get_waterbutler_configs(
-        resource=resource, provider_name=provider_name, file_version=file_version,
-    )
+        waterbutler_settings, waterbutler_credentials = _get_waterbutler_configs(
+            resource=resource, provider_name=provider_name, file_version=file_version,
+        )
+    else:
+        result = request_helpers.get_waterbutler_config(
+            gv_addon_pk=f'{waterbutler_data['nid']}:{waterbutler_data['provider']}',
+            requested_resource=resource,
+            requesting_user=auth.user,
+            addon_type='configured-storage-addons',
+        )
+        waterbutler_settings = result.get_attribute('config')
+        waterbutler_credentials = result.get_attribute('credentials')
 
     _enqueue_metrics(
         file_version=file_version,

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

@@ -1554,7 +1554,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