Eagerly check token validity and handle invalid tokens in flask helpers #205
+242
−154
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes evaluation eager and adds handling for potential errors to the FlaskAuthStateBuilder. Tests are updated minimally in order to pass in the updated paradigm.
Rather than using a mocked `AuthState` object, our preference is to mock HTTP with `responses` and use a real instance of the object. This leads to more effective testing. A discrepancy was revealed in one of the test fixtures in which the effective identity ID was not included in the list of all identities in the mocked token introspect data. `RegisteredResponse.replace()` is used to put default success mocks in place and replace them as necessary for specific tests.
sirosen
requested review from
ada-globus,
derek-globus,
jakeglobus,
kurtmckee and
MaxTueckeGlobus
as code owners
This was referenced Feb 7, 2025
Draft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset makes one primary breaking change and calls out another (more subtle) one as well.
The first and most notable change here is that
AuthStatenow callsAuthState.introspect_token()on init.This removes error behaviors which were previously lazy -- for example, the first access to
AuthState.effective_identitycould raise an error.Instead, users of
AuthStateshould anticipate that the token has already been validated.This does not guarantee that no calls to the
AuthStatewill do IO and result in errors -- notably the codepath which reaches out to Groups is intentionally left untouched here for two reasons:Secondarily, now that
AuthState(...)can raise an error, it is clearly possible to handle that gracefully in theFlaskAuthStateBuilder.That component now translates the generic errors which are used by
AuthStateinto an appropriate error for the flask components, which then gets handled and translated into anUnauthorizederror in the default handler.Because
FlaskAuthStateBuilderhas new and different error raising behavior, it is also noted as a breaking change.In the course of testing this, I also addressed some unnecessarily aggressive mocking in the testsuite, which resulted in a number of tests evaluating against mocks when it is perfectly feasible for them to evaluate against the real
AuthStateobject.NB: I assessed some other paths to resolving the issues with uncaught errors, like modifying the flask-blueprint error handler more extensively.
Although many such options are reasonable, my determination was that they do not solve the underlying issue, which is the ability of this object to make errors appear on property access at unpredictable times in the application lifecycle.