OIDC Callback Error Redirect (#1174)

internal/errors/code.go

@@ -85,4 +85,7 @@ const (
 
 	// External system errors are reserved codes 3000-3999
 	Unavailable Code = 3000 // Unavailable represents that an external system is unavailable
+
+	// OIDC authentication provided errors
+	OidcProviderCallbackError Code = 4000 // OidcProviderCallbackError represents an error that is passed by the OIDC provider to the callback endpoint
 )

internal/errors/code_test.go

@@ -242,6 +242,11 @@ func TestCode_Both_String_Info(t *testing.T) {
 			c:    Unavailable,
 			want: Unavailable,
 		},
+		{
+			name: "OidcProviderCallbackError",
+			c:    OidcProviderCallbackError,
+			want: OidcProviderCallbackError,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

internal/errors/info.go

@@ -188,4 +188,8 @@ var errorCodeInfo = map[Code]Info{
 		Message: "external system unavailable",
 		Kind:    External,
 	},
+	OidcProviderCallbackError: {
+		Message: "oidc provider callback error",
+		Kind:    External,
+	},
 }

internal/servers/controller/handler.go

@@ -35,7 +35,6 @@ import (
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers/roles"
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers/scopes"
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers/users"
-	"google.golang.org/protobuf/encoding/protojson"
 )
 
 type HandlerProperties struct {
@@ -70,18 +69,7 @@ func handleGrpcGateway(c *Controller, props HandlerProperties) (http.Handler, er
 	ctx := props.CancelCtx
 	mux := runtime.NewServeMux(
 		runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.HTTPBodyMarshaler{
-			Marshaler: &runtime.JSONPb{
-				MarshalOptions: protojson.MarshalOptions{
-					// Ensures the json marshaler uses the snake casing as defined in the proto field names.
-					UseProtoNames: true,
-					// Do not add fields set to zero value to json.
-					EmitUnpopulated: false,
-				},
-				UnmarshalOptions: protojson.UnmarshalOptions{
-					// Allows requests to contain unknown fields.
-					DiscardUnknown: true,
-				},
-			},
+			Marshaler: handlers.JSONMarshaler(),
 		}),
 		runtime.WithErrorHandler(handlers.ErrorHandler(c.logger)),
 		runtime.WithForwardResponseOption(handlers.OutgoingInterceptor),

internal/servers/controller/handlers/authmethods/oidc.go

@@ -202,6 +202,9 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat
 // error details.
 func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.AuthenticateRequest) (*pbs.AuthenticateResponse, error) {
 	const op = "authmethod_service.(Service).authenticateOidcCallback"
+	// TODO: Return all errors (including the validate request based errors
+	//   in the redirect URL once we start looking at the url used for this
+	//   request instead of requiring the API URL to be set on the auth method.
 	if req == nil {
 		return nil, errors.New(errors.InvalidParameter, op, "Nil request.")
 	}
@@ -222,28 +225,34 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti
 	}
 
 	errRedirectBase := fmt.Sprintf(oidc.AuthenticationErrorsEndpoint, am.GetApiUrl())
-	errResponse := func(err error) *pbs.AuthenticateResponse {
+	errResponse := func(err error) (*pbs.AuthenticateResponse, error) {
 		u := make(url.Values)
-		// TODO: Decide how to format the domain error to match OIDC error format
-		u.Add("error", err.Error())
+		pbErr := handlers.ToApiError(err)
+		out, err := handlers.JSONMarshaler().Marshal(pbErr)
+		if err != nil {
+			return nil, errors.Wrap(err, op, errors.WithMsg("unable to marshal the error for callback"))
+		}
+		u.Add("error", string(out))
 		errRedirect := fmt.Sprintf("%s?%s", errRedirectBase, u.Encode())
-		return &pbs.AuthenticateResponse{Command: callbackCommand, Attributes: &structpb.Struct{
-			Fields: map[string]*structpb.Value{
-				"final_redirect_url": structpb.NewStringValue(errRedirect),
-			},
-		}}
+		respAttrs, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackResponse{
+			FinalRedirectUrl: errRedirect,
+		})
+		if err != nil {
+			return nil, errors.Wrap(err, op, errors.WithMsg("failed creating error redirect response"))
+		}
+		return &pbs.AuthenticateResponse{Command: callbackCommand, Attributes: respAttrs}, nil
 	}
 
 	attrs := new(pb.OidcAuthMethodAuthenticateCallbackRequest)
 	// Note that this conversion has already happened in the validate call so we don't expect errors here.
 	if err := handlers.StructToProto(req.GetAttributes(), attrs, handlers.WithDiscardUnknownFields(true)); err != nil {
-		return errResponse(err), nil
+		return errResponse(err)
 	}
 
 	var finalRedirectUrl string
 	if attrs.GetError() != "" {
-		// TODO: Package the OIDC error into the redirectUrl
-		return errResponse(fmt.Errorf("Error: %q, Details: %q", attrs.GetError(), attrs.GetErrorDescription())), nil
+		err := errors.Wrap(fmt.Errorf("Error: %q, Details: %q", attrs.GetError(), attrs.GetErrorDescription()), op, errors.WithCode(errors.OidcProviderCallbackError))
+		return errResponse(err)
 	}
 	finalRedirectUrl, err = oidc.Callback(
 		ctx,
@@ -254,14 +263,14 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti
 		attrs.GetState(),
 		attrs.GetCode())
 	if err != nil {
-		return errResponse(errors.New(errors.InvalidParameter, op, "Callback validation failed.", errors.WithWrap(err))), nil
+		return errResponse(errors.New(errors.InvalidParameter, op, "Callback validation failed.", errors.WithWrap(err)))
 	}
 
 	respAttrs, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackResponse{
 		FinalRedirectUrl: finalRedirectUrl,
 	})
 	if err != nil {
-		return errResponse(errors.New(errors.Internal, op, "Error marshaling parameters after successful callback", errors.WithWrap(err))), nil
+		return errResponse(errors.New(errors.Internal, op, "Error marshaling parameters after successful callback", errors.WithWrap(err)))
 	}
 
 	return &pbs.AuthenticateResponse{Command: req.GetCommand(), Attributes: respAttrs}, nil

internal/servers/controller/handlers/authmethods/oidc_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"regexp"
 	"sort"
 	"testing"
@@ -1654,3 +1655,107 @@ func TestAuthenticate_OIDC_Token(t *testing.T) {
 		})
 	}
 }
