Skip to content
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

Allow mounted Gradio apps to work with external / arbitrary authentication providers #7557

Merged
merged 24 commits into from Mar 5, 2024

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Feb 28, 2024

Final auth-related PR and after this, I'm working on other stuff! But I think this one is pretty cool as it allows mounted Gradio apps to work with e.g. Google OAuth, which is a long-requested feature.

Closes: #2790
Closes: #7005

Example running on Spaces: https://huggingface.co/spaces/gradio/oauth-example
Example running on AWS: http://35.85.61.147.nip.io/main/ (since this is HTTP, I will need to manually approve your email if you'd like to try. DM me if you would like that)

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 28, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook failed! Details
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/6a836240a67f47c827cae56c4a9c31799f1b836b/gradio-4.19.2-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@6a836240a67f47c827cae56c4a9c31799f1b836b#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 28, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Allow mounted Gradio apps to work with external / arbitrary authentication providers

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@abidlabs abidlabs marked this pull request as ready for review February 28, 2024 22:06
@abidlabs abidlabs mentioned this pull request Feb 28, 2024
@abidlabs
Copy link
Member Author

Also the recent Hub outage made me realize that we have more flaky tests than we realized. I've marked ~8 additional tests as flaky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted this test because its rather simplistic and makes our functional tests depend on the Hub and therefore flaky. The functionality that this test is testing can be sufficiently tested by a backend python test with a fastapi test client that makes a request to the api endpoint

@pievalentin
Copy link

I would happily test it in Azure AD once it get merged!

@abidlabs
Copy link
Member Author

abidlabs commented Mar 1, 2024

Thanks @pievalentin if you'd like, you can already test it by installing gradio like this:

pip install https://gradio-builds.s3.amazonaws.com/f78a94031344a956d40cbff23698188b8a900f9f/gradio-4.19.2-py3-none-any.whl

In fact, if you could test it, we'd appreciate it so that we can incorporate your feedback into the PR itself. If it helps, here's an example OAuth running on Spaces: https://huggingface.co/spaces/gradio/oauth-example

@pievalentin
Copy link

Sounds good! I have a lot on my plate at work but I think I will have time to try it this weekend. Looking at your example it looks straightforward :)

@aliabid94
Copy link
Collaborator

When I test it out using the spaces link, it opens a new tab for google auth, and then after login, the new tab becomes the logged in gradio app. Is this the expected flow?

@abidlabs
Copy link
Member Author

abidlabs commented Mar 5, 2024

Yes because of Spaces IFrames. In non-iframes it should work directly

@aliabid94
Copy link
Collaborator

Even when opening https://gradio-oauth-example.hf.space/main/ directly, this was the case.

@abidlabs
Copy link
Member Author

abidlabs commented Mar 5, 2024

Even when opening https://gradio-oauth-example.hf.space/main/ directly, this was the case.

Yes sorry I hardcoded this behavior to open in a new tab but I can change the AWS EC2 one to open in the same tab

@abidlabs
Copy link
Member Author

abidlabs commented Mar 5, 2024

@aliabid94 see here for an example opening in same page: http://35.85.61.147.nip.io/

Copy link
Collaborator

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @abidlabs ! I don't think passing both auth and auth_dependency makes sense. Should we disallow that? Can we consolidate auth and auth_dependency?

@abidlabs
Copy link
Member Author

abidlabs commented Mar 5, 2024

Thanks @freddyaboulton! Good point, I'll raise a ValueError if both are provided in order to not cause any unexpected security issues for users

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@@ -25,7 +25,7 @@ class DownloadButton(Component):
def __init__(
self,
label: str = "Download",
value: str | list[str] | Callable | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that was a mistake before, download button can only let you download a single file at a time

@abidlabs
Copy link
Member Author

abidlabs commented Mar 5, 2024

Thanks @aliabid94 and @freddyaboulton for the nice reviews!

@abidlabs abidlabs merged commit 4d5789e into main Mar 5, 2024
6 of 7 checks passed
@abidlabs abidlabs deleted the auth-dependency branch March 5, 2024 20:41
@pngwn pngwn mentioned this pull request Mar 5, 2024
@abidlabs abidlabs mentioned this pull request Mar 5, 2024

@app.route('/login')
async def login(request: Request):
redirect_uri = request.url_for('auth')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abidlabs maybe need to add to documentation for how to handle https:

from urllib.parse import urlparse, urlunparse

@app.route('/login')
async def login(request: Request):
    parsed_url = urlparse(str(request.url_for('auth')))
    modified_url = parsed_url._replace(scheme='https')
    redirect_uri = urlunparse(modified_url)
    return await oauth.google.authorize_redirect(request, redirect_uri)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's a good idea. I'll make a quick PR to add this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secure Gradio via FastAPI with external authentication Google Authentication
7 participants