From ab1cb0e65feda575b49fa9b5b1df331366a451ff Mon Sep 17 00:00:00 2001 From: Emmie Maeda Date: Sun, 6 Oct 2024 22:29:35 -0400 Subject: [PATCH 1/5] Rename check_latest_revision -> assert_latest_revision. --- deepwell/src/services/page/service.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/deepwell/src/services/page/service.rs b/deepwell/src/services/page/service.rs index c55c9d3c77..a18311b587 100644 --- a/deepwell/src/services/page/service.rs +++ b/deepwell/src/services/page/service.rs @@ -110,7 +110,7 @@ impl PageService { ..Default::default() }; let page = model.update(txn).await?; - check_latest_revision(&page); + assert_latest_revision(&page); // Build and return Ok(CreatePageOutput { @@ -200,7 +200,7 @@ impl PageService { ..Default::default() }; let page = model.update(txn).await?; - check_latest_revision(&page); + assert_latest_revision(&page); // Build and return Ok(revision_output) @@ -282,7 +282,7 @@ impl PageService { ..Default::default() }; let page = model.update(txn).await?; - check_latest_revision(&page); + assert_latest_revision(&page); // Build and return @@ -343,7 +343,7 @@ impl PageService { ..Default::default() }; let page = model.update(txn).await?; - check_latest_revision(&page); + assert_latest_revision(&page); Ok((output, page_id).into()) } @@ -413,7 +413,7 @@ impl PageService { ..Default::default() }; let page = model.update(txn).await?; - check_latest_revision(&page); + assert_latest_revision(&page); Ok((output, slug).into()) } @@ -830,7 +830,7 @@ impl PageService { } } -fn check_latest_revision(page: &PageModel) { +fn assert_latest_revision(page: &PageModel) { // Even in production, we want to assert that this invariant holds. // // We cannot set the column itself to NOT NULL because of cyclic update From 16889e1afb75b7f76b4688627105a57e869a9d13 Mon Sep 17 00:00:00 2001 From: Emmie Maeda Date: Sun, 6 Oct 2024 22:41:17 -0400 Subject: [PATCH 2/5] Add docs for assert_latest_revision(). --- deepwell/src/services/page/service.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/deepwell/src/services/page/service.rs b/deepwell/src/services/page/service.rs index a18311b587..9a8f328a04 100644 --- a/deepwell/src/services/page/service.rs +++ b/deepwell/src/services/page/service.rs @@ -830,6 +830,7 @@ impl PageService { } } +/// Ensure that the page has a properly-set `latest_revision_id` column. fn assert_latest_revision(page: &PageModel) { // Even in production, we want to assert that this invariant holds. // From 4b25adfd825a51a291bea62ef115275629348ac3 Mon Sep 17 00:00:00 2001 From: Emmie Maeda Date: Sun, 6 Oct 2024 23:57:21 -0400 Subject: [PATCH 3/5] Use comment marker NOTE. --- deepwell/src/services/page/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepwell/src/services/page/service.rs b/deepwell/src/services/page/service.rs index 9a8f328a04..0c217ac3e3 100644 --- a/deepwell/src/services/page/service.rs +++ b/deepwell/src/services/page/service.rs @@ -444,7 +444,7 @@ impl PageService { PageRevisionService::get_latest(ctx, site_id, page_id), )?; - // Note: we can't just copy the wikitext_hash because we + // NOTE: we can't just copy the wikitext_hash because we // need its actual value for rendering. // This isn't run here, but in PageRevisionService::create(). let wikitext = TextService::get(ctx, &target_revision.wikitext_hash).await?; From b0823b0aaca18d84d49131ec452914bf365f684d Mon Sep 17 00:00:00 2001 From: Emmie Maeda Date: Mon, 7 Oct 2024 00:04:16 -0400 Subject: [PATCH 4/5] Add Error::NotLatestRevisionId for last revision arguments. --- deepwell/src/services/error.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/deepwell/src/services/error.rs b/deepwell/src/services/error.rs index 1277f274bb..d7bcdc414f 100644 --- a/deepwell/src/services/error.rs +++ b/deepwell/src/services/error.rs @@ -131,6 +131,9 @@ pub enum Error { #[error("Cannot hide the wikitext for the latest page revision")] CannotHideLatestRevision, + #[error("Revision ID passed for this operation is not the latest")] + NotLatestRevisionId, + #[error("The regular expression found in the database is invalid")] FilterRegexInvalid(regex::Error), @@ -388,6 +391,7 @@ impl Error { Error::BlobTooBig => 4023, Error::BlobNotUploaded => 4024, Error::BlobSizeMismatch => 4025, + Error::NotLatestRevisionId => 4027, // 4100 -- Localization Error::LocaleInvalid(_) => 4100, From f9a41678ac5aa692bf02bb93d6e328618f7845df Mon Sep 17 00:00:00 2001 From: Emmie Maeda Date: Mon, 7 Oct 2024 00:04:35 -0400 Subject: [PATCH 5/5] Add check for last_revision_id in PageService. --- deepwell/src/services/page/service.rs | 77 ++++++++++++++++++++++++--- deepwell/src/services/page/structs.rs | 4 ++ 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/deepwell/src/services/page/service.rs b/deepwell/src/services/page/service.rs index 0c217ac3e3..6f5446036d 100644 --- a/deepwell/src/services/page/service.rs +++ b/deepwell/src/services/page/service.rs @@ -21,6 +21,7 @@ use super::prelude::*; use crate::models::page::{self, Entity as Page, Model as PageModel}; use crate::models::page_category::Model as PageCategoryModel; +use crate::models::page_revision::Model as PageRevisionModel; use crate::services::filter::{FilterClass, FilterType}; use crate::services::page_revision::{ CreateFirstPageRevision, CreateFirstPageRevisionOutput, CreatePageRevision, @@ -126,6 +127,7 @@ impl PageService { EditPage { site_id, page: reference, + last_revision_id, revision_comments: comments, user_id, body: @@ -138,7 +140,11 @@ impl PageService { }: EditPage<'_>, ) -> Result> { let txn = ctx.transaction(); - let PageModel { page_id, .. } = Self::get(ctx, site_id, reference).await?; + let PageModel { + page_id, + latest_revision_id, + .. + } = Self::get(ctx, site_id, reference).await?; // Perform filter validation Self::run_filter( @@ -154,10 +160,12 @@ impl PageService { ) .await?; - // Get latest revision + // Get and check latest revision let last_revision = PageRevisionService::get_latest(ctx, site_id, page_id).await?; + check_last_revision(Some(&last_revision), latest_revision_id, last_revision_id)?; + // Create new revision // // A response of None means no revision was created @@ -213,18 +221,22 @@ impl PageService { site_id, page: reference, mut new_slug, + last_revision_id, revision_comments: comments, user_id, }: MovePage<'_>, ) -> Result { let txn = ctx.transaction(); - let PageModel { page_id, slug: old_slug, + latest_revision_id, .. } = Self::get(ctx, site_id, reference).await?; + // Check last revision ID argument + check_last_revision(None, latest_revision_id, last_revision_id)?; + // Check that a move is actually taking place, // and that a page with that slug doesn't already exist. normalize(&mut new_slug); @@ -310,17 +322,24 @@ impl PageService { DeletePage { site_id, page: reference, + last_revision_id, user_id, revision_comments: comments, }: DeletePage<'_>, ) -> Result { let txn = ctx.transaction(); - let PageModel { page_id, .. } = Self::get(ctx, site_id, reference).await?; + let PageModel { + page_id, + latest_revision_id, + .. + } = Self::get(ctx, site_id, reference).await?; - // Get latest revision + // Get and check latest revision let last_revision = PageRevisionService::get_latest(ctx, site_id, page_id).await?; + check_last_revision(Some(&last_revision), latest_revision_id, last_revision_id)?; + // Create tombstone revision // This also updates backlinks, includes, etc let output = PageRevisionService::create_tombstone( @@ -430,13 +449,18 @@ impl PageService { RollbackPage { site_id, page: reference, + last_revision_id, revision_number, revision_comments: comments, user_id, }: RollbackPage<'_>, ) -> Result> { let txn = ctx.transaction(); - let PageModel { page_id, .. } = Self::get(ctx, site_id, reference).await?; + let PageModel { + page_id, + latest_revision_id, + .. + } = Self::get(ctx, site_id, reference).await?; // Get target revision and latest revision let (target_revision, last_revision) = try_join!( @@ -444,6 +468,9 @@ impl PageService { PageRevisionService::get_latest(ctx, site_id, page_id), )?; + // Check last revision ID + check_last_revision(Some(&last_revision), latest_revision_id, last_revision_id)?; + // NOTE: we can't just copy the wikitext_hash because we // need its actual value for rendering. // This isn't run here, but in PageRevisionService::create(). @@ -830,7 +857,45 @@ impl PageService { } } +/// Verifies that a `last_revision_id` passed into this function is actually the latest. +/// +/// This is to avoid issues wherein a user edits overs a more recently-updated page +/// without realizing it, since attempting to make this edit would cause the backend +/// to produce an error saying that the request had too old of a revision ID and thus +/// the page would need to be refreshed. +/// +/// This check is intended for before an operation has run. +fn check_last_revision( + last_revision_model: Option<&PageRevisionModel>, + page_latest_revision_id: Option, + arg_last_revision_id: i64, +) -> Result<()> { + // Only check if we have this model fetched anyways + if let Some(model) = last_revision_model { + assert_eq!( + model.revision_id, + page_latest_revision_id.expect("Page row has NULL latest_revision_id"), + "Page table has an inconsistent last_revision_id column value", + ); + } + + // Perform main check, ensure that the argument matches the latest + if page_latest_revision_id != Some(arg_last_revision_id) { + error!( + "Latest revision ID in page struct is {}, but user argument has ID {}", + page_latest_revision_id.unwrap(), + arg_last_revision_id, + ); + + return Err(Error::NotLatestRevisionId); + } + + Ok(()) +} + /// Ensure that the page has a properly-set `latest_revision_id` column. +/// +/// This check is intended for after an operation has run. fn assert_latest_revision(page: &PageModel) { // Even in production, we want to assert that this invariant holds. // diff --git a/deepwell/src/services/page/structs.rs b/deepwell/src/services/page/structs.rs index 0b3967f2e2..372ca50bfc 100644 --- a/deepwell/src/services/page/structs.rs +++ b/deepwell/src/services/page/structs.rs @@ -146,6 +146,7 @@ pub struct GetPageScoreOutput { pub struct EditPage<'a> { pub site_id: i64, pub page: Reference<'a>, + pub last_revision_id: i64, pub revision_comments: String, pub user_id: i64, @@ -166,6 +167,7 @@ pub struct EditPageBody { pub struct MovePage<'a> { pub site_id: i64, pub page: Reference<'a>, + pub last_revision_id: i64, pub new_slug: String, pub revision_comments: String, pub user_id: i64, @@ -185,6 +187,7 @@ pub struct MovePageOutput { pub struct DeletePage<'a> { pub site_id: i64, pub page: Reference<'a>, + pub last_revision_id: i64, pub revision_comments: String, pub user_id: i64, } @@ -217,6 +220,7 @@ pub struct RestorePageOutput { pub struct RollbackPage<'a> { pub site_id: i64, pub page: Reference<'a>, + pub last_revision_id: i64, pub revision_number: i32, pub revision_comments: String, pub user_id: i64,