[fb-1377] SetGRPCMetadata should always set the cookie, whether there was a cookie there to begin with or not (#2065)

* make CookieName an exported constant

* fix SetGRPCMetadata

this will actually set the grpc metadata even if there are no cookies in the
metadata already.

* gofmt yourself

authn/authenticate.go

@@ -23,6 +23,9 @@ import (
 	"golang.org/x/oauth2"
 )
 
+// CookieName is the name of the cookie that holds the refreshed auth token.
+const CookieName = "molecula-chip"
+
 // cachedGroups is used to hold groups and when they were last cached
 type cachedGroups struct {
 	cacheTime time.Time
@@ -76,7 +79,7 @@ type Auth struct {
 func NewAuth(logger logger.Logger, url string, scopes []string, authURL, tokenURL, groupEndpoint, logout, clientID, clientSecret, secretKey string) (auth *Auth, err error) {
 	auth = &Auth{
 		logger:         logger,
-		cookieName:     "molecula-chip",
+		cookieName:     CookieName,
 		groupEndpoint:  groupEndpoint,
 		logoutEndpoint: logout,
 		fbURL:          url,
@@ -316,6 +319,8 @@ func (a *Auth) SetGRPCMetadata(ctx context.Context, md metadata.MD, token string
 			}
 			cookies = append(cookies, cookie)
 		}
+	} else {
+		cookies = []string{a.cookieName + "=" + token}
 	}
 	md["cookie"] = cookies
 	return grpc.SetHeader(ctx, md)

authn/authenticate_internal_test.go

@@ -52,6 +52,71 @@ func NewTestAuth(t *testing.T) *Auth {
 	}
 	return a
 }
+
+func TestSetGRPCMetadata(t *testing.T) {
+	a := NewTestAuth(t)
+	for name, md := range map[string]metadata.MD{
+		"empty":        {},
+		"something":    {"cookie": []string{a.cookieName + "=something"}},
+		"otherCookies": {"cookie": []string{a.cookieName + "=something", "blah=blah"}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			ogCookies, _ := md["cookie"]
+			ctx := grpc.NewContextWithServerTransportStream(
+				metadata.NewIncomingContext(context.TODO(),
+					md,
+				),
+				NewServerTransportStream(),
+			)
+			md, ok := metadata.FromIncomingContext(ctx)
+			if !ok {
+				t.Fatalf("expected ok, got: %v", ok)
+			}
+			err := a.SetGRPCMetadata(ctx, md, "this is a token!")
+			if err != nil {
+				t.Fatalf("expected no errors, got: %v", err)
+			}
+			if err := grpc.SendHeader(ctx, md); err != nil {
+				t.Fatalf("expected no errors, got: %v", err)
+			}
+			md, ok = metadata.FromIncomingContext(ctx)
+			if !ok {
+				t.Fatalf("expected ok, got: %v", ok)
+			}
+			c, ok := md["cookie"]
+			if !ok {
+				t.Fatalf("expected ok, got: %v", ok)
+			}
+			var cookie string
+			for _, cookie = range c {
+				if strings.HasPrefix(cookie, a.cookieName) {
+					break
+				}
+			}
+			if exp, got := a.cookieName+"=this is a token!", cookie; got != exp {
+				t.Fatalf("expected '%v', got '%v'", exp, got)
+			}
+
+			for _, cookie = range c {
+				if strings.HasPrefix(cookie, a.cookieName) {
+					continue
+				}
+				found := false
+
+				for _, ogCookie := range ogCookies {
+					if cookie == ogCookie {
+						found = true
+						break
+					}
+				}
+				if !found {
+					t.Fatal("SetGRPCMetadata did not maintain the previous cookie list")
+				}
+			}
+		})
+	}
+}
+
 func TestAuth(t *testing.T) {
 	a := NewTestAuth(t)
 	t.Run("SetCookie", func(t *testing.T) {
@@ -69,42 +134,7 @@ func TestAuth(t *testing.T) {
 			t.Fatalf("path=%s, want %s", got, want)
 		}
 	})
-	t.Run("SetGRPCMetadata", func(t *testing.T) {
-		md := metadata.MD{
-			"cookie": []string{a.cookieName + "=something"},
-		}
-		ctx := grpc.NewContextWithServerTransportStream(
-			metadata.NewIncomingContext(context.TODO(),
-				md,
-			),
-			NewServerTransportStream(),
-		)
-		md, ok := metadata.FromIncomingContext(ctx)
-		if !ok {
-			t.Fatalf("expected ok, got: %v", ok)
-		}
-		err := a.SetGRPCMetadata(ctx, md, "this is a token!")
-		if err != nil {
-			t.Fatalf("expected no errors, got: %v", err)
-		}
-		md, ok = metadata.FromIncomingContext(ctx)
-		if !ok {
-			t.Fatalf("expected ok, got: %v", ok)
-		}
-		c, ok := md["cookie"]
-		if !ok {
-			t.Fatalf("expected ok, got: %v", ok)
-		}
-		var cookie string
-		for _, cookie = range c {
-			if strings.HasPrefix(cookie, a.cookieName) {
-				break
-			}
-		}
-		if exp, got := a.cookieName+"=this is a token!", cookie; got != exp {
-			t.Fatalf("expected '%v', got '%v'", exp, got)
-		}
-	})
+
 	t.Run("KeyLength", func(t *testing.T) {
 		_, err := NewAuth(
 			logger.NewStandardLogger(os.Stdout),

http_handler.go

@@ -3764,7 +3764,7 @@ func getToken(r *http.Request) string {
 		}
 		return parts[1]
 	}
-	cookie, err := r.Cookie("molecula-chip")
+	cookie, err := r.Cookie(authn.CookieName)
 	if err != nil {
 		return ""
 	}

http_handler_internal_test.go

@@ -285,7 +285,7 @@ func TestAuthentication(t *testing.T) {
 	expiredToken = "Bearer " + expiredToken
 
 	validCookie := &http.Cookie{
-		Name:     "molecula-chip",
+		Name:     authn.CookieName,
 		Value:    token.AccessToken,
 		Path:     "/",
 		Secure:   true,

server/grpc.go

@@ -1697,8 +1697,8 @@ func Valid(ctx context.Context, auth *authn.Auth) (context.Context, error) {
 		}
 		cookies := strings.Split(c[0], "; ")
 		for _, cookie := range cookies {
-			if strings.HasPrefix(cookie, "molecula-chip") {
-				authorization = strings.Split(cookie, "molecula-chip=")[1:]
+			if strings.HasPrefix(cookie, authn.CookieName) {
+				authorization = strings.Split(cookie, authn.CookieName+"=")[1:]
 				break
 			}
 		}