From 5e1df3f30a6f6556dc8d1b1f97b68f3a1ac8fd4b Mon Sep 17 00:00:00 2001 From: reesporte <45641995+reesporte@users.noreply.github.com> Date: Mon, 16 May 2022 17:07:02 -0500 Subject: [PATCH] [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 | 7 +- authn/authenticate_internal_test.go | 102 ++++++++++++++++++---------- http_handler.go | 2 +- http_handler_internal_test.go | 2 +- server/grpc.go | 4 +- 5 files changed, 76 insertions(+), 41 deletions(-) diff --git a/authn/authenticate.go b/authn/authenticate.go index 9b60ca797..3e3e96928 100644 --- a/authn/authenticate.go +++ b/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) diff --git a/authn/authenticate_internal_test.go b/authn/authenticate_internal_test.go index 41e841c8d..67aa6d695 100644 --- a/authn/authenticate_internal_test.go +++ b/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), diff --git a/http_handler.go b/http_handler.go index cc19a0e8b..757c811e5 100644 --- a/http_handler.go +++ b/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 "" } diff --git a/http_handler_internal_test.go b/http_handler_internal_test.go index f496cf492..f5237deaa 100644 --- a/http_handler_internal_test.go +++ b/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, diff --git a/server/grpc.go b/server/grpc.go index c05cd5561..99099f761 100644 --- a/server/grpc.go +++ b/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 } }