+
+func TestAuthenticate_OIDC_Callback_ErrorRedirect(t *testing.T) {
+	s := getSetup(t)
+
+	directErrorCases := []struct {
+		name string
+		req  *pbs.AuthenticateRequest
+	}{
+		{
+			name: "No Code Or Error",
+			req: &pbs.AuthenticateRequest{
+				AuthMethodId: s.authMethod.GetPublicId(),
+				Command:      "callback",
+				Attributes: func() *structpb.Struct {
+					st, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackRequest{
+						State: "anything",
+					})
+					require.NoError(t, err)
+					return st
+				}(),
+			},
+		},
+		{
+			name: "No Auth Method Id",
+			req: &pbs.AuthenticateRequest{
+				Command: "callback",
+				Attributes: func() *structpb.Struct {
+					st, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackRequest{
+						Code:  "anythingworks",
+						State: "anything",
+					})
+					require.NoError(t, err)
+					return st
+				}(),
+			},
+		},
+		{
+			name: "No State",
+			req: &pbs.AuthenticateRequest{
+				AuthMethodId: s.authMethod.GetPublicId(),
+				Command:      "callback",
+				Attributes: func() *structpb.Struct {
+					st, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackRequest{
+						Code: "anythingworks",
+					})
+					require.NoError(t, err)
+					return st
+				}(),
+			},
+		},
+	}
+	for _, tc := range directErrorCases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := s.authMethodService.Authenticate(auth.DisabledAuthTestContext(s.iamRepoFn, s.org.GetPublicId()), tc.req)
+			require.Error(t, err)
+		})
+	}
+
+	redirectUrlErrorCases := []struct {
+		name        string
+		attrs       *pb.OidcAuthMethodAuthenticateCallbackRequest
+		errorSubStr string
+	}{
+		{
+			name: "Cant decrypt state",
+			attrs: &pb.OidcAuthMethodAuthenticateCallbackRequest{
+				Code:  "anythingworkshere",
+				State: "cant decrypt this!",
+			},
+			errorSubStr: url.QueryEscape("unable to decode message"),
+		},
+		{
+			name: "OIDC Error",
+			attrs: &pb.OidcAuthMethodAuthenticateCallbackRequest{
+				State:            "foo",
+				Error:            "some_error",
+				ErrorDescription: "error description",
+			},
+			errorSubStr: url.QueryEscape("error description"),
+		},
+	}
+	for _, tc := range redirectUrlErrorCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := s.authMethodService.Authenticate(auth.DisabledAuthTestContext(s.iamRepoFn, s.org.GetPublicId()),
+				&pbs.AuthenticateRequest{
+					AuthMethodId: s.authMethod.GetPublicId(),
+					Command:      "callback",
+					Attributes: func() *structpb.Struct {
+						st, err := handlers.ProtoToStruct(tc.attrs)
+						require.NoError(t, err)
+						return st
+					}(),
+				})
+
+			require.NoError(t, err)
+			respAttrs := &pb.OidcAuthMethodAuthenticateCallbackResponse{}
+			require.NoError(t, handlers.StructToProto(got.GetAttributes(), respAttrs))
+			assert.Contains(t, respAttrs.FinalRedirectUrl, fmt.Sprintf("%s/authentication-error?", s.authMethod.GetApiUrl()))
+			u, err := url.Parse(respAttrs.GetFinalRedirectUrl())
+			require.NoError(t, err)
+			assert.Contains(t, u.RawQuery, tc.errorSubStr)
+		})
+	}
+}

