fix: actually fetch access token from session #370
Conversation
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.78%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
…ugh_access_token fix: actually fetch access token from session (Sourcery refactored)
…mmunity/supabase-py into j0/pass_through_access_token
Codecov ReportBase: 78.75% // Head: 78.88% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #370 +/- ##
===========================================
+ Coverage 78.75% 78.88% +0.13%
===========================================
Files 9 9
Lines 160 180 +20
===========================================
+ Hits 126 142 +16
- Misses 34 38 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| return { | ||
| "apiKey": self.supabase_key, | ||
| "Authorization": f"Bearer {self.supabase_key}", | ||
| "Authorization": f"Bearer {token}", |
There was a problem hiding this comment.
I think this line is executed only when the client is initialized, that is before sign-in. We need something that sets the header not only after sign-in but also every time the access token is refreshed. Maybe we can add a function like Client.ensure_fresh_session() that would get the token from auth.get_session() (which refreshes access token if needed) and then set the authorization header for all sub-components of the Client.
There was a problem hiding this comment.
Yup, I agree, there's a refresh that is needed. In the js lib there's a refresh triggered when the auth state changes. I don't quite have bandwidth to look into this outside of weekends but if anyone has bandwidth feel free to pick it up
There was a problem hiding this comment.
It feels natural for the correct token to be set when calling Client.auth.set_session(). Could the headers just be updated as a part of this workflow?
There was a problem hiding this comment.
If we use set_session() to set the headers, the typical workflow will be:
- sign_in
- set_session()
- call to retrieve data from the DB
// some time later, when the access token might have expired - set_session() again
- another call to retrieve data from the DB
Forcing people to call set_session() every time before DB calls seems redundant. Am I missing something?
We could change the implementation of Client.from_() to take care of token refresh and header, then the user can work in a more intuitive way.
- sign in
- call DB
|
Can we have someone approve this please so this can get merged |
|
Can someone with write access finalize this? @J0 @kamrik @anand2312 Would really appreciate it <3 |
|
Hey everyone, Sorry for the incommunicado - I don't think the changes in the PR actually fix the issue as there's still a need to properly implement the refresh of the I'm going to go ahead and close this PR for now. In hindsight, I should have done this earlier so sorry about this. For anyone who is looking for how to do access token verification for a Python backend you can consider taking a look at the good work done by StanGirard: QuivrHQ/quivr#144 |
Pass through access token
Aims to fix #58 and #185
TODO
_init_postgrest_client needs to be modified too