-
Notifications
You must be signed in to change notification settings - Fork 111
identity/role: GetWalletID swallows storage errors, so a transient DB blip creates a duplicate wallet #2170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor, and I think pre-existing: when |
||
| } | ||
|
|
||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Either keep |
||
| 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) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Worth adding one more |
||
| }) | ||
|
|
||
| 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) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
// fabric-smart-client@v0.17.0/platform/view/services/storage/kvs/kvs.go:110-113
it, err := o.store.GetStateSetIterator(ctx, o.namespace, ids...)
if err != nil {
return result // <- error dropped, empty result
}So on a genuine failure (connection reset, timeout, The awkward part is that
Either way, a KVS test that makes the underlying store fail and asserts the error propagates would lock this down — right now |
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc says
WalletIDUnknown"guards against aWalletIDResolutionthat was constructed without going throughGetWalletID", but nothing actually guards. With onlyBound()andFailed()defined, a zero-valueWalletIDResolution{}is indistinguishable fromWalletIDUnboundat every call site — i.e. the zero value silently means "authoritative miss, safe to create a wallet", which is precisely the interpretation this type was introduced to make impossible. A mock, or a future constructor that forgets to setStatus, gets the dangerous default.Related: the field comment below tells callers to inspect via
Bound/Unbound/Failed, but there is noUnbound()method. Adding it and branching exhaustively fixes both:…then at each call site treat "neither
Bound()norUnbound()" as a failure.