From f54858431b93347b76b446c860dd1a16444a66d3 Mon Sep 17 00:00:00 2001 From: Zahin Mohammad Date: Fri, 14 Nov 2025 18:27:26 -0500 Subject: [PATCH 1/2] feat(sdk-core): update wallet share acceptance for multi-user-key TICKET: WP-6769 --- modules/bitgo/test/v2/unit/wallets.ts | 469 +++++++++++++++++- modules/sdk-core/src/bitgo/wallet/iWallet.ts | 9 + modules/sdk-core/src/bitgo/wallet/iWallets.ts | 7 + modules/sdk-core/src/bitgo/wallet/wallets.ts | 71 ++- 4 files changed, 548 insertions(+), 8 deletions(-) diff --git a/modules/bitgo/test/v2/unit/wallets.ts b/modules/bitgo/test/v2/unit/wallets.ts index a55e4b200d..094bc83aa2 100644 --- a/modules/bitgo/test/v2/unit/wallets.ts +++ b/modules/bitgo/test/v2/unit/wallets.ts @@ -1384,7 +1384,7 @@ describe('V2 Wallets:', function () { const shareId = 'test_case_1'; const walletShareNock = nock(bgUrl) - .get(`/api/v2/tbtc/walletshare/${shareId}`) + .get(`/api/v2/ofc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1416,7 +1416,7 @@ describe('V2 Wallets:', function () { }); const acceptShareNock = nock(bgUrl) - .post(`/api/v2/tbtc/walletshare/${shareId}`, (body: any) => { + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { return false; } @@ -1455,7 +1455,7 @@ describe('V2 Wallets:', function () { }); const acceptShareNock = nock(bgUrl) - .post(`/api/v2/tbtc/walletshare/${shareId}`, (body: any) => { + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { return false; } @@ -1519,7 +1519,7 @@ describe('V2 Wallets:', function () { const enterpriseId = 'test_case_6'; const walletShareNock = nock(bgUrl) - .get(`/api/v2/tbtc/walletshare/${shareId}`) + .get(`/api/v2/ofc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1534,7 +1534,7 @@ describe('V2 Wallets:', function () { }); const acceptShareNock = nock(bgUrl) - .post(`/api/v2/tbtc/walletshare/${shareId}`, (body: any) => { + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { return false; } @@ -1645,7 +1645,7 @@ describe('V2 Wallets:', function () { const enterpriseId = 'test_case_6'; const walletShareNock = nock(bgUrl) - .get(`/api/v2/tbtc/walletshare/${shareId}`) + .get(`/api/v2/ofc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1660,7 +1660,7 @@ describe('V2 Wallets:', function () { }); const acceptShareNock = nock(bgUrl) - .post(`/api/v2/tbtc/walletshare/${shareId}`, (body: any) => { + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { return false; } @@ -1728,6 +1728,461 @@ describe('V2 Wallets:', function () { }); }); + describe('Wallet share where userMultiKeyRotationRequired is set true', () => { + const sandbox = sinon.createSandbox(); + let ofcWallets; + + before(function () { + const ofcCoin = bitgo.coin('ofc'); + ofcWallets = ofcCoin.wallets(); + }); + + afterEach(function () { + sandbox.verifyAndRestore(); + }); + + it('should throw error when password not provided', async function () { + const shareId = 'test_multi_key_1'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + }); + + await ofcWallets + .acceptShare({ walletShareId: shareId }) + .should.be.rejectedWith('userPassword param must be provided to generate user keychain'); + walletShareNock.done(); + }); + + it('should successfully accept share with userMultiKeyRotationRequired and send pub and encryptedPrv', async function () { + const shareId = 'test_multi_key_2'; + const userPassword = 'test_password_123'; + const walletId = 'test_wallet_123'; + + // Mock wallet share response + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + wallet: walletId, + }); + + // Mock keychain creation - baseCoin.keychains().create() returns { prv, pub } + // We need to create a proper keychain object with prv property + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, // OFC uses secp256k1 keys, so prv format + pub: testKeychain.pub, + }; + + // Stub keychains().create() and encrypt() BEFORE calculating encryptedPrv + const keychainsInstance = ofcWallets.baseCoin.keychains(); + const keychainsStub = sandbox.stub(keychainsInstance, 'create').returns(keychain); + + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + const encryptStub = sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + // Mock the updateShare API call + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { + // Verify that pub and encryptedPrv are included + if (body.walletShareId !== shareId || body.state !== 'accepted' || !body.pub || !body.encryptedPrv) { + return false; + } + // Verify pub is a valid format (secp256k1 public key for OFC) + if (!ofcWallets.baseCoin.isValidPub(body.pub)) { + return false; + } + return true; + }) + .reply(200, { changed: true, state: 'accepted' }); + + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); + should.equal(res.changed, true); + should.equal(res.state, 'accepted'); + + // Verify keychain creation was called + should.equal(keychainsStub.calledOnce, true); + // Verify encrypt was called with the correct parameters + should.equal(encryptStub.calledWith({ input: keychain.prv, password: userPassword }), true); + + walletShareNock.done(); + acceptShareNock.done(); + }); + + it('should NOT trigger reshare when userMultiKeyRotationRequired is true', async function () { + const shareId = 'test_multi_key_3'; + const userPassword = 'test_password_123'; + const walletId = 'test_wallet_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + wallet: walletId, + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { changed: true, state: 'accepted' }); + + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + // Stub reshareWalletWithSpenders to verify it's NOT called + const reshareStub = sandbox.stub(Wallets.prototype, 'reshareWalletWithSpenders'); + + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); + should.equal(res.changed, true); + should.equal(res.state, 'accepted'); + + // Verify reshare was NOT called (unlike keychainOverrideRequired case) + should.equal(reshareStub.called, false); + + walletShareNock.done(); + acceptShareNock.done(); + }); + + it('should handle bulk accept share with userMultiKeyRotationRequired', async function () { + const shareId = 'test_multi_key_bulk_1'; + const userPassword = 'test_password_123'; + + // Mock listSharesV2 to return a share with userMultiKeyRotationRequired + // Note: userMultiKeyRotationRequired shares don't have a keychain, so they won't be filtered + // by the bulkAcceptShare filter, but processAcceptShare will handle them + sandbox.stub(Wallets.prototype, 'listSharesV2').resolves({ + incoming: [ + { + id: shareId, + coin: 'ofc', + walletLabel: 'test wallet', + fromUser: 'fromUser', + toUser: 'toUser', + wallet: 'wallet123', + permissions: ['admin', 'spend', 'view'], + state: 'active', + userMultiKeyRotationRequired: true, + // No keychain - this is the key difference for multi-user-key shares + }, + ], + outgoing: [], + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Set up stubs first + const keychainsInstance = ofcWallets.baseCoin.keychains(); + const keychainsStub = sandbox.stub(keychainsInstance, 'create').returns(keychain); + + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + const encryptStub = sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + // Mock bulk update API - this is called via bulkUpdateWalletShare + nock(bgUrl) + .put('/api/v2/walletshares/update', (body: any) => { + if (!body.shares || body.shares.length !== 1) { + return false; + } + const share = body.shares[0]; + return ( + share.walletShareId === shareId && + share.status === 'accept' && + share.pub === keychain.pub && + share.encryptedPrv === encryptedPrv + ); + }) + .reply(200, { + acceptedWalletShares: [{ walletShareId: shareId }], + rejectedWalletShares: [], + walletShareUpdateErrors: [], + }); + + // Note: bulkAcceptShare filters for shares WITH keychains, but userMultiKeyRotationRequired + // shares don't have keychains, so they go through a different path + // We need to stub the processAcceptShare path or use bulkUpdateWalletShare directly + // For this test, we'll use bulkUpdateWalletShare which calls processAcceptShare internally + const result = await ofcWallets.bulkUpdateWalletShare({ + shares: [{ walletShareId: shareId, status: 'accept' }], + userLoginPassword: userPassword, + }); + + should.equal(result.acceptedWalletShares.length, 1); + should.deepEqual(result.acceptedWalletShares[0], { walletShareId: 'test_multi_key_bulk_1' }); + should.equal(keychainsStub.calledOnce, true); + should.equal(encryptStub.calledWith({ input: keychain.prv, password: userPassword }), true); + }); + + it('should work with newWalletPassphrase parameter', async function () { + const shareId = 'test_multi_key_4'; + const userPassword = 'test_password_123'; + const newWalletPassphrase = 'new_wallet_passphrase_456'; + const walletId = 'test_wallet_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + wallet: walletId, + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Stub create() and encrypt() BEFORE calculating encryptedPrv to ensure consistency + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + + // Should encrypt with newWalletPassphrase if provided (as per implementation) + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: newWalletPassphrase }); + const encryptStub = sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.pub === keychain.pub && + body.encryptedPrv === encryptedPrv + ); + }) + .reply(200, { changed: true, state: 'accepted' }); + + const res = await ofcWallets.acceptShare({ + walletShareId: shareId, + userPassword, + newWalletPassphrase, + }); + + should.equal(res.changed, true); + should.equal(res.state, 'accepted'); + // Verify encrypt was called with newWalletPassphrase + should.equal(encryptStub.calledWith({ input: keychain.prv, password: newWalletPassphrase }), true); + + walletShareNock.done(); + acceptShareNock.done(); + }); + + it('should prioritize userMultiKeyRotationRequired over keychainOverrideRequired', async function () { + const shareId = 'test_multi_key_5'; + const userPassword = 'test_password_123'; + const walletId = 'test_wallet_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + keychainOverrideRequired: true, // Both set, but userMultiKeyRotationRequired should take precedence + permissions: ['admin', 'spend', 'view'], + wallet: walletId, + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Set up stubs first + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + // Should use the multi-user-key flow (no signature/payload/keyId) + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { + // Verify it's using multi-user-key flow (has pub and encryptedPrv, but no signature/payload/keyId) + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.pub === keychain.pub && + body.encryptedPrv === encryptedPrv && + !body.signature && + !body.payload && + !body.keyId + ); + }) + .reply(200, { changed: true, state: 'accepted' }); + const reshareStub = sandbox.stub(Wallets.prototype, 'reshareWalletWithSpenders'); + + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); + + should.equal(res.changed, true); + should.equal(res.state, 'accepted'); + // Verify reshare was NOT called (multi-user-key flow) + should.equal(reshareStub.called, false); + + walletShareNock.done(); + acceptShareNock.done(); + }); + + it('should handle keychain creation failure', async function () { + const shareId = 'test_multi_key_6'; + const userPassword = 'test_password_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + }); + + const keychainError = new Error('Failed to create keychain'); + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').throws(keychainError); + + await ofcWallets + .acceptShare({ walletShareId: shareId, userPassword }) + .should.be.rejectedWith('Failed to create keychain'); + + walletShareNock.done(); + }); + + it('should handle encryption failure', async function () { + const shareId = 'test_multi_key_7'; + const userPassword = 'test_password_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + const encryptionError = new Error('Encryption failed'); + sandbox.stub(bitgo, 'encrypt').throws(encryptionError); + + await ofcWallets + .acceptShare({ walletShareId: shareId, userPassword }) + .should.be.rejectedWith('Encryption failed'); + + walletShareNock.done(); + }); + + it('should work with view-only permissions', async function () { + const shareId = 'test_multi_key_8'; + const userPassword = 'test_password_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['view'], // View-only permissions + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Set up stubs first + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.pub === keychain.pub && + body.encryptedPrv === encryptedPrv + ); + }) + .reply(200, { changed: true, state: 'accepted' }); + + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); + + should.equal(res.changed, true); + should.equal(res.state, 'accepted'); + + walletShareNock.done(); + acceptShareNock.done(); + }); + + it('should verify exact values sent in API request', async function () { + const shareId = 'test_multi_key_9'; + const userPassword = 'test_password_123'; + const walletId = 'test_wallet_123'; + + const walletShareNock = nock(bgUrl) + .get(`/api/v2/ofc/walletshare/${shareId}`) + .reply(200, { + userMultiKeyRotationRequired: true, + permissions: ['admin', 'spend', 'view'], + wallet: walletId, + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Set up stubs first + const keychainsInstance = ofcWallets.baseCoin.keychains(); + sandbox.stub(keychainsInstance, 'create').returns(keychain); + + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: userPassword }); + sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + let capturedBody: any; + const acceptShareNock = nock(bgUrl) + .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { + capturedBody = body; + return true; + }) + .reply(200, { changed: true, state: 'accepted' }); + + await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); + + // Verify exact values + should.equal(capturedBody.walletShareId, shareId); + should.equal(capturedBody.state, 'accepted'); + should.equal(capturedBody.pub, keychain.pub); + should.equal(capturedBody.encryptedPrv, encryptedPrv); + should.not.exist(capturedBody.signature); + should.not.exist(capturedBody.payload); + should.not.exist(capturedBody.keyId); + + walletShareNock.done(); + acceptShareNock.done(); + }); + }); + it('should share a wallet to viewer', async function () { const shareId = '12311'; diff --git a/modules/sdk-core/src/bitgo/wallet/iWallet.ts b/modules/sdk-core/src/bitgo/wallet/iWallet.ts index 80123bff15..81c1d3f4db 100644 --- a/modules/sdk-core/src/bitgo/wallet/iWallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/iWallet.ts @@ -657,7 +657,16 @@ export interface WalletShare { enterprise?: string; message?: string; pendingApprovalId?: string; + /** + * If true, the wallet share requires the user to provide a new keychain. + * When accepted, the wallet will be re-shared with all other spenders. + */ keychainOverrideRequired?: boolean; + /** + * If true, the wallet share requires the user to provide to generate a new key-pair. + * In addition to the encrypted private key, the user must provide the public key. + * */ + userMultiKeyRotationRequired?: boolean; isUMSInitiated?: boolean; keychain?: BulkWalletShareKeychain; } diff --git a/modules/sdk-core/src/bitgo/wallet/iWallets.ts b/modules/sdk-core/src/bitgo/wallet/iWallets.ts index f1389a5ab5..81b05c5255 100644 --- a/modules/sdk-core/src/bitgo/wallet/iWallets.ts +++ b/modules/sdk-core/src/bitgo/wallet/iWallets.ts @@ -120,6 +120,7 @@ export interface UpdateShareOptions { keyId?: string; signature?: string; payload?: string; + pub?: string; } export interface AcceptShareOptions { @@ -138,6 +139,11 @@ export interface BulkAcceptShareOptions { export interface AcceptShareOptionsRequest { walletShareId: string; encryptedPrv: string; + /** + * The public associated to the encrypted private key. + * Required for userMultiKeyRotationRequired shares. + */ + pub?: string; } export interface BulkUpdateWalletShareOptions { @@ -156,6 +162,7 @@ export interface BulkUpdateWalletShareOptionsRequest { keyId?: string; signature?: string; payload?: string; + pub?: string; } export interface BulkUpdateWalletShareResponse { diff --git a/modules/sdk-core/src/bitgo/wallet/wallets.ts b/modules/sdk-core/src/bitgo/wallet/wallets.ts index 77faeeac69..f56e76794f 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallets.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallets.ts @@ -882,6 +882,32 @@ export class Wallets implements IWallets { let encryptedPrv = params.overrideEncryptedPrv; const walletShare = await this.getShare({ walletShareId: params.walletShareId }); + + // Multi-user-key case: requires user to provide their own public key in addition to the encrypted private key + if (walletShare.userMultiKeyRotationRequired) { + if (_.isUndefined(params.userPassword)) { + throw new Error('userPassword param must be provided to generate user keychain'); + } + + const walletKeychain = this.baseCoin.keychains().create(); + const encryptedPrv = this.bitgo.encrypt({ + password: params.newWalletPassphrase || params.userPassword, + input: walletKeychain.prv, + }); + + const updateParams: UpdateShareOptions = { + walletShareId: params.walletShareId, + state: 'accepted', + encryptedPrv: encryptedPrv, + pub: walletKeychain.pub, + }; + + // Note: Unlike keychainOverrideRequired, we do NOT reshare the wallet with spenders + // This is a key difference - multi-user-key wallets don't require reshare + return this.updateShare(updateParams); + } + + // Keychain override case: requires user keychain creation and signing if ( walletShare.keychainOverrideRequired && walletShare.permissions.indexOf('admin') !== -1 && @@ -927,6 +953,7 @@ export class Wallets implements IWallets { } return response; } + // Return right away if there is no keychain to decrypt, or if explicit encryptedPrv was provided if (!walletShare.keychain || !walletShare.keychain.encryptedPrv || encryptedPrv) { return this.updateShare({ @@ -1011,7 +1038,7 @@ export class Wallets implements IWallets { const walletShares = params.walletShareIds .map((walletShareId) => walletShareMap[walletShareId]) - .filter((walletShare) => walletShare && walletShare.keychain); + .filter((walletShare) => walletShare && (walletShare.keychain || walletShare.userMultiKeyRotationRequired)); if (!walletShares.length) { throw new Error('No valid wallet shares found to accept'); @@ -1028,6 +1055,26 @@ export class Wallets implements IWallets { }); const newWalletPassphrase = params.newWalletPassphrase || params.userLoginPassword; const keysForWalletShares = walletShares.flatMap((walletShare) => { + // Handle userMultiKeyRotationRequired case - these shares don't have keychains + if (walletShare.userMultiKeyRotationRequired) { + if (!params.userLoginPassword) { + throw new Error('userLoginPassword param must be provided to generate user keychain'); + } + const walletKeychain = this.baseCoin.keychains().create(); + const encryptedPrv = this.bitgo.encrypt({ + password: newWalletPassphrase, + input: walletKeychain.prv, + }); + return [ + { + walletShareId: walletShare.id, + encryptedPrv: encryptedPrv, + pub: walletKeychain.pub, + }, + ]; + } + + // Standard case: shares with keychains if (!walletShare.keychain) { return []; } @@ -1292,6 +1339,28 @@ export class Wallets implements IWallets { ]; } + // Multi-user-key case: requires user to provide their own public key + if (walletShare.userMultiKeyRotationRequired) { + if (!userLoginPassword) { + throw new Error('userLoginPassword param must be provided to generate user keychain'); + } + + const walletKeychain = this.baseCoin.keychains().create(); + const encryptedPrv = this.bitgo.encrypt({ + password: newWalletPassphrase || userLoginPassword, + input: walletKeychain.prv, + }); + + return [ + { + walletShareId, + status: 'accept' as const, + encryptedPrv: encryptedPrv, + pub: walletKeychain.pub, + }, + ]; + } + // Return right away if there is no keychain to decrypt if (!walletShare.keychain || !walletShare.keychain.encryptedPrv) { return [ From b8780146792be01bca897313ba64ebfa0b6d237e Mon Sep 17 00:00:00 2001 From: Zahin Mohammad Date: Fri, 14 Nov 2025 19:11:19 -0500 Subject: [PATCH 2/2] fix(sdk-core): use newWalletPassphrase in processAcceptShare for bulk operations Fixed processAcceptShare to correctly use newWalletPassphrase when provided for both keychainOverrideRequired and userMultiKeyRotationRequired cases in bulk wallet share operations. Updated validation to accept either newWalletPassphrase or userLoginPassword for userMultiKeyRotationRequired shares. Added comprehensive tests to verify: - Bulk accept share with userMultiKeyRotationRequired and newWalletPassphrase - Bulk accept share with keychainOverrideRequired and newWalletPassphrase - Error handling when neither password is provided - Correct usage of newWalletPassphrase vs userLoginPassword Fixed existing keychainOverrideRequired tests to use correct API endpoints (ofc instead of tbtc) and made nock matchers more flexible to handle dynamic signatures and payloads. WP-6769 TICKET: WP-6769 --- modules/bitgo/test/v2/unit/wallets.ts | 285 ++++++++++++++++--- modules/sdk-core/src/bitgo/wallet/iWallet.ts | 2 +- modules/sdk-core/src/bitgo/wallet/wallets.ts | 6 +- 3 files changed, 250 insertions(+), 43 deletions(-) diff --git a/modules/bitgo/test/v2/unit/wallets.ts b/modules/bitgo/test/v2/unit/wallets.ts index 094bc83aa2..7707001000 100644 --- a/modules/bitgo/test/v2/unit/wallets.ts +++ b/modules/bitgo/test/v2/unit/wallets.ts @@ -37,6 +37,7 @@ import * as moduleBitgo from '@bitgo/sdk-core'; describe('V2 Wallets:', function () { const bitgo = TestBitGo.decorate(BitGo, { env: 'mock' }); let wallets; + let ofcWallets; let bgUrl; before(function () { @@ -46,6 +47,7 @@ describe('V2 Wallets:', function () { const basecoin = bitgo.coin('tbtc'); wallets = basecoin.wallets(); + ofcWallets = bitgo.coin('ofc').wallets(); bgUrl = common.Environments[bitgo.getEnv()].uri; }); @@ -517,8 +519,6 @@ describe('V2 Wallets:', function () { }); it('should generate Go Account wallet', async () => { - const ofcWallets = bitgo.coin('ofc').wallets(); - const params: GenerateWalletOptions = { label: 'Go Account Wallet', passphrase: 'go_account_password', @@ -1391,7 +1391,7 @@ describe('V2 Wallets:', function () { }); // Validate accept share case - await wallets + await ofcWallets .acceptShare({ walletShareId: shareId }) .should.be.rejectedWith('userPassword param must be provided to decrypt shared key'); walletShareNock.done(); @@ -1403,13 +1403,13 @@ describe('V2 Wallets:', function () { const userPassword = 'test_case_2'; // create a user key const keyChainNock = nock(bgUrl) - .post('/api/v2/tbtc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) + .post('/api/v2/ofc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) .reply(200, (uri, requestBody) => { return { id: keychainId, encryptedPrv: requestBody['encryptedPrv'], pub: requestBody['pub'] }; }); const walletShareInfoNock = nock(bgUrl) - .get(`/api/v2/tbtc/walletshare/${shareId}`) + .get(`/api/v2/ofc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1417,17 +1417,21 @@ describe('V2 Wallets:', function () { const acceptShareNock = nock(bgUrl) .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { - if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { - return false; - } - return true; + // Check required fields are present (signature and payload are dynamic) + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.keyId === keychainId && + typeof body.signature === 'string' && + typeof body.payload === 'string' + ); }) .reply(200, { changed: false }); // Stub wallet share wallet method const walletShareStub = sandbox.stub(Wallet.prototype, 'shareWallet').onCall(0).resolves('success'); - const res = await wallets.acceptShare({ walletShareId: shareId, userPassword }); + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); should.equal(res.changed, false); keyChainNock.done(); walletShareInfoNock.done(); @@ -1442,13 +1446,13 @@ describe('V2 Wallets:', function () { // create a user key const keyChainNock = nock(bgUrl) - .post('/api/v2/tbtc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) + .post('/api/v2/ofc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) .reply(200, (uri, requestBody) => { return { id: keychainId, encryptedPrv: requestBody['encryptedPrv'], pub: requestBody['pub'] }; }); const walletShareInfoNock = nock(bgUrl) - .get(`/api/v2/tbtc/walletshare/${shareId}`) + .get(`/api/v2/ofc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1456,17 +1460,21 @@ describe('V2 Wallets:', function () { const acceptShareNock = nock(bgUrl) .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { - if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { - return false; - } - return true; + // Check required fields are present (signature and payload are dynamic) + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.keyId === keychainId && + typeof body.signature === 'string' && + typeof body.payload === 'string' + ); }) .reply(200, { changed: true, state: 'not_accepted' }); // Stub wallet share wallet method const walletShareStub = sandbox.stub(Wallet.prototype, 'shareWallet').onCall(0).resolves('success'); - const res = await wallets.acceptShare({ walletShareId: shareId, userPassword }); + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); should.equal(res.changed, true); should.equal(res.state, 'not_accepted'); keyChainNock.done(); @@ -1528,22 +1536,26 @@ describe('V2 Wallets:', function () { // create a user key const keyChainCreateNock = nock(bgUrl) - .post('/api/v2/tbtc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) + .post('/api/v2/ofc/key', _.conforms({ pub: (p) => p.startsWith('xpub') })) .reply(200, (uri, requestBody) => { return { id: keychainId, encryptedPrv: requestBody['encryptedPrv'], pub: requestBody['pub'] }; }); const acceptShareNock = nock(bgUrl) .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { - if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { - return false; - } - return true; + // Check required fields are present (signature and payload are dynamic) + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.keyId === keychainId && + typeof body.signature === 'string' && + typeof body.payload === 'string' + ); }) .reply(200, { changed: true, state: 'accepted' }); const walletInfoNock = nock(bgUrl) - .get(`/api/v2/tbtc/wallet/${walletId}`) + .get(`/api/v2/ofc/wallet/${walletId}`) .reply(200, { users: [spenderUserOne.payload, spenderUserTwo.payload, adminUser.payload, viewerUser.payload], enterprise: enterpriseId, @@ -1588,7 +1600,7 @@ describe('V2 Wallets:', function () { skipKeychain: false, }; - const res = await wallets.acceptShare({ walletShareId: shareId, userPassword }); + const res = await ofcWallets.acceptShare({ walletShareId: shareId, userPassword }); should.equal(res.changed, true); should.equal(res.state, 'accepted'); keyChainCreateNock.done(); @@ -1645,7 +1657,7 @@ describe('V2 Wallets:', function () { const enterpriseId = 'test_case_6'; const walletShareNock = nock(bgUrl) - .get(`/api/v2/ofc/walletshare/${shareId}`) + .get(`/api/v2/tbtc/walletshare/${shareId}`) .reply(200, { keychainOverrideRequired: true, permissions: ['admin', 'spend', 'view'], @@ -1660,11 +1672,15 @@ describe('V2 Wallets:', function () { }); const acceptShareNock = nock(bgUrl) - .post(`/api/v2/ofc/walletshare/${shareId}`, (body: any) => { - if (body.walletShareId !== shareId || body.state !== 'accepted' || body.keyId !== keychainId) { - return false; - } - return true; + .post(`/api/v2/tbtc/walletshare/${shareId}`, (body: any) => { + // Check required fields are present (signature and payload are dynamic) + return ( + body.walletShareId === shareId && + body.state === 'accepted' && + body.keyId === keychainId && + typeof body.signature === 'string' && + typeof body.payload === 'string' + ); }) .reply(200, { changed: true, state: 'accepted' }); @@ -1730,12 +1746,6 @@ describe('V2 Wallets:', function () { describe('Wallet share where userMultiKeyRotationRequired is set true', () => { const sandbox = sinon.createSandbox(); - let ofcWallets; - - before(function () { - const ofcCoin = bitgo.coin('ofc'); - ofcWallets = ofcCoin.wallets(); - }); afterEach(function () { sandbox.verifyAndRestore(); @@ -1929,6 +1939,126 @@ describe('V2 Wallets:', function () { should.equal(encryptStub.calledWith({ input: keychain.prv, password: userPassword }), true); }); + it('should handle bulk accept share with userMultiKeyRotationRequired and newWalletPassphrase', async function () { + const shareId = 'test_multi_key_bulk_2'; + const userPassword = 'test_password_123'; + const newWalletPassphrase = 'new_wallet_passphrase_456'; + + // Mock listSharesV2 to return a share with userMultiKeyRotationRequired + sandbox.stub(Wallets.prototype, 'listSharesV2').resolves({ + incoming: [ + { + id: shareId, + coin: 'ofc', + walletLabel: 'test wallet', + fromUser: 'fromUser', + toUser: 'toUser', + wallet: 'wallet123', + permissions: ['admin', 'spend', 'view'], + state: 'active', + userMultiKeyRotationRequired: true, + // No keychain - this is the key difference for multi-user-key shares + }, + ], + outgoing: [], + }); + + const testKeychain = bitgo.coin('ofc').keychains().create(); + const keychain = { + prv: testKeychain.prv, + pub: testKeychain.pub, + }; + + // Set up stubs first + const keychainsInstance = ofcWallets.baseCoin.keychains(); + const keychainsStub = sandbox.stub(keychainsInstance, 'create').returns(keychain); + + // Should encrypt with newWalletPassphrase if provided (as per implementation) + const encryptedPrv = bitgo.encrypt({ input: keychain.prv, password: newWalletPassphrase }); + const encryptStub = sandbox.stub(bitgo, 'encrypt').returns(encryptedPrv); + + // Mock bulk update API - this is called via bulkUpdateWalletShare + nock(bgUrl) + .put('/api/v2/walletshares/update', (body: any) => { + if (!body.shares || body.shares.length !== 1) { + return false; + } + const share = body.shares[0]; + return ( + share.walletShareId === shareId && + share.status === 'accept' && + share.pub === keychain.pub && + share.encryptedPrv === encryptedPrv + ); + }) + .reply(200, { + acceptedWalletShares: [{ walletShareId: shareId }], + rejectedWalletShares: [], + walletShareUpdateErrors: [], + }); + + const result = await ofcWallets.bulkUpdateWalletShare({ + shares: [{ walletShareId: shareId, status: 'accept' }], + userLoginPassword: userPassword, + newWalletPassphrase: newWalletPassphrase, + }); + + should.equal(result.acceptedWalletShares.length, 1); + should.deepEqual(result.acceptedWalletShares[0], { walletShareId: shareId }); + should.equal(keychainsStub.calledOnce, true); + // Verify encrypt was called with newWalletPassphrase (not userPassword) + should.equal(encryptStub.calledWith({ input: keychain.prv, password: newWalletPassphrase }), true); + }); + + it('should throw error when neither userLoginPassword nor newWalletPassphrase provided for bulk accept', async function () { + const shareId = 'test_multi_key_bulk_3'; + + // Mock listSharesV2 to return a share with userMultiKeyRotationRequired + sandbox.stub(Wallets.prototype, 'listSharesV2').resolves({ + incoming: [ + { + id: shareId, + coin: 'ofc', + walletLabel: 'test wallet', + fromUser: 'fromUser', + toUser: 'toUser', + wallet: 'wallet123', + permissions: ['admin', 'spend', 'view'], + state: 'active', + userMultiKeyRotationRequired: true, + }, + ], + outgoing: [], + }); + + // Mock user settings API call (required by bulkUpdateWalletShare) + nock(bgUrl).get('/api/v1/user/settings').reply(200, {}); + + // Mock bulkUpdateWalletShareRequest - when there are errors, it's called with empty array + nock(bgUrl) + .put('/api/v2/walletshares/update', (body: any) => { + return body.shares && body.shares.length === 0; + }) + .reply(200, { + acceptedWalletShares: [], + rejectedWalletShares: [], + walletShareUpdateErrors: [], + }); + + // bulkUpdateWalletShare uses Promise.allSettled, so errors are caught and returned in walletShareUpdateErrors + // We need to check the result for errors instead of expecting a rejection + const result = await ofcWallets.bulkUpdateWalletShare({ + shares: [{ walletShareId: shareId, status: 'accept' }], + }); + + // Verify that the error was captured in walletShareUpdateErrors + should.equal(result.walletShareUpdateErrors.length, 1); + should.equal(result.walletShareUpdateErrors[0].walletShareId, shareId); + should(result.walletShareUpdateErrors[0].reason).containEql( + 'userLoginPassword param must be provided to generate user keychain' + ); + }); + it('should work with newWalletPassphrase parameter', async function () { const shareId = 'test_multi_key_4'; const userPassword = 'test_password_123'; @@ -2838,8 +2968,9 @@ describe('V2 Wallets:', function () { ]); }); - it('should handle special override cases and reshare with spenders', async () => { - const walletPassphrase = 'bitgo1234'; + it('should handle keychainOverrideRequired with newWalletPassphrase in bulk operations', async () => { + const userLoginPassword = 'bitgo1234'; + const newWalletPassphrase = 'new_wallet_passphrase_456'; // Mock listSharesV2 to return a share with keychainOverrideRequired sinon.stub(Wallets.prototype, 'listSharesV2').resolves({ @@ -2859,18 +2990,92 @@ describe('V2 Wallets:', function () { outgoing: [], }); + // Mock user settings API call (required by bulkUpdateWalletShare) + nock(bgUrl).get('/api/v1/user/settings').reply(200, {}); + + // Mock getECDHKeychain (required by bulkUpdateWalletShare when userLoginPassword is provided) + sinon.stub(bitgo, 'getECDHKeychain').resolves({ + encryptedXprv: 'encryptedXprv', + }); + // Setup for the baseCoin.keychains().createUserKeychain const userKeychain = { id: 'key1', pub: 'pubKey', encryptedPrv: 'encryptedPrivateKey', }; - sinon.stub(wallets.baseCoin.keychains(), 'createUserKeychain').resolves(userKeychain); + const createUserKeychainStub = sinon + .stub(wallets.baseCoin.keychains(), 'createUserKeychain') + .resolves(userKeychain); // Mock decrypt and signMessage sinon.stub(bitgo, 'decrypt').returns('decryptedPrivateKey'); sinon.stub(wallets.baseCoin, 'signMessage').resolves(Buffer.from('signature')); + // Mock bulkUpdateWalletShareRequest + const bulkUpdateStub = sinon.stub(Wallets.prototype, 'bulkUpdateWalletShareRequest').resolves({ + acceptedWalletShares: ['share1'], + rejectedWalletShares: [], + walletShareUpdateErrors: [], + }); + + // Mock reshareWalletWithSpenders (called for keychainOverrideRequired cases) + const reshareStub = sinon.stub(Wallets.prototype, 'reshareWalletWithSpenders').resolves(); + + const result = await wallets.bulkUpdateWalletShare({ + shares: [{ walletShareId: 'share1', status: 'accept' }], + userLoginPassword: userLoginPassword, + newWalletPassphrase: newWalletPassphrase, + }); + + // Verify createUserKeychain was called with newWalletPassphrase (not userLoginPassword) + createUserKeychainStub.calledOnce.should.be.true(); + createUserKeychainStub.calledWith(newWalletPassphrase).should.be.true(); + + // Verify the result + assert.deepEqual(result, { + acceptedWalletShares: ['share1'], + rejectedWalletShares: [], + walletShareUpdateErrors: [], + }); + + bulkUpdateStub.calledOnce.should.be.true(); + // Verify reshare was called for keychainOverrideRequired case + reshareStub.calledOnce.should.be.true(); + }); + + it('should handle special override cases and reshare with spenders', async () => { + const walletPassphrase = 'bitgo1234'; + // Mock listSharesV2 to return a share with keychainOverrideRequired + sinon.stub(Wallets.prototype, 'listSharesV2').resolves({ + incoming: [ + { + id: 'share1', + coin: 'ofc', + walletLabel: 'testing', + fromUser: 'dummyFromUser', + toUser: 'dummyToUser', + wallet: 'wallet1', + permissions: ['admin', 'spend'], + state: 'active', + keychainOverrideRequired: true, + }, + ], + outgoing: [], + }); + + // Setup for the baseCoin.keychains().createUserKeychain + const userKeychain = { + id: 'key1', + pub: 'pubKey', + encryptedPrv: 'encryptedPrivateKey', + }; + sinon.stub(ofcWallets.baseCoin.keychains(), 'createUserKeychain').resolves(userKeychain); + + // Mock decrypt and signMessage + sinon.stub(bitgo, 'decrypt').returns('decryptedPrivateKey'); + sinon.stub(ofcWallets.baseCoin, 'signMessage').resolves(Buffer.from('signature')); + // Mock getECDHKeychain sinon.stub(bitgo, 'getECDHKeychain').resolves({ encryptedXprv: 'encryptedXprv', @@ -2886,7 +3091,7 @@ describe('V2 Wallets:', function () { // Mock reshareWalletWithSpenders const reshareStub = sinon.stub(Wallets.prototype, 'reshareWalletWithSpenders').resolves(); - const result = await wallets.bulkUpdateWalletShare({ + const result = await ofcWallets.bulkUpdateWalletShare({ shares: [{ walletShareId: 'share1', status: 'accept' }], userLoginPassword: walletPassphrase, }); diff --git a/modules/sdk-core/src/bitgo/wallet/iWallet.ts b/modules/sdk-core/src/bitgo/wallet/iWallet.ts index 81c1d3f4db..6af9e33dce 100644 --- a/modules/sdk-core/src/bitgo/wallet/iWallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/iWallet.ts @@ -663,7 +663,7 @@ export interface WalletShare { */ keychainOverrideRequired?: boolean; /** - * If true, the wallet share requires the user to provide to generate a new key-pair. + * If true, the wallet share requires the user to to generate a new key-pair. * In addition to the encrypted private key, the user must provide the public key. * */ userMultiKeyRotationRequired?: boolean; diff --git a/modules/sdk-core/src/bitgo/wallet/wallets.ts b/modules/sdk-core/src/bitgo/wallet/wallets.ts index f56e76794f..e4d136f35e 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallets.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallets.ts @@ -1310,7 +1310,9 @@ export class Wallets implements IWallets { throw new Error('userLoginPassword param must be provided to decrypt shared key'); } - const walletKeychain = await this.baseCoin.keychains().createUserKeychain(userLoginPassword); + const walletKeychain = await this.baseCoin + .keychains() + .createUserKeychain(newWalletPassphrase || userLoginPassword); if (!walletKeychain.encryptedPrv) { throw new Error('encryptedPrv was not found on wallet keychain'); } @@ -1341,7 +1343,7 @@ export class Wallets implements IWallets { // Multi-user-key case: requires user to provide their own public key if (walletShare.userMultiKeyRotationRequired) { - if (!userLoginPassword) { + if (!(newWalletPassphrase || userLoginPassword)) { throw new Error('userLoginPassword param must be provided to generate user keychain'); }