-
Notifications
You must be signed in to change notification settings - Fork 726
Add helpers to handle OAuth in a FastAPI app #2684
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
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
thanks a lot @Wauplin for the detailed code comments and the test suite ❤️ i guess this will be ready to merge after adding the documentation and fixing the test. also, let's open a PR in Gradio to remove the duplicated code after the release!
The org's profile picture URL. OpenID Connect field. | ||
is_enterprise (`bool`): | ||
Whether the org is an enterprise org. Hugging Face field. | ||
can_pay (`Optional[bool]`, *optional*): |
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.
can_pay (`Optional[bool]`, *optional*): | |
can_pay (`bool`, *optional*): |
same for the other optional attributes in this dataclass and OAuthUserInfo
dataclass
from starlette.middleware.sessions import SessionMiddleware | ||
except ImportError as e: | ||
raise ImportError( | ||
"Cannot initialize OAuth to due a missing library. Please run `pip install huggingface_hub[oauth]` or add " |
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.
(nit)
"Cannot initialize OAuth to due a missing library. Please run `pip install huggingface_hub[oauth]` or add " | |
"Cannot initialize OAuth due to a missing library. Please run `pip install huggingface_hub[oauth]` or add " |
# Little hack for the tests to work | ||
# On staging, the redirect_url can be only "http://localhost:3000". Here I'm simply mocking the URL to work and | ||
# remove any query parameters from the URL. In the test after the Hub call, we will replace back the URL with the |
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.
👍
tests/test_oauth.py
Outdated
def test_get_mocked_oauth_info(): | ||
oauth_info = _get_mocked_oauth_info() |
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.
def test_get_mocked_oauth_info(): | |
oauth_info = _get_mocked_oauth_info() | |
def test_get_mocked_oauth_info(monkeypatch): | |
monkeypatch.setenv("HF_TOKEN", TOKEN) | |
oauth_info = _get_mocked_oauth_info() | |
This should fix the failing test. Alternatively, one can patch the get_token()
function
Hi @Wauplin @hanouticelina and team 👋, Thanks a lot for this great addition! 🙌 Would it be possible to fix the failing checks and resolve the conflicts so this can be merged? My optimal scenario would be to have this included in the next Let me know if there's anything I can do to help speed things up! 🚀 @Wauplin Also, one small note: I noticed the same_site="none" setting for the OAuth session cookie. Just wanted to check if that’s intentional. While it enables cross-site requests (which might be needed), it’s also the least secure option and must be used with Really appreciate all the work being done here. Thanks again! |
Hi @echatzikyriakidis, thanks for your feedback and glad to know you're interested in this feature! I would be keen to know what's your use case for HF+OAuth? Regarding the timeline, I have to admit that we lowered its priority since I first worked on it. Short term I won't be able to work on it (ongoing parental leave 🤗). That been said, I'd be very open to an external contribution if you're up for it! Would you like to open a PR on top of my PR to push this to the finish line? I just resolved the conflicts and hopefully it's a matter of documenting how to use it (+1 test to fix but can't remember why). If you already have a use case waiting for it, it'd be good to try it on your side as well (using |
Regarding app.add_middleware(
SessionMiddleware,
secret_key=hashlib.sha256(session_secret.encode()).hexdigest(),
same_site="none",
https_only=True,
) The problem is that we're using it for HF Spaces which are running under |
Hi @Wauplin, thanks so much for your reply and support!
At Medoid AI, we’ve built a new open-source project and we want to try publishing its demo as a HF Space. We’re using HF+OAuth to leverage users’ identities for rate limiting, since the app has a backend service and we need to manage usage. We also plan to release more open-source projects and demos with the same rate limiting setup on HF Spaces. So, not having this feature is currently a blocker for us when it comes to deploying demos with HF+OAuth.
First of all, wishing you all the best on this incredible journey of becoming a parent! I hope everything goes smoothly for you. Regarding the feature — were you the only one working on this? If others can help, is there a chance we could give it higher priority? I’m more than happy to check it, but I’m not sure what would be expected of me, and my time is also limited. I’m a bit concerned that if I take it on without the proper context or understanding of the process, it might actually cause more delays. Could you clarify how much effort is involved? Is this mainly a code review, documentation review, new code to be implemented, or something like integration or unit testing? Regarding its usage, I’ve been using it for the past two weeks in a Space (a Dockerized web app based on FastAPI) by adding it in the requirements.txt file like this:
It’s been working as expected, even with the latest commits from today after merging the main branch. However, I’m not sure if it will continue to work in the future, since we can’t lock the version with a PyPI package (this relates to HF_HUB_DISABLE_EXPERIMENTAL_WARNING). That’s why this is currently a blocker for us. Maybe we could go ahead and publish it, and just set a reminder to check for the official release so we can update the version then. Regarding the After performing thorough testing across the following dimensions:
I can say that it works smooth in the majority of the 32 test cases I ran. There are a few cases that I think are worth discussing, though I believe they are probably corner cases without security impact:
For us, all of the above, aren’t blockers for releasing the demo, and we can likely address them later — perhaps after the new PyPI version is released. Hope this information help. What can we do from here to finalize this? |
👋 Hello @Wauplin! I work with @echatzikyriakidis and thought I’d help push this across the finish line. First of all — congratulations, and wishing you all the best on your journey into parenthood! 🎉 Regarding the pull request, I’d love to know what you think would be most helpful at this stage. Would you prefer I focus on fixing the 5 failing tests, improving documentation, or is there something else you’d prioritize? You've done a great job so far and it is really hard to tell what’s left to polish! 😄 |
Hi @echatzikyriakidis and @thanosKivertzikidis, thanks for the kind words and for proposing your help to get this PR merged 🤗 Very glad to learn about your use case! It looks like you've already thoroughly tested the solution which makes me much more confident on the fact that it can be (easily-ish) used. To answer on what needs to be done precisely to get this finished, I've listed:
Does this make sense to you? If you are willing to work on it, could you open a new PR on top of mine pointing to the |
Hello @Wauplin! Also, apologies for the multiple commits on my fork (and their confusing commit messages). The frustrating part is that the tests pass locally without any issues, but things go south when they run on GitHub. I know it's cliché to say "it works on my machine," but... unfortunately, it really does. 😅 My intuition points to a library version mismatch or something along those lines. fastapi 0.115.12 If I forgot to include any information, please let me know |
* Fix mypy errors: use Union[int, str] and suppress add_middleware type issues * Suggested fix test_get_mocked_oauth_info for expired HF_TOKEN in test_oauth * formatting issue * add documentation for oauth * Relax regex in assertRaisesRegex to match full error message in HfHubHTTPError Changed regex from "Reference already exists" to r"Reference .* already exists" to account for "Reference 'refs/heads/branch-in' already exists" error message. * workflow files reset to the original settings * Update docs/source/en/package_reference/oauth.md * Update docs/source/en/package_reference/oauth.md --------- Co-authored-by: Lucain <[email protected]> Co-authored-by: Lucain <[email protected]>
Thanks a ton for finishing this up @thanosKivertzikidis ! 🤗 I've merged your PR so we are ready to finally get this to Looking forward to seeing your use case! |
@Wauplin @hanouticelina @thanosKivertzikidis Thank you all for your support on this, really excited to prepare a new PyPI release! Quick question: is the release process automated, scheduled, or manual? Now that the changes are in main, is there a way for us to trigger a release? Thanks again! |
Release is manual and only executed on our end. We have extra validations checks to perform (with other HF library) so we tend to do one release every few weeks. In the meantime, I would recommend to install from source until it's published. I can't promise when exactly it'll be . |
This PR imports the code to handle OAuth in gradio to
huggingface_hub
. Goal is to make it available for non-Gradio apps (say autotrain-advanced for instance @abhishekkrthakur).I also added tests to make sure the workflow still works correctly. Thanks @coyotte508 for huggingface/huggingface.js#1050 and advices :)
Missing parts:
I have flagged this feature as "experimental" to be able to introduce breaking changes in the future.
Note that for now, the integration only works for Spaces integrations but the same codebase could be tweaked to handle any kind of websites. That's lower priority though (the JS integration is much better for that).
Demo: https://huggingface.co/spaces/Wauplin/fastapi-oauth