feat: add auth providers #320

g-linville · 2025-01-07T19:53:42Z

This PR adds auth provider tools for Google and GitHub. This is to replace the built-in auth providers that are currently in the Obot gateway code, but will soon be refactored to use these instead.

Once refactored, Obot will reverse proxy traffic to these daemon tools, which will then either respond on their own (for the /obot* paths) or handle it with the oauth2proxy.

The /obot-get-state path is used to get information about the logged in user. Obot will send over a serialized request, which gets reconstructed into an http.Request here in the auth provider, so that we can extract the state from the cookie using the oauth2proxy library.

thedadams

In addition to these comments, can we do a little more consolidation? Now that we are adding more Go-based model providers, there is an effort to combine as much of the common logic as is reasonable. Can we do the same thing here? Perhaps a common package at the top level?

thedadams · 2025-01-10T20:10:52Z

github-auth-provider/main.go

+	mux.HandleFunc("/", oauthProxy.ServeHTTP)
+
+	fmt.Printf("listening on 127.0.0.1:%s\n", port)
+	if err := http.ListenAndServe("127.0.0.1:"+port, mux); err != nil {


ListenAndServe always returns an error. I think we should catch the "server closed" error and not exit 1 in that case.

thedadams · 2025-01-10T20:12:54Z

github-auth-provider/main.go

+		for k, v := range sr.Header {
+			reqObj.Header[k] = v
+		}


Does something like this work? Maybe a cast is needed?

Suggested change

for k, v := range sr.Header {

reqObj.Header[k] = v

}

reqObj.Header = sr.Header

done. it works

thedadams · 2025-01-10T20:13:44Z

github-auth-provider/main.go

+		return
+	}
+
+	w.WriteHeader(http.StatusOK)


This is written the first time we write to w.

thedadams · 2025-01-10T20:15:33Z

github-auth-provider/tool.gpt

+Description: Auth provider for GitHub
+Metadata: envVars: OBOT_GITHUB_AUTH_PROVIDER_CLIENT_ID,OBOT_GITHUB_AUTH_PROVIDER_CLIENT_SECRET,OBOT_AUTH_PROVIDER_COOKIE_SECRET,OBOT_AUTH_PROVIDER_EMAIL_DOMAINS
+Metadata: optionalEnvVars: OBOT_GITHUB_AUTH_PROVIDER_TEAMS,OBOT_GITHUB_AUTH_PROVIDER_ORG,OBOT_GITHUB_AUTH_PROVIDER_REPO,OBOT_GITHUB_AUTH_PROVIDER_TOKEN,OBOT_GITHUB_AUTH_PROVIDER_ALLOW_USERS
+Credential: ../model-provider-credential as github-auth-provider


Should we consider renaming this credential to something more descriptive?

I renamed this to placeholder-credential

thedadams · 2025-01-10T20:16:04Z

google-auth-provider/pkg/env/env.go

+	"strings"
+)
+
+func LoadEnvForStruct[T any](s *T) error {


Can we have only one of these?

I moved this into a common package

thedadams · 2025-01-10T20:16:30Z

google-auth-provider/main.go

+		return
+	}
+
+	w.WriteHeader(http.StatusOK)


Same comment about this already being written when we write w.

thedadams · 2025-01-10T20:17:01Z

google-auth-provider/main.go

