-
Notifications
You must be signed in to change notification settings - Fork 375
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 7 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,128 @@ | ||
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 | ||
) | ||
|
||
console.info(state) | ||
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. Should this be here? 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. Nope (at least not in this form) |
||
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', | ||
}) | ||
.status(422) | ||
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. did you mean to include this here? 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. I did not! |
||
return | ||
} | ||
|
||
res.json({ success: true }) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,15 @@ | ||
import { ContractKit, newKit } from '@celo/contractkit' | ||
import { Sequelize } from 'sequelize' | ||
import { fetchEnv } from './env' | ||
import Attestation, { AttestationStatic } from './models/attestation' | ||
|
||
export let sequelize: Sequelize | undefined | ||
|
||
export function initializeDB() { | ||
if (sequelize === undefined) { | ||
sequelize = new Sequelize(fetchEnv('DB_URL')) | ||
sequelize = new Sequelize(fetchEnv('DATABASE_URL')) | ||
return sequelize.authenticate() as Promise<void> | ||
} | ||
|
||
return Promise.resolve() | ||
} | ||
|
||
|
@@ -20,3 +20,21 @@ export function initializeKit() { | |
kit = newKit(fetchEnv('CELO_PROVIDER')) | ||
} | ||
} | ||
|
||
export async function existingAttestationRequest( | ||
phoneNumber: string, | ||
account: string, | ||
issuer: string | ||
): Promise<AttestationStatic | null> { | ||
const AttestationTable = await Attestation(sequelize!) | ||
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. Don't know anything about sequelize, just wondering if it'd be better to cache this AttestationTable object instead of recreating it? maybe a non-issue 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. I personally don't like global caching too much so eventually there should probably be a cache manager, but I figured for now it's no big deal 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 you but it above in the global scope, it's not actually global caching, it's module level caching, which is totally fine. If you don't export the cache var, it won't pollute your global scope. I think the bias against is mostly a holdover from browser days when the polluting the global scope was a big issue. 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. I changed it to module level caching! |
||
return AttestationTable.findOne({ where: { phoneNumber, account, issuer } }) | ||
} | ||
|
||
export async function persistAttestationRequest( | ||
phoneNumber: string, | ||
account: string, | ||
issuer: string | ||
) { | ||
const AttestationTable = await Attestation(sequelize!) | ||
return AttestationTable.create({ phoneNumber, account, issuer }) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
'use strict' | ||
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. This looks like a compiled file. Why is this here? 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. It's a generated file by sequelize scaffold. It looks like it is supposed to give unified access to all the models, but since we are not using that, I will remove the file |
||
|
||
const fs = require('fs') | ||
const path = require('path') | ||
const Sequelize = require('sequelize') | ||
const basename = path.basename(__filename) | ||
const env = process.env.NODE_ENV || 'development' | ||
const config = require(__dirname + '/../config/config.json')[env] | ||
const db = {} | ||
|
||
let sequelize | ||
if (config.use_env_variable) { | ||
sequelize = new Sequelize(process.env[config.use_env_variable], config) | ||
} else { | ||
sequelize = new Sequelize(config.database, config.username, config.password, config) | ||
} | ||
|
||
fs.readdirSync(__dirname) | ||
.filter((file) => { | ||
return file.indexOf('.') !== 0 && file !== basename && file.slice(-3) === '.js' | ||
}) | ||
.forEach((file) => { | ||
const model = sequelize['import'](path.join(__dirname, file)) | ||
db[model.name] = model | ||
}) | ||
|
||
Object.keys(db).forEach((modelName) => { | ||
if (db[modelName].associate) { | ||
db[modelName].associate(db) | ||
} | ||
}) | ||
|
||
db.sequelize = sequelize | ||
db.Sequelize = Sequelize | ||
|
||
module.exports = db |
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.