feat: auth providers #1085

g-linville · 2024-12-31T18:56:08Z

This PR is a substantial refactoring of our auth providers so that they are now brought in as external tools rather than being built into the gateway. We also now allow multiple auth providers to be configured at the same time.

There is a known bug in the frontend where you have to refresh the page after configuring the auth provider in order to see the change. It's taken me long enough to get this PR ready, so I figured it would be best to fix that in a follow up (or have Ryan or Ivy figure it out for me, lol). Fixed. Thanks, Ivy!

I will also do another follow-up PR to support deconfiguring auth providers. Currently, this is unimplemented in the backend.

ui/admin/app/components/auth-and-model-providers/ProviderForm.tsx

ivyjeong13 · 2025-01-16T19:21:42Z

ui/admin/app/components/auth-and-model-providers/ProviderForm.tsx

+		value: parameters[optionalParameterKey] ?? "",
+	}));
+
+export function ProviderForm({


nit: Wondering we could break the model provider / auth provider logic into a parent component and pass that in as props to ProviderForm, ProviderConfigure, ProviderDeconfigure.

Thinking something like:

// called in auth_providers.tsx specific context <ProviderForm onConfigure={configureAuthProvider} loading={configureAuthProvider.isLoading || isLoading} tooltips={AuthProviderRequiredTooltips} /> // called in model_providers.tsx specific context <ProviderForm onConfigure={configureModelProvider} loading={configureModelProvider.isLoading || isLoading} tooltips={ModelProviderRequiredTooltips} />

Then when you have to call them, you're not worried about handling the other provider type cases (or if something else uses this in the future).

If it's alright, I'd like for this to be in a follow-up.

ryanhopperlowe · 2025-01-16T21:47:39Z

I'm being prompted to setup an auth provider, but I don't see any to pick from

ryanhopperlowe · 2025-01-16T21:49:08Z

ui/admin/app/components/auth-and-model-providers/Bootstrap.tsx

+	const handleSubmit = async (event: React.FormEvent) => {
+		event.preventDefault();
+		try {
+			const result = await fetch(ApiRoutes.bootstrap.login().url, {
+				method: "POST",
+				headers: {
+					Authorization: `Bearer ${token}`,
+				},
+			});
+
+			if (result.status === 401) {
+				setError("Invalid token");
+				return;
+			} else if (result.status !== 200) {
+				setError("Failed to login: " + result.statusText);
+				return;
+			}
+
+			setError("");
+			window.location.href = "/admin/auth-providers";
+		} catch (e) {
+			setError("Failed to login: " + e);
+		}
+	};


Let's Create a BootstrapService for all of these methods

Sounds good. If it's okay, I'd like for that to be a follow-up refinement. For this PR, the UI just needs to work. Non-optimal things about the code can be fixed afterward.

ryanhopperlowe · 2025-01-16T22:37:44Z

ui/admin/app/components/auth-and-model-providers/ProviderDeconfigure.tsx

+	);
+}
+
+function warningMessage(t: string | undefined): string | undefined {


nit:

Suggested change

function warningMessage(t: string | undefined): string | undefined {

function warningMessage(t?: string) {

ryanhopperlowe · 2025-01-16T22:41:35Z

ui/admin/app/components/auth-and-model-providers/ProviderIcon.tsx

+					provider.id !== CommonModelProviderIds.AZURE_OPENAI &&
+					provider.id !== CommonAuthProviderIds.GOOGLE &&
+					provider.id !== CommonAuthProviderIds.GITHUB,


nit:

Suggested change

provider.id !== CommonModelProviderIds.AZURE_OPENAI &&

provider.id !== CommonAuthProviderIds.GOOGLE &&

provider.id !== CommonAuthProviderIds.GITHUB,

![CommonModelProviderIds.AZURE_OPENAI, CommonAuthProviderIds.GOOGLE, CommonAuthProviderIds.GITHUB].includes(provider.id)

ryanhopperlowe · 2025-01-16T23:12:08Z

ui/admin/app/lib/model/providers.ts

+export type ProviderStatus = {
 	configured: boolean;
-	modelsBackPopulated?: boolean;
 	icon?: string;
 	requiredConfigurationParameters?: string[];
+	optionalConfigurationParameters?: string[];
 	missingConfigurationParameters?: string[];
 };


These should follow a more True-to-life pattern:

type ProviderBase = {...} type ModelProviderBase = ProviderBase & {...} type ModelProvider = EntityMeta & ProviderBase & { type: 'modelprovider' } type AuthProviderBase = ProviderBase & {...} type AuthProvider = EntityMeta & AuthProviderBase & { type: 'authprovider' } type Provider = ModelProvider | AuthProvider

I'd like for this to be a follow-up improvement.

ryanhopperlowe · 2025-01-16T23:17:05Z

ui/admin/app/routes/_auth.tsx

+		case error instanceof AxiosError &&
+			[401, 403].includes(error.response?.status ?? 0):


@g-linville can you explain why you needed to add this? I'd like to make sure our first layer of error handling can catch this and convert it to the proper error type

I don't 100% remember but I think it had to do with the error we get back when we hit /api/me with my new changes to the backend.

g-linville · 2025-01-17T01:17:14Z

@ryanhopperlowe you need to download and build my branch of the tools repo from this PR: obot-platform/tools#320

And then set the OBOT_SERVER_TOOL_REGISTRY environment variable to point to your local copy of it when you run make dev. This will make the auth providers show up.

g-linville · 2025-01-17T02:17:55Z

Sorry for force pushing again. There were more conflicts that I needed to fix.

njhale

Some nits and suggestions, nothing that should hold this up.

pkg/accesstoken/accesstoken.go

njhale · 2025-01-16T18:40:06Z

pkg/api/handlers/authprovider.go

+	// Allow for updating credentials. The only way to update a credential is to delete the existing one and recreate it.
+	if err := ap.gptscript.DeleteCredential(req.Context(), string(ref.UID), ref.Name); err != nil && !strings.HasSuffix(err.Error(), "credential not found") {
+		return fmt.Errorf("failed to update credential: %w", err)
+	}
+
+	for key, val := range envVars {
+		if val == "" {
+			delete(envVars, key)
+		}
+	}
+
+	if err := ap.gptscript.CreateCredential(req.Context(), gptscript.Credential{
+		Context:  string(ref.UID),
+		ToolName: ref.Name,
+		Type:     gptscript.CredentialTypeTool,
+		Env:      envVars,
+	}); err != nil {
+		return fmt.Errorf("failed to create credential for auth provider %q: %w", ref.Name, err)
+	}


If the env hasn't changed this will still delete and recreate the credential and stop the provider even the env hasn't changed, right? Would it be better to check for changes to the credential first?

All I did here was copy the logic that we used for model providers. I think @thedadams may have been the one who implemented that, and would know the answer to this.

pkg/gateway/server/oauth.go

njhale · 2025-01-17T03:15:58Z

... And then set the OBOT_SERVER_TOOL_REGISTRY environment variable to point to your local copy of it when you run make dev. This will make the auth providers show up.

@g-linville @ryanhopperlowe this has recently changed due to #1193 merging.

Now, instead of setting OBOT_SERVER_TOOL_REGISTRY, you'll need set the GPTSCRIPT_TOOL_REMAP environment variable; e.g.

export GPTSCRIPT_TOOL_REMAP='github.com/obot-platform/tools=/Users/nick/projects/obot-platform/tools'

g-linville · 2025-01-17T15:04:48Z

@njhale @ryanhopperlowe for testing with this branch, the old env var should be used to set the registry, since I have not (yet) brought in those changes from main to this branch.

StrongMonkey · 2025-01-17T15:56:38Z

pkg/api/handlers/authprovider.go

+	var credEnvVars map[string]string
+	if ref.Status.Tool != nil {
+		if envVars := ref.Status.Tool.Metadata["envVars"]; envVars != "" {
+			fmt.Printf("revealing creds for auth provider %q\n", ref.Name)


Nit: this is debug log? if not should we use our standard log library to print?

Thanks for finding these! I forgot to remove them before. Pushed the fix

StrongMonkey · 2025-01-17T17:30:34Z

pkg/api/handlers/authprovider.go

+		return types.NewErrBadRequest("%q is not an auth provider", ref.Name)
+	}
+
+	fmt.Printf("revealing creds for auth provider %q\n", ref.Name)


StrongMonkey · 2025-01-17T17:35:17Z

pkg/bootstrap/bootstrap.go

+		token = fmt.Sprintf("%x", bytes)
+
+		// We deliberately only print the token if it was not provided by the user.
+		fmt.Printf("Bootstrap token: %s\n", token)


Do we print it so that user can grab it from the log? if so I would suggest the log to be more descriptive.

g-linville · 2025-01-20T14:30:09Z

Rebased again to fix conflicts.

thedadams · 2025-01-20T17:14:02Z

apiclient/types/authprovider.go

+
+type AuthProviderManifest struct {
+	Name          string `json:"name"`
+	Namespace     string `json:"namespace"`


This shouldn't be part of the manifest.

Are you sure? Name is part of the ModelProviderManifest. We need a namespace for auth providers, since I was told I can't assume that they will always be in default.

thedadams · 2025-01-20T17:25:13Z

pkg/api/router/router.go

@@ -36,6 +37,7 @@ func Router(services *services.Services) (http.Handler, error) {
 	sendgridWebhookHandler := sendgrid.NewInboundWebhookHandler(services.StorageClient, services.EmailServerName, services.SendgridWebhookUsername, services.SendgridWebhookPassword)

 	// Version
+


nit: remove line

thedadams · 2025-01-20T17:26:39Z

pkg/api/server/server.go

-			// If this is not a request coming from browser or the proxy is not enabled, then return 403.
-			if !isOAuthPath && (s.proxyServer == nil || req.Method != http.MethodGet || slices.Contains(user.GetGroups(), authz.AuthenticatedGroup) || !strings.Contains(strings.ToLower(req.UserAgent()), "mozilla")) {
+		if strings.HasPrefix(req.URL.Path, "/api/") {
+			if !s.authorizer.Authorize(req, user) {


This if-statement should not be nested.

If we bring it outside of if strings.HasPrefix(req.URL.Path, "/api/"), then we end up denying all unauthenticated requests to the user UI, which basically breaks everything. It needs to be nested in here.

If that is the case, then that should be fixed outside of this. This definitely should not be nested.

thedadams · 2025-01-20T17:28:50Z

pkg/bootstrap/bootstrap.go

+	return nil
+}
+
+func (b *Bootstrap) Logout(req api.Context) error {


Can we delete the bootstrap user from the database in this case? If it doesn't make sense to do here, can we do it at some point?

I'm okay with doing this, but I'm not sure that it's really necessary?

thedadams · 2025-01-20T17:37:32Z

pkg/gateway/server/dispatcher/dispatcher.go

 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 )

 type Dispatcher struct {
 	invoker    *invoke.Invoker
 	gptscript  *gptscript.GPTScript
 	client     kclient.Client
-	lock       *sync.RWMutex
-	urls       map[string]*url.URL
+	modelLock  *sync.RWMutex


nit: the dispatcher can be separated from the "transformer" and the dispatcher made generic with the implementation used for both model and auth providers.

I'd like for this to be a later change if it is needed.

I agree. Which is why it is a nit.

thedadams · 2025-01-20T17:51:44Z

pkg/proxy/proxy.go

+				return
+			}
+
+			http.Error(w, "no auth providers configured", http.StatusInternalServerError)


Should this be a bad request?

thedadams · 2025-01-20T17:54:49Z

pkg/proxy/proxy.go

+		URL:    req.URL.String(),
+		Header: make(map[string][]string),
+	}
+	for k, v := range req.Header {


sr.Header = req.Header

thedadams · 2025-01-20T17:57:34Z

pkg/services/config.go

 	EmailServerName            string   `usage:"The name of the email server to display for email receivers"`
 	Docker                     bool     `usage:"Enable Docker support" default:"false" env:"OBOT_DOCKER"`
 	EnvKeys                    []string `usage:"The environment keys to pass through to the GPTScript server" env:"OBOT_ENV_KEYS"`
+	KnowledgeSetIngestionLimit int      `usage:"The maximum number of files to ingest into a knowledge set" default:"3000" env:"OBOT_KNOWLEDGESET_INGESTION_LIMIT" name:"knowledge-set-ingestion-limit"`
+	NoReplyEmailAddress        string   `usage:"The email to use for no-reply emails from obot"`
+	DisableAuthentication      bool     `usage:"Disable authentication" default:"false" env:"OBOT_DISABLE_AUTHENTICATION"`


If one were to use this as a CLI flag, using --disable-authentication seems that it would actually enable authentication.

Good point. I'll invert this

thedadams · 2025-01-20T17:57:55Z

pkg/services/config.go

 	EmailServerName            string   `usage:"The name of the email server to display for email receivers"`
 	Docker                     bool     `usage:"Enable Docker support" default:"false" env:"OBOT_DOCKER"`
 	EnvKeys                    []string `usage:"The environment keys to pass through to the GPTScript server" env:"OBOT_ENV_KEYS"`
+	KnowledgeSetIngestionLimit int      `usage:"The maximum number of files to ingest into a knowledge set" default:"3000" env:"OBOT_KNOWLEDGESET_INGESTION_LIMIT" name:"knowledge-set-ingestion-limit"`
+	NoReplyEmailAddress        string   `usage:"The email to use for no-reply emails from obot"`


Is this related to your changes?

Nope. Might have been an accident when trying to resolve conflicts. Maybe this got added and removed at some point? I can take it away.

thedadams · 2025-01-20T17:58:30Z

pkg/services/config.go

@@ -23,6 +23,7 @@ import (
 	"github.com/obot-platform/obot/pkg/api/authn"
 	"github.com/obot-platform/obot/pkg/api/authz"
 	"github.com/obot-platform/obot/pkg/api/server"
+	bootstrap2 "github.com/obot-platform/obot/pkg/bootstrap"


nit: I don't think this import needs to be renamed.

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

Signed-off-by: Donnie Adams <[email protected]>

g-linville force-pushed the auth-provider-refactor branch 2 times, most recently from b52009b to 1192c32 Compare January 2, 2025 21:56

g-linville changed the title ~~WIP~~ WIP: auth providers Jan 7, 2025

g-linville force-pushed the auth-provider-refactor branch 2 times, most recently from 049c10a to 6d0ab66 Compare January 15, 2025 21:21

g-linville changed the title ~~WIP: auth providers~~ feat: auth providers Jan 16, 2025

g-linville force-pushed the auth-provider-refactor branch from 9c7b10d to 64f7594 Compare January 16, 2025 17:20

g-linville marked this pull request as ready for review January 16, 2025 17:20

g-linville requested review from drpebcak, thedadams, ivyjeong13, njhale, StrongMonkey, iwilltry42 and ryanhopperlowe January 16, 2025 17:28

g-linville force-pushed the auth-provider-refactor branch from 64f7594 to 0b4d773 Compare January 16, 2025 18:24

ivyjeong13 reviewed Jan 16, 2025

View reviewed changes

ui/admin/app/components/auth-and-model-providers/ProviderForm.tsx Show resolved Hide resolved

ivyjeong13 reviewed Jan 16, 2025

View reviewed changes

ryanhopperlowe reviewed Jan 16, 2025

View reviewed changes

g-linville requested a review from ryanhopperlowe January 17, 2025 01:21

g-linville force-pushed the auth-provider-refactor branch from 17e202c to 78654e2 Compare January 17, 2025 02:17

njhale approved these changes Jan 17, 2025

View reviewed changes

StrongMonkey reviewed Jan 17, 2025

View reviewed changes

g-linville requested a review from StrongMonkey January 17, 2025 18:30

g-linville mentioned this pull request Jan 17, 2025

Refactor auth providers into tools #1090

Open

g-linville force-pushed the auth-provider-refactor branch from 538836b to f5b843a Compare January 20, 2025 14:29

thedadams requested changes Jan 20, 2025

View reviewed changes

g-linville and others added 5 commits January 20, 2025 17:45

feat: auth providers

815fff6

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

fix lint error

5900128

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

PR feedback

b83eab2

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

fix authz

4c53bfe

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

detect when we should set a new cookie from the auth provider

c36a669

Signed-off-by: Donnie Adams <[email protected]>

thedadams force-pushed the auth-provider-refactor branch from 704e9b8 to c36a669 Compare January 20, 2025 22:46

add onMount import back in

ed378b0

Signed-off-by: Donnie Adams <[email protected]>

thedadams enabled auto-merge (squash) January 20, 2025 23:09

thedadams approved these changes Jan 20, 2025

View reviewed changes

thedadams merged commit a0c0635 into obot-platform:main Jan 20, 2025
3 checks passed

	function warningMessage(t: string \| undefined): string \| undefined {
	function warningMessage(t?: string) {

		case error instanceof AxiosError &&
		[401, 403].includes(error.response?.status ?? 0):

		@@ -36,6 +37,7 @@ func Router(services *services.Services) (http.Handler, error) {
		sendgridWebhookHandler := sendgrid.NewInboundWebhookHandler(services.StorageClient, services.EmailServerName, services.SendgridWebhookUsername, services.SendgridWebhookPassword)

		// Version

feat: auth providers #1085

feat: auth providers #1085

Conversation

g-linville commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhopperlowe commented Jan 16, 2025

ryanhopperlowe Jan 16, 2025 • edited Loading

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

ryanhopperlowe Jan 16, 2025 • edited Loading

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 17, 2025

g-linville commented Jan 17, 2025

njhale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-linville Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

njhale commented Jan 17, 2025

g-linville commented Jan 17, 2025

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 20, 2025

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 Jan 20, 2025 • edited Loading

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

Choose a reason for hiding this comment

g-linville commented Dec 31, 2024 •

edited

Loading

ryanhopperlowe Jan 16, 2025 •

edited

Loading

ryanhopperlowe Jan 16, 2025 •

edited

Loading

g-linville Jan 17, 2025 •

edited

Loading

g-linville Jan 20, 2025 •

edited

Loading