Repository navigation
feat(fx-api): add POST /user/breaches/resolutions for the Firefox API (MNTOR-5345) - #6874
Conversation
POST /user/breaches/resolutions needs the same HIBP matching and data class rules as GET. Move getAffectingBreaches, the served data classes, and the resolution types into a module both endpoints import. getFirefoxUserBreaches now only maps the result; GET output is unchanged.
Marks breaches resolved, like the privacy panel's "Mark as done": every served data class the breach leaked, for every monitored email it affected. Breach names that match nothing affecting the user are skipped and counted, so a stale or renamed breach can never leave a Firefox client stuck retrying a rejected batch. Unknown body keys and query params are ignored, logged by name, and counted under a new location label. Resolutions are counted in firefox_api_resolution_total.
5cf444a to
a3fdfac
Compare
Rows written before 2023 can hold an entry with only isResolved and no resolutionsChecked. Resolving such a breach threw a TypeError, so the route 500ed on every retry. Default the list to empty, as GET and the website's bulk resolve already do.
Test with a real database that a POST naming one known breach and one unknown breach still resolves the known one.
POST takes no query params, so ignoring them needs no code.
| const KNOWN_KEYS = ["breachId"]; | ||
|
|
||
| /** Whether this is a plain JSON object */ | ||
| function isObject(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
I hate that this is a function that I have to write... If there's a better way than this, please let me know.
There was a problem hiding this comment.
question(non-blocking): Why do you have to write this vs. just doing a null check?
There was a problem hiding this comment.
I was thinking we had to do this over just a null check due to the code works where this helper is used. For example, let me link to the two places and explain my thoughts, but I'm totally open to changing this
- Validating each entry. I think you're right here. IE a null check would work.
- For Collecting unknown keys I think this one needs more than a null check, do you agree? I tried it in a Node REPL. If an entry is a string or an array instead of an object,
Object.keysreturns index numbers, which we'd then count and log as unknown params:
> Object.keys("LinkedIn")
[ '0', '1', '2', '3', '4', '5', '6', '7' ]
> Object.keys(["LinkedIn"])
[ '0' ]So for #2 we need to rule out null, strings and arrays, which is what the helper does. Since I already had it, I reused it for #1 too.
I think introducing a tool like Zod would likely be overkill for just this situation, but I've been super happy with that sort of tooling or Pydantic on the python side of things. And it might make a slew of these of things easier to deal with, and earlier on in the code.
I'm going to merge this PR , but will cut another one if we want to change things. just let me know
There was a problem hiding this comment.
factoring this out to a common function, so both the firefox GET and POST can use this.
There was a problem hiding this comment.
the factored out functionality mentioned above...
There was a problem hiding this comment.
I've chatted with Kaitlyn and Jonathan on the frontend team about these changes. It should honestly make their efforts even easier...
| ); | ||
|
|
||
| const candidates = getFilteredBreaches(await getBreaches()) | ||
| .filter(isServable) |
There was a problem hiding this comment.
I can change the approach from "map/filter/reduce` to a bunch of loops if others feel strongly on this.
joeherm
left a comment
There was a problem hiding this comment.
These changes look good! Appreciate all the tests. Left a few small questions and suggestions, but nothing blocking.
| } | ||
| } | ||
|
|
||
| // Case-insensitive dedupe |
There was a problem hiding this comment.
question(non-blocking): is this necessary? do we do any validation when users sign up with extra emails for this stuff (i.e. unique enforcements at the DB level)?
There was a problem hiding this comment.
This code wasn't introduced in this PR, it was moved from src/app/functions/server/getFirefoxUserBreaches.ts.
but to answer your question, I think we do need it. There's no unique constraint on email_addresses.email. Here's the table on a fully migrated local DB (\d email_addresses), only verification_token is unique:
Indexes:
"email_addresses_pkey" PRIMARY KEY, btree (id)
"email_addresses_email_idx" btree (email)
"email_addresses_sha1_idx" btree (sha1)
"email_addresses_verification_token_unique" UNIQUE CONSTRAINT, btree (verification_token)
i generated this by running:
$ docker exec blurts-server-postgres-1 psql -U blurts -d blurts -c "\d email_addresses"So I think we still need this check, but have an oppurtunity to make it not required in the future. I eventually want to sit down and review the entire database schema tbh, and see if there are other changes we can make to make the application code simpler
|
|
||
| await expect( | ||
| resolveFirefoxUserBreaches(makeSubscriber(), new Set(["Adobe"])), | ||
| ).rejects.toThrow("db down"); |
There was a problem hiding this comment.
question(non-blocking): what will be returned to the end user in this case?
There was a problem hiding this comment.
The user will get a 500 as it propagates up to withFxaUser around here
| function parseBreachNames(body: unknown): Set<string> | string { | ||
| if (!Array.isArray(body) || body.length === 0) { | ||
| return "Body must be a non-empty array."; | ||
| } | ||
|
|
||
| const breachNames = new Set<string>(); | ||
| for (const [i, entry] of body.entries()) { | ||
| if (!isObject(entry)) return `[${i}] must be an object.`; | ||
| if (typeof entry.breachId !== "string" || entry.breachId === "") { | ||
| return `[${i}].breachId must be a non-empty string.`; | ||
| } | ||
| breachNames.add(entry.breachId); | ||
| } | ||
| return breachNames; | ||
| } |
There was a problem hiding this comment.
quibble question (non-blocking): any existing functions that could be re-used here?
There was a problem hiding this comment.
I did not find anything related. I was talking to Joe above, I think I want to add Zod to this project to sort of solve these sort of issues. I'll cut a backlog ticket
| $ref: "#/components/responses/Unauthorized" | ||
| "403": | ||
| $ref: "#/components/responses/Forbidden" | ||
| "404": |
There was a problem hiding this comment.
question (non-blocking): are these cases being moved into a different response status code?
There was a problem hiding this comment.
No, they're gone rather than moved. We dropped dataClasses from the request, and I chatted with the frontend team about this change. As on their side of the house, they can only click "approve all" and don't have to provide us any data class value
References:
Jira: MNTOR-5345
Description
Adds
POST /api/firefox/v1/user/breaches/resolutionsfrom openapi.yml, the second route behindwithFxaUser.It backs the privacy panel's "Mark as done" button. For each breach name in the body, it resolves passwords and email addresses, whichever the breach leaked, for every verified email the breach affected. Here's a convenient link to the finalized Figma.
This route is off everywhere until launch, behind the existing
FirefoxApifeature flag.Observability
firefox_api_resolution_total{outcome, data_class}, whereoutcomeisresolved,already-resolvedorskipped. A rise inskippedmeans Firefox holds breach names we no longer serve.firefox_api_unknown_param_totalgets alocationlabel (queryorbody), shared with GET.Some WIP panels can be found here
How to test
Unit tests:
The integration test runs the whole route against your real local Postgres. We mock FxA, HIBP and Redis. To run it locally:
Checklist (Definition of Done)