From 61e236f76da37612162c94a8644927c8d2bc1ee0 Mon Sep 17 00:00:00 2001 From: Todd Knight Date: Thu, 29 Apr 2021 14:47:13 -0700 Subject: [PATCH] OIDC Callback Error Redirect (#1174) --- internal/errors/code.go | 3 + internal/errors/code_test.go | 5 + internal/errors/info.go | 4 + internal/servers/controller/handler.go | 14 +-- .../controller/handlers/authmethods/oidc.go | 35 +++--- .../handlers/authmethods/oidc_test.go | 105 ++++++++++++++++++ .../servers/controller/handlers/errors.go | 10 +- .../servers/controller/handlers/marshler.go | 23 ++++ 8 files changed, 170 insertions(+), 29 deletions(-) create mode 100644 internal/servers/controller/handlers/marshler.go diff --git a/internal/errors/code.go b/internal/errors/code.go index 43464e8ba..ba26a7ce1 100644 --- a/internal/errors/code.go +++ b/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 ) diff --git a/internal/errors/code_test.go b/internal/errors/code_test.go index 8499adc9a..515bdc940 100644 --- a/internal/errors/code_test.go +++ b/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) { diff --git a/internal/errors/info.go b/internal/errors/info.go index b0990dacd..2b0af967e 100644 --- a/internal/errors/info.go +++ b/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, + }, } diff --git a/internal/servers/controller/handler.go b/internal/servers/controller/handler.go index 1e351d1dc..1c3ce76c4 100644 --- a/internal/servers/controller/handler.go +++ b/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), diff --git a/internal/servers/controller/handlers/authmethods/oidc.go b/internal/servers/controller/handlers/authmethods/oidc.go index 1b081a45e..2d9e48119 100644 --- a/internal/servers/controller/handlers/authmethods/oidc.go +++ b/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 diff --git a/internal/servers/controller/handlers/authmethods/oidc_test.go b/internal/servers/controller/handlers/authmethods/oidc_test.go index 6308511cf..4bb888d92 100644 --- a/internal/servers/controller/handlers/authmethods/oidc_test.go +++ b/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) + }) + } +} diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go index de3d0a954..581a0dc3a 100644 --- a/internal/servers/controller/handlers/errors.go +++ b/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 +} diff --git a/internal/servers/controller/handlers/marshler.go b/internal/servers/controller/handlers/marshler.go new file mode 100644 index 000000000..4226f6e34 --- /dev/null +++ b/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, + }, + } +}