APP-3031: make auth0.go generic to auth providers #216

ohEmily · 2023-10-25T19:23:56Z

(app PR that incorporates these changes)

Tiny PR that I believe (🤞) includes all the changes needed to auth0.go to make it so that we can cutover the app backend to FusionAuth without any further goutils changes.

I didn't rename auth0.go because it'll destroy the version history on this file if I do a squash and merge (only thing enabled for our repo afaik).

Tested that auth still works in app if I:
0) [local setup] add replace go.viam.com/utils => ../goutils and then go mod tidy

replace uses of Auth0Config with AuthProviderConfig in 2 places in server/server.go

diff --git a/server/server.go b/server/server.go
index 9b8a5643..29d00820 100644
--- a/server/server.go
+++ b/server/server.go
@@ -299,7 +299,7 @@ type simpleServer struct {
 	unauthenticatedMux    *goji.Mux
 	grpcServer            rpc.Server
 	auth0State            io.Closer
-	auth0Config           uweb.Auth0Config
+	auth0Config           uweb.AuthProviderConfig
 	webTokenKeyProvider   jwks.KeyProvider
 	jWKSKeyProvider       rpc.TokenVerificationKeyProvider
 	sessions              *uweb.SessionManager

and

@@ -640,7 +640,7 @@ func (ss *simpleServer) setupDependencies(ctx context.Context) error {
 		return errors.Wrap(err, "error unmarshaling auth0 data")
 	}

-	ss.auth0Config = uweb.Auth0Config{
+	ss.auth0Config = uweb.AuthProviderConfig{
 		Domain:     m["domain"],
 		ClientID:   m["clientId"],
 		Secret:     m["secret"],

I'll update goutils in app with the changes above once this is ready to merge.

mcous · 2023-10-25T20:28:44Z

Bringing in the council of Michaels on this one

ohEmily · 2023-10-25T21:02:08Z

One thing I'm not clear on is whether I broke the automatic call to Close() by futzing around with utilization of the io.Closer interface. Any ideas? Could produce some resource leaks.

michaellee1019

LGTM, some naming suggestions.

michaellee1019 · 2023-10-26T20:16:46Z

web/auth0.go

@@ -23,30 +23,43 @@ import (
 	"go.viam.com/utils"
 )

-// Auth0Config config for auth0.
-type Auth0Config struct {
+// AuthProviderConfig config options with constants that will probably need to be manually configured after


[nit] consider renaming this file to oidc.go?

I didn't want to rename as part of this PR as it would destroy the commit history on the squash and merge. I think I should do a git mv as a follow up

Fine with me!

web/auth0.go

mcous

LGTM

michaellee1019 · 2023-10-30T16:29:39Z

One thing I'm not clear on is whether I broke the automatic call to Close() by futzing around with utilization of the io.Closer interface. Any ideas? Could produce some resource leaks.

As long as the app side still calls Close() like it does here: https://github.com/viamrobotics/app/blob/main/server/server.go#L1691, then I don't think there is anything to be concerned about. Its just refactoring and the Close call will be the same.

ohEmily · 2023-11-13T17:19:41Z

Finally gearing up to merge this since we moved forward the testing and doesn't look like we'll need more app changes?

Dismissing due to ongoing changes, will re-review when ready

ohEmily · 2023-12-18T20:35:20Z

web/auth0.go

+	// http.SetCookie(w, &http.Cookie{
+	// 	Name:     h.redirectStateCookieName,
+	// 	Value:    "",
+	// 	Path:     "/",
+	// 	MaxAge:   -1,
+	// 	Secure:   r.TLS != nil,
+	// 	SameSite: http.SameSiteLaxMode,
+	// 	HttpOnly: true,
+	// })


[q] @DTCurrie, so this should be turned off for FusionAuth only? Or for both Auth0 and FusionAuth?

ohEmily · 2023-12-18T20:35:56Z

web/auth0.go

+	http.SetCookie(w, &http.Cookie{
+		Name:     "viam.auth.token",
+		Value:    token.AccessToken,
+		Path:     "/",
+		Expires:  token.Expiry,
+		Secure:   r.TLS != nil,
+		SameSite: http.SameSiteLaxMode,
+		HttpOnly: true,
+	})
+
+	http.SetCookie(w, &http.Cookie{
+		Name:     "viam.auth.refresh",
+		Value:    token.RefreshToken,
+		Path:     "/",
+		Expires:  token.Expiry,
+		Secure:   r.TLS != nil,
+		SameSite: http.SameSiteLaxMode,
+		HttpOnly: true,
+	})
+
+	http.SetCookie(w, &http.Cookie{
+		Name:     "viam.auth.expiry",
+		Value:    token.Expiry.Format(time.RFC3339),
+		Path:     "/",
+		Expires:  token.Expiry,
+		Secure:   r.TLS != nil,
+		SameSite: http.SameSiteLaxMode,
+		HttpOnly: true,
+	})


@DTCurrie same here -- is there any harm in this being sent for Auth0 as well as FusionAuth? Trying to split this out into different auth providers.

I didn’t actually comment out that first cookie originally, but so don’t think there is any harm in uncommenting it our now. It shouldn’t interfere with what I added.

The changes I made in app don’t really care about auth0 or fusion auth, so the added logic should conceivably work for both. Basically we set temporary cookies with the access token and it’s expiry, then on the front end we call the new token endpoint I added to return those values and clear the temp cookies. Then the front end caches those in local storage and uses them from there. If the expiry time has passed, it sends the user to log back in.

thanks for the lightning fast response!

APP-3031: make auth0.go generic to auth providers

ad10cc2

viambot added the safe to test Mark as safe to test label Oct 25, 2023

ohEmily requested a review from mcous October 25, 2023 19:24

mcous requested a review from michaellee1019 October 25, 2023 20:28

APP-3031: linter

8d7f459

viambot removed the safe to test Mark as safe to test label Oct 25, 2023

viambot added the safe to test Mark as safe to test label Oct 25, 2023

APP-3031: update comment to appease the lint gods

5a3773f

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 26, 2023

michaellee1019 approved these changes Oct 30, 2023

View reviewed changes

mcous previously approved these changes Oct 30, 2023

View reviewed changes

APP-3031: add token endpoint

e8cbcb2

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 3, 2023

APP-3031: try sending AccessToken

7714635

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 3, 2023

APP-3031: stop erasing cookie in callback handler

849ea2d

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 3, 2023

APP-3031: write access_token in response as text, read from app session

17309b9

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 3, 2023

name update: authOIConfig -> oidcConfig

8d6b101

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 13, 2023

DTCurrie added 2 commits November 17, 2023 13:13

Add token route

e7f0d52

Merge branch 'APP-3031' of github.com:viamrobotics/goutils into APP-3031

76f6d7e

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 17, 2023

Change callback to auth/callback

11adf81

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 17, 2023

Go back to /callback and set up temp cookies to fetch token

36b74fd

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Dec 1, 2023

Fix expiry formatting

1a9d1e8

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Dec 4, 2023

ohEmily commented Dec 18, 2023

View reviewed changes

ohEmily added 2 commits December 18, 2023 16:46

APP-3031: comment cookie back in

c49ac0f

Merge branch 'main' into APP-3031

84564ee

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Dec 18, 2023

APP-3031: linter failures

ccd6cfb

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Dec 19, 2023

ohEmily merged commit da61878 into main Dec 19, 2023
7 checks passed

ohEmily deleted the APP-3031 branch December 19, 2023 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APP-3031: make auth0.go generic to auth providers #216

APP-3031: make auth0.go generic to auth providers #216

ohEmily commented Oct 25, 2023 •

edited

Loading

mcous commented Oct 25, 2023

ohEmily commented Oct 25, 2023

michaellee1019 left a comment

michaellee1019 Oct 26, 2023

ohEmily Oct 30, 2023

michaellee1019 Oct 30, 2023

mcous left a comment

michaellee1019 commented Oct 30, 2023

ohEmily commented Nov 13, 2023

ohEmily Dec 18, 2023 •

edited

Loading

ohEmily Dec 18, 2023

DTCurrie Dec 18, 2023

ohEmily Dec 18, 2023

APP-3031: make auth0.go generic to auth providers #216

APP-3031: make auth0.go generic to auth providers #216

Conversation

ohEmily commented Oct 25, 2023 • edited Loading

mcous commented Oct 25, 2023

ohEmily commented Oct 25, 2023

michaellee1019 left a comment

Choose a reason for hiding this comment

michaellee1019 Oct 26, 2023

Choose a reason for hiding this comment

ohEmily Oct 30, 2023

Choose a reason for hiding this comment

michaellee1019 Oct 30, 2023

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

michaellee1019 commented Oct 30, 2023

ohEmily commented Nov 13, 2023

ohEmily Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

ohEmily Dec 18, 2023

Choose a reason for hiding this comment

DTCurrie Dec 18, 2023

Choose a reason for hiding this comment

ohEmily Dec 18, 2023

Choose a reason for hiding this comment

ohEmily commented Oct 25, 2023 •

edited

Loading

ohEmily Dec 18, 2023 •

edited

Loading