diff --git a/token/services/identity/driver/storage.go b/token/services/identity/driver/storage.go index 29670aef7e..7561fc8e26 100644 --- a/token/services/identity/driver/storage.go +++ b/token/services/identity/driver/storage.go @@ -83,7 +83,10 @@ type WalletID = string // //go:generate counterfeiter -o mock/wss.go -fake-name WalletStoreService . WalletStoreService type WalletStoreService interface { - // GetWalletID fetches a walletID that is bound to the identity passed + // GetWalletID fetches a walletID that is bound to the identity passed. + // It returns an empty WalletID and no error if the identity has no stored + // binding; a non-nil error indicates a genuine storage failure and must not + // be interpreted by callers as "no binding". GetWalletID(ctx context.Context, identity token.Identity, roleID int) (WalletID, error) // GetWalletIDs fetches all walletID's that have been stored so far without duplicates GetWalletIDs(ctx context.Context, roleID int) ([]WalletID, error) diff --git a/token/services/identity/role/registry.go b/token/services/identity/role/registry.go index 14bf084992..dceffce6b8 100644 --- a/token/services/identity/role/registry.go +++ b/token/services/identity/role/registry.go @@ -22,6 +22,87 @@ type WalletFactory interface { NewWallet(ctx context.Context, id idriver.WalletID, role idriver.IdentityRoleType, is IdentitySupport, info idriver.IdentityInfo) (driver.Wallet, error) } +// WalletIDStatus classifies the outcome of resolving an identity to the wallet id +// bound to it. It exists so callers branch on an explicit, named state instead of +// re-deriving intent from the ambiguous "(string, error)" shape — where ("", nil), +// ("", err) and ("id", nil) each mean something different and the difference is easy +// to get wrong (see issue #2063). +type WalletIDStatus int + +const ( + // WalletIDUnknown is the zero value and never returned by GetWalletID. It is the + // guard against a WalletIDResolution constructed without going through GetWalletID + // (a mock, or a future constructor that forgets to set Status): because callers act + // on a fallthrough only when authoritative() is true — Bound or Unbound — this zero + // value is treated as a lookup failure, not as a safe "no binding" that would create + // a duplicate wallet. + WalletIDUnknown WalletIDStatus = iota + // WalletIDBound means storage holds a wallet id for the identity. WalletID is set. + WalletIDBound + // WalletIDUnbound means storage answered authoritatively that the identity has no + // wallet binding. This is a definitive, successful miss: it is safe to fall through + // to the next resolution step and, ultimately, to create a wallet. + WalletIDUnbound + // WalletIDFailed means the storage lookup itself failed (timeout, connection reset, + // ...), so whether a binding exists is UNKNOWN. Err carries the cause. Callers MUST + // NOT treat this as WalletIDUnbound: doing so lets a transient blip masquerade as an + // unregistered identity and triggers the creation of a duplicate wallet. + WalletIDFailed +) + +// WalletIDResolution is the explicit result of resolving an identity to its bound +// wallet id. It is the single shared value every wallet-lookup fallback branches on, +// so the "not found" vs "could not check" distinction is decided once (in GetWalletID) +// rather than re-inferred at each call site. +type WalletIDResolution struct { + // Status is the outcome of the lookup. Always inspect it via Bound/Unbound/Failed + // before reading the other fields, and branch exhaustively: any status that is + // neither Bound nor Unbound (Failed, or the WalletIDUnknown zero value) is not + // authoritative and must abort the lookup rather than fall through to creation. + Status WalletIDStatus + // WalletID is the bound wallet id; meaningful only when Status is WalletIDBound. + WalletID idriver.WalletID + // Err is the underlying storage failure; set only when Status is WalletIDFailed. + Err error +} + +// Bound reports whether the identity has a wallet id bound to it. +func (r WalletIDResolution) Bound() bool { return r.Status == WalletIDBound } + +// Unbound reports whether storage answered authoritatively that the identity has no +// wallet binding. This is the ONLY non-Bound state that a caller may act on by falling +// through to the next resolution step and, ultimately, wallet creation. Every other +// state — Failed, or the zero value produced by a WalletIDResolution built without +// going through GetWalletID — leaves the binding unknown and must abort the lookup. +func (r WalletIDResolution) Unbound() bool { return r.Status == WalletIDUnbound } + +// Failed reports whether the storage lookup failed, leaving the binding unknown. +// A failed resolution must abort the enclosing lookup, never fall through to creation. +func (r WalletIDResolution) Failed() bool { return r.Status == WalletIDFailed } + +// authoritative reports whether the resolution definitively answers whether a wallet is +// bound — i.e. it came back from GetWalletID as Bound or Unbound. Any other status +// (Failed, or the zero-value WalletIDUnknown of a resolution built outside GetWalletID) +// is non-authoritative: the binding is unknown and the caller MUST abort rather than +// fall through to wallet creation. This is the guard the WalletIDUnknown zero value was +// introduced to provide. +func (r WalletIDResolution) authoritative() bool { return r.Bound() || r.Unbound() } + +// abortError returns the error a non-authoritative resolution must abort a wallet lookup +// with. It preserves the storage cause for a WalletIDFailed and synthesizes one for a +// zero-value / unknown resolution (whose Err is nil), so an unknown status can never +// collapse into a nil error — via errors.WithMessagef(nil, ...) returning nil — and be +// silently mistaken for a successful lookup. It must only be called once Bound and +// Unbound have been ruled out (i.e. authoritative() is false). +func (r WalletIDResolution) abortError(id driver.WalletLookupID) error { + cause := r.Err + if cause == nil { + cause = errors.Errorf("non-authoritative wallet id resolution status [%d]", r.Status) + } + + return errors.WithMessagef(cause, "failed to lookup wallet [%s]", id) +} + // Registry manages wallets whose long-term identities have a given role. // // Concurrency and invariants: @@ -82,11 +163,17 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver if ok { r.Logger.DebugfContext(ctx, "lookup failed, check if there is a wallet for identity [%s]", passedIdentity) // is this identity registered - wID, err := r.GetWalletID(ctx, passedIdentity) - if err == nil && len(wID) != 0 { - r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, wID) + res := r.GetWalletID(ctx, passedIdentity) + if !res.authoritative() { + // A storage failure — or a resolution that never went through GetWalletID — + // leaves the binding unknown; it must not be treated as "not registered", or + // a transient blip would fall through to wallet creation and duplicate state. + return nil, nil, "", res.abortError(id) + } + if res.Bound() { + r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID) // we got a hit - walletID = wID + walletID = res.WalletID ident = passedIdentity fail = false } @@ -111,36 +198,48 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver if ok { r.Logger.DebugfContext(ctx, "no wallet found, check if there is a wallet for identity [%s]", passedIdentity) // is this identity registered - passedWalletID, err := r.GetWalletID(ctx, passedIdentity) - if err == nil && len(passedWalletID) != 0 { - r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, passedWalletID) + res := r.GetWalletID(ctx, passedIdentity) + if !res.authoritative() { + // A storage failure — or a resolution that never went through GetWalletID — + // leaves the binding unknown; it must not be treated as "not registered", or + // a transient blip would fall through to wallet creation and duplicate state. + return nil, nil, "", res.abortError(id) + } + if res.Bound() { + r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID) // we got a hit r.WalletMu.RLock() - walletEntry, ok = r.Wallets[passedWalletID] + walletEntry, ok = r.Wallets[res.WalletID] r.WalletMu.RUnlock() if ok { - return walletEntry, nil, passedWalletID, nil + return walletEntry, nil, res.WalletID, nil } - r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, passedWalletID) + r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, res.WalletID) } - walletIdentifiers = append(walletIdentifiers, passedWalletID) + walletIdentifiers = append(walletIdentifiers, res.WalletID) } r.Logger.DebugfContext(ctx, "no wallet found for [%s] at [%s]", passedIdentity, logging.Prefix(wID)) if len(ident) != 0 { - identityWID, err := r.GetWalletID(ctx, ident) - r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%s]", ident, identityWID, err) - if err == nil && len(identityWID) != 0 { + res := r.GetWalletID(ctx, ident) + r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%d]", ident, res.WalletID, res.Status) + if !res.authoritative() { + // A storage failure — or a resolution that never went through GetWalletID — + // leaves the binding unknown; it must not be treated as "not registered", or + // a transient blip would fall through to wallet creation and duplicate state. + return nil, nil, "", res.abortError(id) + } + if res.Bound() { r.WalletMu.RLock() - w, ok := r.Wallets[identityWID] + w, ok := r.Wallets[res.WalletID] r.WalletMu.RUnlock() if ok { - r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), identityWID) + r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), res.WalletID) - return w, nil, identityWID, nil + return w, nil, res.WalletID, nil } } - walletIdentifiers = append(walletIdentifiers, identityWID) + walletIdentifiers = append(walletIdentifiers, res.WalletID) } for _, walletIdentifier := range walletIdentifiers { @@ -233,16 +332,31 @@ func (r *Registry) GetIdentityMetadata(ctx context.Context, identity driver.Iden return json.Unmarshal(raw, &meta) } -// GetWalletID returns the wallet identifier bound to the passed identity -func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) { +// GetWalletID resolves the wallet identifier bound to the passed identity. +// +// It is the single point that translates the storage layer's (WalletID, error) +// convention into an explicit WalletIDResolution, so no caller has to re-derive the +// meaning of ("", nil) vs ("", err). The storage contract reports an unbound identity +// as ("", nil); a non-nil error is a genuine storage failure (timeout, connection +// reset, ...) whose result is therefore WalletIDFailed, never WalletIDUnbound. Keeping +// the two apart here is what prevents a transient blip from looking like an +// unregistered identity and triggering the creation of a duplicate wallet (issue #2063). +func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) WalletIDResolution { wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID())) if err != nil { - //nolint:nilerr - return "", nil + return WalletIDResolution{ + Status: WalletIDFailed, + Err: errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity), + } + } + if len(wID) == 0 { + r.Logger.DebugfContext(ctx, "no wallet bound to identity [%s]", identity) + + return WalletIDResolution{Status: WalletIDUnbound} } r.Logger.DebugfContext(ctx, "wallet [%s] is bound to identity [%s]", wID, identity) - return wID, nil + return WalletIDResolution{Status: WalletIDBound, WalletID: wID} } func (r *Registry) WalletByID(ctx context.Context, role idriver.IdentityRoleType, id driver.WalletLookupID) (driver.Wallet, error) { diff --git a/token/services/identity/role/registry_test.go b/token/services/identity/role/registry_test.go index ad0ddd00cc..61cbfa2f88 100644 --- a/token/services/identity/role/registry_test.go +++ b/token/services/identity/role/registry_test.go @@ -135,17 +135,100 @@ func TestBindIdentityAndContainsAndMetadataAndGetWalletID(t *testing.T) { require.NoError(t, reg.GetIdentityMetadata(ctx, []byte("id"), "w", &meta)) require.Equal(t, "v", meta["k"]) - // GetWalletID when storage returns value + // GetWalletID when storage returns a bound wallet id. storage.GetWalletIDReturns("w", nil) - wid, err := reg.GetWalletID(ctx, []byte("id")) - require.NoError(t, err) - require.Equal(t, "w", wid) - - // GetWalletID when storage returns error -> suppressed to empty string + res := reg.GetWalletID(ctx, []byte("id")) + require.Equal(t, role.WalletIDBound, res.Status) + require.True(t, res.Bound()) + require.NoError(t, res.Err) + require.Equal(t, "w", res.WalletID) + + // GetWalletID when storage authoritatively reports no binding -> WalletIDUnbound, + // a clean miss with no error and no wallet id. + storage.GetWalletIDReturns("", nil) + resUnbound := reg.GetWalletID(ctx, []byte("id")) + require.Equal(t, role.WalletIDUnbound, resUnbound.Status) + require.False(t, resUnbound.Bound()) + require.True(t, resUnbound.Unbound()) + require.False(t, resUnbound.Failed()) + require.NoError(t, resUnbound.Err) + require.Empty(t, resUnbound.WalletID) + + // GetWalletID when storage fails -> WalletIDFailed with the error preserved, so a + // transient storage blip cannot masquerade as "no binding". storage.GetWalletIDReturns("", errors.New("boom")) - wid2, err2 := reg.GetWalletID(ctx, []byte("id")) - require.NoError(t, err2) - require.Empty(t, wid2) + resFailed := reg.GetWalletID(ctx, []byte("id")) + require.Equal(t, role.WalletIDFailed, resFailed.Status) + require.True(t, resFailed.Failed()) + require.False(t, resFailed.Bound()) + require.False(t, resFailed.Unbound()) + require.Error(t, resFailed.Err) + require.Empty(t, resFailed.WalletID) +} + +// TestWalletIDResolution_ZeroValueIsNotAuthoritative guards the WalletIDUnknown zero +// value: a WalletIDResolution built without going through GetWalletID (a mock, or a +// future constructor that forgets to set Status) must be indistinguishable from a +// failure at every call site, never from an authoritative "unbound" miss. Callers act +// on a fallthrough only when a resolution is Bound or Unbound, so the zero value — +// being neither — is treated as a lookup failure rather than a safe "create a wallet". +func TestWalletIDResolution_ZeroValueIsNotAuthoritative(t *testing.T) { + var zero role.WalletIDResolution + + require.Equal(t, role.WalletIDUnknown, zero.Status) + require.False(t, zero.Bound(), "zero value must not read as a bound wallet id") + require.False(t, zero.Unbound(), "zero value must not read as an authoritative miss") + require.False(t, zero.Failed(), "zero value carries no storage error") + require.NoError(t, zero.Err) + require.Empty(t, zero.WalletID) +} + +// TestLookup_StorageErrorDoesNotFallThrough pins the fix for #2063: a transient +// storage error out of GetWalletID must abort Lookup rather than be swallowed as +// "no binding". Otherwise the fallback chain would fall through to wallet creation +// and persist a duplicate wallet for an identity that already has one. +func TestLookup_StorageErrorDoesNotFallThrough(t *testing.T) { + t.Run("MapToIdentity fails, view-identity fallback hits storage error", func(t *testing.T) { + reg, storage, role, wf := newRegistryWithFakes() + ctx := t.Context() + + role.MapToIdentityReturns(nil, "", errors.New("no mapping")) + storage.GetWalletIDReturns("", errors.New("transient DB blip")) + + _, _, _, err := reg.Lookup(ctx, []byte("id-with-binding")) + require.Error(t, err) + require.Equal(t, 0, wf.NewWalletCallCount()) + }) + + t.Run("MapToIdentity succeeds, cache miss, view-identity fallback hits storage error", func(t *testing.T) { + reg, storage, role, wf := newRegistryWithFakes() + ctx := t.Context() + + // mapping resolves to a wallet id that is not in the cache, so Lookup falls + // through to the identity->wallet storage probe, which fails transiently. + role.MapToIdentityReturns([]byte("id-with-binding"), "w-not-cached", nil) + storage.GetWalletIDReturns("", errors.New("transient DB blip")) + + _, _, _, err := reg.Lookup(ctx, []byte("id-with-binding")) + require.Error(t, err) + require.Equal(t, 0, wf.NewWalletCallCount()) + }) +} + +// TestWalletByID_StorageErrorDoesNotCreateDuplicate ensures that when the cache +// misses and the storage lookup fails transiently, WalletByID surfaces the error +// instead of creating a brand-new wallet (the duplicate-wallet bug of #2063). +func TestWalletByID_StorageErrorDoesNotCreateDuplicate(t *testing.T) { + reg, storage, role, wf := newRegistryWithFakes() + ctx := t.Context() + + role.MapToIdentityReturns(nil, "", errors.New("no mapping")) + storage.GetWalletIDReturns("", errors.New("transient DB blip")) + + w, err := reg.WalletByID(ctx, 0, []byte("id-with-binding")) + require.Error(t, err) + require.Nil(t, w) + require.Equal(t, 0, wf.NewWalletCallCount()) } func TestWalletIDs_MergesRoleAndStorage(t *testing.T) { diff --git a/token/services/storage/db/kvs/walletdb.go b/token/services/storage/db/kvs/walletdb.go index c3472b2f8b..45730319cc 100644 --- a/token/services/storage/db/kvs/walletdb.go +++ b/token/services/storage/db/kvs/walletdb.go @@ -85,9 +85,16 @@ func (s *WalletStore) GetWalletID(ctx context.Context, identity driver2.Identity if err != nil { return "", errors.Wrapf(err, "failed to create key") } + // kvs.Get returns an error both for a missing key and for a real store failure. + // The WalletStoreService contract requires that "no binding" be reported as ("", nil) + // so that callers can distinguish it from a transient storage error; probe with Exists + // first (as IdentityExists does) and only Get when a binding is present. + if !s.kvs.Exists(ctx, k) { + return "", nil + } var wID storage.WalletID if err := s.kvs.Get(ctx, k, &wID); err != nil { - return "", err + return "", errors.Wrapf(err, "failed to get wallet id for identity [%v]", idHash) } return wID, nil diff --git a/token/services/storage/db/kvs/walletdb_test.go b/token/services/storage/db/kvs/walletdb_test.go index 1912378a38..b6261e5a7e 100644 --- a/token/services/storage/db/kvs/walletdb_test.go +++ b/token/services/storage/db/kvs/walletdb_test.go @@ -50,3 +50,34 @@ func TestWalletStoreGetConfID(t *testing.T) { require.NoError(t, err) assert.Empty(t, got) } + +// TestWalletStoreGetWalletID asserts the not-found contract that the role Registry relies on to +// tell a transient storage error apart from "this identity has no binding": an unbound identity +// must resolve to ("", nil), and a bound identity must round-trip its wallet id. If GetWalletID +// returned an error for a missing key (as kvs.Get does), the registry would abort every lookup +// for a genuinely-unregistered identity instead of creating its wallet. +func TestWalletStoreGetWalletID(t *testing.T) { + backend, err := NewInMemory() + require.NoError(t, err) + // NewInMemory shares a global in-memory backing store across the package's tests, so use a + // tmsID and identities unique to this test to stay isolated from any other stored bindings. + tmsID := token.TMSID{Network: "getwalletid", Channel: "getwalletid", Namespace: "getwalletid"} + db := NewWalletStore(backend, tmsID) + ctx := t.Context() + + // miss: never bound -> ("", nil), NOT an error + got, err := db.GetWalletID(ctx, []byte("gwid-grace"), 0) + require.NoError(t, err) + assert.Empty(t, got) + + // bound under role 0 -> round-trips the wallet id + require.NoError(t, db.StoreIdentity(ctx, []byte("gwid-grace"), "eID", "grace_wallet", 0, nil, "conf-1")) + got, err = db.GetWalletID(ctx, []byte("gwid-grace"), 0) + require.NoError(t, err) + assert.Equal(t, "grace_wallet", got) + + // the same identity under a different role is still an independent miss + got, err = db.GetWalletID(ctx, []byte("gwid-grace"), 1) + require.NoError(t, err) + assert.Empty(t, got) +} diff --git a/token/services/storage/db/sql/common/wallet_test_utils.go b/token/services/storage/db/sql/common/wallet_test_utils.go index 5736980508..bb59786580 100644 --- a/token/services/storage/db/sql/common/wallet_test_utils.go +++ b/token/services/storage/db/sql/common/wallet_test_utils.go @@ -37,6 +37,28 @@ func TestGetWalletID(t *testing.T, store walletStoreConstructor) { gomega.Expect(actualWalletID).To(gomega.Equal(output)) } +// TestGetWalletIDNotFound pins the not-found contract of the WalletStoreService: an identity with +// no stored binding resolves to ("", nil), never an error. The role Registry relies on this to tell +// a transient storage failure apart from "this identity was never registered" (issue #2063). +func TestGetWalletIDNotFound(t *testing.T, store walletStoreConstructor) { + gomega.RegisterTestingT(t) + db, mockDB, err := sqlmock.New() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + tokenID := token.Identity([]byte("1234")) + roleID := 5 + mockDB. + ExpectQuery("SELECT wallet_id FROM WALLETS WHERE \\(identity_hash = \\$1\\) AND \\(role_id = \\$2\\)"). + WithArgs(tokenID.UniqueID(), roleID). + WillReturnError(sql.ErrNoRows) + + actualWalletID, err := store(db).GetWalletID(t.Context(), tokenID, roleID) + + gomega.Expect(mockDB.ExpectationsWereMet()).To(gomega.Succeed()) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(actualWalletID).To(gomega.BeEmpty()) +} + func TestGetWalletIDs(t *testing.T, store walletStoreConstructor) { gomega.RegisterTestingT(t) db, mockDB, err := sqlmock.New() diff --git a/token/services/storage/db/sql/postgres/wallet_test.go b/token/services/storage/db/sql/postgres/wallet_test.go index 654c830e22..1fd6318953 100644 --- a/token/services/storage/db/sql/postgres/wallet_test.go +++ b/token/services/storage/db/sql/postgres/wallet_test.go @@ -24,6 +24,10 @@ func TestGetWalletID(t *testing.T) { common2.TestGetWalletID(t, mockWalletStore) } +func TestGetWalletIDNotFound(t *testing.T) { + common2.TestGetWalletIDNotFound(t, mockWalletStore) +} + func TestGetWalletIDs(t *testing.T) { common2.TestGetWalletIDs(t, mockWalletStore) } diff --git a/token/services/storage/db/sql/sqlite/wallet_test.go b/token/services/storage/db/sql/sqlite/wallet_test.go index 635fe91859..85f7315d9e 100644 --- a/token/services/storage/db/sql/sqlite/wallet_test.go +++ b/token/services/storage/db/sql/sqlite/wallet_test.go @@ -24,6 +24,10 @@ func TestGetWalletID(t *testing.T) { common2.TestGetWalletID(t, mockWalletStore) } +func TestGetWalletIDNotFound(t *testing.T) { + common2.TestGetWalletIDNotFound(t, mockWalletStore) +} + func TestGetWalletIDs(t *testing.T) { common2.TestGetWalletIDs(t, mockWalletStore) }