yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
MIT License
158 stars 21 forks source link

Add Sanity-Check to `TokenAuth.auth_flow` #100

Closed Bibo-Joshi closed 1 month ago

Bibo-Joshi commented 1 month ago

Hi,

I've been playing around with this nice lib a bit and had issues getting it to work with token authentication. After a bit of debugging I found out that the issue was that I had already added a "bearer" in front of my token. After removing it, everthing worked as expected. I suggest that the lib checks if a prexif is already present before adding it.

Best Hinrich

yanyongyu commented 1 month ago

I think this is an invalid use case since the token is directly generated from github but not the http header content with token or bearer prefix. They are different both in format and meaning. I think this problem is due to incorrect usage by the user, rather than a compatibility issue that the sdk needs to consider.

Bibo-Joshi commented 1 month ago

Hi, thanks for your reply. For completeness, let me share an MWE:

```python import asyncio from githubkit import GitHub, TokenAuthStrategy async def main(): gh = GitHub( base_url="https://api.github.com", auth=TokenAuthStrategy("bearer github_pat_abc"), ) async with gh: print(await gh.rest.repos.async_list_contributors(owner="octocat", repo="Hello-World")) if __name__ == "__main__": asyncio.run(main()) ```

This example gives me a 401 RequestFailed exception. I get a 200 OK response only if I remove the "bearer" from the token or if I apply the proposed change.

I can agree that there is a distinction between the plain token provided by GitHub and what needs to be written in the HTTP Header. However, this distinction may not be always present to people not working too much directly with HTTP headers (like me) and with the proposed change one would have a simple and rather non-intrusive convenience check around this. Indeed, I copied the "bearer" from another project where I was using the gql library for which I had to add to manually add it to the headers and the difference was not obvious to me in that moment.


In any case, feel free to close if you see fit, just wanted to pitch something that would have helped me :)

yanyongyu commented 1 month ago

Hi, thanks for your example. In my opinion, it's a better practice to raise an 401 unauthorized exception to user as this will remind the user that he provided an incorrect token.

If the user is not familiar with the meaning of HTTP status codes, i can add an additional http status reason to the response and exception. This may help users to quickly locate the problems.

yanyongyu commented 1 month ago

I have just add a simple http status reason phrase to the response and exception objects' repr. 401 response will show reason Unauthorized in the traceback now.