From ee55a89d5a6c6a80a7716e88e00e3db9059a65a9 Mon Sep 17 00:00:00 2001 From: Wout Slakhorst Date: Wed, 2 Oct 2024 08:59:48 +0200 Subject: [PATCH] fixes for rfc021 client status codes (#3433) --- auth/api/iam/api.go | 14 +++++++----- auth/client/iam/client.go | 26 +++++++++++++++++++++- auth/client/iam/client_test.go | 11 +++++++++ auth/client/iam/openid4vp.go | 8 +++++-- auth/client/iam/openid4vp_test.go | 22 +++++++++++------- docs/_static/auth/v2.yaml | 7 +++--- vdr/didsubject/mock.go | 37 +++++++++++++++++++++++++++++++ 7 files changed, 106 insertions(+), 19 deletions(-) diff --git a/auth/api/iam/api.go b/auth/api/iam/api.go index b3f61b156..f8d660bed 100644 --- a/auth/api/iam/api.go +++ b/auth/api/iam/api.go @@ -27,33 +27,34 @@ import ( "encoding/json" "errors" "fmt" - "github.com/labstack/echo/v4" - "github.com/lestrrat-go/jwx/v2/jwk" - "github.com/nuts-foundation/nuts-node/http/cache" - "github.com/nuts-foundation/nuts-node/http/user" - "github.com/nuts-foundation/nuts-node/vdr/didsubject" "html/template" "net/http" "net/url" "strings" "time" + "github.com/labstack/echo/v4" + "github.com/lestrrat-go/jwx/v2/jwk" "github.com/lestrrat-go/jwx/v2/jwt" "github.com/nuts-foundation/go-did/did" "github.com/nuts-foundation/nuts-node/audit" "github.com/nuts-foundation/nuts-node/auth" "github.com/nuts-foundation/nuts-node/auth/api/iam/assets" + iamclient "github.com/nuts-foundation/nuts-node/auth/client/iam" "github.com/nuts-foundation/nuts-node/auth/log" "github.com/nuts-foundation/nuts-node/auth/oauth" "github.com/nuts-foundation/nuts-node/core" nutsCrypto "github.com/nuts-foundation/nuts-node/crypto" nutsHttp "github.com/nuts-foundation/nuts-node/http" + "github.com/nuts-foundation/nuts-node/http/cache" + "github.com/nuts-foundation/nuts-node/http/user" "github.com/nuts-foundation/nuts-node/jsonld" "github.com/nuts-foundation/nuts-node/policy" "github.com/nuts-foundation/nuts-node/storage" "github.com/nuts-foundation/nuts-node/vcr" "github.com/nuts-foundation/nuts-node/vcr/pe" "github.com/nuts-foundation/nuts-node/vdr" + "github.com/nuts-foundation/nuts-node/vdr/didsubject" "github.com/nuts-foundation/nuts-node/vdr/resolver" ) @@ -202,6 +203,9 @@ func (r Wrapper) ResolveStatusCode(err error) int { resolver.ErrDIDNotManagedByThisNode: http.StatusBadRequest, pe.ErrNoCredentials: http.StatusPreconditionFailed, didsubject.ErrSubjectNotFound: http.StatusNotFound, + iamclient.ErrInvalidClientCall: http.StatusBadRequest, + iamclient.ErrBadGateway: http.StatusBadGateway, + iamclient.ErrPreconditionFailed: http.StatusPreconditionFailed, }) } diff --git a/auth/client/iam/client.go b/auth/client/iam/client.go index 1dc1d9b05..68a216ad4 100644 --- a/auth/client/iam/client.go +++ b/auth/client/iam/client.go @@ -22,6 +22,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "github.com/lestrrat-go/jwx/v2/jws" "github.com/lestrrat-go/jwx/v2/jwt" @@ -40,6 +41,12 @@ import ( "github.com/nuts-foundation/nuts-node/vcr/pe" ) +// ErrInvalidClientCall is returned when the node makes a http call as client based on wrong information passed by the client. +var ErrInvalidClientCall = errors.New("invalid client call") + +// ErrBadGateway is returned when the node makes a http call as client and the upstream returns an unexpected result. +var ErrBadGateway = errors.New("upstream returned unexpected result") + // HTTPClient holds the server address and other basic settings for the http client type HTTPClient struct { strictMode bool @@ -63,7 +70,14 @@ func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, oauth } var metadata oauth.AuthorizationServerMetadata if err = hb.doGet(ctx, metadataURL.String(), &metadata); err != nil { - return nil, err + // if this is a core.HttpError and the status code >= 500 then we want the caller to receive a 502 Bad Gateway + // we do this by changing the status code of the error + // any other error should result in a 400 Bad Request + if httpErr, ok := err.(core.HttpError); ok && httpErr.StatusCode >= 500 { + httpErr.StatusCode = http.StatusBadGateway + return nil, httpErr + } + return nil, errors.Join(ErrInvalidClientCall, err) } return &metadata, err } @@ -93,6 +107,16 @@ func (hb HTTPClient) PresentationDefinition(ctx context.Context, presentationDef return nil, err } var presentationDefinition pe.PresentationDefinition + err = hb.doRequest(ctx, request, &presentationDefinition) + if err != nil { + // a 404 (defined by scope) should result in a 400 for the client + // any other error should result in a 502 Bad Gateway + if httpErr, ok := err.(core.HttpError); ok && httpErr.StatusCode == 404 { + return nil, errors.Join(ErrInvalidClientCall, err) + } + return nil, errors.Join(ErrBadGateway, err) + } + return &presentationDefinition, hb.doRequest(ctx, request, &presentationDefinition) } diff --git a/auth/client/iam/client_test.go b/auth/client/iam/client_test.go index 3c37deed7..bb205eff0 100644 --- a/auth/client/iam/client_test.go +++ b/auth/client/iam/client_test.go @@ -78,6 +78,17 @@ func TestHTTPClient_OAuthAuthorizationServerMetadata(t *testing.T) { assert.Equal(t, "GET", handler.Request.Method) assert.Equal(t, "/.well-known/oauth-authorization-server/iam/123", handler.Request.URL.Path) }) + t.Run("error - server error changes status code to 502", func(t *testing.T) { + handler := http2.Handler{StatusCode: http.StatusInternalServerError} + tlsServer, client := testServerAndClient(t, &handler) + + _, err := client.OAuthAuthorizationServerMetadata(ctx, tlsServer.URL) + + require.Error(t, err) + httpErr, ok := err.(core.HttpError) + require.True(t, ok) + assert.Equal(t, http.StatusBadGateway, httpErr.StatusCode) + }) } func TestHTTPClient_PresentationDefinition(t *testing.T) { diff --git a/auth/client/iam/openid4vp.go b/auth/client/iam/openid4vp.go index 437cd9543..5fc02e084 100644 --- a/auth/client/iam/openid4vp.go +++ b/auth/client/iam/openid4vp.go @@ -22,6 +22,7 @@ package iam import ( "context" "encoding/json" + "errors" "fmt" "github.com/nuts-foundation/nuts-node/http/client" "github.com/nuts-foundation/nuts-node/vcr/credential" @@ -46,6 +47,9 @@ import ( "github.com/nuts-foundation/nuts-node/vdr/resolver" ) +// ErrPreconditionFailed is returned when a precondition is not met. +var ErrPreconditionFailed = errors.New("precondition failed") + var _ Client = (*OpenID4VPClient)(nil) type OpenID4VPClient struct { @@ -282,7 +286,7 @@ func (c *OpenID4VPClient) RequestRFC021AccessToken(ctx context.Context, clientID for key := range maps.Keys(allMethods) { availableMethods = append(availableMethods, key) } - return nil, fmt.Errorf("did method mismatch, requested: %v, available: %v", metadata.DIDMethodsSupported, availableMethods) + return nil, errors.Join(ErrPreconditionFailed, fmt.Errorf("did method mismatch, requested: %v, available: %v", metadata.DIDMethodsSupported, availableMethods)) } // each additional credential can be used by each DID @@ -320,7 +324,7 @@ func (c *OpenID4VPClient) RequestRFC021AccessToken(ctx context.Context, clientID } dpopHeader, dpopKid, err = c.dpop(ctx, *subjectDID, *request) if err != nil { - return nil, fmt.Errorf("failed tocreate DPoP header: %w", err) + return nil, fmt.Errorf("failed to create DPoP header: %w", err) } } diff --git a/auth/client/iam/openid4vp_test.go b/auth/client/iam/openid4vp_test.go index 7f106c53b..13df76769 100644 --- a/auth/client/iam/openid4vp_test.go +++ b/auth/client/iam/openid4vp_test.go @@ -230,7 +230,8 @@ func TestIAMClient_AuthorizationServerMetadata(t *testing.T) { _, err := ctx.client.AuthorizationServerMetadata(context.Background(), ctx.tlsServer.URL) require.Error(t, err) - assert.EqualError(t, err, "failed to retrieve remote OAuth Authorization Server metadata: server returned HTTP 404 (expected: 200)") + assert.ErrorIs(t, err, ErrInvalidClientCall) + assert.ErrorContains(t, err, "server returned HTTP 404 (expected: 200)") }) } @@ -275,7 +276,9 @@ func TestRelyingParty_RequestRFC021AccessToken(t *testing.T) { response, err := ctx.client.RequestRFC021AccessToken(context.Background(), subjectClientID, subjectID, ctx.verifierURL.String(), scopes, false, nil) - assert.EqualError(t, err, "did method mismatch, requested: [other], available: [test]") + require.Error(t, err) + assert.ErrorIs(t, err, ErrPreconditionFailed) + assert.ErrorContains(t, err, "did method mismatch, requested: [other], available: [test]") assert.Nil(t, response) }) t.Run("with additional credentials", func(t *testing.T) { @@ -357,8 +360,9 @@ func TestRelyingParty_RequestRFC021AccessToken(t *testing.T) { _, err := ctx.client.RequestRFC021AccessToken(context.Background(), subjectClientID, subjectID, ctx.verifierURL.String(), scopes, false, nil) - assert.Error(t, err) - assert.EqualError(t, err, "failed to retrieve presentation definition: server returned HTTP 404 (expected: 200)") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidClientCall) + assert.ErrorContains(t, err, "server returned HTTP 404 (expected: 200)") }) t.Run("error - failed to get authorization server metadata", func(t *testing.T) { ctx := createClientServerTestContext(t) @@ -366,8 +370,9 @@ func TestRelyingParty_RequestRFC021AccessToken(t *testing.T) { _, err := ctx.client.RequestRFC021AccessToken(context.Background(), subjectClientID, subjectID, ctx.verifierURL.String(), scopes, false, nil) - assert.Error(t, err) - assert.EqualError(t, err, "failed to retrieve remote OAuth Authorization Server metadata: server returned HTTP 404 (expected: 200)") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidClientCall) + assert.ErrorContains(t, err, "server returned HTTP 404 (expected: 200)") }) t.Run("error - faulty presentation definition", func(t *testing.T) { ctx := createClientServerTestContext(t) @@ -379,8 +384,9 @@ func TestRelyingParty_RequestRFC021AccessToken(t *testing.T) { _, err := ctx.client.RequestRFC021AccessToken(context.Background(), subjectClientID, subjectID, ctx.verifierURL.String(), scopes, false, nil) - assert.Error(t, err) - assert.EqualError(t, err, "failed to retrieve presentation definition: unable to unmarshal response: unexpected end of JSON input") + require.Error(t, err) + assert.ErrorIs(t, err, ErrBadGateway) + assert.ErrorContains(t, err, "unable to unmarshal response: unexpected end of JSON input") }) t.Run("error - failed to build vp", func(t *testing.T) { ctx := createClientServerTestContext(t) diff --git a/docs/_static/auth/v2.yaml b/docs/_static/auth/v2.yaml index f772b31ef..a31302e74 100644 --- a/docs/_static/auth/v2.yaml +++ b/docs/_static/auth/v2.yaml @@ -14,9 +14,10 @@ paths: It'll initiate a s2s (RFC021) flow. error returns: - * 400 - one of the parameters has the wrong format or an OAuth error occurred - * 412 - the organization wallet does not contain the correct credentials - * 503 - the authorizer could not be reached or returned an error + * 400 - one of the parameters has the wrong format, an OAuth error occurred, or the http client calling the authorizer returned an error due to incorrect input + * 412 - the organization wallet does not contain the correct credentials or doesn't support the right DID methods + * 502 - the authorizer returned an error + * 503 - the authorizer could not be reached tags: - auth parameters: diff --git a/vdr/didsubject/mock.go b/vdr/didsubject/mock.go index d94d10b2a..564ea3395 100644 --- a/vdr/didsubject/mock.go +++ b/vdr/didsubject/mock.go @@ -336,6 +336,43 @@ func (mr *MockManagerMockRecorder) UpdateService(ctx, subject, serviceID, servic return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateService", reflect.TypeOf((*MockManager)(nil).UpdateService), ctx, subject, serviceID, service) } +// MockDocumentMigration is a mock of DocumentMigration interface. +type MockDocumentMigration struct { + ctrl *gomock.Controller + recorder *MockDocumentMigrationMockRecorder +} + +// MockDocumentMigrationMockRecorder is the mock recorder for MockDocumentMigration. +type MockDocumentMigrationMockRecorder struct { + mock *MockDocumentMigration +} + +// NewMockDocumentMigration creates a new mock instance. +func NewMockDocumentMigration(ctrl *gomock.Controller) *MockDocumentMigration { + mock := &MockDocumentMigration{ctrl: ctrl} + mock.recorder = &MockDocumentMigrationMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDocumentMigration) EXPECT() *MockDocumentMigrationMockRecorder { + return m.recorder +} + +// MigrateDIDHistoryToSQL mocks base method. +func (m *MockDocumentMigration) MigrateDIDHistoryToSQL(id did.DID, subject string, getHistory func(did.DID, int) ([]orm.MigrationDocument, error)) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "MigrateDIDHistoryToSQL", id, subject, getHistory) + ret0, _ := ret[0].(error) + return ret0 +} + +// MigrateDIDHistoryToSQL indicates an expected call of MigrateDIDHistoryToSQL. +func (mr *MockDocumentMigrationMockRecorder) MigrateDIDHistoryToSQL(id, subject, getHistory any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MigrateDIDHistoryToSQL", reflect.TypeOf((*MockDocumentMigration)(nil).MigrateDIDHistoryToSQL), id, subject, getHistory) +} + // MockCreationOptions is a mock of CreationOptions interface. type MockCreationOptions struct { ctrl *gomock.Controller