From 872bf727a69c168f0799ab30b93b279f3bacbfc9 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Fri, 2 Aug 2024 14:35:08 -0500 Subject: [PATCH 1/2] Add UUID checking for add_app_data_access_scope In order to help users catch avoidable errors during login flows, this adds a validation check which confirms that the UUID given to `add_app_data_access_scope` is a valid UUID str. It also checks type -- guarding against `None` producing strings like `.../None/data_access` and similar type errors. The check is defined in the utils module for potential reuse, but no extra usage sites are added in this changeset. --- .../20240802_143010_sirosen_check_uuids.rst | 5 ++++ src/globus_sdk/services/transfer/client.py | 1 + src/globus_sdk/utils.py | 20 ++++++++++++++++ .../experimental/test_client_integration.py | 17 ++++++++----- tests/unit/test_utils.py | 24 +++++++++++++++++++ 5 files changed, 61 insertions(+), 6 deletions(-) create mode 100644 changelog.d/20240802_143010_sirosen_check_uuids.rst diff --git a/changelog.d/20240802_143010_sirosen_check_uuids.rst b/changelog.d/20240802_143010_sirosen_check_uuids.rst new file mode 100644 index 000000000..0b6486eea --- /dev/null +++ b/changelog.d/20240802_143010_sirosen_check_uuids.rst @@ -0,0 +1,5 @@ +Changed +~~~~~~~ + +- ``TransferClient.add_app_data_access_scope`` now raises an error if it is + given an invalid collection ID. (:pr:`NUMBER`) diff --git a/src/globus_sdk/services/transfer/client.py b/src/globus_sdk/services/transfer/client.py index aa2034d32..f53fc69fd 100644 --- a/src/globus_sdk/services/transfer/client.py +++ b/src/globus_sdk/services/transfer/client.py @@ -143,6 +143,7 @@ def add_app_data_access_scope(self, collection_id: UUIDLike) -> TransferClient: res = client.operation_ls(COLLECTION_ID) """ + utils.check_uuid(collection_id, name="collection_id") base_scope = Scope(TransferScopes.all) data_access_scope = Scope( GCSCollectionScopeBuilder(str(collection_id)).data_access, diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index b566f9b01..fce38066b 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -70,6 +70,26 @@ def b64str(s: str) -> str: return b64encode(s.encode("utf-8")).decode("utf-8") +def check_uuid(s: UUIDLike, *, name: str) -> t.Literal[True]: + """ + Raise an error if the input is not a UUID + + :raises TypeError: if the input is not a UUID|str + :raises ValueError: if the input is a non-UUID str + """ + if isinstance(s, uuid.UUID): + return True + elif not isinstance(s, str): + raise TypeError(f"'{name}' must be a UUID or str (value='{s}')") + + try: + uuid.UUID(s) + except ValueError as e: + raise ValueError(f"'{name}' must be a valid UUID (value='{s}')") from e + + return True + + def slash_join(a: str, b: str | None) -> str: """ Join a and b with a single slash, regardless of whether they already diff --git a/tests/unit/experimental/test_client_integration.py b/tests/unit/experimental/test_client_integration.py index d8abc3795..86b57f235 100644 --- a/tests/unit/experimental/test_client_integration.py +++ b/tests/unit/experimental/test_client_integration.py @@ -1,3 +1,5 @@ +import uuid + import pytest import globus_sdk @@ -44,22 +46,25 @@ def test_transfer_client_default_scopes(app): def test_transfer_client_add_app_data_access_scope(app): client = globus_sdk.TransferClient(app=app) - client.add_app_data_access_scope("collection_id") + collection_id = str(uuid.UUID(int=0)) + client.add_app_data_access_scope(collection_id) str_list = [str(s) for s in app.get_scope_requirements("transfer.api.globus.org")] - expected = "urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/collection_id/data_access]" # noqa + expected = f"urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/{collection_id}/data_access]" # noqa: E501 assert expected in str_list def test_transfer_client_add_app_data_access_scope_chaining(app): + collection_id_1 = str(uuid.UUID(int=1)) + collection_id_2 = str(uuid.UUID(int=2)) ( globus_sdk.TransferClient(app=app) - .add_app_data_access_scope("collection_id_1") - .add_app_data_access_scope("collection_id_2") + .add_app_data_access_scope(collection_id_1) + .add_app_data_access_scope(collection_id_2) ) str_list = [str(s) for s in app.get_scope_requirements("transfer.api.globus.org")] - expected_1 = "urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/collection_id_1/data_access]" # noqa - expected_2 = "urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/collection_id_2/data_access]" # noqa + expected_1 = f"urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/{collection_id_1}/data_access]" # noqa: E501 + expected_2 = f"urn:globus:auth:scope:transfer.api.globus.org:all[*https://auth.globus.org/scopes/{collection_id_2}/data_access]" # noqa: E501 assert expected_1 in str_list assert expected_2 in str_list diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 990332106..94e42a9c1 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -98,3 +98,27 @@ def y(self_or_cls): ) def test_safe_strseq_iter(value, expected_result): assert list(utils.safe_strseq_iter(value)) == expected_result + + +@pytest.mark.parametrize("value", (uuid.UUID(int=0), str(uuid.UUID(int=1)))) +def test_check_uuid_ok(value): + # no error and returns True + assert utils.check_uuid(value, name="foo") + + +@pytest.mark.parametrize("value", (str(uuid.UUID(int=0))[:-1], "")) +def test_check_uuid_fails_value(value): + with pytest.raises(ValueError, match="'foo' must be a valid UUID") as excinfo: + utils.check_uuid(value, name="foo") + + err = excinfo.value + assert f"value='{value}'" in str(err) + + +@pytest.mark.parametrize("value", (object(), None, ["bar"])) +def test_check_uuid_fails_type(value): + with pytest.raises(TypeError, match="'foo' must be a UUID or str") as excinfo: + utils.check_uuid(value, name="foo") + + err = excinfo.value + assert f"value='{value}'" in str(err) From b8370a4f14e7d1f0b96340e15ff751f44e12e2cf Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Fri, 2 Aug 2024 14:43:36 -0500 Subject: [PATCH 2/2] Oh, pylint, hush --- src/globus_sdk/utils.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index fce38066b..6c01cb0cf 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -74,8 +74,20 @@ def check_uuid(s: UUIDLike, *, name: str) -> t.Literal[True]: """ Raise an error if the input is not a UUID + :param s: the UUID|str value + :param name: the name for this value to use in error messages + :raises TypeError: if the input is not a UUID|str :raises ValueError: if the input is a non-UUID str + + Example usage: + + .. code-block:: python + + def frob_it(collection_id: UUIDLike) -> Frob: + utils.check_uuid(collection_id, name="collection_id") + return Frob(collection_id) + """ if isinstance(s, uuid.UUID): return True