Validate Attestation Requests #1468

nambrot · 2019-10-24T19:50:35Z

Description

This PR adds more validation of attestation requests. Specifcally it checks for:

well formed requests
validity of the requests on the Attestations contracts
check if we already sent the SMS before

Tested

Ran locally with attestation requests on integration

Related issues

jmrossy · 2019-10-25T16:41:09Z

packages/attestation-service/src/attestation.ts

+    (await existingAttestationRequest(request.phoneNumber, request.account, request.issuer)) !==
+    null
+  ) {
+    throw new Error('Attestation already sent')


So this implies users cannot request another delivery if the SMS did not arrive?

Correct, not at this moment, though we could consider circumstances in which the user could re-request.

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

Would prefer to keep this out for another PR. Raised #1502 for it.

jmrossy · 2019-10-25T16:44:50Z

packages/attestation-service/src/attestation.ts

-  const attestationCode = signAttestation(req.body.phoneNumber, req.body.account)
-  const textMessage = createAttestationTextMessage(attestationCode)
+export async function handleAttestationRequest(
+  _req: express.Request,


why the underscore?

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?)

huh, didn't know that!

jmrossy · 2019-10-25T16:46:41Z

packages/attestation-service/src/attestation.ts

+      attestationRequest.account,
+      attestationRequest.issuer
+    )
+    await retryAsyncWithBackOff(sendSms, 10, [attestationRequest.phoneNumber, textMessage], 1000)


Nit: wrapping the phone number and txt message in an array seems odd to me

That's our current interface into retryAsyncWithBackOff which I don't think we have much alternative to unless we convert all our functions to only take one argument?

if we make change to signature to move up the delay param and then have ...args for the rest, we won't need to wrap the params. But it's nbd

jmrossy · 2019-10-25T16:47:11Z

packages/attestation-service/src/attestation.ts

+        success: false,
+        error: 'Something went wrong while attempting to send SMS, try again later',
+      })
+      .status(422)


did you mean to include this here?

jmrossy · 2019-10-25T16:48:52Z

packages/attestation-service/src/db.ts

+  account: string,
+  issuer: string
+): Promise<AttestationStatic | null> {
+  const AttestationTable = await Attestation(sequelize!)


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

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

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.
But yeah really nbd, just a thought.

I changed it to module level caching!

jmrossy · 2019-10-25T16:50:54Z

packages/attestation-service/src/models/index.js

@@ -0,0 +1,36 @@
+'use strict'


This looks like a compiled file. Why is this here?

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

jmrossy · 2019-10-25T16:53:53Z

packages/attestation-service/src/request.ts

+import { either, isLeft } from 'fp-ts/lib/Either'
+import * as t from 'io-ts'
+
+export function parseRequest<T>(


Is this some kind of fp-ts pattern? This structure of a function that returns a function which uses another function as a param seems more complex than it has to be.

I actually tried to minimize the style impact of fp-ts here, so this should mostly be a single-level function composition. Basically, when one writes an express handler, a developer would have to do some amount of logic to validate that the request has the right payload, types, etc.

To avoid that duplicated logic, a developer could use parseRequest(RequestType, processor) to receive the parsed request as a third argument. So IMO you get the benefit of declarative routes in https://github.com/celo-org/celo-monorepo/pull/1468/files#diff-8b9f369026f23d51728f7ad563386fdcR22, while the actual request handler has to not deal with the validation of the request payload and can just rely on it been validated by parseRequest https://github.com/celo-org/celo-monorepo/pull/1468/files#diff-881b8cc344bdb8ee0e96c73fb379ee6fR94

Okay seems reasonable. I guess I may just have been thrown off by the names them. Maybe handler is a more conventional name for processor and createValidatedHandler is a more accurate name than parseRequest? Unless these are fp-ts terms

Nope, they are just @nambrot™ , so changed them to your suggestion!

jmrossy · 2019-10-28T17:01:23Z

packages/attestation-service/src/attestation.ts

+    request.issuer
+  )
+
+  console.info(state)


Should this be here?

Nope (at least not in this form)

jmrossy · 2019-10-28T17:08:27Z

packages/attestation-service/src/request.ts

+import { either, isLeft } from 'fp-ts/lib/Either'
+import * as t from 'io-ts'
+
+export function parseRequest<T>(


Okay seems reasonable. I guess I may just have been thrown off by the names them. Maybe handler is a more conventional name for processor and createValidatedHandler is a more accurate name than parseRequest? Unless these are fp-ts terms

codecov · 2019-10-28T18:31:06Z

Codecov Report

Merging #1468 into master will decrease coverage by 8.41%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1468      +/-   ##
==========================================
- Coverage   73.74%   65.33%   -8.42%     
==========================================
  Files         277      271       -6     
  Lines        7420     8181     +761     
  Branches      660      501     -159     
==========================================
- Hits         5472     5345     -127     
- Misses       1836     2716     +880     
- Partials      112      120       +8

Flag	Coverage Δ
#mobile	`65.33% <50%> (-8.42%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/tokens/saga.ts	`87.83% <50%> (-2.02%)`	⬇️
packages/mobile/src/web3/selectors.ts	`80% <0%> (-20%)`	⬇️
packages/mobile/src/alert/reducer.ts	`60% <0%> (-12.73%)`	⬇️
packages/mobile/src/import/reducer.ts	`42.85% <0%> (-12.7%)`	⬇️
packages/mobile/src/utils/dynamicLink.ts	`64.7% <0%> (-12.22%)`	⬇️
packages/mobile/src/networkInfo/reducer.ts	`60% <0%> (-11.43%)`	⬇️
packages/mobile/src/home/reducers.ts	`50% <0%> (-10%)`	⬇️
packages/mobile/src/app/reducers.ts	`33.33% <0%> (-9.53%)`	⬇️
packages/mobile/src/invite/JoinCelo.tsx	`72.72% <0%> (-8.97%)`	⬇️
packages/mobile/src/exchange/reducer.ts	`66.66% <0%> (-8.34%)`	⬇️
... and 261 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578a8e2...8358842. Read the comment docs.

* master: (62 commits) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) [Wallet] Improve error handling around account creation and keystore ops (#1497) Add CI test for checking licenses and misc package.json cleanup (#1550) [Wallet] Implement SMS invite on iOS (#1541) CI: brings back to master (#1554) Validators: uses Ethereum address for proof of possession (#1494) Validate Attestation Requests (#1468) Rename hosted node references to forno (#1511) Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508) Added txpool family to geth apis. Sorted geth cmd options (#1462) [Wallet] Fix yarn dev command for running android (#1534) [Wallet] iOS info plist changes and version bump (#1539) ... # Conflicts: # yarn.lock

nambrot added 6 commits October 24, 2019 09:58

Add getAttestationState method

8de7d7a

Add validation of the request

205bba1

Properly validate attestation

d348eba

Fix signing

5438b10

Rename DB_URL to DATABASE_URL

ea9eca4

Move models

ac044bf

nambrot requested a review from jmrossy as a code owner October 24, 2019 19:50

nambrot assigned mcortesi and jmrossy Oct 24, 2019

yarn.lock

a71bbeb

jmrossy reviewed Oct 25, 2019

View reviewed changes

nambrot added 5 commits October 25, 2019 10:58

Fix verify

e2e65a6

PR comments

0aaf883

Fix lint

7e03c2a

Merge branch 'master' into nambrot/attestation-service-validation

b97b29c

Allow getTokenContract to be properly typed

4871d71

nambrot requested a review from cmcewen as a code owner October 26, 2019 01:08

jmrossy reviewed Oct 28, 2019

View reviewed changes

jmrossy assigned nambrot and unassigned jmrossy Oct 28, 2019

nambrot mentioned this pull request Oct 28, 2019

Users should be able re-request attestations #1502

Closed

PR comments

d1edb72

nambrot assigned jmrossy and unassigned nambrot and mcortesi Oct 28, 2019

Fix lint

b049f61

jmrossy approved these changes Oct 30, 2019

View reviewed changes

jmrossy assigned nambrot and unassigned jmrossy Oct 30, 2019

Merge branch 'master' into nambrot/attestation-service-validation

8358842

nambrot added the automerge Have PR merge automatically when checks pass label Oct 30, 2019

celo-ci-bot-user merged commit 51c359d into master Oct 30, 2019

celo-ci-bot-user deleted the nambrot/attestation-service-validation branch October 30, 2019 22:09

snyk-bot mentioned this pull request Sep 16, 2021

[Snyk] Fix for 1 vulnerabilities denissnykio/celo-monorepo#653

Open

snyk-bot mentioned this pull request Apr 12, 2022

[Snyk] Fix for 24 vulnerabilities denissnykio/celo-monorepo#825

Open

This was referenced Aug 5, 2022

[Snyk] Fix for 23 vulnerabilities denissnykio/celo-monorepo#842

Open

[Snyk] Fix for 23 vulnerabilities denissnykio/celo-monorepo#843

Open

snyk-bot mentioned this pull request Sep 8, 2022

[Snyk] Fix for 25 vulnerabilities denissnykio/celo-monorepo#849

Open

This was referenced Oct 6, 2022

[Snyk] Fix for 25 vulnerabilities denissnykio/celo-monorepo#863

Open

[Snyk] Fix for 25 vulnerabilities denissnykio/celo-monorepo#864

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 15 vulnerabilities leojoy95/celo-monorepo#649

Open

[Snyk] Fix for 10 vulnerabilities leojoy95/celo-monorepo#651

Open

[Snyk] Fix for 10 vulnerabilities leojoy95/celo-monorepo#652

Open

lili2311 mentioned this pull request Aug 15, 2023

[Snyk-dev] Fix for 3 vulnerabilities lili2311/lerna-celo-monorepo#84

Open

leojoy95 mentioned this pull request May 16, 2024

[Snyk] Fix for 52 vulnerabilities leojoy95/celo-monorepo#975

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Attestation Requests #1468

Validate Attestation Requests #1468

nambrot commented Oct 24, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 28, 2019

nambrot Oct 28, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 28, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 28, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 28, 2019

nambrot Oct 28, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 25, 2019

nambrot Oct 25, 2019

jmrossy Oct 28, 2019

nambrot Oct 28, 2019

jmrossy Oct 28, 2019

nambrot Oct 28, 2019

jmrossy Oct 28, 2019

codecov bot commented Oct 28, 2019 •

edited

Loading

Validate Attestation Requests #1468

Validate Attestation Requests #1468

Conversation

nambrot commented Oct 24, 2019

Description

Tested

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 28, 2019 • edited Loading

Codecov Report

codecov bot commented Oct 28, 2019 •

edited

Loading