Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix asyncify for users client where token is not required #506

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

cgearing
Copy link
Contributor

@cgearing cgearing commented Jul 10, 2023

Changes

This PR fixes a bug that is currently in the Auth0 SDK asyncify wrapper where it falls through to the AsyncManagementClient when the class being wrapped doesn't inherit from the AuthenticationBase class. This doesn't work for the Users client, as that shouldn't require a management token.

Currently, using the Users client with the async wrapper fails with the following error:

TypeError: asyncify.<locals>.AsyncManagementClient.__init__() missing 1 required positional argument: 'token'

The approach I took was to implement a new BareAsyncClient for this case, however I'm not 100% wedded to this approach so feel free to tell me to have another go!

Testing

I've added a test for this functionality to check that we don't need to add the token when calling the Users client when wrapped in an async context.

  • This change adds integration test coverage

Checklist

@cgearing cgearing requested a review from a team as a code owner July 10, 2023 09:44
@@ -21,6 +22,17 @@ def asyncify(cls):
if callable(getattr(cls, func)) and not func.startswith("_")
]

class BareAsyncClient(cls):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix @cgearing! Would you mind just calling this UsersAsyncClient and I'd be happy to approve the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Sorry I missed your comment!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cgearing!

@adamjmcgrath adamjmcgrath merged commit c5131b6 into auth0:master Jul 24, 2023
4 checks passed
@adamjmcgrath adamjmcgrath mentioned this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants