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

Send API token expiry notification emails #8290

Merged
merged 22 commits into from
May 15, 2024

Conversation

hi-rustin
Copy link
Member

@hi-rustin hi-rustin commented Mar 12, 2024

ref #8154

Description

This PR adds a new background job to send the expiry notification for tokens that will expire within 3 days.

As #6664 (comment) states, such emails are seen as a requirement before we can change the default expiration setting on the API token creation page.

Currently, GitHub only sends notifications twice - one after 6 days and another after 12 hours, and after it has expired.

Perhaps we should consider adopting a similar approach, although it may be too aggressive.
So I just picked 3 days as the default value. It should be easy to extend the current design to send multiple emails by reusing 'expiry_notification_at' multiple times.

Explanation of Changes

  1. It added a new field expiry_notification_at to the api_tokens table.
  2. It added a new background job called SendTokenExpiryNotifications that will scan all the expiry tokens within 3 days and send an email to the users.
  3. It added an integration test to check we sent the right email.

@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch 3 times, most recently from 811cc48 to 8638983 Compare March 27, 2024 14:23
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 92.63804% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 88.53%. Comparing base (153a4b5) to head (394f1af).

❗ Current head 394f1af differs from pull request most recent head cb19848. Consider uploading reports for the commit cb19848 to get more accurate results

Files Patch % Lines
src/worker/jobs/expiry_notification.rs 93.08% 11 Missing ⚠️
src/admin/enqueue_job.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8290      +/-   ##
==========================================
+ Coverage   88.45%   88.53%   +0.07%     
==========================================
  Files         275      275              
  Lines       27255    27101     -154     
==========================================
- Hits        24108    23993     -115     
+ Misses       3147     3108      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch from b9bdf9d to 80d1907 Compare March 29, 2024 15:26
@hi-rustin hi-rustin marked this pull request as ready for review April 1, 2024 12:42
@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch from 80d1907 to af6b1b3 Compare April 1, 2024 12:49
@hi-rustin hi-rustin changed the title feat: add expiry_notification_at migration feat: add ExpiryNotification job Apr 1, 2024
@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch 2 times, most recently from fd03865 to 8f2f2e5 Compare April 1, 2024 12:55
@hi-rustin hi-rustin marked this pull request as draft April 1, 2024 13:14
@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch 2 times, most recently from f4b1858 to 0a1214c Compare April 5, 2024 07:40
Copy link
Member Author

@hi-rustin hi-rustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

src/worker/jobs/expiry_notification.rs Outdated Show resolved Hide resolved
@hi-rustin hi-rustin changed the title feat: add ExpiryNotification job feat: add CheckAboutToExpireToken job Apr 5, 2024
@hi-rustin hi-rustin marked this pull request as ready for review April 5, 2024 07:44
@hi-rustin
Copy link
Member Author

Tested locally:

  1. Start the crates.io in the docker
crates.io on  rustin-patch-expiry_notification_job [$!] via 🐳 orbstack is 📦 v0.0.0 via  v20.12.0 via 🦀 v1.77.0 docker ps
CONTAINER ID   IMAGE               COMMAND                  CREATED          STATUS          PORTS                                       NAMES
ea2f555e8f9f   cratesio-frontend   "pnpm start"             40 minutes ago   Up 15 minutes   0.0.0.0:4200->4200/tcp, :::4200->4200/tcp   cratesio-frontend-1
a3f3d61c8730   c944ce643ba1        "cargo run --bin bac…"   40 minutes ago   Up 2 minutes                                                cratesio-worker-1
d0dc3fabda26   cratesio-backend    "/app/docker_entrypo…"   40 minutes ago   Up 14 minutes   0.0.0.0:8888->8888/tcp, :::8888->8888/tcp   cratesio-backend-1
13a8ba48f41e   postgres:13         "docker-entrypoint.s…"   40 minutes ago   Up 40 minutes   127.0.0.1:5432->5432/tcp                    cratesio-postgres-1
  1. Create a new token which will expire within one day
image
  1. Trigger a background job: DATABASE_URL=postgresql://postgres:password@localhost:5432/cargo_registry ./target/debug/crates-admin enqueue-job check_about_to_expire_token
  2. Check the email from the worker's tmp dir
Message-ID: <MJOpaJaGzSXbORzJjtAbjXqAcc0STbPw@crates.io>
To: rustin.liu@gmail.com
From: noreply@crates.io
Subject: Your token is about to expire
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Fri, 05 Apr 2024 08:26:31 +0000

Hi @hi-rustin,

We noticed your token "test_token" will expire in about 3 days.

If this token is still needed, visit https://crates.io/settings/tokens/new =
to generate a new one.

Thanks,
The crates.io team
  1. trigger it again: DATABASE_URL=postgresql://postgres:password@localhost:5432/cargo_registry ./target/debug/crates-admin enqueue-job check_about_to_expire_token
  2. Check no more new emails sent by the worker

@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch from 2922fbc to 97411c9 Compare April 5, 2024 12:55
@hi-rustin hi-rustin requested a review from a team April 5, 2024 12:56
src/models/token.rs Outdated Show resolved Hide resolved
@eth3lbert
Copy link
Contributor

A couple of comments, but overall this looks good to me! 👍

@eth3lbert
Copy link
Contributor

I also noticed that since we don't have an index on expired_at, I'm not sure how longfind_tokens_expiring_within_days will take. We might need to investigate this further in production.

@hi-rustin hi-rustin force-pushed the rustin-patch-expiry_notification_job branch 3 times, most recently from d0efb44 to c2ca897 Compare April 8, 2024 02:02
hi-rustin and others added 22 commits May 15, 2024 16:41
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Using multiple `filter()` calls is equivalent to using `.and()`, but reduces the indentation level and makes the code slightly more readable :)
We can calculate the cut-off datetime on the application side instead of relying on `.num_days().day()`, which was slightly error-prone for intervals that are not full days.
@Turbo87 Turbo87 force-pushed the rustin-patch-expiry_notification_job branch from 394f1af to cb19848 Compare May 15, 2024 14:42
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I've rebased the branch and added a couple of small commits on top. Thanks for working on this!

@Turbo87 Turbo87 enabled auto-merge May 15, 2024 14:46
@Turbo87 Turbo87 merged commit 37a63de into rust-lang:main May 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants