-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add OAuthCredentialsStrategy #1558
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for having a go at this!
Testing should be similar to ClientAuthorizationStrategy
, but if you run into any issues, feel free to message me on Discord or posting it here and will help you out!
Co-authored-by: davfsa <[email protected]> Signed-off-by: theonlydoublee <[email protected]>
I ran into an issue with making the tests for |
Use a |
Getting an error of Full Nox Traceback: https://pastebin.com/f6qkWLkU |
Thought I fixed those typing errors for mypy. Will fix when back home |
- Moved `_is_expired` after properties - Fixed mypy issues - Removed spec usage for Mock on mock_token - Moved strategy into a fixture
Everything should be ready |
- Remove unnecessary lock in tests - Remove `scopes` argument (Discord doesn't even document it any more) - Fix typehint for refresh_token
@@ -2960,6 +2960,12 @@ async def authorize_access_token( | |||
) -> applications.OAuth2AuthorizationToken: | |||
"""Authorize an OAuth2 token using the authorize code grant type. | |||
|
|||
.. warning:: | |||
There is no way to ensure what scopes are granted in the token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's seemingly no reason to put this note here since there isn't a scopes
parameter? Also this change seems outside the scope of this PR
*, | ||
scopes: undefined.UndefinedOr[ | ||
typing.Sequence[typing.Union[applications.OAuth2Scope, str]] | ||
] = undefined.UNDEFINED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be removed. This is a part of the Oauth2 spec and Discord does use this field these days.
I don't know how I feel about this tbh. The reason this wasn't implemented in the first place was since it felt like having this implemented as a standard thing would just be encouraging bad behaviour (which is still the case). When handling Oauth2 tokens you'd really want to be storing the current version of the token (likely in a database) to persist it between bot restarts and only refreshing it after it's expired but this doesn't really allow for that. Also I'm not sure what the use case is for this. |
I was thinking of using it for a restapp for interacting with user's information. If this isn't used, I'll use it for my personal project without it being in hikari itself. |
Could that be fixed by just allowing passing the token and refresh token to the strategy? |
Summary
Add
OAuthCredentialsStrategy
tohikari\impl\rest.py
similar toClientCredentialsStrategy
to allow simple OAuth2 token generation and refreshing.I have used this code on my own project and it works from my testing.
I do not know how to make the tests for this except by copying from the ones for
ClientCredentialsStrategy
and I tried but I do not know how to use the mock stuff. I am also not sure which tests I will need. Don't be afraid to be harsh giving feedback on what to do or I did.Idea from: https://discord.com/channels/574921006817476608/1088176194874257428
Checklist
nox
and all the pipelines have passed.Related issues