From cf62ee5726de0cd3c8a771821872488ff90a3a39 Mon Sep 17 00:00:00 2001 From: Vit Zikmund Date: Wed, 6 Mar 2024 17:22:42 +0100 Subject: [PATCH 1/4] feat: use endpoints compatible with LFS server discovery Additionally, add support for the in the URI to contain slashes, as some git repositories(e.g. GitLab) can contain multiple levels of organization groups/namespaces. --- giftless/transfer/basic_streaming.py | 4 ++-- giftless/view.py | 7 ++++++- tests/test_batch_api.py | 14 +++++++------- tests/test_error_responses.py | 4 ++-- tests/test_middleware.py | 9 +++++---- tests/transfer/test_basic_external_adapter.py | 4 ++-- 6 files changed, 24 insertions(+), 18 deletions(-) diff --git a/giftless/transfer/basic_streaming.py b/giftless/transfer/basic_streaming.py index b62432b..491b995 100644 --- a/giftless/transfer/basic_streaming.py +++ b/giftless/transfer/basic_streaming.py @@ -33,7 +33,7 @@ class VerifyView(BaseView): make the test structures a little less weird? """ - route_base = "//objects/storage" + route_base = "objects/storage" def __init__(self, storage: VerifiableStorage) -> None: self.storage = storage @@ -75,7 +75,7 @@ def get_verify_url( class ObjectsView(BaseView): """Provides methods for object storage management.""" - route_base = "//objects/storage" + route_base = "objects/storage" def __init__(self, storage: StreamingStorage) -> None: self.storage = storage diff --git a/giftless/view.py b/giftless/view.py index 8472c46..9b13f39 100644 --- a/giftless/view.py +++ b/giftless/view.py @@ -23,6 +23,11 @@ class BaseView(FlaskView): "flask-classful/default": representation.output_git_lfs_json, } + route_prefix: ClassVar = "/.git/info/lfs/" + # [flask-classful bug/feat?] Placeholders in route_prefix not skipped for + # building the final rule for methods with them (FlaskView.build_rule). + base_args: ClassVar = ["organization", "repo"] + trailing_slash = False @classmethod @@ -64,7 +69,7 @@ def _is_authorized( class BatchView(BaseView): """Batch operations.""" - route_base = "//objects/batch" + route_base = "objects/batch" def post(self, organization: str, repo: str) -> dict[str, Any]: """Batch operations.""" diff --git a/tests/test_batch_api.py b/tests/test_batch_api.py index ac8f9b3..7874c1c 100644 --- a/tests/test_batch_api.py +++ b/tests/test_batch_api.py @@ -12,7 +12,7 @@ def test_upload_batch_request(test_client: FlaskClient) -> None: """Test basic batch API with a basic successful upload request.""" request_payload = batch_request_payload(operation="upload") response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 200 @@ -40,7 +40,7 @@ def test_download_batch_request( create_file_in_storage(storage_path, "myorg", "myrepo", oid, size=8) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 200 @@ -70,7 +70,7 @@ def test_download_batch_request_two_files_one_missing( request_payload["objects"].append({"oid": "12345679", "size": 5555}) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 200 @@ -104,7 +104,7 @@ def test_download_batch_request_two_files_missing( request_payload["objects"].append({"oid": "12345679", "size": 5555}) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 404 @@ -139,7 +139,7 @@ def test_download_batch_request_two_files_one_mismatch( ) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 200 @@ -177,7 +177,7 @@ def test_download_batch_request_one_file_mismatch( ) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 422 @@ -206,7 +206,7 @@ def test_download_batch_request_two_files_different_errors( ) response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 422 diff --git a/tests/test_error_responses.py b/tests/test_error_responses.py index 4bc1c2b..e2c71df 100644 --- a/tests/test_error_responses.py +++ b/tests/test_error_responses.py @@ -7,7 +7,7 @@ def test_error_response_422(test_client: FlaskClient) -> None: """Test an invalid payload error.""" response = test_client.post( - "/myorg/myrepo/objects/batch", + "/myorg/myrepo.git/info/lfs/objects/batch", json=batch_request_payload(delete_keys=["operation"]), ) @@ -30,7 +30,7 @@ def test_error_response_403(test_client: FlaskClient) -> None: read-only setup. """ response = test_client.post( - "/myorg/myrepo/objects/batch", + "/myorg/myrepo.git/info/lfs/objects/batch", json=batch_request_payload(operation="upload"), ) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index bd64c98..2d1c080 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -44,17 +44,18 @@ def test_upload_request_with_x_forwarded_middleware( """ request_payload = batch_request_payload(operation="upload") response = test_client.post( - "/myorg/myrepo/objects/batch", json=request_payload + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload ) assert response.status_code == 200 json = cast(dict[str, Any], response.json) upload_action = json["objects"][0]["actions"]["upload"] href = upload_action["href"] - assert href == "http://localhost/myorg/myrepo/objects/storage/12345678" + base_uri = "myorg/myrepo.git/info/lfs" + assert href == f"http://localhost/{base_uri}/objects/storage/12345678" response = test_client.post( - "/myorg/myrepo/objects/batch", + "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload, headers={ "X-Forwarded-Host": "mycompany.xyz", @@ -70,5 +71,5 @@ def test_upload_request_with_x_forwarded_middleware( href = upload_action["href"] assert ( href - == "https://mycompany.xyz:1234/lfs/myorg/myrepo/objects/storage/12345678" + == "https://mycompany.xyz:1234/lfs/myorg/myrepo.git/info/lfs/objects/storage/12345678" ) diff --git a/tests/transfer/test_basic_external_adapter.py b/tests/transfer/test_basic_external_adapter.py index ee3f636..c8c7ffe 100644 --- a/tests/transfer/test_basic_external_adapter.py +++ b/tests/transfer/test_basic_external_adapter.py @@ -45,7 +45,7 @@ def test_upload_action_new_file() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", + "href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify", "header": {}, "expires_in": 43200, }, @@ -73,7 +73,7 @@ def test_upload_action_extras_are_passed() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", + "href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify", "header": {}, "expires_in": 43200, }, From f7e07aca76dfd87fe466a8a211a03d98886cd458 Mon Sep 17 00:00:00 2001 From: Vit Zikmund Date: Thu, 7 Mar 2024 00:36:13 +0100 Subject: [PATCH 2/4] feat(endpoint): use legacy endpoints along new ones This entails a dynamically created class voodoo for the change not to poison too much of the existing code. We should add test alternatives for those currently changed that they work properly with the LEGACY_ENDPOINTS turned off. --- giftless/config.py | 15 ++++++++++- giftless/transfer/basic_streaming.py | 12 ++++++--- giftless/view.py | 26 ++++++++++++++++--- tests/test_middleware.py | 4 +-- tests/transfer/test_basic_external_adapter.py | 4 +-- 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/giftless/config.py b/giftless/config.py index 0d541d7..e38a139 100644 --- a/giftless/config.py +++ b/giftless/config.py @@ -1,5 +1,6 @@ """Configuration handling helper functions and default configuration.""" import os +import warnings from pathlib import Path from typing import Any @@ -29,9 +30,10 @@ } default_config = { - "TRANSFER_ADAPTERS": Extensible(default_transfer_config), "TESTING": False, "DEBUG": False, + "LEGACY_ENDPOINTS": True, + "TRANSFER_ADAPTERS": Extensible(default_transfer_config), "AUTH_PROVIDERS": ["giftless.auth.allow_anon:read_only"], "PRE_AUTHORIZED_ACTION_PROVIDER": { "factory": "giftless.auth.jwt:factory", @@ -55,6 +57,17 @@ def configure(app: Flask, additional_config: dict | None = None) -> Flask: """Configure a Flask app using Figcan managed configuration object.""" config = _compose_config(additional_config) app.config.update(config) + if app.config["LEGACY_ENDPOINTS"]: + warnings.warn( + FutureWarning( + "LEGACY_ENDPOINTS (starting with '//') are enabled" + " as the default. They will be eventually removed in favor of" + " those starting with '/.git/info/lfs/')." + " Switch your clients to them and set the configuration" + " option to False to disable this warning." + ), + stacklevel=1, + ) return app diff --git a/giftless/transfer/basic_streaming.py b/giftless/transfer/basic_streaming.py index 491b995..9893a31 100644 --- a/giftless/transfer/basic_streaming.py +++ b/giftless/transfer/basic_streaming.py @@ -10,7 +10,7 @@ from typing import Any, BinaryIO, cast import marshmallow -from flask import Flask, Response, request, url_for +from flask import Flask, Response, current_app, request, url_for from flask_classful import route from webargs.flaskparser import parser @@ -61,7 +61,10 @@ def get_verify_url( cls, organization: str, repo: str, oid: str | None = None ) -> str: """Get the URL for upload / download requests for this object.""" - op_name = f"{cls.__name__}:verify" + # Use the legacy endpoint when enabled + # see giftless.view.BaseView:register for details + legacy = "Legacy" if current_app.config["LEGACY_ENDPOINTS"] else "" + op_name = f"{legacy}{cls.__name__}:verify" url: str = url_for( op_name, organization=organization, @@ -138,7 +141,10 @@ def get_storage_url( oid: str | None = None, ) -> str: """Get the URL for upload / download requests for this object.""" - op_name = f"{cls.__name__}:{operation}" + # Use the legacy endpoint when enabled + # see giftless.view.BaseView:register for details + legacy = "Legacy" if current_app.config["LEGACY_ENDPOINTS"] else "" + op_name = f"{legacy}{cls.__name__}:{operation}" url: str = url_for( op_name, organization=organization, diff --git a/giftless/view.py b/giftless/view.py index 9b13f39..39e97fe 100644 --- a/giftless/view.py +++ b/giftless/view.py @@ -1,5 +1,5 @@ """Flask-Classful View Classes.""" -from typing import Any, ClassVar +from typing import Any, ClassVar, cast from flask import Flask from flask_classful import FlaskView @@ -31,10 +31,30 @@ class BaseView(FlaskView): trailing_slash = False @classmethod - def register(cls, *args: Any, **kwargs: Any) -> Any: + def register(cls, app: Flask, *args: Any, **kwargs: Any) -> Any: if kwargs.get("base_class") is None: kwargs["base_class"] = BaseView - return super().register(*args, **kwargs) + if ( + app.config["LEGACY_ENDPOINTS"] + and kwargs.get("route_prefix") is None + and not hasattr(cls, "_legacy_") # break the cycle + ): + # To span any transition required for the switch to the current + # endpoint URI, create a "Legacy" class "copy" of this view and + # register it too, for both the views and their endpoints to + # coexist. + legacy_view = type( + f"Legacy{cls.__name__}", + (cls,), + { + "route_prefix": "//", + "_legacy_": True, + }, + ) + legacy_view = cast(BaseView, legacy_view) + legacy_view.register(app, *args, **kwargs) + + return super().register(app, *args, **kwargs) @classmethod def _check_authorization( diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 2d1c080..acc4da2 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -51,7 +51,7 @@ def test_upload_request_with_x_forwarded_middleware( json = cast(dict[str, Any], response.json) upload_action = json["objects"][0]["actions"]["upload"] href = upload_action["href"] - base_uri = "myorg/myrepo.git/info/lfs" + base_uri = "myorg/myrepo" assert href == f"http://localhost/{base_uri}/objects/storage/12345678" response = test_client.post( @@ -71,5 +71,5 @@ def test_upload_request_with_x_forwarded_middleware( href = upload_action["href"] assert ( href - == "https://mycompany.xyz:1234/lfs/myorg/myrepo.git/info/lfs/objects/storage/12345678" + == f"https://mycompany.xyz:1234/lfs/{base_uri}/objects/storage/12345678" ) diff --git a/tests/transfer/test_basic_external_adapter.py b/tests/transfer/test_basic_external_adapter.py index c8c7ffe..ee3f636 100644 --- a/tests/transfer/test_basic_external_adapter.py +++ b/tests/transfer/test_basic_external_adapter.py @@ -45,7 +45,7 @@ def test_upload_action_new_file() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify", + "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", "header": {}, "expires_in": 43200, }, @@ -73,7 +73,7 @@ def test_upload_action_extras_are_passed() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify", + "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", "header": {}, "expires_in": 43200, }, From 3718c69e98b702518b9becd87c1970b2d6870c7f Mon Sep 17 00:00:00 2001 From: Vit Zikmund Date: Fri, 8 Mar 2024 12:39:09 +0100 Subject: [PATCH 3/4] chore(endpoint): test legacy endpoints turned off --- tests/conftest.py | 8 +++++-- tests/helpers.py | 13 +++++++++++ tests/test_middleware.py | 22 ++++++++++++++----- tests/transfer/test_basic_external_adapter.py | 18 +++++++++++---- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index fdfe08c..583b56b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,7 @@ import pathlib import shutil from collections.abc import Generator +from typing import Any import flask import pytest @@ -10,6 +11,7 @@ from giftless.app import init_app from giftless.auth import allow_anon, authentication +from tests.helpers import legacy_endpoints_id @pytest.fixture @@ -22,12 +24,14 @@ def storage_path(tmp_path: pathlib.Path) -> Generator: shutil.rmtree(path) -@pytest.fixture -def app(storage_path: str) -> flask.Flask: +@pytest.fixture(params=[False], ids=legacy_endpoints_id) +def app(storage_path: str, request: Any) -> flask.Flask: """Session fixture to configure the Flask app.""" + legacy_endpoints = request.param app = init_app( additional_config={ "TESTING": True, + "LEGACY_ENDPOINTS": legacy_endpoints, "TRANSFER_ADAPTERS": { "basic": { "options": {"storage_options": {"path": storage_path}} diff --git a/tests/helpers.py b/tests/helpers.py index 9352d00..ee82f72 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -3,6 +3,8 @@ from pathlib import Path from typing import Any +import flask + def batch_request_payload( delete_keys: Sequence[str] = (), **kwargs: Any @@ -39,3 +41,14 @@ def create_file_in_storage( with Path(repo_path / filename).open("wb") as f: for c in (b"0" for _ in range(size)): f.write(c) + + +def legacy_endpoints_id(enabled: bool) -> str: + return "legacy-ep" if enabled else "current-ep" + + +def expected_uri_prefix(app: flask.Flask, *args: str) -> str: + core_prefix = "/".join(args) + if not app.config.get("LEGACY_ENDPOINTS"): + return core_prefix + ".git/info/lfs" + return core_prefix diff --git a/tests/test_middleware.py b/tests/test_middleware.py index acc4da2..11673b9 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -1,17 +1,21 @@ """Tests for using middleware and some specific middleware.""" from typing import Any, cast +import flask import pytest -from flask import Flask from flask.testing import FlaskClient from giftless.app import init_app -from .helpers import batch_request_payload +from .helpers import ( + batch_request_payload, + expected_uri_prefix, + legacy_endpoints_id, +) @pytest.fixture -def app(storage_path: str) -> Flask: +def app(storage_path: str) -> flask.Flask: """Session fixture to configure the Flask app.""" return init_app( additional_config={ @@ -36,7 +40,11 @@ def app(storage_path: str) -> Flask: @pytest.mark.usefixtures("_authz_full_access") +@pytest.mark.parametrize( + "app", [False, True], ids=legacy_endpoints_id, indirect=True +) def test_upload_request_with_x_forwarded_middleware( + app: flask.Flask, test_client: FlaskClient, ) -> None: """Test the ProxyFix middleware generates correct URLs if @@ -51,8 +59,10 @@ def test_upload_request_with_x_forwarded_middleware( json = cast(dict[str, Any], response.json) upload_action = json["objects"][0]["actions"]["upload"] href = upload_action["href"] - base_uri = "myorg/myrepo" - assert href == f"http://localhost/{base_uri}/objects/storage/12345678" + exp_uri_prefix = expected_uri_prefix(app, "myorg", "myrepo") + assert ( + href == f"http://localhost/{exp_uri_prefix}/objects/storage/12345678" + ) response = test_client.post( "/myorg/myrepo.git/info/lfs/objects/batch", @@ -71,5 +81,5 @@ def test_upload_request_with_x_forwarded_middleware( href = upload_action["href"] assert ( href - == f"https://mycompany.xyz:1234/lfs/{base_uri}/objects/storage/12345678" + == f"https://mycompany.xyz:1234/lfs/{exp_uri_prefix}/objects/storage/12345678" ) diff --git a/tests/transfer/test_basic_external_adapter.py b/tests/transfer/test_basic_external_adapter.py index ee3f636..cdcc206 100644 --- a/tests/transfer/test_basic_external_adapter.py +++ b/tests/transfer/test_basic_external_adapter.py @@ -2,11 +2,13 @@ from typing import Any from urllib.parse import urlencode +import flask import pytest from giftless.storage import ExternalStorage from giftless.storage.exc import ObjectNotFoundError from giftless.transfer import basic_external +from tests.helpers import expected_uri_prefix, legacy_endpoints_id def test_factory_returns_object() -> None: @@ -26,13 +28,17 @@ def test_factory_returns_object() -> None: @pytest.mark.usefixtures("app_context") -def test_upload_action_new_file() -> None: +@pytest.mark.parametrize( + "app", [False, True], ids=legacy_endpoints_id, indirect=True +) +def test_upload_action_new_file(app: flask.Flask) -> None: adapter = basic_external.factory( f"{__name__}:MockExternalStorageBackend", {}, 900, ) response = adapter.upload("myorg", "myrepo", "abcdef123456", 1234) + exp_uri_prefix = expected_uri_prefix(app, "myorg", "myrepo") assert response == { "oid": "abcdef123456", @@ -45,7 +51,7 @@ def test_upload_action_new_file() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", + "href": f"http://giftless.local/{exp_uri_prefix}/objects/storage/verify", "header": {}, "expires_in": 43200, }, @@ -54,13 +60,17 @@ def test_upload_action_new_file() -> None: @pytest.mark.usefixtures("app_context") -def test_upload_action_extras_are_passed() -> None: +@pytest.mark.parametrize( + "app", [False, True], ids=legacy_endpoints_id, indirect=True +) +def test_upload_action_extras_are_passed(app: flask.Flask) -> None: adapter = basic_external.factory( f"{__name__}:MockExternalStorageBackend", {}, 900 ) response = adapter.upload( "myorg", "myrepo", "abcdef123456", 1234, {"filename": "foo.csv"} ) + exp_uri_prefix = expected_uri_prefix(app, "myorg", "myrepo") assert response == { "oid": "abcdef123456", @@ -73,7 +83,7 @@ def test_upload_action_extras_are_passed() -> None: "expires_in": 900, }, "verify": { - "href": "http://giftless.local/myorg/myrepo/objects/storage/verify", + "href": f"http://giftless.local/{exp_uri_prefix}/objects/storage/verify", "header": {}, "expires_in": 43200, }, From 07839040fb0b8cbf3408dd7b0d0a0412f2ddf056 Mon Sep 17 00:00:00 2001 From: Vit Zikmund Date: Fri, 8 Mar 2024 14:23:33 +0100 Subject: [PATCH 4/4] chore(endpoint): ignore the future warning --- pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index ad08f26..0182cbc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -126,6 +126,8 @@ filterwarnings = [ "ignore:.*pkg_resources\\.declare_namespace:DeprecationWarning", # Bug in kopf "ignore:.*require all values to be sortable:DeprecationWarning:kopf.*", + # Our warnings towards users + "ignore::FutureWarning", ] # The python_files setting is not for test detection (pytest will pick up any # test files named *_test.py without this setting) but to enable special