+	mux.HandleFunc("/", oauthProxy.ServeHTTP)
+
+	fmt.Printf("listening on 127.0.0.1:%s\n", port)
+	if err := http.ListenAndServe("127.0.0.1:"+port, mux); err != nil {


Catch "server closed" error.

g-linville · 2025-01-16T17:07:12Z

@thedadams I created a common package to hold code that would be used by all auth providers. For now, it only includes the env for struct stuff.

g-linville · 2025-01-16T17:09:43Z

Sorry for the force push. Needed to rebase to fix a conflict.

thedadams

All the model providers need this metadata field.

thedadams · 2025-01-17T22:23:50Z

github-auth-provider/tool.gpt

+Metadata: envVars: OBOT_GITHUB_AUTH_PROVIDER_CLIENT_ID,OBOT_GITHUB_AUTH_PROVIDER_CLIENT_SECRET,OBOT_AUTH_PROVIDER_COOKIE_SECRET,OBOT_AUTH_PROVIDER_EMAIL_DOMAINS
+Metadata: optionalEnvVars: OBOT_GITHUB_AUTH_PROVIDER_TEAMS,OBOT_GITHUB_AUTH_PROVIDER_ORG,OBOT_GITHUB_AUTH_PROVIDER_REPO,OBOT_GITHUB_AUTH_PROVIDER_TOKEN,OBOT_GITHUB_AUTH_PROVIDER_ALLOW_USERS
+Credential: ../placeholder-credential as github-auth-provider
+


Suggested change

Metadata: noUserAuth: github-auth-provider

thedadams · 2025-01-17T22:24:59Z

google-auth-provider/tool.gpt

+Description: Auth provider for Google
+Metadata: envVars: OBOT_GOOGLE_AUTH_PROVIDER_CLIENT_ID,OBOT_GOOGLE_AUTH_PROVIDER_CLIENT_SECRET,OBOT_AUTH_PROVIDER_COOKIE_SECRET,OBOT_AUTH_PROVIDER_EMAIL_DOMAINS
+Credential: ../placeholder-credential as google-auth-provider
+


Suggested change

Metadata: noUserAuth: google-auth-provider

Signed-off-by: Grant Linville <[email protected]>

g-linville · 2025-01-18T03:26:40Z

Force pushed to rebase on main and fix conflicts.

thedadams · 2025-01-18T18:47:44Z

@g-linville Did you mean to close this?

g-linville · 2025-01-20T14:22:38Z

Nope, lol. Not sure how I managed to close it.

thedadams

The newly added auth provider tools need the Metadata: noUserAuth directive added to their tools.

thedadams · 2025-01-20T15:29:19Z

google-auth-provider/main.go

+	Email             string     `json:"email"`
+}
+
+func obotGetState(p *oauth2proxy.OAuthProxy) http.HandlerFunc {


It would be great if this function could be imported from a shared package for all auth providers. This is very likely to be exactly the same for all auth providers.

I would be OK with this change being made when we add the next auth provider.

thedadams · 2025-01-20T15:29:42Z

google-auth-provider/main.go

+	}
+}
+
+func obotGetIconURL(w http.ResponseWriter, r *http.Request) {


Same comment here about importing this from a shared package.

Doesn't look like this was done.

Oh I didn't pay close enough attention to this one lol. Sorry. I would like to keep this out of the common package, since the function is a bit different for our current two providers.

Signed-off-by: Grant Linville <[email protected]>

g-linville changed the title ~~add google auth provider~~ feat: add auth providers Jan 8, 2025

g-linville marked this pull request as ready for review January 8, 2025 15:29

g-linville requested review from drpebcak, thedadams, njhale, StrongMonkey and iwilltry42 January 8, 2025 15:30

drpebcak approved these changes Jan 8, 2025

View reviewed changes

thedadams requested changes Jan 10, 2025

View reviewed changes

g-linville requested a review from thedadams January 16, 2025 17:05

g-linville force-pushed the auth-providers branch from efbb3c7 to 78621cc Compare January 16, 2025 17:09

This was referenced Jan 17, 2025

feat: auth providers obot-platform/obot#1085

Merged

Refactor auth providers into tools obot-platform/obot#1090

Open

thedadams requested changes Jan 17, 2025

View reviewed changes

feat: auth providers

c521325

Signed-off-by: Grant Linville <[email protected]>

g-linville force-pushed the auth-providers branch from 51ac9d5 to c521325 Compare January 18, 2025 03:26

g-linville closed this Jan 18, 2025

g-linville reopened this Jan 20, 2025

thedadams requested changes Jan 20, 2025

View reviewed changes

add to common package

4898755

Signed-off-by: Grant Linville <[email protected]>

g-linville requested a review from thedadams January 20, 2025 15:37

g-linville added 2 commits January 20, 2025 11:10

add metadata

89e338b

Signed-off-by: Grant Linville <[email protected]>

fix

4ac3840

Signed-off-by: Grant Linville <[email protected]>

thedadams approved these changes Jan 20, 2025

View reviewed changes

g-linville merged commit f97eedd into obot-platform:main Jan 20, 2025
1 check passed

g-linville deleted the auth-providers branch January 20, 2025 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add auth providers #320

feat: add auth providers #320

g-linville commented Jan 7, 2025 •

edited

Loading

thedadams left a comment

thedadams Jan 10, 2025

g-linville Jan 16, 2025

thedadams Jan 10, 2025

g-linville Jan 16, 2025

thedadams Jan 10, 2025

thedadams Jan 10, 2025

g-linville Jan 16, 2025

thedadams Jan 10, 2025

g-linville Jan 16, 2025

thedadams Jan 10, 2025

g-linville Jan 16, 2025

thedadams Jan 10, 2025

g-linville Jan 16, 2025

g-linville commented Jan 16, 2025

g-linville commented Jan 16, 2025

thedadams left a comment

thedadams Jan 17, 2025 •

edited

Loading

thedadams Jan 17, 2025

g-linville commented Jan 18, 2025

thedadams commented Jan 18, 2025

g-linville commented Jan 20, 2025

thedadams left a comment

thedadams Jan 20, 2025

g-linville Jan 20, 2025

thedadams Jan 20, 2025

g-linville Jan 20, 2025

thedadams Jan 20, 2025

g-linville Jan 20, 2025

feat: add auth providers #320

feat: add auth providers #320

Conversation

g-linville commented Jan 7, 2025 • edited Loading

thedadams 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-linville commented Jan 16, 2025

g-linville commented Jan 16, 2025

thedadams left a comment

Choose a reason for hiding this comment

thedadams Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-linville commented Jan 18, 2025

thedadams commented Jan 18, 2025

g-linville commented Jan 20, 2025

thedadams 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

g-linville commented Jan 7, 2025 •

edited

Loading

thedadams Jan 17, 2025 •

edited

Loading