From 30e1d589a57041bcb39270da89a92981816aed3a Mon Sep 17 00:00:00 2001 From: alecps Date: Fri, 8 Oct 2021 13:29:53 +0100 Subject: [PATCH 1/7] first batch of misc changes --- packages/attestation-service/src/env.ts | 13 +++--- .../attestation-service/src/lookup/vonage.ts | 15 ++----- packages/attestation-service/src/sms/index.ts | 2 +- .../attestation-service/src/sms/vonage.ts | 40 ++++++++----------- 4 files changed, 26 insertions(+), 44 deletions(-) diff --git a/packages/attestation-service/src/env.ts b/packages/attestation-service/src/env.ts index ca1ef8f02c6..87a8e457ef4 100644 --- a/packages/attestation-service/src/env.ts +++ b/packages/attestation-service/src/env.ts @@ -5,22 +5,19 @@ if (process.env.CONFIG) { dotenv.config({ path: process.env.CONFIG }) } -const checkEnv = (name: string) => - (process.env[name] === undefined || process.env[name] === '') && - (process.env[name.toLowerCase()] === undefined || process.env[name.toLowerCase()] === '') +const _fetchEnv = (name: string) => process.env[name] || process.env[name.toLowerCase()] export function fetchEnv(name: string): string { - if (checkEnv(name)) { + const env = _fetchEnv(name) + if (!env) { console.error(`ENV var '${name}' was not defined`) throw new Error(`ENV var '${name}' was not defined`) } - return (process.env[name] || process.env[name.toLowerCase()]) as string + return env as string } export function fetchEnvOrDefault(name: string, defaultValue: string): string { - return checkEnv(name) - ? defaultValue - : ((process.env[name] || process.env[name.toLowerCase()]) as string) + return _fetchEnv(name) || defaultValue } export function isYes(value: string) { diff --git a/packages/attestation-service/src/lookup/vonage.ts b/packages/attestation-service/src/lookup/vonage.ts index 9da6eedcb25..fceb8117e70 100644 --- a/packages/attestation-service/src/lookup/vonage.ts +++ b/packages/attestation-service/src/lookup/vonage.ts @@ -25,17 +25,10 @@ export class VonageLookupProvider extends LookupProvider { constructor(apiKey: string, apiSecret: string, applicationId: string) { super() this.applicationId = applicationId - if (applicationId) { - this.client = new Vonage({ - apiKey, - apiSecret, - }) - } else { - this.client = new Vonage({ - apiKey, - apiSecret, - }) - } + this.client = new Vonage({ + apiKey, + apiSecret, + }) } lookup = async (number: string) => diff --git a/packages/attestation-service/src/sms/index.ts b/packages/attestation-service/src/sms/index.ts index bbbf202a804..79e844ce2a8 100644 --- a/packages/attestation-service/src/sms/index.ts +++ b/packages/attestation-service/src/sms/index.ts @@ -263,7 +263,7 @@ export async function startSendSms( ) attestation.credentials = `[${verifiableCredential}]` } catch (e) { - logger.error({ e: e.message }) + logger.error({ e: e.message }, 'Error issuing phone-number-type credential') } if (!countryCode) { diff --git a/packages/attestation-service/src/sms/vonage.ts b/packages/attestation-service/src/sms/vonage.ts index 5fab912f9a1..80c44b1eade 100644 --- a/packages/attestation-service/src/sms/vonage.ts +++ b/packages/attestation-service/src/sms/vonage.ts @@ -13,41 +13,33 @@ const phoneUtil = PhoneNumberUtil.getInstance() export class VonageSmsProvider extends SmsProvider { static fromEnv() { + const _unsupportedRegionCodes = readUnsupportedRegionsFromEnv( + 'VONAGE_UNSUPPORTED_REGIONS', + 'VONAGE_BLACKLIST', + 'NEXMO_UNSUPPORTED_REGIONS', + 'NEXMO_BLACKLIST' + ) + const _balanceMetric = isYes( + fetchEnvOrDefault( + 'VONAGE_ACCOUNT_BALANCE_METRIC', + fetchEnvOrDefault('NEXMO_ACCOUNT_BALANCE_METRIC', '') + ) + ) try { return new VonageSmsProvider( fetchEnv('VONAGE_KEY'), fetchEnv('VONAGE_SECRET'), fetchEnvOrDefault('VONAGE_APPLICATION', ''), - readUnsupportedRegionsFromEnv( - 'VONAGE_UNSUPPORTED_REGIONS', - 'VONAGE_BLACKLIST', - 'NEXMO_UNSUPPORTED_REGIONS', - 'NEXMO_BLACKLIST' - ), - isYes( - fetchEnvOrDefault( - 'VONAGE_ACCOUNT_BALANCE_METRIC', - fetchEnvOrDefault('NEXMO_ACCOUNT_BALANCE_METRIC', '') - ) - ) + _unsupportedRegionCodes, + _balanceMetric ) } catch (e) { return new VonageSmsProvider( fetchEnv('NEXMO_KEY'), fetchEnv('NEXMO_SECRET'), fetchEnvOrDefault('NEXMO_APPLICATION', ''), - readUnsupportedRegionsFromEnv( - 'VONAGE_UNSUPPORTED_REGIONS', - 'VONAGE_BLACKLIST', - 'NEXMO_UNSUPPORTED_REGIONS', - 'NEXMO_BLACKLIST' - ), - isYes( - fetchEnvOrDefault( - 'VONAGE_ACCOUNT_BALANCE_METRIC', - fetchEnvOrDefault('NEXMO_ACCOUNT_BALANCE_METRIC', '') - ) - ) + _unsupportedRegionCodes, + _balanceMetric ) } } From 949510dbfd95758dd9f9f71bb2bcc2332cf498d4 Mon Sep 17 00:00:00 2001 From: alecps Date: Fri, 8 Oct 2021 15:20:59 +0100 Subject: [PATCH 2/7] update README --- packages/attestation-service/README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/attestation-service/README.md b/packages/attestation-service/README.md index d7a248144d9..ed7c11c4247 100644 --- a/packages/attestation-service/README.md +++ b/packages/attestation-service/README.md @@ -12,11 +12,11 @@ You can use the following environment variables to configure the attestation ser - `ATTESTATION_SIGNER_ADDRESS` - The address of the key with which attestations should be signed. - `SMS_PROVIDERS` - A comma-separated list of providers you want to configure, we currently support: -`nexmo` +`vonage` -- `NEXMO_KEY` - The API key to the Nexmo API -- `NEXMO_SECRET` - The API secret to the Nexmo API -- `NEXMO_BLACKLIST` - A comma-sperated list of country codes you do not want to serve +- `VONAGE_KEY` - The API key to the Vonage API +- `VONAGE_SECRET` - The API secret to the Vonage API +- `VONAGE_BLACKLIST` - A comma-sperated list of country codes you do not want to serve `twilio` @@ -55,6 +55,8 @@ Here is a list of the error codes and messages returned by the Attestation Servi - `Mismatching issuer, I am ${address}` - The attestation request references an issuer address that does not match that of the AS that actually received the request. - `NO_INCOMPLETE_ATTESTATION_FOUND_ERROR / 'No incomplete attestation found'` - The Attestations contract has no record that this AS was randomly assigned as an issuer for the given account/identifier pair. +- `I am not an authorized issuer for this account` - The AS is not authorized to issue a PhoneNumberType VerifiableCredential for this user. This means the Attestations contract has no record that this AS was randomly assigned as an issuer for the given account/identifier pair. +- `Can't issue a credential for an incomplete attestation` - The user has requested a PhoneNumberType VerifiableCredential from an issuer with whom the user has not completed a phone number ownership attestation. - `ATTESTATION_ERROR / 'Valid attestation could not be provided'` - A call to validateAttestationCode in the Attestations contract has failed. This method checks that (1) there is a non-expired incomplete attestation assigned to the issuer whose signature constitutes the given attestation code. - `'Invalid securityCodePrefix'` - A security code prefix with an incorrect length was provided in the attestation request. - `'Invalid smsRetrieverAppSig'` - The provided smsRetrieverAppSig does not match the regex `'^[\\w+]{5,12}$'` From f4c77f165812c537ca74ba4ad74407076f172882 Mon Sep 17 00:00:00 2001 From: alecps Date: Fri, 8 Oct 2021 15:33:00 +0100 Subject: [PATCH 3/7] adds/resolves some todo comments --- .../attestation-service/src/lookup/index.ts | 21 ++++++++----------- .../requestHandlers/verifiable_credential.ts | 21 +++++++++---------- .../sdk/utils/src/verifiableCredential.ts | 4 ++-- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/packages/attestation-service/src/lookup/index.ts b/packages/attestation-service/src/lookup/index.ts index a3a297bc149..48b377377c7 100644 --- a/packages/attestation-service/src/lookup/index.ts +++ b/packages/attestation-service/src/lookup/index.ts @@ -61,32 +61,29 @@ export function configuredLookupProviders() { export const issueAttestationPhoneNumberTypeCredential = async ( attestation: AttestationModel, - logger: any + logger: Logger ) => { try { const phoneNumberTypeProvider = await lookupPhoneNumber(attestation, logger) + const attestationSignerAddress = getAttestationSignerAddress().toLowerCase() const credential = VerifiableCredentialUtils.getPhoneNumberTypeJSONLD( - attestation.phoneNumberType, + attestation.phoneNumberType, // TODO(Alec): does setting this value in lookupPhoneNumber actually work? attestation.account.toLowerCase(), - getAttestationSignerAddress().toLowerCase(), + attestationSignerAddress, attestation.identifier, phoneNumberTypeProvider ) - const proofOptions = VerifiableCredentialUtils.getProofOptions( - getAttestationSignerAddress().toLowerCase() - ) + const proofOptions = VerifiableCredentialUtils.getProofOptions(attestationSignerAddress) + return await VerifiableCredentialUtils.issueCredential( credential, proofOptions, async (signInput) => useKit(async (kit) => SignatureUtils.serializeSignature( - await kit.connection.signTypedData( - getAttestationSignerAddress().toLowerCase(), - signInput - ) + await kit.connection.signTypedData(attestationSignerAddress, signInput) ) ) ) @@ -106,7 +103,7 @@ async function lookupPhoneNumber( logger.info( { provider: provider.type, - phoneNumber: obfuscateNumber(attestation.phoneNumber), + obfuscatedPhoneNumber: obfuscateNumber(attestation.phoneNumber), }, 'Lookup phoneNumber' ) @@ -123,7 +120,7 @@ async function lookupPhoneNumber( logger.info( { provider: provider.type, - attempt: attestation.attempt, + attempt: attestation.attempt, // TODO(Alec): Shouldn't we increment attempt here? error: errorMsg, }, 'Phone number lookup failed' diff --git a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts index 272fc3e7d21..11b016342d6 100644 --- a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts +++ b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts @@ -14,15 +14,16 @@ import { } from '../request' export class VerifiableCredentialHandler { - constructor(public readonly verifiableCredentialRequest: VerifiableCredentialRequest) {} + constructor(public readonly verifiableCredentialRequest: VerifiableCredentialRequest) {} // TODO(Alec): what's the point of this? async signVerifiableCredential(signingInput: string) { + // TODO(Alec): where is this used? return useKit((kit) => kit.connection.sign(signingInput, getAttestationSignerAddress().toLowerCase()) ) } - async validateRequest(issuer: string, issuers: string[]) { + validateRequest(issuer: string, issuers: string[]) { const address = getAccountAddress() if (!eqAddress(address, issuer)) { throw new ErrorWithResponse(`Mismatching issuer, I am ${address}`, 422) @@ -36,17 +37,15 @@ export class VerifiableCredentialHandler { async doCredential(account: string, issuer: string, identifier: string, logger: Logger) { const attestations = await useKit((kit) => kit.contracts.getAttestations()) - const issuers = await attestations.getAttestationIssuers(identifier, account) - // Checks if the attestation service is an authorized issuer - await this.validateRequest(issuer, issuers) + this.validateRequest(issuer, await attestations.getAttestationIssuers(identifier, account)) const state = await attestations.getAttestationState(identifier, account, issuer) // Checks if the attestation is marked as completed - if (state.attestationState === AttestationState.Complete) { + if (state.attestationState !== AttestationState.Complete) { throw new ErrorWithResponse(`Can't issue a credential for an incomplete attestation`, 422) - } + } // TODO(Alec): Does this happen before the attestation completes? const attestation = await findAttestationByKey({ identifier, @@ -54,11 +53,11 @@ export class VerifiableCredentialHandler { account, }) - if (attestation) { - return JSON.parse(await issueAttestationPhoneNumberTypeCredential(attestation, logger)) - } else { - throw new Error('Unable to find attestation') + if (!attestation) { + throw new Error('Unable to find attestation in db') } + + return JSON.parse(await issueAttestationPhoneNumberTypeCredential(attestation, logger)) } } diff --git a/packages/sdk/utils/src/verifiableCredential.ts b/packages/sdk/utils/src/verifiableCredential.ts index fffb659bdec..137defb1abb 100644 --- a/packages/sdk/utils/src/verifiableCredential.ts +++ b/packages/sdk/utils/src/verifiableCredential.ts @@ -6,7 +6,7 @@ import { completeIssueCredential, prepareIssueCredential, verifyCredential } fro * @param phoneNumberTypeProvider The lookup provider of the phone number type * @param subject Subject of the verifiable credential, usually a Valora user * @param issuer Address of whom is issuing this credential, usually getAttestationSignerAddress() - * @param identifier Transaction identifier + * @param identifier ODIS identifier for the phone number */ export const getPhoneNumberTypeJSONLD = ( phoneNumberType: string, @@ -19,7 +19,7 @@ export const getPhoneNumberTypeJSONLD = ( '@context': [ 'https://www.w3.org/2018/credentials/v1', { - phoneNumberType: 'https://docs.celo.org/phone_types', + phoneNumberType: 'https://docs.celo.org/phone_types', // TODO(Alec): do we need to add docs for these pages? phoneNumberTypeProvider: 'https://docs.celo.org/phone_type_providers', identifier: 'https://docs.celo.org/identifier', PhoneNumberType: 'https://docs.celo.org/PhoneNumberType', From 504f5a140b1de7418b4b372556bd261627bab0b2 Mon Sep 17 00:00:00 2001 From: alecps Date: Fri, 8 Oct 2021 15:48:46 +0100 Subject: [PATCH 4/7] resolves some todos --- .../src/requestHandlers/verifiable_credential.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts index 11b016342d6..89379e0a889 100644 --- a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts +++ b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts @@ -14,10 +14,9 @@ import { } from '../request' export class VerifiableCredentialHandler { - constructor(public readonly verifiableCredentialRequest: VerifiableCredentialRequest) {} // TODO(Alec): what's the point of this? + constructor(public readonly verifiableCredentialRequest: VerifiableCredentialRequest) {} async signVerifiableCredential(signingInput: string) { - // TODO(Alec): where is this used? return useKit((kit) => kit.connection.sign(signingInput, getAttestationSignerAddress().toLowerCase()) ) From e7d9fd882ca07a8e877482aefa42e67862d86f34 Mon Sep 17 00:00:00 2001 From: alecps Date: Sun, 10 Oct 2021 12:31:25 +0100 Subject: [PATCH 5/7] resolves last couple todo items --- packages/attestation-service/src/lookup/index.ts | 4 ++-- .../src/requestHandlers/verifiable_credential.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/attestation-service/src/lookup/index.ts b/packages/attestation-service/src/lookup/index.ts index 48b377377c7..82a7883ee04 100644 --- a/packages/attestation-service/src/lookup/index.ts +++ b/packages/attestation-service/src/lookup/index.ts @@ -68,7 +68,7 @@ export const issueAttestationPhoneNumberTypeCredential = async ( const attestationSignerAddress = getAttestationSignerAddress().toLowerCase() const credential = VerifiableCredentialUtils.getPhoneNumberTypeJSONLD( - attestation.phoneNumberType, // TODO(Alec): does setting this value in lookupPhoneNumber actually work? + attestation.phoneNumberType, attestation.account.toLowerCase(), attestationSignerAddress, attestation.identifier, @@ -120,7 +120,7 @@ async function lookupPhoneNumber( logger.info( { provider: provider.type, - attempt: attestation.attempt, // TODO(Alec): Shouldn't we increment attempt here? + attempt: attestation.attempt, error: errorMsg, }, 'Phone number lookup failed' diff --git a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts index 89379e0a889..caea9dcfe97 100644 --- a/packages/attestation-service/src/requestHandlers/verifiable_credential.ts +++ b/packages/attestation-service/src/requestHandlers/verifiable_credential.ts @@ -44,7 +44,7 @@ export class VerifiableCredentialHandler { // Checks if the attestation is marked as completed if (state.attestationState !== AttestationState.Complete) { throw new ErrorWithResponse(`Can't issue a credential for an incomplete attestation`, 422) - } // TODO(Alec): Does this happen before the attestation completes? + } const attestation = await findAttestationByKey({ identifier, From b15e8c42900a4b9aa411ec4162fa6765e6e0d70b Mon Sep 17 00:00:00 2001 From: alecps Date: Tue, 12 Oct 2021 18:59:54 +0100 Subject: [PATCH 6/7] addresses feedback --- packages/attestation-service/README.md | 2 +- packages/attestation-service/src/env.ts | 2 +- .../attestation-service/src/lookup/vonage.ts | 16 ++++++++++++---- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/attestation-service/README.md b/packages/attestation-service/README.md index ed7c11c4247..f8ff32888f4 100644 --- a/packages/attestation-service/README.md +++ b/packages/attestation-service/README.md @@ -12,7 +12,7 @@ You can use the following environment variables to configure the attestation ser - `ATTESTATION_SIGNER_ADDRESS` - The address of the key with which attestations should be signed. - `SMS_PROVIDERS` - A comma-separated list of providers you want to configure, we currently support: -`vonage` +`vonage` (previously `nexmo`) - `VONAGE_KEY` - The API key to the Vonage API - `VONAGE_SECRET` - The API secret to the Vonage API diff --git a/packages/attestation-service/src/env.ts b/packages/attestation-service/src/env.ts index 87a8e457ef4..523e17f3381 100644 --- a/packages/attestation-service/src/env.ts +++ b/packages/attestation-service/src/env.ts @@ -13,7 +13,7 @@ export function fetchEnv(name: string): string { console.error(`ENV var '${name}' was not defined`) throw new Error(`ENV var '${name}' was not defined`) } - return env as string + return env } export function fetchEnvOrDefault(name: string, defaultValue: string): string { diff --git a/packages/attestation-service/src/lookup/vonage.ts b/packages/attestation-service/src/lookup/vonage.ts index fceb8117e70..4e649214f2a 100644 --- a/packages/attestation-service/src/lookup/vonage.ts +++ b/packages/attestation-service/src/lookup/vonage.ts @@ -25,10 +25,18 @@ export class VonageLookupProvider extends LookupProvider { constructor(apiKey: string, apiSecret: string, applicationId: string) { super() this.applicationId = applicationId - this.client = new Vonage({ - apiKey, - apiSecret, - }) + if (applicationId) { + this.client = new Vonage({ + apiKey, + apiSecret, + applicationId, + }) + } else { + this.client = new Vonage({ + apiKey, + apiSecret, + }) + } } lookup = async (number: string) => From aa6759bdd491f136b25dbff63cf08819d5c6dffe Mon Sep 17 00:00:00 2001 From: alecps Date: Tue, 12 Oct 2021 19:14:54 +0100 Subject: [PATCH 7/7] removes TODO statement --- packages/sdk/utils/src/verifiableCredential.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk/utils/src/verifiableCredential.ts b/packages/sdk/utils/src/verifiableCredential.ts index 137defb1abb..cf51e9c36aa 100644 --- a/packages/sdk/utils/src/verifiableCredential.ts +++ b/packages/sdk/utils/src/verifiableCredential.ts @@ -19,7 +19,7 @@ export const getPhoneNumberTypeJSONLD = ( '@context': [ 'https://www.w3.org/2018/credentials/v1', { - phoneNumberType: 'https://docs.celo.org/phone_types', // TODO(Alec): do we need to add docs for these pages? + phoneNumberType: 'https://docs.celo.org/phone_types', phoneNumberTypeProvider: 'https://docs.celo.org/phone_type_providers', identifier: 'https://docs.celo.org/identifier', PhoneNumberType: 'https://docs.celo.org/PhoneNumberType',