diff --git a/internal/verifier/apiv1/direct_post_decline_test.go b/internal/verifier/apiv1/direct_post_decline_test.go new file mode 100644 index 000000000..c77290aa0 --- /dev/null +++ b/internal/verifier/apiv1/direct_post_decline_test.go @@ -0,0 +1,386 @@ +package apiv1 + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + "time" + + "github.com/SUNET/vc/internal/verifier/notify" + "github.com/SUNET/vc/pkg/cache" + "github.com/SUNET/vc/pkg/helpers" + "github.com/SUNET/vc/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// declineClient is a verifier with one live session, which is all an +// error response needs: it carries no presentation to verify. +func declineClient(t *testing.T, state string) (*Client, *cache.AuthorizationContext) { + t.Helper() + ctx := context.Background() + + client, _ := CreateTestClientWithMock(t, nil) + notifyService, _ := notify.New(ctx, client.cfg, logger.NewSimple("test")) + client.notify = notifyService + + authCtx := &cache.AuthorizationContext{ + SessionID: "declining-session", + State: state, + Nonce: "nonce", + ClientID: "x509_san_dns:verifier.example.com", + Status: cache.SessionStatusAwaitingPresentation, + } + require.NoError(t, client.cacheService.AuthContext.Save(ctx, authCtx)) + return client, authCtx +} + +// The live failure: the user declines, the engine POSTs a plain +// error form to response_uri, and the verifier answered 500 "failed to +// get KID from request: failed to parse JWT header: unexpected end of +// JSON input" because it looked for a JWE header in an empty string. +// +// OpenID4VP 1.0 section 8.3.1 permits an unencrypted error response even +// for direct_post.jwt, and section 8.2's example is exactly this form. +func TestDirectPostAcceptsAPlainDecline(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + reply, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", + ErrorDescription: "consent-declined", + State: "declined-state", + }) + require.NoError(t, err, "a declined presentation is an outcome, not a server failure") + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, cache.SessionStatusError, stored.Status, "the session must be terminal") + assert.Equal(t, "access_denied", stored.Error) + assert.Equal(t, "consent-declined", stored.ErrorDescription) +} + +// The half that actually unsticks the UI. /ui/completion reports a +// missing response code as "pending", so recording the decline is not +// enough on its own - the session has to have an outcome that is +// neither pending nor complete. +func TestDeclinedSessionHasATerminalOutcome(t *testing.T) { + ctx := context.Background() + client, authCtx := declineClient(t, "declined-state") + + before, err := client.SessionOutcome(ctx, authCtx.SessionID) + require.NoError(t, err) + require.False(t, before.Declined(), "the session has not been answered yet") + require.Empty(t, before.ResponseCode) + + _, err = client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", State: "declined-state", + }) + require.NoError(t, err) + + after, err := client.SessionOutcome(ctx, authCtx.SessionID) + require.NoError(t, err) + assert.True(t, after.Declined(), "the UI would poll pending forever") + assert.Equal(t, "access_denied", after.Error) + assert.Empty(t, after.ResponseCode, "a declined session has no response code") +} + +// state is what ties an error response to a request. OpenID4VP 1.0 +// section 14.3.2: the Verifier SHOULD check that it corresponds to a +// recent Authorization Request. +func TestDirectPostDeclineRequiresAKnownState(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct{ name, state string }{ + {"no state at all", ""}, + {"a state this verifier is not waiting for", "someone-elses-state"}, + } { + t.Run(tc.name, func(t *testing.T) { + client, authCtx := declineClient(t, "declined-state") + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", State: tc.state, + }) + require.Error(t, err, "accepting this would let anyone who can reach response_uri end a session") + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusBadRequest, shaped.HTTPStatus) + + // And the real session is untouched: silently answering 200 + // would leave it pending while reporting success. + stored, getErr := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: authCtx.State}) + require.NoError(t, getErr) + assert.Empty(t, stored.Error, "an unrelated session must not be ended by this") + }) + } +} + +// Neither a presentation nor an error is the caller's mistake, and it +// used to reach GetKID and come back as a 500. +func TestDirectPostWithNeitherPresentationNorErrorIsABadRequest(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{}) + require.Error(t, err) + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusBadRequest, shaped.HTTPStatus, "a malformed request is not a server failure") + assert.False(t, shaped.IsUnclassified(), "the caller gets a req_id and no idea what to fix") + assert.NotContains(t, err.Error(), "JWT header", + "the old message described a JWE the caller never claimed to send") +} + +// A wallet that CAN encrypt returns its error the way section 5.6 says +// it SHOULD - "Both successful and error responses SHOULD be returned +// using the supplied Response Mode" - so the same outcome arrives inside +// the envelope. It has no vp_token, and would otherwise be reported as a +// malformed presentation. +func TestDirectPostAcceptsAnEncryptedDecline(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + reply, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.NoError(t, err) + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, cache.SessionStatusError, stored.Status) +} + +// unreachableAuthContextStore answers every lookup the way a store whose +// backend is down does: with an operational error, not a miss. The +// distinction is the whole point - ErrNoDocuments means "no such state", +// anything else means "I cannot tell you". +type unreachableAuthContextStore struct { + cache.AuthContextStore + err error +} + +func (s unreachableAuthContextStore) Get(context.Context, *cache.AuthorizationContext) (*cache.AuthorizationContext, error) { + return nil, s.err +} + +// A wallet's error response must not be able to undo an outcome the +// session already has. Without the guard a late or replayed error +// overwrote a completed session's status while its VerifierResponseCode +// survived, so /ui/completion said "declined" and /ui/resume said +// "complete" for the same session - two answers, both read from stored +// state, with no way to tell which actually happened. +func TestDirectPostDeclineDoesNotOverwriteAnOutcome(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + arrange func(*cache.AuthorizationContext) + status cache.SessionStatus + wantErr string + }{ + { + name: "a session that has already completed", + arrange: func(a *cache.AuthorizationContext) { + a.Status = cache.SessionStatusCompleted + a.VerifierResponseCode = "response-code" + }, + status: cache.SessionStatusCompleted, + wantErr: "", + }, + { + name: "a session that has already been declined", + arrange: func(a *cache.AuthorizationContext) { + a.Status = cache.SessionStatusError + a.Error = "access_denied" + a.ErrorDescription = "the first refusal" + }, + status: cache.SessionStatusError, + wantErr: "access_denied", + }, + } { + t.Run(tc.name, func(t *testing.T) { + client, authCtx := declineClient(t, "declined-state") + tc.arrange(authCtx) + require.NoError(t, client.cacheService.AuthContext.Update(ctx, authCtx)) + + // Answered, not refused: the wallet is told its POST was + // received. Returning an error would invite it to retry + // against a session that is over. + reply, err := client.directPostError(ctx, "server_error", "a later, losing refusal", "declined-state") + require.NoError(t, err) + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, tc.status, stored.Status, "the first outcome must stand") + assert.Equal(t, tc.wantErr, stored.Error, "a later error response must not rewrite the recorded one") + assert.NotEqual(t, "a later, losing refusal", stored.ErrorDescription) + }) + } +} + +// A backend that cannot answer is not an unknown state. Answering 400 +// here tells a wallet its own request was malformed during an outage, +// and the real session stays pending with nobody retrying. +func TestDirectPostDeclineOnAnUnreachableBackendIsNotABadRequest(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + client.cacheService.AuthContext = unreachableAuthContextStore{ + AuthContextStore: client.cacheService.AuthContext, + err: errors.New("connection refused"), + } + + _, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.Error(t, err) + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusServiceUnavailable, shaped.HTTPStatus, + "an outage must be retryable, not reported as the caller's mistake") + assert.NotEqual(t, http.StatusBadRequest, shaped.HTTPStatus) + assert.NotContains(t, err.Error(), "connection refused", + "the wallet learns the verifier is unavailable, not what its backend is") +} + +// The description is wallet-controlled input, and the model bounds it at +// 512 bytes. Save validates AFTER the fields are assigned, so mutating +// the stored document in place left a memory-backed deployment with the +// rejected value and a terminal status on the live session while a +// Mongo-backed one kept the old one - the same request, two outcomes, +// decided by the cache backend. +func TestDirectPostDeclineWithAnOversizedDescriptionLeavesTheSessionIntact(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + _, err := client.directPostError(ctx, "access_denied", strings.Repeat("x", 513), "declined-state") + require.Error(t, err, "a value the model refuses to store must not be reported as stored") + + stored, getErr := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, getErr) + assert.Equal(t, cache.SessionStatusAwaitingPresentation, stored.Status, + "a failed save must not leave the session terminal") + assert.Empty(t, stored.Error, "a failed save must not leave the session carrying a refusal") + assert.Empty(t, stored.ErrorDescription, "the rejected value must not be readable from the live session") +} + +// /ui/resume is what a reloaded tab asks. Before this it answered +// "pending" with the original QR for a request the user had already +// declined, so the two endpoints disagreed: /ui/completion said declined +// while /ui/resume offered the code again. +func TestUIResumeReportsADeclinedSession(t *testing.T) { + ctx := context.Background() + client, authCtx := declineClient(t, "declined-state") + + _, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.NoError(t, err) + + reply, err := client.UIResume(ctx, authCtx.SessionID) + require.NoError(t, err) + require.NotNil(t, reply) + assert.Equal(t, UIResumeDeclined, reply.Status, "a reload must not rebuild the QR for a refused request") + assert.Equal(t, "access_denied", reply.Error) + assert.Equal(t, "consent-declined", reply.ErrorDescription) + assert.Empty(t, reply.AuthorizationRequest, "there is nothing left to scan") + assert.Empty(t, reply.ResponseCode, "a declined session has no response code") +} + +// A declined context has been answered. Handing it to a fresh +// interaction would reuse the state and nonce the user just refused, and +// the decline recorded on it would then describe a request nobody made. +func TestDeclinedSessionIsNotReusedByANewInteraction(t *testing.T) { + ctx := t.Context() + client := newSigningTestClient(t) + // A real interaction is needed here (not declineClient's bare + // context) because the reuse path runs through UIInteraction, and + // the decline it answers broadcasts on the notify service. + notifyService, err := notify.New(ctx, client.cfg, logger.NewSimple("test")) + require.NoError(t, err) + client.notify = notifyService + + first, err := client.UIInteraction(ctx, &UIInteractionRequest{DCQLQuery: createTestDCQLForVP(t)}) + require.NoError(t, err) + require.NotEmpty(t, first.SessionID) + + authCtx, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + require.NotEmpty(t, authCtx.State) + require.True(t, isReusableAuthContext(authCtx), "the premise: it was reusable before the refusal") + + _, err = client.directPostError(ctx, "access_denied", "consent-declined", authCtx.State) + require.NoError(t, err) + + declined, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + assert.False(t, isReusableAuthContext(declined), "a refused session is answered, not in flight") + + second, err := client.UIInteraction(ctx, &UIInteractionRequest{ + DCQLQuery: createTestDCQLForVP(t), + SessionID: first.SessionID, + }) + require.NoError(t, err) + assert.NotEqual(t, first.SessionID, second.SessionID, + "a new request after a refusal must get its own session") + + // And the refusal still describes the request it was given, not the + // new one. + stillDeclined, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + assert.Equal(t, "access_denied", stillDeclined.Error) +} + +// The other direction of "first outcome wins". The deadline was the +// only thing checked before a presentation was accepted, so a wallet +// that posted credentials after the user had declined - a refusal and a +// presentation racing, or a replay - wrote a VerifierResponseCode onto a +// session whose Error and terminal status stayed put. /ui/completion +// then reported "declined" and /ui/resume "complete" for one session. +func TestVerificationDirectPostRefusesAnAlreadyAnsweredSession(t *testing.T) { + ctx := t.Context() + + answeredSession := func(t *testing.T, decline bool) (*Client, string, string) { + t.Helper() + client, _ := CreateTestClientWithMock(t, nil) + notifyService, err := notify.New(ctx, client.cfg, logger.NewSimple("test")) + require.NoError(t, err) + client.notify = notifyService + + sessionID, state := "answered-verification-session", "answered-state" + session := pendingSession(sessionID, state, time.Now().Add(10*time.Minute).Unix()) + session.EphemeralEncryptionKeyID = sessionID + require.NoError(t, client.cacheService.AuthContext.Save(ctx, session)) + + if decline { + _, err = client.directPostError(ctx, "access_denied", "consent-declined", state) + require.NoError(t, err) + } + return client, sessionID, state + } + + // The premise, so the test below is about the guard and not about a + // verifier that refuses every presentation: the same session, the + // same deadline, is not refused as answered while it is still live. + t.Run("a live session is not refused as answered", func(t *testing.T) { + client, sessionID, state := answeredSession(t, false) + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Response: encryptedVPResponse(t, client, sessionID, state), + }) + assert.NotErrorIs(t, err, ErrSessionAlreadyAnswered) + }) + + t.Run("a declined session refuses the presentation that follows", func(t *testing.T) { + client, sessionID, state := answeredSession(t, true) + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Response: encryptedVPResponse(t, client, sessionID, state), + }) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSessionAlreadyAnswered) + assert.NotErrorIs(t, err, ErrSessionExpired, "the deadline has not passed; the session was answered") + + stored, err := client.cacheService.AuthContext.GetByID(ctx, sessionID) + require.NoError(t, err) + assert.Equal(t, "access_denied", stored.Error, "the refusal must still be what this session says happened") + assert.Empty(t, stored.VerifierResponseCode, "accepting it would have made the two UI endpoints disagree") + }) +} diff --git a/internal/verifier/apiv1/errors.go b/internal/verifier/apiv1/errors.go index 810334316..c84755adf 100644 --- a/internal/verifier/apiv1/errors.go +++ b/internal/verifier/apiv1/errors.go @@ -152,6 +152,22 @@ var ( HTTPStatus: http.StatusBadRequest, } + // ErrSessionAlreadyAnswered is a presentation for a session that + // already reached an outcome - most plainly, one the user declined + // a moment earlier, with the wallet's POST still in flight. + // + // Separate from ErrSessionExpired because it is a different fact + // and the wallet should be able to tell them apart: a deadline + // passed, versus an answer this session already has. Accepting it + // would write a response code onto a session whose Error and + // terminal status stay put, so /ui/completion would report + // "declined" and /ui/resume "complete" for the same session. + ErrSessionAlreadyAnswered = &OAuthError{ + ErrorCode: ErrCodeInvalidGrant, + ErrorDescription: "The session has already been answered", + HTTPStatus: http.StatusBadRequest, + } + ErrInvalidVP = &OAuthError{ ErrorCode: ErrCodeInvalidRequest, ErrorDescription: "Invalid verifiable presentation", diff --git a/internal/verifier/apiv1/handlers_ui.go b/internal/verifier/apiv1/handlers_ui.go index 47794a0d8..a98182542 100644 --- a/internal/verifier/apiv1/handlers_ui.go +++ b/internal/verifier/apiv1/handlers_ui.go @@ -807,6 +807,13 @@ func isReusableAuthContext(authCtx *cache.AuthorizationContext) bool { if authCtx.VerifierResponseCode != "" { return false } + // A refused session has been answered too. Reusing one would hand a + // fresh interaction the state and nonce the user already declined, + // and the decline recorded on it would then describe a request + // nobody made. + if authSessionIsTerminal(authCtx) { + return false + } if authCtx.ExpiresAt != 0 && authCtx.ExpiresAt <= time.Now().Unix() { return false } @@ -878,20 +885,55 @@ func (c *Client) IsActiveAuthSession(ctx context.Context, sessionID string) (act // client can retry instead of latching onto a bogus "pending" and // never reconciling the lost completion. func (c *Client) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) { + outcome, err := c.SessionOutcome(ctx, sessionID) + if err != nil { + return "", err + } + return outcome.ResponseCode, nil +} + +// SessionOutcome is what a polling UI needs to stop polling: the +// response code of a completed session, or the error of a declined one. +// +// The two used to be one value, and only one of them existed. A wallet +// that answered with an Authorization Error Response - a user pressing +// "no", which OpenID4VP 1.0 section 8.5 calls access_denied - left the +// session with no response code and no other way to say so, and +// /ui/completion reports a missing response code as "pending". The UI +// then polled until the cache evicted the context, and the relying +// party never learned of a decision the user had already made. +type SessionOutcome struct { + // ResponseCode is set when the wallet presented. + ResponseCode string + // Error and ErrorDescription are set when it refused. The + // description is the wallet's own text and is shown to an operator, + // never acted on. + Error string + ErrorDescription string +} + +// Declined reports whether the wallet answered with an error. +func (o SessionOutcome) Declined() bool { return o.Error != "" } + +func (c *Client) SessionOutcome(ctx context.Context, sessionID string) (SessionOutcome, error) { if sessionID == "" { - return "", nil + return SessionOutcome{}, nil } authCtx, err := c.cacheService.AuthContext.GetByID(ctx, sessionID) if err != nil { if errors.Is(err, cache.ErrNoDocuments) { - return "", nil + return SessionOutcome{}, nil } - return "", err + return SessionOutcome{}, err } if authCtx == nil { - return "", nil + return SessionOutcome{}, nil } - return authCtx.VerifierResponseCode, nil + return SessionOutcome{ + ResponseCode: authCtx.VerifierResponseCode, + Error: authCtx.Error, + ErrorDescription: authCtx.ErrorDescription, + }, nil } // UIResumeStatus names the possible outcomes of a /ui/resume call. @@ -910,6 +952,9 @@ const ( // UIResumeExpired means the auth context exists but its request object // has been evicted, so the QR / request_uri is no longer resolvable. UIResumeExpired UIResumeStatus = "expired" + // UIResumeDeclined means the wallet refused: the user said no, and + // there is nothing left to wait for. + UIResumeDeclined UIResumeStatus = "declined" ) // UIResumeReply carries one of three alternatives selected by Status: @@ -930,6 +975,10 @@ type UIResumeReply struct { // Complete-only field. ResponseCode string `json:"response_code,omitempty"` + + // Declined-only fields. + Error string `json:"error,omitempty"` + ErrorDescription string `json:"error_description,omitempty"` } // UIResume reports whether the wallet interaction named by sessionID is @@ -958,6 +1007,18 @@ func (c *Client) UIResume(ctx context.Context, sessionID string) (*UIResumeReply ResponseCode: authCtx.VerifierResponseCode, }, nil } + // A refusal is an outcome, so a reload must not rebuild the QR for + // it. Without this a reloaded tab resumed "pending" with the + // original QR for a request the user had already declined, and the + // two endpoints disagreed: /ui/completion said declined while + // /ui/resume offered the code again. + if authCtx.Error != "" { + return &UIResumeReply{ + Status: UIResumeDeclined, + Error: authCtx.Error, + ErrorDescription: authCtx.ErrorDescription, + }, nil + } // Past the presentation deadline there is nothing to resume. Checked // after the completed case, so a finished session still hands back its // response code, and before the request object, which outlives the diff --git a/internal/verifier/apiv1/handlers_verification.go b/internal/verifier/apiv1/handlers_verification.go index d68035b9b..05e1f03a3 100644 --- a/internal/verifier/apiv1/handlers_verification.go +++ b/internal/verifier/apiv1/handlers_verification.go @@ -8,11 +8,13 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "net/url" "slices" "strings" "github.com/SUNET/vc/pkg/cache" + "github.com/SUNET/vc/pkg/helpers" "github.com/SUNET/vc/pkg/jose" "github.com/SUNET/vc/pkg/mdoc" "github.com/SUNET/vc/pkg/model" @@ -75,6 +77,29 @@ func (c *Client) VerificationRequestObject(ctx context.Context, req *Verificatio type VerificationDirectPostRequest struct { Response string `json:"response" form:"response"` + + // Error, ErrorDescription and State carry an Authorization Error + // Response - the Wallet answering the request with a refusal + // instead of a presentation. + // + // OpenID4VP 1.0 section 8.3.1: "If a Wallet is unable to generate an + // encrypted response, it MAY send an error response without + // encryption as per Section 8.2." So a PLAIN error form is + // conformant even when the request asked for direct_post.jwt, and + // section 8.2's own example is exactly these three parameters + // posted as a form. + // + // Section 5.6 says both successful and error responses SHOULD use + // the supplied Response Mode, so an error may also arrive encrypted + // inside Response. Both are accepted; see directPostError. + // + // Until these existed a decline bound to an empty struct and the + // handler went looking for a JWE header in "", which failed as + // "unexpected end of JSON input" - a 500 for the one outcome that + // is not an error at all, with the session left pending forever. + Error string `json:"error,omitempty" form:"error"` + ErrorDescription string `json:"error_description,omitempty" form:"error_description"` + State string `json:"state,omitempty" form:"state"` // DCAPI marks a response returned inside a navigator.credentials.get // call and forwarded here by the verifier's own page, rather than POSTed // by a wallet reached through request_uri. The two bind their mdoc @@ -100,9 +125,122 @@ type VerificationDirectPostResponse struct { RedirectURI string `json:"redirect_uri,omitempty"` } +// directPostError records an Authorization Error Response and ends the +// session. +// +// The point is the ENDING. A declined request is an outcome the relying +// party is entitled to, and before this the only thing that happened was +// a log line: the session stayed pending until the cache evicted it, so +// a user who pressed "no" left the verifier waiting for an answer they +// had already given. +// +// state is validated against a live session (OpenID4VP 1.0 section +// 14.3.2: the Verifier SHOULD check that state corresponds to a recent +// Authorization Request). An unknown state is refused rather than +// ignored - accepting one would let anyone who can reach response_uri +// end a session they know nothing about, and silently ignoring one +// would leave the real session pending while answering 200. +// +// Nothing of the wallet's text is echoed back to the caller: the reply +// carries only the redirect, and the error reaches the operator through +// the session, not through this response body. +func (c *Client) directPostError(ctx context.Context, code, description, state string) (*VerificationDirectPostResponse, error) { + if state == "" { + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "an error response must carry the state of the request it answers", http.StatusBadRequest) + } + + authCtx, err := c.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: state}) + switch { + case errors.Is(err, cache.ErrNoDocuments) || (err == nil && authCtx == nil): + // Deliberately the same answer as a state that never existed: + // whether a given state is live is not something this endpoint + // should confirm to an unauthenticated caller. + c.log.Warn("direct_post error response for an unknown state", "error", code) + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "state does not correspond to a request this verifier is waiting for", http.StatusBadRequest) + case err != nil: + // A backend that cannot answer is NOT an unknown state. Saying + // 400 here would tell a wallet its own request was malformed + // during a Mongo outage, and the real session would stay + // pending with nobody retrying. 503 is the honest answer and + // names no backend detail. + c.log.Error(err, "could not look up the session for a direct_post error response", "error", code) + return nil, helpers.NewErrorDetailsWithStatus("temporarily_unavailable", + "the session could not be looked up; retry", http.StatusServiceUnavailable) + } + + if sessionExpired(authCtx) { + c.log.Info("direct_post error response for an expired session", "state", state, "error", code) + return nil, ErrSessionExpired + } + + // First outcome wins. Without this a late error overwrites a + // completed session while its VerifierResponseCode survives, so + // /ui/completion reports "declined" and /ui/resume reports + // "complete" for the same session - two answers, both from stored + // state, with no way to tell which happened. + if authSessionIsTerminal(authCtx) { + c.log.Info("direct_post error response for a session that already reached an outcome", + "state", state, "error", code, "status", authCtx.Status) + return &VerificationDirectPostResponse{}, nil + } + + c.log.Warn("the wallet returned an error response instead of a presentation", + "state", state, "error", code, "session_id", authCtx.SessionID) + + // Mutate a COPY. MemoryStore.Get hands back the stored pointer + // itself, and Save validates after these assignments - so a wallet + // sending an error_description past the model's 512-byte bound + // would fail the save with the live session already carrying the + // rejected value and a terminal status, while a Mongo-backed + // deployment kept the old one. The description is wallet-controlled + // input, which is exactly the kind that finds that edge. + updated := *authCtx + updated.Status = cache.SessionStatusError + updated.Error = code + updated.ErrorDescription = description + if err := c.cacheService.AuthContext.Save(ctx, &updated); err != nil { + c.log.Error(err, "failed to record the wallet's error response", "state", state) + return nil, err + } + authCtx = &updated + + // The same nudge a presentation sends, so a waiting UI learns now + // rather than on its next poll. The browser is told the outcome, not + // the wallet's prose. + c.notify.Submit(authCtx.SessionID, map[string]string{"error": code}) + + // OpenID4VP 1.0 section 8.2 allows the Response URI to return + // redirect_uri for an error response, and the engine reads it back + // (go-wallet-backend internal/engine/oid4vp.go submitErrorResponse), + // so a same-device decline can still land the user somewhere. + // Omitted here: where a declined flow should send the browser is a + // verifier UX decision, and inventing one would send users to a page + // nobody designed. 200 with an empty object is what the engine + // already tolerates. + return &VerificationDirectPostResponse{}, nil +} + func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDirectPostRequest) (*VerificationDirectPostResponse, error) { c.log.Debug("Verification direct-post") + // A refusal, before anything looks for a JWE. An error response + // carries no `response`, so every step below it - the KID, the + // ephemeral key, the decryption - is looking for something that was + // never sent. + if req.Error != "" && req.Response == "" { + return c.directPostError(ctx, req.Error, req.ErrorDescription, req.State) + } + + // Neither a presentation nor an error is a malformed request, and + // it is the caller's to fix. It used to reach GetKID and come back + // as a 500. + if req.Response == "" { + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "the response carries neither a presentation nor an error", http.StatusBadRequest) + } + // Extract KID from JWE header kid, err := req.GetKID() if err != nil { @@ -126,6 +264,23 @@ func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDi return nil, err } + // An error response returned the way section 5.6 says it SHOULD be - + // "Both successful and error responses SHOULD be returned using the + // supplied Response Mode" - so for direct_post.jwt it arrives + // encrypted, inside the same envelope a presentation would use. + // + // Checked before the presentation parse rather than after it fails, + // because an error response legitimately has no vp_token and would + // otherwise be reported as a malformed presentation. + var encryptedError struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + State string `json:"state"` + } + if err := json.Unmarshal(decryptedJWE, &encryptedError); err == nil && encryptedError.Error != "" { + return c.directPostError(ctx, encryptedError.Error, encryptedError.ErrorDescription, encryptedError.State) + } + // Parse response parameters using openid4vp vpResponse := openid4vp.VPResponse{} if err := json.Unmarshal(decryptedJWE, &vpResponse); err != nil { @@ -151,6 +306,19 @@ func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDi return nil, ErrSessionExpired } + // The other direction of "first outcome wins". A presentation that + // lands after the session was answered - the user declined and the + // wallet sent anyway, or a response is replayed - would otherwise + // write a VerifierResponseCode while Error and the terminal status + // stay put, and the two UI endpoints would then disagree about what + // happened. Refused rather than ignored: the wallet is handing over + // credentials, and a 200 would say they were accepted. + if authSessionIsTerminal(authCtx) { + c.log.Info("Verification direct post for a session that already reached an outcome", + "state", vpResponse.State, "status", authCtx.Status) + return nil, ErrSessionAlreadyAnswered + } + // Generate response code responseCode := uuid.NewString() callbackURL, err := url.JoinPath(c.cfg.Verifier.PublicURL, "/verification/callback") diff --git a/internal/verifier/httpserver/api.go b/internal/verifier/httpserver/api.go index a6747495b..a8bdee1b2 100644 --- a/internal/verifier/httpserver/api.go +++ b/internal/verifier/httpserver/api.go @@ -26,6 +26,7 @@ type Apiv1 interface { UIMetadata(ctx context.Context) (*apiv1.UIMetadataReply, error) IsActiveAuthSession(ctx context.Context, sessionID string) (active bool, expiresAt int64, err error) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) + SessionOutcome(ctx context.Context, sessionID string) (apiv1.SessionOutcome, error) UIResume(ctx context.Context, sessionID string) (*apiv1.UIResumeReply, error) // OIDC Provider diff --git a/internal/verifier/httpserver/endpoints_session_preference_test.go b/internal/verifier/httpserver/endpoints_session_preference_test.go index 64179d769..7ef08b3e3 100644 --- a/internal/verifier/httpserver/endpoints_session_preference_test.go +++ b/internal/verifier/httpserver/endpoints_session_preference_test.go @@ -268,6 +268,10 @@ func (unimplementedApiv1) UIMetadata(ctx context.Context) (*apiv1.UIMetadataRepl func (unimplementedApiv1) IsActiveAuthSession(ctx context.Context, sessionID string) (bool, int64, error) { panic("IsActiveAuthSession not implemented in test") } +func (unimplementedApiv1) SessionOutcome(ctx context.Context, sessionID string) (apiv1.SessionOutcome, error) { + panic("SessionOutcome not implemented in test") +} + func (unimplementedApiv1) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) { panic("CompletedResponseCode not implemented in test") } diff --git a/internal/verifier/httpserver/endpoints_ui.go b/internal/verifier/httpserver/endpoints_ui.go index 6845d1fe0..9c767f4cc 100644 --- a/internal/verifier/httpserver/endpoints_ui.go +++ b/internal/verifier/httpserver/endpoints_ui.go @@ -98,7 +98,7 @@ func (s *Service) endpointUICompletion(ctx context.Context, c *gin.Context) (any return nil, nil } - responseCode, err := s.apiv1.CompletedResponseCode(ctx, sessionID) + outcome, err := s.apiv1.SessionOutcome(ctx, sessionID) if err != nil { // A backend error (e.g. transient Mongo outage) must not be // collapsed to "pending" - the browser would stop retrying and @@ -107,11 +107,23 @@ func (s *Service) endpointUICompletion(ctx context.Context, c *gin.Context) (any span.SetStatus(codes.Error, err.Error()) return nil, err } - if responseCode == "" { + // A refusal is terminal. Reported before the response code, which a + // declined session does not have: without this the UI reads the + // missing code as "pending" and polls until the context is evicted, + // so a user who pressed "no" leaves the relying party waiting for an + // answer they already gave. + if outcome.Declined() { + return gin.H{ + "status": "declined", + "error": outcome.Error, + "error_description": outcome.ErrorDescription, + }, nil + } + if outcome.ResponseCode == "" { c.JSON(http.StatusOK, gin.H{"status": "pending"}) return nil, nil } - return gin.H{"status": "complete", "response_code": responseCode}, nil + return gin.H{"status": "complete", "response_code": outcome.ResponseCode}, nil } // endpointUIResume rebuilds the state of a verifier UI reloaded mid-flow. diff --git a/internal/verifier/staticembed/notify-decline.js b/internal/verifier/staticembed/notify-decline.js new file mode 100644 index 000000000..df2cf3bbb --- /dev/null +++ b/internal/verifier/staticembed/notify-decline.js @@ -0,0 +1,77 @@ +/** + * Reading a refusal out of an /ui/notify message. + * + * The notify stream carries two kinds of terminal news for a waiting + * page: a completion, which carries a redirect_uri, and a refusal, which + * does not. The listener used to filter on `redirect_uri` alone, so a + * refusal was dropped and the page waited for a wallet that had already + * answered. This is the half that decides, kept separate so it can be + * tested without an EventSource. + */ + +/** A refusal with no error code of its own. Section 8.5 of OpenID4VP + * 1.0 names access_denied for a user who did not consent, and that is + * what a wallet with nothing more specific to say sends. */ +const DEFAULT_DECLINE_ERROR = "access_denied"; + +/** + * The error code of a refusal carried by one SSE message, or "" when the + * message is not a refusal. + * + * A completion is never a refusal, even if some field of it happens to + * contain the substring "error": the presence of redirect_uri settles + * it, and that check comes first. + * + * The payload SHOULD be JSON. It is parsed leniently because this is the + * last hop of a flow the user is watching: a message that says "error" + * in a shape we did not predict is still better treated as the refusal + * it plainly is than left to time out. + * + * @param {unknown} data One SSE message's data field. + * @returns {string} The error code, or "" if this message is not a refusal. + */ +export function declineFromNotifyMessage(data) { + if (!data || typeof data !== "string") return ""; + if (data.includes("redirect_uri")) return ""; + if (!data.includes("error")) return ""; + + try { + const parsed = JSON.parse(data); + if (parsed && typeof parsed === "object") { + if (typeof parsed.error === "string" && parsed.error) return parsed.error; + // A well-formed object that named no code is still a + // refusal - "error_description only" is a shape wallets do + // send - and treating it as noise would hang the page. + if (typeof parsed.error_description === "string" && parsed.error_description) { + return DEFAULT_DECLINE_ERROR; + } + return ""; + } + } catch { + // Not JSON. Fall through to the textual form below. + } + + const match = data.match(/error[=:]\s*["']?([^"'\s,}]+)/); + if (match && match[1]) return match[1]; + return ""; +} + +/** + * The description a refusal carried, or "" when it carried none. Shown + * as supporting text and never acted on: it is the wallet's own prose. + * + * @param {unknown} data One SSE message's data field. + * @returns {string} + */ +export function declineDescriptionFromNotifyMessage(data) { + if (!data || typeof data !== "string") return ""; + try { + const parsed = JSON.parse(data); + if (parsed && typeof parsed === "object" && typeof parsed.error_description === "string") { + return parsed.error_description; + } + } catch { + return ""; + } + return ""; +} diff --git a/internal/verifier/staticembed/presentation-definition.html b/internal/verifier/staticembed/presentation-definition.html index dbd4a72b7..10d95935d 100644 --- a/internal/verifier/staticembed/presentation-definition.html +++ b/internal/verifier/staticembed/presentation-definition.html @@ -40,7 +40,7 @@

+ + -