test(oidc): increase coverage around invalid state parameter on callback

errors/errors.go

@@ -11,6 +11,13 @@ func As[E error](err error) (e E, _ bool) {
 	return e, errors.As(err, &e)
 }
 
+// AsMatch is the same as As but it returns just a boolean to represent
+// whether or not the wrapped type matches the type parameter.
+func AsMatch[E error](err error) (match bool) {
+	_, match = As[E](err)
+	return
+}
+
 // New creates a new error with errors.New
 func New(s string) error {
 	return errors.New(s)
@@ -55,12 +62,12 @@ func EmptyFieldError(field string) error {
 	return InvalidFieldError(field, "must not be empty")
 }
 
-// ErrForbidding is returned when an operation is attempted which is forbidden
-// for the identified caller.
-type ErrForbidden string
+// ErrUnauthenticated is returned when an operation is attempted by an unauthenticated
+// client in an authenticated context.
+type ErrUnauthenticated string
 
 // Error() returns the underlying string of the error.
-func (e ErrForbidden) Error() string {
+func (e ErrUnauthenticated) Error() string {
 	return string(e)
 }
 

go.mod

@@ -45,7 +45,7 @@ require (
 	go.opentelemetry.io/otel/trace v1.11.2
 	go.uber.org/zap v1.24.0
 	golang.org/x/exp v0.0.0-20221012211006-4de253d81b95
-	golang.org/x/oauth2 v0.2.0
+	golang.org/x/net v0.2.0
 	golang.org/x/sync v0.1.0
 	google.golang.org/grpc v1.51.0
 	google.golang.org/protobuf v1.28.1
@@ -124,7 +124,7 @@ require (
 	go.uber.org/atomic v1.9.0 // indirect
 	go.uber.org/multierr v1.8.0 // indirect
 	golang.org/x/crypto v0.0.0-20220525230936-793ad666bf5e // indirect
-	golang.org/x/net v0.2.0 // indirect
+	golang.org/x/oauth2 v0.2.0 // indirect
 	golang.org/x/sys v0.2.0 // indirect
 	golang.org/x/text v0.4.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect

internal/server/auth/method/oidc/server.go

@@ -26,6 +26,21 @@ const (
 // was not configured
 var errProviderNotFound = errors.ErrNotFound("provider not found")
 
+// Server is the core OIDC server implementation for Flipt.
+// It supports two primary operations:
+// - AuthorizeURL
+// - Callback
+// These are two legs of the OIDC/OAuth flow.
+// Step 1 is Flipt establishes a URL directed at the delegated authentication service (e.g. Google).
+// The URL is configured using the client ID configured for the provided, a state parameter used to
+// prevent CSRF attacks and a callback URL directing back to the Callback operation.
+// Step 2 the user-agent navigates to the authorizer and establishes authenticity with them.
+// Once established they're redirected to the Callback operation with an authenticity code.
+// Step 3 the Callback operation uses this "code" and exchanges with the authorization service
+// for an ID Token. The validity of the response is checked (signature verified) and then the identity
+// details contained in this response are used to create a temporary Flipt client token.
+// This client token can be used to access the rest of the Flipt API.
+// Given the user-agent is requestin using HTTP the token is instead established as an HTTP cookie.
 type Server struct {
 	logger *zap.Logger
 	store  storageauth.Store
@@ -71,8 +86,10 @@ func (s *Server) AuthorizeURL(ctx context.Context, req *auth.AuthorizeURLRequest
 }
 
 // Callback attempts to authenticate a callback request from a delegated authorization service.
-// The supplied state metadata compared with the request state and ensured to be equal.
-// The provided code is exchanged with the associated provider for an "id_token".
+// Given the request includes a "state" parameter then the requests metadata is interrogated
+// for the "flipt_client_state" metadata key.
+// This entry must exist and the value match the request state.
+// The provided code is exchanged with the associated authorization service provider for an "id_token".
 // We verify the retrieved "id_token" is valid and for our client.
 // Once verified we extract the users associated email address.
 // Given all this completes successfully then we established an associated clientToken in
@@ -87,16 +104,16 @@ func (s *Server) Callback(ctx context.Context, req *auth.CallbackRequest) (_ *au
 	if req.State != "" {
 		md, ok := metadata.FromIncomingContext(ctx)
 		if !ok {
-			return nil, errors.New("missing metadata")
+			return nil, errors.NewErrorf[errors.ErrUnauthenticated]("missing state parameter")
 		}
 
 		state, ok := md["flipt_client_state"]
 		if !ok || len(state) < 1 {
-			return nil, errors.New("missing state")
+			return nil, errors.NewErrorf[errors.ErrUnauthenticated]("missing state parameter")
 		}
 
 		if req.State != state[0] {
-			return nil, errors.New("unexpected state")
+			return nil, errors.NewErrorf[errors.ErrUnauthenticated]("unexpected state parameter")
 		}
 	}
 

internal/server/auth/method/oidc/server_test.go

@@ -187,12 +187,32 @@ func Test_Server(t *testing.T) {
 		location = resp.Header.Get("Location")
 	})
 
-	t.Log("Redirecting to callback URL:", location)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, location, nil)
+	require.NoError(t, err)
 
-	t.Run("Callback", func(t *testing.T) {
+	t.Run("Callback (missing state)", func(t *testing.T) {
+		// using the default client which has no state cookie
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+	})
+
+	t.Run("Callback (invalid state)", func(t *testing.T) {
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, location, nil)
 		require.NoError(t, err)
 
+		req.Header.Set("Cookie", "flipt_client_state=abcdef")
+		// using the default client which has no state cookie
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+	})
+
+	t.Run("Callback", func(t *testing.T) {
 		resp, err := client.Do(req)
 		require.NoError(t, err)
 		defer resp.Body.Close()

internal/server/middleware/grpc/middleware.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/md5"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"time"
 
@@ -51,25 +50,18 @@ func ErrorUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnarySe
 		return
 	}
 
-	var errnf errs.ErrNotFound
-	if errors.As(err, &errnf) {
-		err = status.Error(codes.NotFound, err.Error())
-		return
-	}
-
-	var errin errs.ErrInvalid
-	if errors.As(err, &errin) {
-		err = status.Error(codes.InvalidArgument, err.Error())
-		return
-	}
-
-	var errv errs.ErrValidation
-	if errors.As(err, &errv) {
-		err = status.Error(codes.InvalidArgument, err.Error())
-		return
+	code := codes.Internal
+	switch {
+	case errs.AsMatch[errs.ErrNotFound](err):
+		code = codes.NotFound
+	case errs.AsMatch[errs.ErrInvalid](err),
+		errs.AsMatch[errs.ErrValidation](err):
+		code = codes.InvalidArgument
+	case errs.AsMatch[errs.ErrUnauthenticated](err):
+		code = codes.Unauthenticated
 	}
 
-	err = status.Error(codes.Internal, err.Error())
+	err = status.Error(code, err.Error())
 	return
 }
 

internal/server/middleware/grpc/middleware_test.go

@@ -98,6 +98,11 @@ func TestErrorUnaryInterceptor(t *testing.T) {
 			wantErr:  errors.EmptyFieldError("bar"),
 			wantCode: codes.InvalidArgument,
 		},
+		{
+			name:     "unauthenticated error",
+			wantErr:  errors.NewErrorf[errors.ErrUnauthenticated]("user %q not found", "foo"),
+			wantCode: codes.Unauthenticated,
+		},
 		{
 			name:     "other error",
 			wantErr:  errors.New("foo"),