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: add auth providers #320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

g-linville
Copy link
Collaborator

@g-linville g-linville commented Jan 7, 2025

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.

Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville changed the title add google auth provider feat: add auth providers Jan 8, 2025
@g-linville g-linville marked this pull request as ready for review January 8, 2025 15:29
Copy link
Contributor

@thedadams thedadams left a comment

Choose a reason for hiding this comment

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

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?

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines +148 to +150
for k, v := range sr.Header {
reqObj.Header[k] = v
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return
}

w.WriteHeader(http.StatusOK)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is written the first time we write to w.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider renaming this credential to something more descriptive?

"strings"
)

func LoadEnvForStruct[T any](s *T) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have only one of these?

return
}

w.WriteHeader(http.StatusOK)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch "server closed" error.

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.

3 participants