-
Notifications
You must be signed in to change notification settings - Fork 333
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
chore: migrate FailedAuthRequest to pg #9500
Conversation
ip: string | ||
email: string | ||
time?: Date | ||
} | ||
|
||
export default class FailedAuthRequest { | ||
id: string |
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.
id now managed by Postgres
.count() | ||
.ge(Threshold.MAX_DAILY_PASSWORD_ATTEMPTS) as unknown as boolean | ||
}).run() | ||
const {failOnAccount, failOnTime} = (await pg |
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.
This is an equivalent query to what we were doing with Rethink. Just 1 trip and returns an object with these two fields. While the Kysely is pretty readable, here's what the query compiles to:
WITH "byAccount" AS (
SELECT
COUNT("id") AS "attempts"
FROM
"FailedAuthRequest"
WHERE
"ip" = $1
AND "email" = $2
AND "time" >= $3
),
"byTime" AS (
SELECT
COUNT("id") AS "attempts"
FROM
"FailedAuthRequest"
WHERE
"ip" = $4
AND "time" >= $5
)
SELECT
"byAccount"."attempts" >= $6 AS "failOnAccount",
"byTime"."attempts" >= $7 AS "failOnTime"
FROM
"byAccount",
"byTime";
a108d21
to
2608302
Compare
'batchSize is smaller than the number of items that share the same cursor. Increase batchSize' | ||
) | ||
} | ||
return nextBatch.slice(0, lastMatchingTime) |
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.
-1 this should be nextBatch.slice(0, lastMatchingTime + 1)
, see your scheduled job PR. I also think we don't need to have this migration at all. Because it's bound to only 1 day, if an attacker gets double the amount of tries for 1 day, that shouldn't be too risky.
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.
@Dschoordsch, actually it should nextBatch.slice(0, lastMatchingTime)
! When I first read this code, I thought that +1 was correct...then I did another migration and ran into some mysteriously missing rows. When I stepped through with a debugger I caught what was going on. See:
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.
Indeed this is an ephemeral table. I would be happy to nuke this migration if you come back with a +1 comment
a147252
to
7f3d851
Compare
@Dschoordsch updated with your review comments. Note: I rebased to |
7f3d851
to
b97ac11
Compare
@jordanh I just noticed what you wrote. Please re-request review next time (by pressing the refresh button next to my changes requested). I only regularly check the view https://github.com/ParabolInc/parabol/pulls?q=is%3Apr+is%3Aopen+user-review-requested%3A%40me and might miss this otherwise. |
Description
Resolves #9499
Testing scenarios
FailedAuthRequest
table in PGFinal checklist