-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Added OAuth JWT authorization support #8
Conversation
src/neptune_api/client.py
Outdated
@@ -220,11 +249,39 @@ def set_httpx_client(self, client: httpx.Client) -> "AuthenticatedClient": | |||
self._client = client | |||
return self | |||
|
|||
def get_auth_client(self) -> Client: |
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.
shouldn't this method return instance of AuthenticatedClient
? (fuction names implies it)
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.
Nope, this one doesn't care about authorization and it's used for the purpose of token exchange/refresh.
src/neptune_api/client.py
Outdated
@@ -182,34 +192,53 @@ class AuthenticatedClient: | |||
_follow_redirects: bool = field(default=False, kw_only=True, alias="follow_redirects") | |||
_httpx_args: Dict[str, Any] = field(factory=dict, kw_only=True, alias="httpx_args") | |||
_client: Optional[httpx.Client] = field(default=None, init=False) | |||
_auth_client: Optional[Client] = field(default=None, init=False) |
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.
what is the purpose of _auth_client
? Is it a not authorized client that is used to interact with parts of api that do not require any security tokens?
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.
For token refreshing purposes to prevent cycle over "making a request -> having to refresh token -> making a query to refresh token -> having to refresh token -> ...". I've renamed it to be more self-explaining 😉
) | ||
# TODO: Error handling | ||
data = response.json() | ||
# This should never happen, but just in case |
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.
if this should never happen maybe lets log it
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.
Sure, saved for later as we have to figure out how to introduce logger properly that could be controlled via main package.
No description provided.