-
Notifications
You must be signed in to change notification settings - Fork 369
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
Validate Attestation Requests #1468
Changes from 14 commits
8de7d7a
205bba1
d348eba
5438b10
ea9eca4
ac044bf
a71bbeb
e2e65a6
0aaf883
7e03c2a
b97b29c
4871d71
d1edb72
b049f61
8358842
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 |
---|---|---|
@@ -1,11 +1,11 @@ | ||
{ | ||
"development": { | ||
"use_env_variable": "DB_URL" | ||
"use_env_variable": "DATABASE_URL" | ||
}, | ||
"staging": { | ||
"use_env_variable": "DB_URL" | ||
"use_env_variable": "DATABASE_URL" | ||
}, | ||
"production": { | ||
"use_env_variable": "DB_URL" | ||
"use_env_variable": "DATABASE_URL" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
'use strict' | ||
module.exports = { | ||
up: async (queryInterface, Sequelize) => { | ||
const transaction = await queryInterface.sequelize.transaction() | ||
|
||
try { | ||
await queryInterface.createTable('Attestations', { | ||
id: { | ||
allowNull: false, | ||
autoIncrement: true, | ||
primaryKey: true, | ||
type: Sequelize.INTEGER, | ||
}, | ||
account: { | ||
allowNull: false, | ||
type: Sequelize.STRING, | ||
}, | ||
phoneNumber: { | ||
allowNull: false, | ||
type: Sequelize.STRING, | ||
}, | ||
issuer: { | ||
allowNull: false, | ||
type: Sequelize.STRING, | ||
}, | ||
createdAt: { | ||
allowNull: false, | ||
type: Sequelize.DATE, | ||
}, | ||
updatedAt: { | ||
allowNull: false, | ||
type: Sequelize.DATE, | ||
}, | ||
}) | ||
|
||
await queryInterface.addIndex( | ||
'Attestations', | ||
['account', 'phoneNumber', 'issuer'], | ||
{ fields: ['account', 'phoneNumber', 'issuer'], unique: true }, | ||
{ transaction } | ||
) | ||
|
||
await transaction.commit() | ||
} catch (error) { | ||
await transaction.rollback() | ||
throw error | ||
} | ||
}, | ||
down: (queryInterface, Sequelize) => { | ||
return queryInterface.dropTable('Attestations') | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,124 @@ | ||
import { AttestationState } from '@celo/contractkit/lib/wrappers/Attestations' | ||
import { attestToIdentifier, SignatureUtils } from '@celo/utils' | ||
import { privateKeyToAddress } from '@celo/utils/lib/address' | ||
import { retryAsyncWithBackOff } from '@celo/utils/lib/async' | ||
import express from 'express' | ||
import * as t from 'io-ts' | ||
import { existingAttestationRequest, kit, persistAttestationRequest } from './db' | ||
import { Address, AddressType, E164Number, E164PhoneNumberType } from './request' | ||
import { sendSms } from './sms' | ||
function signAttestation(phoneNumber: string, account: string) { | ||
|
||
export const AttestationRequestType = t.type({ | ||
phoneNumber: E164PhoneNumberType, | ||
account: AddressType, | ||
issuer: AddressType, | ||
}) | ||
|
||
export type AttestationRequest = t.TypeOf<typeof AttestationRequestType> | ||
|
||
function getAttestationKey() { | ||
if (process.env.ATTESTATION_KEY === undefined) { | ||
console.error('Did not specify ATTESTATION_KEY') | ||
throw new Error('Did not specify ATTESTATION_KEY') | ||
} | ||
|
||
const signature = attestToIdentifier(phoneNumber, account, process.env.ATTESTATION_KEY) | ||
return process.env.ATTESTATION_KEY | ||
} | ||
|
||
async function validateAttestationRequest(request: AttestationRequest) { | ||
// check if it exists in the database | ||
if ( | ||
(await existingAttestationRequest(request.phoneNumber, request.account, request.issuer)) !== | ||
null | ||
) { | ||
throw new Error('Attestation already sent') | ||
} | ||
const key = getAttestationKey() | ||
const address = privateKeyToAddress(key) | ||
|
||
// TODO: Check with the new Accounts.sol | ||
if (address.toLowerCase() !== request.issuer.toLowerCase()) { | ||
throw new Error(`Mismatching issuer, I am ${address}`) | ||
} | ||
|
||
const attestations = await kit.contracts.getAttestations() | ||
const state = await attestations.getAttestationState( | ||
request.phoneNumber, | ||
request.account, | ||
request.issuer | ||
) | ||
|
||
if (state.attestationState !== AttestationState.Incomplete) { | ||
throw new Error('No incomplete attestation found') | ||
} | ||
|
||
// TODO: Check expiration | ||
return | ||
} | ||
|
||
async function validateAttestation( | ||
attestationRequest: AttestationRequest, | ||
attestationCode: string | ||
) { | ||
const attestations = await kit.contracts.getAttestations() | ||
const isValid = await attestations.validateAttestationCode( | ||
attestationRequest.phoneNumber, | ||
attestationRequest.account, | ||
attestationRequest.issuer, | ||
attestationCode | ||
) | ||
if (!isValid) { | ||
throw new Error('Valid attestation could not be provided') | ||
} | ||
return | ||
} | ||
|
||
function signAttestation(phoneNumber: E164Number, account: Address) { | ||
const signature = attestToIdentifier(phoneNumber, account, getAttestationKey()) | ||
|
||
return SignatureUtils.serializeSignature(signature) | ||
} | ||
|
||
function toBase64(str: string) { | ||
return Buffer.from(str, 'hex').toString('base64') | ||
return Buffer.from(str.slice(2), 'hex').toString('base64') | ||
} | ||
|
||
function createAttestationTextMessage(attestationCode: string) { | ||
return `<#> ${toBase64(attestationCode)} ${process.env.APP_SIGNATURE}` | ||
} | ||
|
||
export async function handleAttestationRequest(req: express.Request, res: express.Response) { | ||
// TODO: Should parse request appropriately | ||
|
||
// TODO: Should validate request here | ||
// const attestations = await kit.contracts.getAttestations() | ||
|
||
// Produce attestation | ||
const attestationCode = signAttestation(req.body.phoneNumber, req.body.account) | ||
const textMessage = createAttestationTextMessage(attestationCode) | ||
export async function handleAttestationRequest( | ||
_req: express.Request, | ||
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. why the underscore? 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. In typescript (and many other languages), a leading underscore indicates the parameter not being used. Not prefixing is I believe a lint violation (or maybe even compiler?) 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. huh, didn't know that! |
||
res: express.Response, | ||
attestationRequest: AttestationRequest | ||
) { | ||
let attestationCode | ||
try { | ||
await validateAttestationRequest(attestationRequest) | ||
attestationCode = signAttestation(attestationRequest.phoneNumber, attestationRequest.account) | ||
await validateAttestation(attestationRequest, attestationCode) | ||
} catch (error) { | ||
console.error(error) | ||
res.status(422).json({ success: false, error: error.toString() }) | ||
return | ||
} | ||
|
||
// Send the SMS | ||
await retryAsyncWithBackOff(sendSms, 10, [req.body.phoneNumber, textMessage], 1000) | ||
try { | ||
const textMessage = createAttestationTextMessage(attestationCode) | ||
await persistAttestationRequest( | ||
attestationRequest.phoneNumber, | ||
attestationRequest.account, | ||
attestationRequest.issuer | ||
) | ||
await retryAsyncWithBackOff(sendSms, 10, [attestationRequest.phoneNumber, textMessage], 1000) | ||
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. Nit: wrapping the phone number and txt message in an array seems odd to me 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. That's our current interface into 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. if we make change to signature to move up the delay param and then have |
||
} catch (error) { | ||
console.error(error) | ||
res.status(500).json({ | ||
success: false, | ||
error: 'Something went wrong while attempting to send SMS, try again later', | ||
}) | ||
return | ||
} | ||
|
||
res.json({ success: true }) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { BuildOptions, DataTypes, Model, Sequelize } from 'sequelize' | ||
|
||
interface AttestationModel extends Model { | ||
readonly id: number | ||
account: string | ||
phoneNumber: string | ||
issuer: string | ||
} | ||
|
||
export type AttestationStatic = typeof Model & | ||
(new (values?: object, options?: BuildOptions) => AttestationModel) | ||
|
||
export default (sequelize: Sequelize) => | ||
sequelize.define('Attestations', { | ||
account: DataTypes.STRING, | ||
phoneNumber: DataTypes.STRING, | ||
issuer: DataTypes.STRING, | ||
}) as AttestationStatic |
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.
So this implies users cannot request another delivery if the SMS did not arrive?
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.
Correct, not at this moment, though we could consider circumstances in which the user could re-request.
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.
Re-requesting is a pretty useful debugging tool for verification atm. I'd say (anecdotally) half the time I get 2/3, the third comes if I retry verification. So a better alternative here would be a retry threshold, which could be just 1 time for now. But I understand that's a fair bit more complexity here so open to deferring that to another PR if you prefer
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.
Would prefer to keep this out for another PR. Raised #1502 for it.