Skip to content
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

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

jordanh
Copy link
Contributor

@jordanh jordanh commented Mar 3, 2024

Description

Resolves #9499

Testing scenarios

  • Attempt to login with bad password
    • Check that attempts are being logged to FailedAuthRequest table in PG
    • Validate exceptions do not occur

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

ip: string
email: string
time?: Date
}

export default class FailedAuthRequest {
id: string
Copy link
Contributor Author

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
Copy link
Contributor Author

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";

@jordanh jordanh force-pushed the chore/9499/migrate-failedauthrequest branch from a108d21 to 2608302 Compare March 3, 2024 17:56
'batchSize is smaller than the number of items that share the same cursor. Increase batchSize'
)
}
return nextBatch.slice(0, lastMatchingTime)
Copy link
Contributor

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.

Copy link
Contributor Author

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:

#9502 (comment)

Copy link
Contributor Author

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

@github-actions github-actions bot added size/s and removed size/m labels Mar 8, 2024
@jordanh jordanh force-pushed the chore/9499/migrate-failedauthrequest branch from a147252 to 7f3d851 Compare March 8, 2024 19:52
@jordanh
Copy link
Contributor Author

jordanh commented Mar 8, 2024

@Dschoordsch updated with your review comments.

Note: I rebased to master to catch latest migration from other branches

@jordanh jordanh force-pushed the chore/9499/migrate-failedauthrequest branch from 7f3d851 to b97ac11 Compare March 8, 2024 20:14
@Dschoordsch
Copy link
Contributor

@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.

@Dschoordsch Dschoordsch self-requested a review March 14, 2024 17:29
@jordanh jordanh merged commit efc0dc9 into master Mar 14, 2024
5 checks passed
@jordanh jordanh deleted the chore/9499/migrate-failedauthrequest branch March 14, 2024 20:14
@github-actions github-actions bot mentioned this pull request Mar 14, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: migrate FailedAuthRequest to pg (1/1)
2 participants