Skip to content

feat(kyc-controller): throw on all errors rather than silently log #10292

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.

Sign up for GitHub

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

Open
jiexi wants to merge 1 commit into
base: main
Choose a base branch
from
+89 −64
Open

feat(kyc-controller): throw on all errors rather than silently log #10292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/kyc-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- **BREAKING:** `KycController` methods that previously recorded a failure only on state (`phase: 'error'`) now also throw after recording it.
- Affects `initialize` (vendor customer creation), `createVendorCustomer`, `acceptTermsAndStartSession` (missing T&C2 / email / terms), `checkKycRequired`, and the MoonPay frame `fail` callback (`handleFrameMessage`).
- A product-scoped MoonPay auto-run therefore rejects `handleFrameMessage` / `onAuthenticated` when the KYC-required check fails.
- Bump `@metamask/profile-sync-controller` from `^32.1.0` to `^32.1.1` ([#10220](https://github.com/MetaMask/core/pull/10220))

## [0.3.0]
Expand Down
126 changes: 76 additions & 50 deletions packages/kyc-controller/src/KycController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,9 @@ describe('KycController', () => {
},
},
async ({ controller, handlers }) => {
await controller.acceptTermsAndStartSession();
await expect(
controller.acceptTermsAndStartSession(),
).rejects.toThrow(/Missing T&C2 acceptance/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing T&C2 acceptance/u);
Expand Down Expand Up @@ -885,10 +887,12 @@ describe('KycController', () => {
},
},
async ({ controller }) => {
await controller.acceptTermsAndStartSession({
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
});
await expect(
controller.acceptTermsAndStartSession({
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
}),
).rejects.toThrow(/Missing email/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing email/u);
Expand All @@ -898,11 +902,13 @@ describe('KycController', () => {

it('fails when no disclaimers were accepted', async () => {
await withController(async ({ controller }) => {
await controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
});
await expect(
controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
}),
).rejects.toThrow(/Missing terms acceptance/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing terms acceptance/u);
Expand Down Expand Up @@ -1162,7 +1168,9 @@ describe('KycController', () => {
async ({ controller, handlers, launcher, moonPayFrames }) => {
handlers.checkKycRequired.mockRejectedValue(new Error('down'));

await moonPayFrames.options.onAuthenticated();
await expect(
moonPayFrames.options.onAuthenticated(),
).rejects.toThrow(/KYC check failed/u);

expect(controller.state.phase).toBe('error');
expect(launcher.launch).not.toHaveBeenCalled();
Expand Down Expand Up @@ -1220,7 +1228,9 @@ describe('KycController', () => {

it('records an error when the handler reports a failure', async () => {
await withController(({ controller, moonPayFrames }) => {
moonPayFrames.options.fail('Check frame returned status: failed');
expect(() =>
moonPayFrames.options.fail('Check frame returned status: failed'),
).toThrow('Check frame returned status: failed');

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toBe(
Expand All @@ -1233,9 +1243,9 @@ describe('KycController', () => {
describe('checkKycRequired', () => {
it('fails without an access token', async () => {
await withController(async ({ controller }) => {
expect(await controller.checkKycRequired({ product: 'ramps' })).toBe(
false,
);
await expect(
controller.checkKycRequired({ product: 'ramps' }),
).rejects.toThrow(/Missing moonpayAccessToken/u);
expect(controller.state.error).toMatch(/Missing moonpayAccessToken/u);
});
});
Expand All @@ -1244,9 +1254,9 @@ describe('KycController', () => {
await withController(
{ options: { state: { moonpayAccessToken: 'a' } } },
async ({ controller }) => {
expect(await controller.checkKycRequired({ product: 'ramps' })).toBe(
false,
);
await expect(
controller.checkKycRequired({ product: 'ramps' }),
).rejects.toThrow(/Missing country/u);
expect(controller.state.error).toMatch(/Missing country/u);
},
);
Expand Down Expand Up @@ -1293,9 +1303,9 @@ describe('KycController', () => {
async ({ controller, handlers }) => {
handlers.checkKycRequired.mockRejectedValue(new Error('down'));

expect(await controller.checkKycRequired({ product: 'ramps' })).toBe(
false,
);
await expect(
controller.checkKycRequired({ product: 'ramps' }),
).rejects.toThrow(/KYC check failed/u);
expect(controller.state.error).toMatch(/KYC check failed/u);
},
);
Expand Down Expand Up @@ -2699,7 +2709,9 @@ describe('KycController', () => {
await withController(async ({ controller, handlers }) => {
handlers.createVendorCustomer.mockRejectedValue(new Error('iron down'));

await controller.initialize({ email: 'a@b.co', vendor: 'iron' });
await expect(
controller.initialize({ email: 'a@b.co', vendor: 'iron' }),
).rejects.toThrow(/Vendor customer creation failed/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(
Expand All @@ -2722,7 +2734,9 @@ describe('KycController', () => {
new Error('iron down'),
);

await controller.initialize({ email: 'a@b.co', vendor: 'iron' });
await expect(
controller.initialize({ email: 'a@b.co', vendor: 'iron' }),
).rejects.toThrow(/Vendor customer creation failed/u);

expect(controller.state.phase).toBe('error');
expect(
Expand Down Expand Up @@ -3041,10 +3055,12 @@ describe('KycController', () => {
await withController(async ({ controller, handlers }) => {
handlers.createVendorCustomer.mockRejectedValue(new Error('nope'));

await controller.createVendorCustomer({
vendor: 'iron',
email: 'a@b.co',
});
await expect(
controller.createVendorCustomer({
vendor: 'iron',
email: 'a@b.co',
}),
).rejects.toThrow(/Vendor customer creation failed/u);

expect(controller.state.activeVendor).toBe('iron');
expect(controller.state.email).toBe('a@b.co');
Expand All @@ -3064,10 +3080,12 @@ describe('KycController', () => {
async ({ controller, handlers }) => {
handlers.createVendorCustomer.mockRejectedValue(new Error('nope'));

await controller.createVendorCustomer({
vendor: 'iron',
email: 'a@b.co',
});
await expect(
controller.createVendorCustomer({
vendor: 'iron',
email: 'a@b.co',
}),
).rejects.toThrow(/Vendor customer creation failed/u);

expect(controller.state.phase).toBe('error');
expect(
Expand Down Expand Up @@ -3568,11 +3586,13 @@ describe('KycController', () => {
},
},
async ({ controller }) => {
// @ts-expect-error T&C2 flags are required
await controller.acceptTermsAndStartSession({
email: 'a@b.co',
product: 'money',
});
await expect(
// @ts-expect-error T&C2 flags are required
controller.acceptTermsAndStartSession({
email: 'a@b.co',
product: 'money',
}),
).rejects.toThrow(/Missing T&C2 acceptance/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing T&C2 acceptance/u);
Expand All @@ -3594,11 +3614,13 @@ describe('KycController', () => {
},
},
async ({ controller, handlers }) => {
// @ts-expect-error both T&C2 flags are required
await controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
});
await expect(
// @ts-expect-error both T&C2 flags are required
controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
}),
).rejects.toThrow(/Missing T&C2 acceptance/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing T&C2 acceptance/u);
Expand Down Expand Up @@ -3657,10 +3679,12 @@ describe('KycController', () => {
},
},
async ({ controller }) => {
await controller.acceptTermsAndStartSession({
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
});
await expect(
controller.acceptTermsAndStartSession({
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
}),
).rejects.toThrow(/Missing email/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(/Missing email/u);
Expand All @@ -3680,11 +3704,13 @@ describe('KycController', () => {
},
},
async ({ controller }) => {
await controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
});
await expect(
controller.acceptTermsAndStartSession({
email: 'a@b.co',
providerDisclaimersAccepted: MOCK_SUMSUB_DISCLAIMERS_ACCEPTED,
idosDisclaimersAccepted: MOCK_IDOS_DISCLAIMERS_ACCEPTED,
}),
).rejects.toThrow(/Missing disclaimer acceptance/u);

expect(controller.state.phase).toBe('error');
expect(controller.state.error).toMatch(
Expand Down
24 changes: 10 additions & 14 deletions packages/kyc-controller/src/KycController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,6 @@
return;
}
this.#fail(`Vendor customer creation failed: ${String(error)}`);
return;
}
}

Expand Down Expand Up @@ -1182,7 +1181,6 @@
!isValidConsentRecordList(idosDisclaimersAccepted)
) {
this.#fail('Missing T&C2 acceptance flags.');
return;
}
const credentialReusabilityConsentGiven =
params?.credentialReusabilityConsentGiven ?? false;
Expand Down Expand Up @@ -1244,11 +1242,9 @@
);
if (!email) {
this.#fail('Missing email for consents session.');
return;
}
if (acceptedDisclaimerIds.length === 0) {
this.#fail('Missing disclaimer acceptance.');
return;
}

const generation = this.#generation;
Expand Down Expand Up @@ -1542,11 +1538,9 @@
);
if (!email) {
this.#fail('Missing email for session creation.');
return;
}
if (!termsAcceptedAt || acceptedDisclaimerIds.length === 0) {
this.#fail('Missing terms acceptance for session creation.');
return;
}

// A new session invalidates any authentication carried over from a prior
Expand Down Expand Up @@ -1666,9 +1660,11 @@
* document-verification sub-flow is launched. When no product is set, this is
* a no-op and the flow stays at `form` for the consumer to drive manually.
*
* Errors are already recorded on state by `checkKycRequired` (`error`
* phase) and `startSumSub` (`sumsub.status = 'failed'`); this method swallows
* them so it can be awaited safely from the frame-message handler.
* `checkKycRequired` records the error phase and rethrows, so this method
* (and therefore `handleFrameMessage`) rejects when the check fails.
* `startSumSub` records `sumsub.status = 'failed'`; this method swallows
* that rethrown error (e.g. SDK unavailable) so it does not surface as an
* unhandled rejection from the frame-message handler.
*/
async #continueAfterAuthentication(): Promise<void> {
const product = this.state.activeProduct;
Expand Down Expand Up @@ -1745,8 +1741,10 @@
* @param params.product - The consuming feature.
* @param params.country - Optional alpha-3 country override.
* @returns Whether KYC is required.
* @throws If the access token or country is missing, or the service call
* fails. The error is also recorded on controller state (`phase: 'error'`).
*/
async checkKycRequired(params: {

Check failure on line 1747 in packages/kyc-controller/src/KycController.ts

View workflow job for this annotation

GitHub Actions / Lint, build, and test / Lint (lint:eslint) (24.x) 

Expected to return a value at the end of async method 'checkKycRequired'
product: KycProduct;
country?: string;
}): Promise<boolean> {
Expand All @@ -1755,12 +1753,10 @@
this.#fail(
'Missing moonpayAccessToken — repeat the authentication step.',
);
return false;
}
const country = params.country ?? this.state.geoCountry;
if (!country) {
this.#fail('Missing country for KYC-required check.');
return false;
}

// Capture the flow generation so we can detect a `reset()` that happens
Expand Down Expand Up @@ -1798,7 +1794,6 @@
return false;
}
this.#fail(`KYC check failed: ${String(error)}`);
return false;
}
}

Expand Down Expand Up @@ -2609,14 +2604,15 @@
}

/**
* Transitions to the error phase with a message.
* Transitions to the error phase with a message, then throws.
*
* @param message - The error message.
*/
#fail(message: string): void {
#fail(message: string): never {
this.#applyUpdate((state) => {
state.error = message;
state.phase = 'error';
});
throw new Error(message);
}
}
Loading