bookmarks_movement: check whether bookmarks are being redirected

Summary:
Currently the source control service will accept bookmark moves for repos that
are the small-repo in a push-redirected megarepo config.  This means it's
possible for users to get bookmarks out of sync with a bookmark move, which
requires manual intervention to fix.

Prevent this from happening by adding a check in `bookmarks_movement`.  If a
repo has push-redirection enabled, then bookmarks of that kind are not allowed
to be moved by the source control service.  Users should move the bookmark in
the source-of-truth repo and allow the backsyncer to move it in the
push-redirected repo.

Reviewed By: mitrandir77

Differential Revision: D34222495

fbshipit-source-id: 64a0fa929af53c9991c2a0f8bb84a21e4a1de9fc
This commit is contained in:
Mark Juggurnauth-Thomas 2022-02-17 08:48:35 -08:00 committed by Facebook GitHub Bot
parent 6d30c47bc4
commit 0bdf6d64d9
7 changed files with 63 additions and 4 deletions

View File

@ -42,6 +42,7 @@ pushrebase_hook = { version = "0.1.0", path = "../../pushrebase/pushrebase_hook"
pushrebase_mutation_mapping = { version = "0.1.0", path = "../../pushrebase_mutation_mapping" }
reachabilityindex = { version = "0.1.0", path = "../../reachabilityindex" }
repo_blobstore = { version = "0.1.0", path = "../../blobrepo/repo_blobstore" }
repo_cross_repo = { version = "0.1.0", path = "../../repo_attributes/repo_cross_repo" }
repo_derived_data = { version = "0.1.0", path = "../../repo_attributes/repo_derived_data" }
repo_identity = { version = "0.1.0", path = "../../repo_attributes/repo_identity" }
repo_read_write_status = { version = "0.1.0", path = "../../repo_client/repo_read_write_status" }

View File

@ -24,7 +24,9 @@ use crate::affected_changesets::{
find_draft_ancestors, log_bonsai_commits_to_scribe, AdditionalChangesets, AffectedChangesets,
};
use crate::repo_lock::check_repo_lock;
use crate::restrictions::{BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization};
use crate::restrictions::{
check_bookmark_sync_config, BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization,
};
use crate::BookmarkMovementError;
use crate::Repo;
@ -132,6 +134,8 @@ impl<'op> CreateBookmarkOp<'op> {
.check_authorized(ctx, bookmark_attrs, self.bookmark)
.await?;
check_bookmark_sync_config(repo, self.bookmark, kind)?;
self.affected_changesets
.check_restrictions(
ctx,

View File

@ -16,7 +16,9 @@ use mononoke_types::ChangesetId;
use repo_read_write_status::RepoReadWriteFetcher;
use crate::repo_lock::check_repo_lock;
use crate::restrictions::{BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization};
use crate::restrictions::{
check_bookmark_sync_config, BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization,
};
use crate::{BookmarkMovementError, Repo};
pub struct DeleteBookmarkOp<'op> {
@ -94,6 +96,8 @@ impl<'op> DeleteBookmarkOp<'op> {
.check_authorized(ctx, bookmark_attrs, self.bookmark)
.await?;
check_bookmark_sync_config(repo, self.bookmark, kind)?;
if bookmark_attrs.is_fast_forward_only(self.bookmark) {
// Cannot delete fast-forward-only bookmarks.
return Err(BookmarkMovementError::DeletionProhibited {

View File

@ -21,6 +21,7 @@ use phases::PhasesRef;
use pushrebase::PushrebaseError;
use pushrebase_mutation_mapping::PushrebaseMutationMappingRef;
use repo_blobstore::RepoBlobstoreRef;
use repo_cross_repo::RepoCrossRepoRef;
use repo_derived_data::RepoDerivedDataRef;
use repo_identity::RepoIdentityRef;
use thiserror::Error;
@ -62,6 +63,7 @@ pub trait Repo = AsBlobRepo
+ PushrebaseMutationMappingRef
+ RepoDerivedDataRef
+ RepoBlobstoreRef
+ RepoCrossRepoRef
+ RepoIdentityRef;
/// An error encountered during an attempt to move a bookmark.
@ -154,6 +156,14 @@ pub enum BookmarkMovementError {
descendant_bookmark: BookmarkName,
},
#[error("Bookmark '{bookmark}' cannot be moved because public bookmarks are being redirected")]
PushRedirectorEnabledForPublic { bookmark: BookmarkName },
#[error(
"Bookmark '{bookmark}' cannot be moved because scratch bookmarks are being redirected"
)]
PushRedirectorEnabledForScratch { bookmark: BookmarkName },
#[error(transparent)]
Error(#[from] anyhow::Error),
}

View File

@ -31,7 +31,9 @@ use repo_read_write_status::RepoReadWriteFetcher;
use crate::affected_changesets::{AdditionalChangesets, AffectedChangesets};
use crate::repo_lock::{check_repo_lock, RepoLockPushrebaseHook};
use crate::restrictions::{BookmarkKindRestrictions, BookmarkMoveAuthorization};
use crate::restrictions::{
check_bookmark_sync_config, BookmarkKindRestrictions, BookmarkMoveAuthorization,
};
use crate::{BookmarkMovementError, Repo};
pub struct PushrebaseOntoBookmarkOp<'op> {
@ -116,6 +118,8 @@ impl<'op> PushrebaseOntoBookmarkOp<'op> {
.check_authorized(ctx, bookmark_attrs, self.bookmark)
.await?;
check_bookmark_sync_config(repo, self.bookmark, kind)?;
if pushrebase_params.block_merges {
let any_merges = self
.affected_changesets

View File

@ -185,3 +185,35 @@ pub(crate) async fn ensure_ancestor_of(
.is_ancestor(ctx, &repo.changeset_fetcher_arc(), target, descendant_cs_id)
.await?)
}
pub(crate) fn check_bookmark_sync_config(
repo: &impl Repo,
bookmark: &BookmarkName,
kind: BookmarkKind,
) -> Result<(), BookmarkMovementError> {
match kind {
BookmarkKind::Public => {
if repo
.repo_cross_repo()
.live_commit_sync_config()
.push_redirector_enabled_for_public(repo.repo_identity().id())
{
return Err(BookmarkMovementError::PushRedirectorEnabledForPublic {
bookmark: bookmark.clone(),
});
}
}
BookmarkKind::Scratch => {
if repo
.repo_cross_repo()
.live_commit_sync_config()
.push_redirector_enabled_for_draft(repo.repo_identity().id())
{
return Err(BookmarkMovementError::PushRedirectorEnabledForScratch {
bookmark: bookmark.clone(),
});
}
}
}
Ok(())
}

View File

@ -25,7 +25,9 @@ use crate::affected_changesets::{
find_draft_ancestors, log_bonsai_commits_to_scribe, AdditionalChangesets, AffectedChangesets,
};
use crate::repo_lock::check_repo_lock;
use crate::restrictions::{BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization};
use crate::restrictions::{
check_bookmark_sync_config, BookmarkKind, BookmarkKindRestrictions, BookmarkMoveAuthorization,
};
use crate::{BookmarkMovementError, Repo};
/// The old and new changeset during a bookmark update.
@ -189,6 +191,8 @@ impl<'op> UpdateBookmarkOp<'op> {
.check_authorized(ctx, bookmark_attrs, self.bookmark)
.await?;
check_bookmark_sync_config(repo, self.bookmark, kind)?;
self.update_policy
.check_update_permitted(
ctx,