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

feat: Create google sign in button and send id token with request #147

Merged
merged 16 commits into from
Jan 25, 2024

Conversation

duwenxin99
Copy link
Contributor

No description provided.

@duwenxin99 duwenxin99 requested a review from a team as a code owner December 20, 2023 23:31
@duwenxin99 duwenxin99 self-assigned this Dec 20, 2023
@duwenxin99 duwenxin99 force-pushed the user-session branch 2 times, most recently from 6f13acd to 4db666a Compare December 21, 2023 19:34
langchain_tools_demo/agent.py Outdated Show resolved Hide resolved
return aiohttp.ClientSession(
connector=await get_connector(),
headers=get_header(),
headers=get_header(user_id_token),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to refresh the user_id_token when it expires? If so, how is that handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the ID tokens ("Authorization" field for cloud run and "User-Id-Token" for user authentication) expire after one hour. I could add a refresher function to re-acquire the headers once every hour in a separate PR maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the session support dynamic credentials? If not we probably need to switch to grabbing the credentials on the fly

from markdown import markdown
from starlette.middleware.sessions import SessionMiddleware

from agent import init_agent, user_agents

GOOGLE_REDIRECT_URI = "http://localhost:8081"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs up be updated to be dynamic -- this changes when we are deployed to cloud run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set to use referer url

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this line

Comment on lines 58 to 59
if request.method == "GET":
request.session.clear() # Do not clear session after login
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should follow up on this -- it needs to be more intentional to reset history here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should add a new endpoint reset-- update the session data, removes history, and deletes agent under the hood

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in merged PR: #152

langchain_tools_demo/main.py Outdated Show resolved Hide resolved
langchain_tools_demo/main.py Show resolved Hide resolved
):
request.session.clear()
form_data = await request.form()
user_id_token = str(form_data.get("credential", ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be a required parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted default value and added check if credential exists

langchain_tools_demo/main.py Outdated Show resolved Hide resolved
data-client_id="548341735270-6qu1l8tttfuhmt7nbfb7a4q6j4hso2f7.apps.googleusercontent.com"
data-context="signin"
data-ux_mode="popup"
data-login_uri="http://localhost:8081/login/google"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be hardcoded to localhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to dynamic URL.

Comment on lines 85 to 91
if "http://" in BASE_URL:
headers = None
else:
# Append ID Token to make authenticated requests to Cloud Run services
headers = {
"Authorization": f"Bearer {get_id_token()}",
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if "http://" in BASE_URL:
headers = None
else:
# Append ID Token to make authenticated requests to Cloud Run services
headers = {
"Authorization": f"Bearer {get_id_token()}",
}
headers = {}
if "http://" not in BASE_URL:
# Append ID Token to make authenticated requests to Cloud Run services
headers["Authorization"] = f"Bearer {get_id_token()}",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified if else statement

from markdown import markdown
from starlette.middleware.sessions import SessionMiddleware

from agent import init_agent, user_agents

GOOGLE_REDIRECT_URI = "http://localhost:8081"
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this line

Comment on lines 91 to 92
else:
return RedirectResponse(url=GOOGLE_REDIRECT_URI)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete these two lines as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

Comment on lines 51 to 55
def get_id_token() -> str:
"""Helper method to generate ID tokens for authenticated requests"""
# Use Application Default Credentials on Cloud Run
if os.getenv("K_SERVICE"):
auth_req = google.auth.transport.requests.Request()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def get_id_token() -> str:
"""Helper method to generate ID tokens for authenticated requests"""
# Use Application Default Credentials on Cloud Run
if os.getenv("K_SERVICE"):
auth_req = google.auth.transport.requests.Request()
id_token = None # (token, expiry_time)
def get_id_token() -> str:
"""Helper method to generate ID tokens for authenticated requests"""
if id_token is None or id_token[1] <= time.now():
auth_req = google.auth.transport.requests.Request()
token = google.oauth2.id_token.fetch_id_token(auth_req, BASE_URL)
id_token = (token, datetime.now() + timedelta(minutes=55))

Copy link
Contributor Author

@duwenxin99 duwenxin99 Jan 23, 2024

Choose a reason for hiding this comment

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

We still need to use the shell command 'gcloud auth print-identity-tokenbecausegcloud auth login` does not set Application Default Credential for local development: https://www.googlecloudcommunity.com/gc/Serverless/fetch-id-token-or-equivalent-locally-with-gcloud-non-default/m-p/552409

@@ -76,7 +76,7 @@
python main.py
```

Note: for hot reloading of the app use: `uvicorn main:app --host 0.0.0.0 --reload`
Note: for hot reloading of the app use: `uvicorn main:app --host 0.0.0.0 --reload --port 8081`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we updating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little irrelevant to this PR. The instruction says to open the page localhost://8081 to view the assistant, but if the 8081 port isn't specified, uvicorn default runs on 8080 so 8081 would be empty.

Comment on lines 32 to 59
def get_id_token():
global CLOUD_RUN_CREDENTIALS

if os.getenv("K_SERVICE"):
auth_req = google.auth.transport.requests.Request()

if CLOUD_RUN_CREDENTIALS is None:
CLOUD_RUN_CREDENTIALS = google.oauth2.id_token.fetch_id_token_credentials(
BASE_URL, auth_req
)

if not CLOUD_RUN_CREDENTIALS.valid:
CLOUD_RUN_CREDENTIALS.refresh(auth_req)
else:
# Use gcloud credentials locally
import subprocess

return (
subprocess.run(
["gcloud", "auth", "print-identity-token"],
stdout=subprocess.PIPE,
check=True,
)
.stdout.strip()
.decode()
)

return CLOUD_RUN_CREDENTIALS.token
Copy link
Collaborator

Choose a reason for hiding this comment

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

Averi and I talked about this a bit and we think this is the right solution here:

Suggested change
def get_id_token():
global CLOUD_RUN_CREDENTIALS
if os.getenv("K_SERVICE"):
auth_req = google.auth.transport.requests.Request()
if CLOUD_RUN_CREDENTIALS is None:
CLOUD_RUN_CREDENTIALS = google.oauth2.id_token.fetch_id_token_credentials(
BASE_URL, auth_req
)
if not CLOUD_RUN_CREDENTIALS.valid:
CLOUD_RUN_CREDENTIALS.refresh(auth_req)
else:
# Use gcloud credentials locally
import subprocess
return (
subprocess.run(
["gcloud", "auth", "print-identity-token"],
stdout=subprocess.PIPE,
check=True,
)
.stdout.strip()
.decode()
)
return CLOUD_RUN_CREDENTIALS.token
def get_id_token():
global CREDENTIALS
if CREDENTIALS is None:
CREDENTIALS, _ = google.auth.default()
if not CREDENTIALS.valid:
CREDENTIALS.refresh(Request())
return CREDENTIALS.id_token

@duwenxin99 duwenxin99 merged commit c0a34a7 into main Jan 25, 2024
9 checks passed
@duwenxin99 duwenxin99 deleted the user-session branch January 25, 2024 16:50
Yuan325 pushed a commit that referenced this pull request Jul 1, 2024
🤖 I have created a release *beep* *boop*
---


## 1.0.0 (2024-06-27)


### Features

* Add "airports" dataset
([#8](#8))
([e566554](e566554))
* add "amenities" dataset
([#9](#9))
([74db3ff](74db3ff))
* add "flights" dataset
([#10](#10))
([eebd3ce](eebd3ce))
* add api endpoints for amenities
([#12](#12))
([bd1c411](bd1c411))
* add app_test cicd workflow
([#157](#157))
([51f0e51](51f0e51))
* add clean up instructions
([#99](#99))
([c9021b1](c9021b1))
* add cloud sql cloudbuild workflow
([#143](#143))
([3dd3444](3dd3444))
* add Cloud SQL MySQL as a datastore provider
([#415](#415))
([d3f43d7](d3f43d7))
* add cloudsql integration test
([#141](#141))
([764ffeb](764ffeb))
* add cloudsql provider
([#5](#5))
([32a063d](32a063d))
* add confirmation for ticket booking
([#407](#407))
([6987280](6987280))
* add cymbal air passenger policy
([#265](#265))
([199a67c](199a67c))
* add endpoint for policy
([#271](#271))
([397ca79](397ca79))
* Add Firestore provider
([#65](#65))
([0cbd8ed](0cbd8ed))
* add GET endpoint for airport
([#11](#11))
([d36fd29](d36fd29))
* add initial "flights" endpoints
([#17](#17))
([da374d9](da374d9))
* add initial readme
([#31](#31))
([cb6d68c](cb6d68c))
* add instructions file for cloudsql_postgres
([#93](#93))
([ca3d2c5](ca3d2c5))
* add instructions for alloydb
([#27](#27))
([a30d5d1](a30d5d1))
* add integration test for postgres
([#123](#123))
([0aeab54](0aeab54))
* add labels syncing config
([#40](#40))
([b27558f](b27558f))
* add mock for retrieval service test
([#126](#126))
([498c6d7](498c6d7))
* add orchestration interface
([#226](#226))
([573c04b](573c04b))
* Add Renovate Config
([#42](#42))
([40c221b](40c221b))
* add search airport tools to langchain agent
([#140](#140))
([94a5df3](94a5df3))
* add smoke test for function calling
([#199](#199))
([5eecedf](5eecedf))
* add test for database export
([#133](#133))
([b114186](b114186))
* Adding Spanner database provider for retrieval service.
([#316](#316))
([8990bc9](8990bc9))
* Create google sign in button and send id token with request
([#147](#147))
([c0a34a7](c0a34a7))
* Create individual user client session
([#137](#137))
([359e5d3](359e5d3))
* Create reset button to clear session cookies
([#152](#152))
([f008c04](f008c04))
* Create Tickets endpoints
([#160](#160))
([26ad5d9](26ad5d9))
* **doc:** Add Firestore setup instruction
([#87](#87))
([e6e6fb6](e6e6fb6))
* **docs:** add code of conduct
([#39](#39))
([0dee8dd](0dee8dd))
* **docs:** update docs
([#36](#36))
([e908e75](e908e75))
* **docs:** update gcloud track for deployment with VPC network
([#80](#80))
([84313c9](84313c9))
* implement signin signout
([#258](#258))
([88b6c83](88b6c83))
* increase font-size and make UI better
([#292](#292))
([c6dd0f6](c6dd0f6))
* inital prototype of extension
([a502f84](a502f84))
* minor UI improvements
([#275](#275))
([7e31ac3](7e31ac3))
* minor UI updates
([#279](#279))
([774703a](774703a))
* refactor demo frontend
([#15](#15))
([4245020](4245020))
* refresh UI
([#239](#239))
([0254ce8](0254ce8))
* replace October flights data
([#37](#37))
([c2c70a2](c2c70a2))
* Replace requests package with aiohttp
([#125](#125))
([4fc0d27](4fc0d27))
* update airport dataset and add search endpoint
([#14](#14))
([baabfe4](baabfe4))
* update amenity embedding to include amenity name
([#144](#144))
([3fe22ce](3fe22ce))
* update amenity_dataset with new hour columns
([#151](#151))
([160d8df](160d8df))
* update app in langchain demo
([#225](#225))
([7c463d9](7c463d9))
* update model to Gemini
([#158](#158))
([09959fd](09959fd))
* Update prompt and tools
([#34](#34))
([499f6a7](499f6a7))
* update service Dockerfile
([#19](#19))
([ea7edc7](ea7edc7))
* update to include user name in chat
([#249](#249))
([070b7e0](070b7e0))
* vertex ai function calling llm
([#188](#188))
([ab1aedc](ab1aedc))


### Bug Fixes

* add dependency
([#195](#195))
([65cc37c](65cc37c))
* add retrieval service service account role to doc
([#121](#121))
([8ea12c9](8ea12c9))
* **ci:** use script over args in all cloudbuild
([#307](#307))
([da8d3f6](da8d3f6))
* **ci:** use script over args in cloudbuild syntax
([#306](#306))
([b5aa667](b5aa667))
* Do not pass None values to session.get()
([#136](#136))
([5d9cbbc](5d9cbbc))
* **docs:** fixed broken links to demo apps
([#132](#132))
([4955cd0](4955cd0))
* **firestore:** Add ID to all documents in Firestore provider
([#94](#94))
([1a02328](1a02328))
* Fix closed connection issue on reset button
([#175](#175))
([21607ec](21607ec))
* Fix get_agent() uuid comparison
([#200](#200))
([bc68633](bc68633))
* fix the refresh icon hiding behind Google banner
([#247](#247))
([1d858ba](1d858ba))
* **langchain_tools_demo:** Add ID Token credential flow for GCE
([#198](#198))
([ed9b6c2](ed9b6c2))
* **langchain_tools_demo:** fix agent concurrency between restarts
([#194](#194))
([2584154](2584154))
* **langchain_tools_demo:** use relative paths for resources
([#192](#192))
([6f8ae0d](6f8ae0d))
* Make `clientId `optional in config.yml
([#207](#207))
([3914d8c](3914d8c))
* missing Client ID
([#196](#196))
([960f6b1](960f6b1))
* sign out when user token invalid
([#329](#329))
([2ec915b](2ec915b))
* strip query params in loginUri
([#425](#425))
([41f3bb7](41f3bb7))
* update AlloyDB instruction order
([#92](#92))
([334257c](334257c))
* update alloydb.md to remove extra trailing space
([#46](#46))
([7f97d33](7f97d33))
* update embeddings and pin model
([#124](#124))
([5842c08](5842c08))
* update renovate config
([#378](#378))
([2ec52ce](2ec52ce))
* update search airport tool
([#148](#148))
([d4b36e9](d4b36e9))
* Use idiomatic python for conditional
([#149](#149))
([18e5e9d](18e5e9d))
* use lazy refresh for AlloyDB and Cloud SQL Connector
([#429](#429))
([c73484d](c73484d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants