feat: adding oauth for gemini #1007

damienrj · 2025-01-31T22:45:22Z

This enables use of Gemini with either an API token, and oauth by setting GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

crates/goose/src/providers/oauth.rs:345

The DEFAULT_REDIRECT_URL is hardcoded in the get_oauth_token_with_endpoints_async function. It should be passed as a parameter.

DEFAULT_REDIRECT_URL.to_string(),

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

crates/goose/src/providers/oauth.rs:325

[nitpick] The get_oauth_token_with_endpoints_async function duplicates logic from get_oauth_token_async. Refactor to avoid code duplication.

pub async fn get_oauth_token_with_endpoints_async(

crates/goose/src/providers/oauth.rs

salman1993 · 2025-02-02T16:13:52Z

the CLI is working for me but the behaviour is surprising. i did not see the OAuth consent screen. i did double check that GOOGLE_API_KEY wasn't set in env var or the keyring so i am not sure how it was still working. is that expected?

the frontend currently requires the API Key though:

goose/ui/desktop/src/components/settings/models/hardcoded_stuff.tsx

Line 71 in 0ca4cf0

Google: ['GOOGLE_API_KEY'],
goose/crates/goose-server/src/routes/providers_and_keys.json

Line 24 in 0ca4cf0

"required_keys": ["GOOGLE_API_KEY"]

damienrj · 2025-02-02T18:04:41Z

the CLI is working for me but the behaviour is surprising. i did not see the OAuth consent screen. i did double check that GOOGLE_API_KEY wasn't set in env var or the keyring so i am not sure how it was still working. is that expected?

the frontend currently requires the API Key though:

goose/ui/desktop/src/components/settings/models/hardcoded_stuff.tsx

Line 71 in 0ca4cf0

Google: ['GOOGLE_API_KEY'],

goose/crates/goose-server/src/routes/providers_and_keys.json

Line 24 in 0ca4cf0

"required_keys": ["GOOGLE_API_KEY"]

That is a good point about the GUI. I definitely was getting the oauth screen with Googe with the credential I made. Then are then cached in '~/Library/Application Support/Goose/google/oauth' so maybe it is there if you authed earlier. I had some debugging strings in earlier that showed it was pulling the Auth. Worth putting back?

salman1993 · 2025-02-02T19:59:18Z

that'd be helpful. the oauth directory doesn't exist for me but very possible its cached somewhere else

❯ ll '~/Library/Application Support/Goose/google/oauth'
"~/Library/Application Support/Goose/google/oauth": No such file or directory (os error 2)

damienrj · 2025-02-03T07:23:53Z

that'd be helpful. the oauth directory doesn't exist for me but very possible its cached somewhere else
❯ ll '~/Library/Application Support/Goose/google/oauth'
"~/Library/Application Support/Goose/google/oauth": No such file or directory (os error 2)

Improved the error message

Failed to create provider: Authentication configuration missing. Please set either GOOGLE_API_KEY or both GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET

And in the generated log file it gives the path, and how long it will be valid.

  2025-02-03T07:05:03.636240Z  INFO goose::providers::oauth: Using cached OAuth token from /Users/damien/Library/Application Support/goose/google/oauth/aa6bd2d7e39124e3cae812c8e772dc8369e1581e0a737ab514660b7971084299.json valid until 2025-02-03 07:41:23.463162 UTC

This leaves goose configure to prompt for an API key. No sure on the best path for the configure and front end since there are now two options for one provider.

wendytang · 2025-02-03T18:22:21Z

crates/goose/src/providers/google.rs

@@ -136,6 +241,8 @@ impl Provider for GoogleProvider {
            vec![
                ConfigKey::new("GOOGLE_API_KEY", true, true, None),


How would the user be able to select oauth as an authentication method if the api key is required first?

Currently via environment variable, some design choices need to be made for how configure/UI work if a Provider has more than one option. So while that is determined left configure and UI to only use the API token.

wendytang

How have you been testing this?

I tried

forcing the oauth configuration and providing valid client id, client secret values -> was able to use gemini in a session
forcing the oauth configuration and providing invalid client id, client secret values -> was able to use gemini in a session
using the api key and providing invalid api key value -> was able to use gemini in a session

so it seems a little strange to me that gemini is usable despite providing invalid credentials

wendytang

I did not mean to approve

kalvinnchau · 2025-02-03T18:11:07Z

crates/goose/src/providers/google.rs

+
+    async fn ensure_auth_header(&self) -> Result<String, ProviderError> {
+        match &self.auth {
+            GoogleAuth::ApiKey(key) => Ok(format!("Bearer {}", key)),


Do we need to format this into a Bearer {} type token if it is only used in the url params key={api_key} ?

I can give that a try, went through a few iterations before I got it working.

crates/goose/src/providers/google.rs

kalvinnchau · 2025-02-03T18:22:30Z

crates/goose/src/providers/oauth.rs


-        fs::create_dir_all(get_base_path()).unwrap();
-        let cache_path = get_base_path().join(format!("{}.json", hash));
+    let base_path = dirs::config_dir()


we need to do some consolidation around our config dir setup (which is out of scope of this PR), but we actually use ~/.config even on macos
dirs::config_dir() would drop us in $HOME/Library/Application Support

we should stick with the hard coded ~/.config` directory within this pr

Sure, can put it in ~/.config

kalvinnchau · 2025-02-03T18:23:26Z

crates/goose/src/providers/oauth.rs

-        let cache_path = get_base_path().join(format!("{}.json", hash));
+    let base_path = dirs::config_dir()
+        .unwrap_or_else(|| PathBuf::from("."))
+        .join("goose/google/oauth");


this would drop databricks oauth tokens in to the google directory, we'll need to change that to either be generic or pass in some other named parameter to distinguish where any specific oauth implementation will go

kalvinnchau · 2025-02-03T18:30:07Z

crates/goose/src/providers/oauth.rs

    let flow = OAuthFlow::new(
        endpoints,
        client_id.to_string(),
+        client_id.to_string(),


this would break any flows using fn get_oauth_token_async?, which i believe breaks the databricks provider. it would just pass in client_id twice, one as the client_secret even if not needed.

also for any callers of this, if it's a public client it wouldn't need the client_secret

Yeah, not sure. I will see about splitting this out.

kalvinnchau · 2025-02-03T18:36:11Z

crates/goose/Cargo.toml

@@ -60,6 +60,7 @@ serde_yaml = "0.9.34"
 once_cell = "1.20.2"
 dirs = "6.0.0"
 rand = "0.8.5"
+oauth2 = { version = "4.4", features = ["reqwest"] }


not super familiar with this library, we use https://crates.io/crates/yup-oauth2 in the goose-mcp crate, do you know what distinctions there are?

some larger questions around the oauth flow:

whats the expected duration of the token?

if short-lived will the changes automatically re-attempt an oauth flow if tokens are expired?

does/should this oauth2 crate and implementation handle refresh tokens?

Can see if it will also work with yup-oauth2, if that is already an existing library.

As to the flow, the token lasts an hour I believe. If the token is expired it will get a new token. Does the databrick token also expire?

Looks to be about the same for databricks, about an hour and then we do the same oauth local browser flow

Yeah we use yup-oauth2 but in a slightly different context, for google's Desktop client OAuth flow, but hopefully it does work

damienrj · 2025-02-03T19:22:58Z

How have you been testing this?

I tried

forcing the oauth configuration and providing valid client id, client secret values -> was able to use gemini in a session

forcing the oauth configuration and providing invalid client id, client secret values -> was able to use gemini in a session

using the api key and providing invalid api key value -> was able to use gemini in a session

so it seems a little strange to me that gemini is usable despite providing invalid credentials

Have been testing with different sets of environment variables GOOGLE_CLIENT_ID=... GOOGLE_CLIENT_SECRECT=..., or GOOGLE_API_KEY...

How have you been testing this?

I tried

forcing the oauth configuration and providing valid client id, client secret values -> was able to use gemini in a session

forcing the oauth configuration and providing invalid client id, client secret values -> was able to use gemini in a session

using the api key and providing invalid api key value -> was able to use gemini in a session

so it seems a little strange to me that gemini is usable despite providing invalid credentials

Maybe the fallback logic for the cached oauth credential needs updating since I think it isn't trying to use the new client_Id, and client_secret if the cached credentials are present. Maybe configure needs to delete the existing credential.

Co-authored-by: Kalvin C <kalvinnchau@users.noreply.github.com>

damienrj force-pushed the damien/gemini branch from ab23c56 to 35f64b6 Compare February 1, 2025 07:12

damienrj requested a review from Copilot February 1, 2025 07:13

Copilot AI reviewed Feb 1, 2025

View reviewed changes

damienrj force-pushed the damien/gemini branch 2 times, most recently from 196fafc to cb6f005 Compare February 1, 2025 07:25

damienrj marked this pull request as ready for review February 1, 2025 07:41

damienrj requested a review from Copilot February 1, 2025 08:16

Copilot AI reviewed Feb 1, 2025

View reviewed changes

crates/goose/src/providers/oauth.rs Outdated Show resolved Hide resolved

damienrj force-pushed the damien/gemini branch from cb6f005 to 759deab Compare February 1, 2025 08:24

feat: adding oauth for gemini

8adfbcd

damienrj force-pushed the damien/gemini branch from 759deab to 8adfbcd Compare February 1, 2025 08:33

salman1993 requested review from kalvinnchau, salman1993 and wendytang February 2, 2025 15:56

Add logging to log file says when are where oauth token is used

7516cdf

damienrj force-pushed the damien/gemini branch from c288b41 to 7516cdf Compare February 3, 2025 06:22

damienrj added 2 commits February 2, 2025 23:05

Add more clear error messages

d87a608

set api as required for configure

3228570

wendytang reviewed Feb 3, 2025

View reviewed changes

wendytang approved these changes Feb 3, 2025

View reviewed changes

wendytang reviewed Feb 3, 2025

View reviewed changes

kalvinnchau requested changes Feb 3, 2025

View reviewed changes

Update crates/goose/src/providers/google.rs

4e08422

Co-authored-by: Kalvin C <kalvinnchau@users.noreply.github.com>

damienrj closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding oauth for gemini #1007

feat: adding oauth for gemini #1007

damienrj commented Jan 31, 2025

Copilot AI left a comment

salman1993 commented Feb 2, 2025 •

edited

Loading

damienrj commented Feb 2, 2025

salman1993 commented Feb 2, 2025

damienrj commented Feb 3, 2025

wendytang Feb 3, 2025

damienrj Feb 3, 2025

wendytang left a comment

wendytang left a comment

kalvinnchau Feb 3, 2025

damienrj Feb 3, 2025

kalvinnchau Feb 3, 2025

damienrj Feb 3, 2025

kalvinnchau Feb 3, 2025

kalvinnchau Feb 3, 2025

damienrj Feb 3, 2025

kalvinnchau Feb 3, 2025

damienrj Feb 3, 2025

kalvinnchau Feb 3, 2025

damienrj commented Feb 3, 2025 •

edited

Loading

		@@ -136,6 +241,8 @@ impl Provider for GoogleProvider {
		vec![
		ConfigKey::new("GOOGLE_API_KEY", true, true, None),

feat: adding oauth for gemini #1007

feat: adding oauth for gemini #1007

Conversation

damienrj commented Jan 31, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

salman1993 commented Feb 2, 2025 • edited Loading

damienrj commented Feb 2, 2025

salman1993 commented Feb 2, 2025

damienrj commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendytang left a comment

Choose a reason for hiding this comment

wendytang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienrj commented Feb 3, 2025 • edited Loading

salman1993 commented Feb 2, 2025 •

edited

Loading

damienrj commented Feb 3, 2025 •

edited

Loading