-
Notifications
You must be signed in to change notification settings - Fork 2
Creates stateless OAuth2 Tumblr functions #37
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| from enum import StrEnum | ||
| from typing import Any, Iterable, Optional | ||
|
|
||
| from pardner.stateless import Scope | ||
| from pardner.stateless.base import ( | ||
| generic_construct_authorization_url, | ||
| generic_fetch_token, | ||
| ) | ||
| from pardner.verticals import Vertical | ||
|
|
||
|
|
||
| class URLs(StrEnum): | ||
| AuthorizationURL = 'https://www.tumblr.com/oauth2/authorize' | ||
| TokenURL = 'https://api.tumblr.com/v2/oauth2/token' | ||
|
|
||
|
|
||
| def scope_for_verticals(verticals: Iterable[Vertical]) -> set[str]: | ||
| # Tumblr only needs 'base' for read access requests | ||
| return {'base'} | ||
|
|
||
|
|
||
| def construct_authorization_url( | ||
| client_id: str, redirect_uri: str, scope: Scope = {'base'} | ||
| ) -> tuple[str, str]: | ||
| """ | ||
| Builds the authorization URL and state for Tumblr. | ||
|
|
||
| :param client_id: Client identifier given by the OAuth provider upon registration. | ||
| :param redirect_uri: The registered callback URI. | ||
| :param scope: The scope of the access request. These may be any string but are | ||
| commonly URIs or various categories such as ``videos`` or ``documents``. | ||
|
|
||
| :returns: the authorization URL and state, respectively. | ||
| """ | ||
| return generic_construct_authorization_url( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do a lot of these services wouldn't it be clean to use class inheritance to automatically inherit the construct_authorization_url, and then wrap it and call super()... if the method even needs to be overridden? This is just meant as food for thought, one often wants to do things 3 times before abstracting or generalizing... we might want to see how the data methods interact before rearchictecting
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what you're suggesting is more or less how I set things up in the "stateful" model (e.g., Maybe it's not worth creating stateless versions of those methods after all and just use the classes + methods that already exists for the stateless case as well. If we are using classes even in the stateless mode, the work is pretty much already done (see below)! The reason I was hesitant to use classes for the stateless case is that the What I propose: revert #34, close this PR, and use the existing classes (BaseTransferService and TumblrTransferService) for the stateless use case. It reduces code duplication and achieves the same thing in a slightly different way as the functions in this PR and #34 . UX with classesinitiate oauth2 from pardner.services import TumblrTransferService
tumblr = TumblrTransferService('client_id', 'client_secret', 'https://redirect.com', [Vertical.FeedPost])
auth_url, state = tumblr.authorization_url()
# forward user to auth_urlcallback url from pardner.services import TumblrTransferService
# need to create new instance because the other one is in a completely different scope
tumblr = TumblrTransferService('client_id', 'client_secret', 'https://redirect.com', [Vertical.FeedPost])
token = tumblr.authorization_url(code = '39040239402')UX without classesinitiate oauth2 from pardner.stateless.tumblr import construct_authorization_url
auth_url, state = construct_authorization_url('client_id', 'https://redirect.com', scope = {'base'))
# forward user to auth_urlcallback url from pardner.stateless.tumblr import fetch_token
token = fetch_token('client_id', 'https://redirect.com', client_secret = 'client_secret', code = '39040239402')
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is very good and I love the thinking - but also I think about continuing it just a little further, and calling the methods to fetch comments, or fetch block list Editing to pull the tumblr context out of storage since this gets triggered AFTER initial setup... and I realize I might not be thinking enough about what parts of this happen in different segments of the process separated by different triggers and server contexts. We might not want the OAuth setup that gets triggered by the first click of the "donate my data" button, to be the same object that gets triggered when the callback URL receives a go-ahead token. vs
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're on the same page about definiting the data-specific methods; what you've laid out is pretty much exactly what I was thinking of doing!
|
||
| URLs.AuthorizationURL, client_id, redirect_uri, scope | ||
| ) | ||
|
|
||
|
|
||
| def fetch_token( | ||
| client_id: str, | ||
| redirect_uri: str, | ||
| authorization_response: Optional[str] = None, | ||
| client_secret: Optional[str] = None, | ||
| code: Optional[str] = None, | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Makes a request to Tumblr's resource server to obtain the access token. | ||
|
|
||
| One of either `code` or `authorization_response` must not be None. | ||
|
|
||
| :param client_id: Client identifier given by the OAuth provider upon registration. | ||
| :param redirect_uri: The registered callback URI. | ||
| :param scope: The scope of the access request. These may be any string but are | ||
| commonly URIs or various categories such as ``videos`` or ``documents``. | ||
| :param authorization_response: the URL (with parameters) the end-user's browser | ||
| redirected to after authorization. | ||
| :param client_secret: The `client_secret` paired to the `client_id`. | ||
| :param code: Authorization code (used by WebApplicationClients). | ||
|
|
||
| :returns: the authorization URL and state, respectively. | ||
| """ | ||
| return generic_fetch_token( | ||
| client_id, | ||
| redirect_uri, | ||
| {'base'}, | ||
| URLs.TokenURL, | ||
| authorization_response, | ||
| client_secret, | ||
| code, | ||
| include_client_id=True, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| from pardner.stateless.tumblr import ( | ||
| URLs, | ||
| construct_authorization_url, | ||
| fetch_token, | ||
| scope_for_verticals, | ||
| ) | ||
| from pardner.verticals import Vertical | ||
| from tests.conftest import get_url_params | ||
|
|
||
|
|
||
| def test_scope_for_verticals(): | ||
| assert scope_for_verticals({Vertical.FeedPost}) == {'base'} | ||
|
|
||
|
|
||
| def test_construct_authorization_url(): | ||
| auth_url, state = construct_authorization_url( | ||
| 'fake_client_id', 'https://redirect_uri' | ||
| ) | ||
| assert auth_url.startswith(URLs.AuthorizationURL) | ||
|
|
||
| auth_url_params = get_url_params(auth_url) | ||
|
|
||
| assert 'client_id' in auth_url_params | ||
| assert auth_url_params['client_id'] == 'fake_client_id' | ||
| assert 'redirect_uri' in auth_url_params | ||
| assert auth_url_params['redirect_uri'] == 'https://redirect_uri' | ||
| assert 'state' in auth_url_params | ||
| assert auth_url_params['state'] == state | ||
| assert 'scope' in auth_url_params | ||
| assert 'base' in auth_url_params['scope'] | ||
|
|
||
|
|
||
| def test_fetch_token_with_code(mock_outbound_requests): | ||
| mock_oauth2session_request, mock_client_parse_request_body_response = ( | ||
| mock_outbound_requests | ||
| ) | ||
| fetch_token( | ||
| 'fake_client_id', | ||
| 'https://redirect_uri', | ||
| client_secret='fake client secret', | ||
| code='the_best_code', | ||
| ) | ||
| mock_oauth2session_request.assert_called_once() | ||
| mock_client_parse_request_body_response.assert_called_once() |
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.
It might get confusing if
stateless/andservices/are importing form each other, but I think it's fine as long as onlyservices/is pulling fromstateless/and not vice-versa.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.
got it that makes sense