From 5fdfb60dcc1274f6f2835d61046cde8bae0b15f8 Mon Sep 17 00:00:00 2001 From: Derek Schlabach Date: Thu, 21 Nov 2024 13:24:28 -0600 Subject: [PATCH 1/3] Recreated failing test case in unit test --- tests/unit/globus_app/test_globus_app.py | 54 ++++++++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/tests/unit/globus_app/test_globus_app.py b/tests/unit/globus_app/test_globus_app.py index 867f23d7..4dfac0e8 100644 --- a/tests/unit/globus_app/test_globus_app.py +++ b/tests/unit/globus_app/test_globus_app.py @@ -15,6 +15,7 @@ GlobusAppConfig, NativeAppAuthClient, RefreshTokenAuthorizer, + TransferClient, UserApp, ) from globus_sdk._testing import load_response @@ -42,15 +43,20 @@ ) -def _mock_token_data_by_rs(): +def _mock_token_data_by_rs( + resource_server: str = "auth.globus.org", + scope: str = "openid", + refresh_token: str | None = "mock_refresh_token", + expiration_delta: int = 300, +): return { "auth.globus.org": TokenStorageData( - resource_server="auth.globus.org", + resource_server=resource_server, identity_id="mock_identity_id", - scope="openid", + scope=scope, access_token="mock_access_token", - refresh_token="mock_refresh_token", - expires_at_seconds=int(time.time() + 300), + refresh_token=refresh_token, + expires_at_seconds=int(time.time() + expiration_delta), token_type="Bearer", ) } @@ -434,13 +440,14 @@ def run_login_flow( raise CustomExitException("mock login attempt") -def test_user_app_expired_authorizer_triggers_login(): +def test_user_app_expired_token_triggers_login(): # Set up token data with an expired access token and no refresh token client_id = "mock_client_id" memory_storage = MemoryTokenStorage() - token_data = _mock_token_data_by_rs() - token_data["auth.globus.org"].expires_at_seconds = int(time.time() - 3600) - token_data["auth.globus.org"].refresh_token = None + token_data = _mock_token_data_by_rs( + refresh_token=None, + expiration_delta=-3600, # Expired by 1 hour + ) memory_storage.store_token_data_by_resource_server(token_data) login_flow_manager = RaisingLoginFlowManagerCounter() @@ -455,6 +462,35 @@ def test_user_app_expired_authorizer_triggers_login(): assert login_flow_manager.counter == 1 +@pytest.mark.xfail(reason="Identified Bug") +def test_client_app_expired_token_is_auto_resolved(): + """ + This test exercises ClientApp token grant behavior. + ClientApps may request updated tokens outside the normal token authorization flow. + """ + client_creds = { + "client_id": "mock_client_id", + "client_secret": "mock_client_secret", + } + meta = load_response("auth.oauth2_client_credentials_tokens").metadata + + memory_storage = MemoryTokenStorage() + token_data = _mock_token_data_by_rs( + resource_server=meta["resource_server"], + scope=meta["scope"], + refresh_token=None, + expiration_delta=-3600, # Expired by 1 hour + ) + memory_storage.store_token_data_by_resource_server(token_data) + + config = GlobusAppConfig(token_storage=memory_storage) + client_app = ClientApp("test-app", **client_creds, config=config) + + transfer = TransferClient(app=client_app, app_scopes=[Scope(meta["scope"])]) + + transfer.task_list() + + def test_client_app_get_authorizer(): client_id = "mock_client_id" client_secret = "mock_client_secret" From f00203533c83ec1debf0c009c5c7be5dc7869154 Mon Sep 17 00:00:00 2001 From: Derek Schlabach Date: Thu, 21 Nov 2024 13:35:35 -0600 Subject: [PATCH 2/3] Removed identity id validation from ClientApps --- ...309_derek_client_app_expired_token_bug.rst | 10 ++++ src/globus_sdk/globus_app/app.py | 56 +++++++------------ src/globus_sdk/globus_app/user_app.py | 22 +++++++- tests/unit/globus_app/test_globus_app.py | 5 +- 4 files changed, 56 insertions(+), 37 deletions(-) create mode 100644 changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst diff --git a/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst b/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst new file mode 100644 index 00000000..aa972c62 --- /dev/null +++ b/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst @@ -0,0 +1,10 @@ + +Changed +~~~~~~~ + +- Removed identity id validation from ``ClientApps``. (:pr:`NUMBER`) + +Fixed +~~~~~ + +- Fixed a bug that would cause ``ClientApp`` token refreshes to fail. (:pr:`NUMBER`) diff --git a/src/globus_sdk/globus_app/app.py b/src/globus_sdk/globus_app/app.py index 57147399..2aec97d9 100644 --- a/src/globus_sdk/globus_app/app.py +++ b/src/globus_sdk/globus_app/app.py @@ -14,7 +14,6 @@ ScopeRequirementsValidator, TokenStorage, TokenValidationError, - UnchangingIdentityIDValidator, ValidatingTokenStorage, ) @@ -92,7 +91,7 @@ def __init__( # create the requisite token storage for the app, with validation based on # the provided parameters - self.token_storage = _build_default_validating_token_storage( + self.token_storage = self._initialize_validating_token_storage( token_storage=self._token_storage, consent_client=consent_client, scope_requirements=self._scope_requirements, @@ -178,6 +177,26 @@ def _initialize_login_client( Initializes and returns an AuthLoginClient to be used in authorization requests. """ + def _initialize_validating_token_storage( + self, + token_storage: TokenStorage, + consent_client: AuthClient, + scope_requirements: t.Mapping[str, t.Sequence[Scope]], + ) -> ValidatingTokenStorage: + """ + Initializes the validating token storage for the app. + """ + validating_token_storage = ValidatingTokenStorage(token_storage) + + # construct ValidatingTokenStorage around the TokenStorage and + # our initial scope requirements + scope_validator = ScopeRequirementsValidator(scope_requirements, consent_client) + + # use validators to enforce invariants about scopes and identity ID + validating_token_storage.validators.append(scope_validator) + + return validating_token_storage + def _resolve_token_storage( self, app_name: str, client_id: UUIDLike, config: GlobusAppConfig ) -> TokenStorage: @@ -391,36 +410,3 @@ def scope_requirements(self) -> dict[str, list[Scope]]: """ # Scopes are mutable objects so we return a deepcopy return copy.deepcopy(self._scope_requirements) - - -def _build_default_validating_token_storage( - *, - token_storage: TokenStorage, - consent_client: AuthClient, - scope_requirements: t.Mapping[str, t.Sequence[Scope]], -) -> ValidatingTokenStorage: - """ - Given the appropriate configuration data, build the default - ValidatingTokenStorage for use within GlobusApp. - - :param token_storage: The token storage to wrap in a ValidatingTokenStorage. - :param consent_client: The app's internal AuthClient instance which is used - to fetch consent information. - :param scope_requirements: The scope requirements for the app. - """ - validating_token_storage = ValidatingTokenStorage(token_storage) - - # construct ValidatingTokenStorage around the TokenStorage and - # our initial scope requirements - scope_validator = ScopeRequirementsValidator(scope_requirements, consent_client) - identity_id_validator = UnchangingIdentityIDValidator() - - # use validators to enforce invariants about scopes and identity ID - validating_token_storage.validators.extend( - ( - scope_validator, - identity_id_validator, - ) - ) - - return validating_token_storage diff --git a/src/globus_sdk/globus_app/user_app.py b/src/globus_sdk/globus_app/user_app.py index 90e73c80..aef01121 100644 --- a/src/globus_sdk/globus_app/user_app.py +++ b/src/globus_sdk/globus_app/user_app.py @@ -3,15 +3,23 @@ import typing as t from globus_sdk import ( + AuthClient, AuthLoginClient, ConfidentialAppAuthClient, GlobusSDKUsageError, NativeAppAuthClient, + Scope, ) from globus_sdk._types import ScopeCollectionType, UUIDLike from globus_sdk.gare import GlobusAuthorizationParameters from globus_sdk.login_flows import CommandLineLoginFlowManager, LoginFlowManager -from globus_sdk.tokenstorage import HasRefreshTokensValidator, NotExpiredValidator +from globus_sdk.tokenstorage import ( + HasRefreshTokensValidator, + NotExpiredValidator, + TokenStorage, + UnchangingIdentityIDValidator, + ValidatingTokenStorage, +) from .app import GlobusApp from .authorizer_factory import ( @@ -136,6 +144,18 @@ def _initialize_login_client( environment=config.environment, ) + def _initialize_validating_token_storage( + self, + token_storage: TokenStorage, + consent_client: AuthClient, + scope_requirements: t.Mapping[str, t.Sequence[Scope]], + ) -> ValidatingTokenStorage: + validating_token_storage = super()._initialize_validating_token_storage( + token_storage, consent_client, scope_requirements + ) + validating_token_storage.validators.append(UnchangingIdentityIDValidator()) + return validating_token_storage + def _initialize_authorizer_factory(self) -> None: if self.config.request_refresh_tokens: self._authorizer_factory = RefreshTokenAuthorizerFactory( diff --git a/tests/unit/globus_app/test_globus_app.py b/tests/unit/globus_app/test_globus_app.py index 4dfac0e8..60db0950 100644 --- a/tests/unit/globus_app/test_globus_app.py +++ b/tests/unit/globus_app/test_globus_app.py @@ -462,7 +462,6 @@ def test_user_app_expired_token_triggers_login(): assert login_flow_manager.counter == 1 -@pytest.mark.xfail(reason="Identified Bug") def test_client_app_expired_token_is_auto_resolved(): """ This test exercises ClientApp token grant behavior. @@ -488,8 +487,12 @@ def test_client_app_expired_token_is_auto_resolved(): transfer = TransferClient(app=client_app, app_scopes=[Scope(meta["scope"])]) + load_response(transfer.task_list) transfer.task_list() + access_token = memory_storage.get_token_data(meta["resource_server"]).access_token + assert access_token == meta["access_token"] + def test_client_app_get_authorizer(): client_id = "mock_client_id" From f1cd2a239a3cd8f6874e199c1eff3ddaca1c2d0c Mon Sep 17 00:00:00 2001 From: Derek Schlabach Date: Thu, 21 Nov 2024 14:08:48 -0600 Subject: [PATCH 3/3] PR suggestions --- ...121_133309_derek_client_app_expired_token_bug.rst | 2 +- src/globus_sdk/globus_app/app.py | 2 +- tests/unit/globus_app/test_globus_app.py | 12 ++++++++---- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst b/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst index aa972c62..1d786951 100644 --- a/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst +++ b/changelog.d/20241121_133309_derek_client_app_expired_token_bug.rst @@ -2,7 +2,7 @@ Changed ~~~~~~~ -- Removed identity id validation from ``ClientApps``. (:pr:`NUMBER`) +- Removed identity ID consistency validation from ``ClientApp``. (:pr:`NUMBER`) Fixed ~~~~~ diff --git a/src/globus_sdk/globus_app/app.py b/src/globus_sdk/globus_app/app.py index 2aec97d9..5d0d65f1 100644 --- a/src/globus_sdk/globus_app/app.py +++ b/src/globus_sdk/globus_app/app.py @@ -192,7 +192,7 @@ def _initialize_validating_token_storage( # our initial scope requirements scope_validator = ScopeRequirementsValidator(scope_requirements, consent_client) - # use validators to enforce invariants about scopes and identity ID + # use validators to enforce invariants about scopes validating_token_storage.validators.append(scope_validator) return validating_token_storage diff --git a/tests/unit/globus_app/test_globus_app.py b/tests/unit/globus_app/test_globus_app.py index 60db0950..a9278da9 100644 --- a/tests/unit/globus_app/test_globus_app.py +++ b/tests/unit/globus_app/test_globus_app.py @@ -50,7 +50,7 @@ def _mock_token_data_by_rs( expiration_delta: int = 300, ): return { - "auth.globus.org": TokenStorageData( + resource_server: TokenStorageData( resource_server=resource_server, identity_id="mock_identity_id", scope=scope, @@ -486,12 +486,16 @@ def test_client_app_expired_token_is_auto_resolved(): client_app = ClientApp("test-app", **client_creds, config=config) transfer = TransferClient(app=client_app, app_scopes=[Scope(meta["scope"])]) - load_response(transfer.task_list) + + starting_token = memory_storage.get_token_data(meta["resource_server"]).access_token + assert starting_token == token_data[meta["resource_server"]].access_token + transfer.task_list() - access_token = memory_storage.get_token_data(meta["resource_server"]).access_token - assert access_token == meta["access_token"] + ending_token = memory_storage.get_token_data(meta["resource_server"]).access_token + assert starting_token != ending_token + assert ending_token == meta["access_token"] def test_client_app_get_authorizer():