fix(server): keep account closure going past one bad grant or request

GETTING_STARTED.md

@@ -899,7 +899,11 @@ request.
 **Closing an account.** `srv.RevokeSubjectGrants(ctx, subject)` revokes
 every grant the registry lists for the user, and, with a CIBA inbox,
 `srv.DenyBackchannelInteractions(ctx, subject, reason)` denies the
-requests still awaiting them. If you map pairwise subs
+requests still awaiting them. Neither sees a grant still in flight, an
+authorization code not yet exchanged or a CIBA approval not yet polled:
+refuse the subject at sign-in from then on, and call
+`RevokeSubjectGrants` again once `Limits.AuthorizationCodeLifetime` and
+`Limits.BackchannelAuthenticationRequestLifetime` have passed. If you map pairwise subs
 (`pairwise.NewMapping`), also call the store's `ForgetSubject(ctx, issuer,
 subject)` (`storage.PairwiseSubjectEraser`, which `memstore` and
 `sqlstore` implement): the subs relying parties hold for the user stop resolving,

server/backchannel_authentication.go

@@ -143,7 +143,7 @@
 // against the client's registration, and persists it for the
 // embedder's own out-of-band authentication component to later decide
 // via CompleteBackchannelAuthentication.
 func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBackchannelAuthenticationRequest) (BackchannelAuthenticationAction, error) {
 	if s.cfg.Endpoints.BackchannelAuthentication.IsZero() {
 		return s.backchannelBeginFail(ctx, "", newError(ErrorServerError, 500, "backchannel authentication is not configured", nil)), nil
 	}
@@ -768,27 +768,44 @@
 // decided or has expired (BackchannelAuthenticationStore's lookup doesn't
 // say); CompleteBackchannelAuthentication is what enforces both, so a
 // stale lookup can at most show a request that can no longer be
-// completed. An unknown handle is an ErrorInvalidRequest *Error.
+// completed. An unknown handle is an ErrorInvalidRequest *Error, and a
+// request whose client is no longer registered ErrorUnauthorizedClient;
+// a store or client repository that couldn't answer is
+// ErrorServerError.
 func (s *Server) LookupBackchannelInteraction(ctx context.Context, handle BackchannelAuthenticationHandle) (BackchannelInteractionRequest, error) {
 	if s.deps.Backchannel == nil {
 		return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "backchannel authentication is not configured", nil)
 	}
+	interaction, _, err := s.lookupBackchannelInteraction(ctx, handle)
+	return interaction, err
+}
+
+// lookupBackchannelInteraction is LookupBackchannelInteraction, also
+// returning the subject Dependencies.BackchannelSubjects bound the
+// request to ("" without one).
+func (s *Server) lookupBackchannelInteraction(ctx context.Context, handle BackchannelAuthenticationHandle) (BackchannelInteractionRequest, string, error) {
 	pending, err := s.deps.Backchannel.LookupBackchannelAuthentication(ctx, backchannelHandleHash(handle))
 	if err != nil {
-		return BackchannelInteractionRequest{}, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", err)
+		if clientStoreUnavailable(ctx, err) {
+			return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "failed to look up the backchannel authentication request", err)
+		}
+		return BackchannelInteractionRequest{}, "", newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", err)
 	}
 	request, err := s.decodeOwnRequestRecord(pending.Request)
 	if err != nil {
-		return BackchannelInteractionRequest{}, recordFailure(err, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", nil), "backchannel authentication request")
+		return BackchannelInteractionRequest{}, "", recordFailure(err, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", nil), "backchannel authentication request")
 	}
 	client, err := s.deps.Clients.ResolveClient(ctx, pending.ClientID)
 	if err != nil {
-		return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's client could not be resolved", err)
+		if clientStoreUnavailable(ctx, err) {
+			return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "the request's client could not be resolved", err)
+		}
+		return BackchannelInteractionRequest{}, "", newError(ErrorUnauthorizedClient, 400, "client is no longer registered", err)
 	}
 	interaction, err := s.backchannelInteraction(client, request.Parameters, request.Resources)
 	if err != nil {
-		return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's extension parameters could not be read back", err)
+		return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "the request's extension parameters could not be read back", err)
 	}
 	interaction.RequiredSubject = request.requiredSubjectForApplication()
-	return interaction, nil
+	return interaction, request.ResolvedSubject, nil
 }

server/backchannel_inbox.go

@@ -147,38 +147,57 @@ func (s *Server) fileBackchannelRequest(ctx context.Context, clientID fapi.Clien
 // application must make sure the user is subject, as it does before
 // approving.
 func (s *Server) ListBackchannelInteractions(ctx context.Context, subject SubjectID) ([]PendingBackchannelInteraction, error) {
+	pending, unreadable, err := s.listBackchannelInteractions(ctx, subject)
+	if err != nil {
+		return nil, err
+	}
+	if unreadable != nil {
+		return nil, unreadable
+	}
+	return pending, nil
+}
+
+// listBackchannelInteractions returns the requests awaiting subject,
+// and, separately, the entries it couldn't read (an undecodable record,
+// or a request the backchannel store couldn't answer for) joined into
+// unreadable, so one bad entry doesn't hide the others from
+// DenyBackchannelInteractions. err is for a listing that failed as a
+// whole.
+func (s *Server) listBackchannelInteractions(ctx context.Context, subject SubjectID) (pending []PendingBackchannelInteraction, unreadable, err error) {
 	if s.deps.BackchannelInbox == nil {
-		return nil, errors.New("server: list backchannel interactions: Dependencies.BackchannelInbox isn't set")
+		return nil, nil, errors.New("server: list backchannel interactions: Dependencies.BackchannelInbox isn't set")
 	}
 	if subject.String() == "" {
-		return nil, errors.New("server: list backchannel interactions: subject is empty")
+		return nil, nil, errors.New("server: list backchannel interactions: subject is empty")
 	}
 	entries, err := s.deps.BackchannelInbox.ListBackchannelRequests(ctx, storage.BackchannelInboxListing{
 		SubjectKey: s.backchannelInboxSubjectKey(subject.String()), Now: s.deps.Clock.Now(),
 	})
 	if err != nil {
-		return nil, fmt.Errorf("server: list backchannel interactions: %w", err)
+		return nil, nil, fmt.Errorf("server: list backchannel interactions: %w", err)
 	}
-	var out []PendingBackchannelInteraction
+	var bad []error
 	for _, entry := range entries {
-		pending, ok, err := s.pendingBackchannelInteraction(ctx, subject, entry)
+		p, ok, err := s.pendingBackchannelInteraction(ctx, subject, entry)
 		if err != nil {
-			return nil, fmt.Errorf("server: list backchannel interactions: %w", err)
+			bad = append(bad, fmt.Errorf("server: list backchannel interactions: %w", err))
+			continue
 		}
 		if ok {
-			out = append(out, pending)
+			pending = append(pending, p)
 		}
 	}
-	slices.SortFunc(out, func(a, b PendingBackchannelInteraction) int {
+	slices.SortFunc(pending, func(a, b PendingBackchannelInteraction) int {
 		return cmp.Or(a.ExpiresAt.Compare(b.ExpiresAt), cmp.Compare(a.ID, b.ID))
 	})
-	return out, nil
+	return pending, errors.Join(bad...), nil
 }
 
 // pendingBackchannelInteraction rebuilds entry's request, reporting
 // false for one that isn't a live request of subject's at this server:
-// written by another issuer, expired, or gone from the backchannel
-// store.
+// written by another issuer, expired, gone from the backchannel store,
+// from a client no longer registered, or bound to another subject than
+// the one it's filed under.
 func (s *Server) pendingBackchannelInteraction(ctx context.Context, subject SubjectID, entry storage.BackchannelInboxEntry) (PendingBackchannelInteraction, bool, error) {
 	var record backchannelInboxRecord
 	if err := decodeRecord(entry.Record, &record.Version, &record); err != nil {
@@ -191,14 +210,19 @@ func (s *Server) pendingBackchannelInteraction(ctx context.Context, subject Subj
 	if err != nil || sha256.Sum256([]byte(record.Handle)) != entry.HandleHash {
 		return PendingBackchannelInteraction{}, false, nil
 	}
-	interaction, err := s.LookupBackchannelInteraction(ctx, handle)
+	interaction, resolved, err := s.lookupBackchannelInteraction(ctx, handle)
 	if err != nil {
 		var serverErr *Error
-		if errors.As(err, &serverErr) && serverErr.Code() == ErrorInvalidRequest {
+		if errors.As(err, &serverErr) && (serverErr.Code() == ErrorInvalidRequest || serverErr.Code() == ErrorUnauthorizedClient) {
 			return PendingBackchannelInteraction{}, false, nil
 		}
 		return PendingBackchannelInteraction{}, false, err
 	}
+	// The request itself names who may approve it: an entry filed under
+	// someone else, as one a database writer moved could be, isn't theirs.
+	if resolved != subject.String() {
+		return PendingBackchannelInteraction{}, false, nil
+	}
 	return PendingBackchannelInteraction{ID: backchannelInboxID(entry.HandleHash), Handle: handle, Interaction: interaction, ExpiresAt: entry.ExpiresAt}, true, nil
 }
 
@@ -224,30 +248,36 @@ func (s *Server) unfileBackchannelRequest(ctx context.Context, handleHash [32]by
 // call or instance, or expired — or not recorded because the store
 // failed, which CompleteBackchannelAuthentication reports the same way,
 // so check ListBackchannelInteractions when every request must be gone.
-// It stops at any other failure, returning how many it had denied and
-// the error; the rest are still listed, so calling it again finishes
-// the job.
+// Any other failure, or an entry it can't read, doesn't stop it: it
+// denies every other request, then returns how many it denied with every
+// failure joined. What failed is still listed, so calling it again
+// finishes the job.
 //
 // DenyBackchannelInteractions doesn't check who is asking: the
 // application must make sure the request is subject's own, or an
 // administrator's.
 func (s *Server) DenyBackchannelInteractions(ctx context.Context, subject SubjectID, reason string) (int, error) {
-	pending, err := s.ListBackchannelInteractions(ctx, subject)
+	pending, unreadable, err := s.listBackchannelInteractions(ctx, subject)
 	if err != nil {
 		return 0, fmt.Errorf("server: deny backchannel interactions: %w", err)
 	}
 	denied := 0
+	failures := []error{unreadable}
 	for _, p := range pending {
 		err := s.CompleteBackchannelAuthentication(ctx, CompleteBackchannelAuthenticationRequest{Handle: p.Handle, Result: Deny(reason)})
 		var serverErr *Error
 		switch {
 		case err == nil:
 			denied++
 		case errors.As(err, &serverErr) && (serverErr.Code() == ErrorInvalidRequest || serverErr.Code() == ErrorExpiredToken):
-			// Decided or expired since it was listed.
+			// Decided or expired since it was listed: its entry is stale.
+			s.unfileBackchannelRequest(ctx, backchannelHandleHash(p.Handle))
 		default:
-			return denied, fmt.Errorf("server: deny backchannel interactions: %w", err)
+			failures = append(failures, err)
 		}
 	}
+	if err := errors.Join(failures...); err != nil {
+		return denied, fmt.Errorf("server: deny backchannel interactions: %w", err)
+	}
 	return denied, nil
 }

server/backchannel_inbox_test.go

@@ -455,3 +455,97 @@ func TestBackchannelSubjectsMustAgreeWithTheIDTokenHint(t *testing.T) {
 		t.Fatalf("begin with a resolver disagreeing with the id_token_hint = %+v, want server_error", action)
 	}
 }
+
+// TestBackchannelInboxSkipsADeregisteredClient: a request from a client
+// no longer registered leaves the listing, rather than failing it for
+// every request the user has.
+func TestBackchannelInboxSkipsADeregisteredClient(t *testing.T) {
+	h := inboxHarness(t, memstore.NewBackchannelInbox())
+	beginFiled(t, h, "user-1")
+	delete(h.clients.clients, testClientID)
+	if pending := inboxOf(t, h, "user-1"); len(pending) != 0 {
+		t.Errorf("inbox with the client deregistered = %d requests, want none", len(pending))
+	}
+	if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err != nil || n != 0 {
+		t.Errorf("DenyBackchannelInteractions = %d, %v, want 0, nil", n, err)
+	}
+}
+
+// outageBackchannelStore fails lookups as an outage while down is set.
+type outageBackchannelStore struct {
+	storage.BackchannelAuthenticationStore
+	down *bool
+}
+
+func (s outageBackchannelStore) LookupBackchannelAuthentication(ctx context.Context, handleHash [32]byte) (storage.LookedUpBackchannelAuthentication, error) {
+	if *s.down {
+		return storage.LookedUpBackchannelAuthentication{}, fmt.Errorf("db: %w", storage.ErrStoreUnavailable)
+	}
+	return s.BackchannelAuthenticationStore.LookupBackchannelAuthentication(ctx, handleHash)
+}
+
+// TestBackchannelInboxReportsAStoreOutage: a backchannel store that
+// can't answer fails the listing and the bulk denial, rather than
+// showing an empty inbox an account closure would take as done.
+func TestBackchannelInboxReportsAStoreOutage(t *testing.T) {
+	down := false
+	h := newHarnessWithBackchannelOptions(t, outageBackchannelStore{memstore.NewBackchannelAuthenticationStore(), &down}, nil, func(d *server.Dependencies) {
+		d.BackchannelSubjects = hintSubjects{known: map[string]bool{"user-1": true}}
+		d.BackchannelInbox = memstore.NewBackchannelInbox()
+	})
+	beginFiled(t, h, "user-1")
+	down = true
+	if _, err := h.server.ListBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1")); err == nil {
+		t.Error("ListBackchannelInteractions with the store down = nil error, want the outage")
+	}
+	if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err == nil || n != 0 {
+		t.Errorf("DenyBackchannelInteractions with the store down = %d, %v, want 0 and the outage", n, err)
+	}
+}
+
+// flakyRemoveInbox fails its first RemoveBackchannelRequest.
+type flakyRemoveInbox struct {
+	*memstore.BackchannelInbox
+	failed *bool
+}
+
+func (b flakyRemoveInbox) RemoveBackchannelRequest(ctx context.Context, handleHash [32]byte) error {
+	if !*b.failed {
+		*b.failed = true
+		return errors.New("inbox down")
+	}
+	return b.BackchannelInbox.RemoveBackchannelRequest(ctx, handleHash)
+}
+
+// TestDenyBackchannelInteractionsClearsStaleEntries: an entry left
+// listed after its request was decided (its removal failed) is removed
+// when a bulk denial finds the request already decided.
+func TestDenyBackchannelInteractionsClearsStaleEntries(t *testing.T) {
+	failed := false
+	h := inboxHarness(t, flakyRemoveInbox{memstore.NewBackchannelInbox(), &failed})
+	required := beginFiled(t, h, "user-1")
+	approveAs(t, h, required.Handle, required.Interaction.Scope, "user-1")
+	if len(inboxOf(t, h, "user-1")) != 1 {
+		t.Fatal("the decided request isn't still listed, so the removal failure wasn't exercised")
+	}
+	if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err != nil || n != 0 {
+		t.Fatalf("DenyBackchannelInteractions = %d, %v, want 0, nil", n, err)
+	}
+	if left := inboxOf(t, h, "user-1"); len(left) != 0 {
+		t.Errorf("inbox after the denial = %d requests, want the stale entry cleared", len(left))
+	}
+}
+
+// TestBackchannelInboxSkipsAnEntryFiledUnderSomeoneElse: an entry whose
+// request is bound to another user than the one it's filed under, as an
+// entry a database writer moved would be, isn't listed for them.
+func TestBackchannelInboxSkipsAnEntryFiledUnderSomeoneElse(t *testing.T) {
+	inbox := &stubInbox{BackchannelInbox: memstore.NewBackchannelInbox()}
+	h := inboxHarness(t, inbox)
+	required := beginFiled(t, h, "user-1")
+	handle := required.Handle.String()
+	inbox.extra = []storage.BackchannelInboxEntry{inboxEntry(t, h, testIssuer, "user-2", handle, sha256.Sum256([]byte(handle)))}
+	if pending := inboxOf(t, h, "user-2"); len(pending) != 0 {
+		t.Errorf("inbox of user-2 = %d requests, want user-1's request left out", len(pending))
+	}
+}

server/dependencies.go

@@ -192,9 +192,11 @@ type Dependencies struct {
 	// shows, from any instance. It needs BackchannelSubjects. A request
 	// is filed before it's stored, so a filing failure fails the
 	// request. Its entries hold the requests' backchannel authentication
-	// handles, so give it storage that encrypts at rest; the subject
+	// handles, so give it storage that encrypts at rest. The subject
 	// binding means a handle read from it can still only be approved by
-	// the user it was filed for. Under AssuranceProduction it must
+	// the user it was filed for; it can still end the request, though, as
+	// any handle can: CompleteBackchannelAuthentication records a Deny,
+	// or an approval by someone else, for whoever presents the handle. Under AssuranceProduction it must
 	// declare storage.StoreAssurance (Durable, and
 	// CrossInstanceConsistent with DeploymentHorizontallyScaled).
 	// Ignored unless Config.Endpoints.BackchannelAuthentication is set.

server/export_test.go

@@ -0,0 +1,5 @@
+package server
+
+// GrantEntryKey exposes grantEntryKey to the server_test package, so a
+// test can write a GrantRegistry entry as the server would.
+func GrantEntryKey(refreshTokenHash [32]byte) string { return grantEntryKey(refreshTokenHash) }