internal/servers/controller/handlers/errors.go

@@ -80,7 +80,7 @@ func NotFoundError() error {
 }
 
 // NotFoundErrorf returns an ApiError indicating a resource couldn't be found.
-func NotFoundErrorf(msg string, a ...interface{}) error {
+func NotFoundErrorf(msg string, a ...interface{}) *apiError {
 	return &apiError{
 		status: http.StatusNotFound,
 		inner: &pb.Error{
@@ -114,7 +114,7 @@ func UnauthenticatedError() error {
 	return unauthenticatedError
 }
 
-func InvalidArgumentErrorf(msg string, fields map[string]string) error {
+func InvalidArgumentErrorf(msg string, fields map[string]string) *apiError {
 	err := ApiErrorWithCodeAndMessage(codes.InvalidArgument, msg)
 	var apiErr *apiError
 	if !errors.As(err, &apiErr) {
@@ -134,7 +134,7 @@ func InvalidArgumentErrorf(msg string, fields map[string]string) error {
 }
 
 // Converts a known errors into an error that can presented to an end user over the API.
-func backendErrorToApiError(inErr error) error {
+func backendErrorToApiError(inErr error) *apiError {
 	stErr := status.Convert(inErr)
 
 	switch {
@@ -216,3 +216,7 @@ func ErrorHandler(logger hclog.Logger) runtime.ErrorHandlerFunc {
 		}
 	}
 }
+
+func ToApiError(e error) *pb.Error {
+	return backendErrorToApiError(e).inner
+}

internal/servers/controller/handlers/marshler.go

@@ -0,0 +1,23 @@
+package handlers
+
+import (
+	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
+	"google.golang.org/protobuf/encoding/protojson"
+)
+
+// JSONMarshaler provides marshaler used for marshalling all proto as JSON
+// in a format expected by the user facing controller API.
+func JSONMarshaler() *runtime.JSONPb {
+	return &runtime.JSONPb{
+		MarshalOptions: protojson.MarshalOptions{
+			// Ensures the json marshaler uses the snake casing as defined in the proto field names.
+			UseProtoNames: true,
+			// Do not add fields set to zero value to json.
+			EmitUnpopulated: false,
+		},
+		UnmarshalOptions: protojson.UnmarshalOptions{
+			// Allows requests to contain unknown fields.
+			DiscardUnknown: true,
+		},
+	}
+}