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

Add OAuthCredentialsStrategy #1558

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

theonlydoublee
Copy link

@theonlydoublee theonlydoublee commented Mar 24, 2023

Summary

Add OAuthCredentialsStrategy to hikari\impl\rest.py similar to ClientCredentialsStrategy 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

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

  • None

Copy link
Member

@davfsa davfsa left a 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!

hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
changes/1558.feature.md Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
Co-authored-by: davfsa <davfsa@gmail.com>
Signed-off-by: theonlydoublee <theonlydoublee@gmail.com>
@theonlydoublee
Copy link
Author

I ran into an issue with making the tests for OAuthCredentialsStrategy and the last tests I have done are getting the error TypeError: object Mock can't be used in 'await' expression. The code is the only tests that are erroring and all for that same error. Altered from ClientCredentialsStrategy

Code: https://pastebin.com/pdFk7Fxf

@Jonxslays
Copy link
Contributor

Use a mock.AsyncMock instead of mock.Mock

@theonlydoublee
Copy link
Author

Getting an error of pytest.PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object AsyncMockMixin._execute_mock_call at 0x00000267A82C7A70>

Full Nox Traceback: https://pastebin.com/f6qkWLkU
Failing Test Code: https://pastebin.com/MzV1D7BX
Updated Class: https://pastebin.com/TbHKr2Ad

@theonlydoublee theonlydoublee marked this pull request as ready for review March 24, 2023 21:30
@theonlydoublee
Copy link
Author

theonlydoublee commented Mar 24, 2023

Thought I fixed those typing errors for mypy. Will fix when back home

tests/hikari/impl/test_rest.py Outdated Show resolved Hide resolved
tests/hikari/impl/test_rest.py Outdated Show resolved Hide resolved
tests/hikari/impl/test_rest.py Outdated Show resolved Hide resolved
hikari/impl/rest.py Outdated Show resolved Hide resolved
- Moved `_is_expired` after properties
- Fixed mypy issues
- Removed spec usage for Mock on mock_token
- Moved strategy into a fixture
@theonlydoublee
Copy link
Author

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
@davfsa davfsa added the enhancement New feature or request label Mar 25, 2023
@davfsa davfsa changed the title Add UserCredentialsStrategy Add OAuthCredentialsStrategy Mar 29, 2023
@@ -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,
Copy link
Collaborator

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,
Copy link
Collaborator

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.

@FasterSpeeding
Copy link
Collaborator

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.

@theonlydoublee
Copy link
Author

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.

@davfsa
Copy link
Member

davfsa commented Apr 1, 2023

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.

Could that be fixed by just allowing passing the token and refresh token to the strategy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants