Merge pull request #10554 from Turbo87/invite-expires-at-2

Simplify `crate_owner_invitations` expiry checks

Author: Turbo87

crates/crates_io_database/src/schema.rs

@@ -222,7 +222,7 @@ diesel::table! {
         /// (Automatically generated by Diesel.)
         token -> Text,
         /// Point in time at which the invitation expires/expired.
-        expires_at -> Nullable<Timestamptz>,
+        expires_at -> Timestamptz,
     }
 }
 

migrations/2025-02-11-122510_not-null-expires-at-column/down.sql

@@ -0,0 +1 @@
+alter table crate_owner_invitations alter column expires_at drop not null;

migrations/2025-02-11-122510_not-null-expires-at-column/up.sql

@@ -0,0 +1 @@
+alter table crate_owner_invitations alter column expires_at set not null;

src/controllers/crate_owner_invitation.rs

@@ -265,7 +265,6 @@ async fn prepare_list(
     }
 
     // Turn `CrateOwnerInvitation`s into `EncodablePrivateCrateOwnerInvitation`.
-    let config = &state.config;
     let mut invitations = Vec::new();
     let mut users_in_response = HashSet::new();
     for invitation in raw_invitations.into_iter() {
@@ -278,7 +277,7 @@ async fn prepare_list(
                 .ok_or_else(|| internal(format!("missing crate with id {}", invitation.crate_id)))?
                 .clone(),
             created_at: invitation.created_at,
-            expires_at: invitation.expires_at(config),
+            expires_at: invitation.expires_at.naive_utc(),
         });
         users_in_response.insert(invitation.invited_user_id);
         users_in_response.insert(invitation.invited_by_user_id);
@@ -342,10 +341,8 @@ pub async fn handle_crate_owner_invitation(
     let invitation =
         CrateOwnerInvitation::find_by_id(user_id, crate_invite.crate_id, &mut conn).await?;
 
-    let config = &state.config;
-
     if crate_invite.accepted {
-        invitation.accept(&mut conn, config).await?;
+        invitation.accept(&mut conn).await?;
     } else {
         invitation.decline(&mut conn).await?;
     }
@@ -370,10 +367,8 @@ pub async fn accept_crate_owner_invitation_with_token(
     let mut conn = state.db_write().await?;
     let invitation = CrateOwnerInvitation::find_by_token(&token, &mut conn).await?;
 
-    let config = &state.config;
-
     let crate_id = invitation.crate_id;
-    invitation.accept(&mut conn, config).await?;
+    invitation.accept(&mut conn).await?;
 
     Ok(json!({
         "crate_owner_invitation": {

src/models/crate_owner_invitation.rs

@@ -4,7 +4,6 @@ use diesel_async::scoped_futures::ScopedFutureExt;
 use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
 use secrecy::SecretString;
 
-use crate::config;
 use crate::models::CrateOwner;
 use crate::schema::{crate_owner_invitations, crate_owners, crates};
 
@@ -27,32 +26,16 @@ impl NewCrateOwnerInvitation {
     pub async fn create(
         &self,
         conn: &mut AsyncPgConnection,
-        config: &config::Server,
     ) -> QueryResult<NewCrateOwnerInvitationOutcome> {
         // Before actually creating the invite, check if an expired invitation already exists
         // and delete it from the database. This allows obtaining a new invite if the old one
         // expired, instead of returning "already exists".
-        conn.transaction(|conn| {
-            async move {
-                // This does a SELECT FOR UPDATE + DELETE instead of a DELETE with a WHERE clause to
-                // use the model's `is_expired` method, centralizing our expiration checking logic.
-                let existing: Option<CrateOwnerInvitation> = crate_owner_invitations::table
-                    .find((self.invited_user_id, self.crate_id))
-                    .for_update()
-                    .first(conn)
-                    .await
-                    .optional()?;
-
-                if let Some(existing) = existing {
-                    if existing.is_expired(config) {
-                        diesel::delete(&existing).execute(conn).await?;
-                    }
-                }
-                QueryResult::Ok(())
-            }
-            .scope_boxed()
-        })
-        .await?;
+        diesel::delete(crate_owner_invitations::table)
+            .filter(crate_owner_invitations::invited_user_id.eq(self.invited_user_id))
+            .filter(crate_owner_invitations::crate_id.eq(self.crate_id))
+            .filter(crate_owner_invitations::expires_at.le(Utc::now()))
+            .execute(conn)
+            .await?;
 
         let res: Option<CrateOwnerInvitation> = diesel::insert_into(crate_owner_invitations::table)
             .values(self)
@@ -83,7 +66,7 @@ pub struct CrateOwnerInvitation {
     pub created_at: NaiveDateTime,
     #[diesel(deserialize_as = String)]
     pub token: SecretString,
-    pub expires_at: Option<DateTime<Utc>>,
+    pub expires_at: DateTime<Utc>,
 }
 
 impl CrateOwnerInvitation {
@@ -105,15 +88,8 @@ impl CrateOwnerInvitation {
             .await
     }
 
-    pub async fn accept(
-        self,
-        conn: &mut AsyncPgConnection,
-        config: &config::Server,
-    ) -> Result<(), AcceptError> {
-        use diesel_async::scoped_futures::ScopedFutureExt;
-        use diesel_async::{AsyncConnection, RunQueryDsl};
-
-        if self.is_expired(config) {
+    pub async fn accept(self, conn: &mut AsyncPgConnection) -> Result<(), AcceptError> {
+        if self.is_expired() {
             let crate_name: String = crates::table
                 .find(self.crate_id)
                 .select(crates::name)
@@ -151,12 +127,8 @@ impl CrateOwnerInvitation {
         Ok(())
     }
 
-    pub fn is_expired(&self, config: &config::Server) -> bool {
-        self.expires_at(config) <= Utc::now().naive_utc()
-    }
-
-    pub fn expires_at(&self, config: &config::Server) -> NaiveDateTime {
-        self.created_at + config.ownership_invitations_expiration
+    pub fn is_expired(&self) -> bool {
+        self.expires_at <= Utc::now()
     }
 }
 

src/models/krate.rs

@@ -408,10 +408,7 @@ impl Crate {
                     expires_at,
                 };
 
-                let creation_ret = invite
-                    .create(conn, &app.config)
-                    .await
-                    .map_err(BoxedAppError::from)?;
+                let creation_ret = invite.create(conn).await.map_err(BoxedAppError::from)?;
 
                 match creation_ret {
                     NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => {

src/tests/owners.rs

@@ -626,10 +626,15 @@ pub async fn expire_invitation(app: &TestApp, crate_id: i32) {
     let mut conn = app.db_conn().await;
 
     let expiration = app.as_inner().config.ownership_invitations_expiration;
-    let created_at = (Utc::now() - expiration).naive_utc();
+
+    let now = Utc::now();
+    let created_at = (now - expiration).naive_utc();
 
     diesel::update(crate_owner_invitations::table)
-        .set(crate_owner_invitations::created_at.eq(created_at))
+        .set((
+            crate_owner_invitations::created_at.eq(created_at),
+            crate_owner_invitations::expires_at.eq(now),
+        ))
         .filter(crate_owner_invitations::crate_id.eq(crate_id))
         .execute(&mut conn)
         .await