mononoke: make it possible to allow moving a bookmark for a hipster group

Summary:
Currently we can only limit which users are allowed to move a bookmark by a
regex. We also want to allow specifying a hipster group.

Reviewed By: krallin

Differential Revision: D27156690

fbshipit-source-id: 99a5678a82f4c34ed2e57625361ba7cdb08ed839
This commit is contained in:
Stanislau Hlebik 2021-03-18 13:00:23 -07:00 committed by Facebook GitHub Bot
parent be1fa084a9
commit 847a91291b
18 changed files with 135 additions and 32 deletions

View File

@ -1,4 +1,4 @@
// @generated SignedSource<<f73b0b26e6676b9bf18e2617a39058bb>>
// @generated SignedSource<<9b1fc1c997b55c61d392b8d091eb0388>>
// DO NOT EDIT THIS FILE MANUALLY!
// This file is a mechanical copy of the version in the configerator repo. To
// modify it, edit the copy in the configerator repo instead and copy it over by
@ -345,10 +345,16 @@ struct RawBookmarkConfig {
// Are non fastforward moves allowed for this bookmark
4: bool only_fast_forward,
// If specified, and if the user's unixname is known, only users matching
// this pattern (regex) will be allowed to move this bookmark.
// If specified, and if the user's unixname is known, only users who
// belong to this group or match allowed_users will be allowed to move this
// bookmark.
5: optional string allowed_users,
// If specified, and if the user's unixname is known, only users who
// belong to this group or match allowed_users will be allowed to move this
// bookmark.
9: optional string allowed_hipster_group,
// If true, only external sync will be allowed to move this
// bookmark.
8: optional bool allow_only_external_sync,

View File

@ -122,7 +122,7 @@ impl AffectedChangesets {
let mut exclude_bookmarks: HashSet<_> = bookmark_attrs
.select(bookmark)
.map(|params| params.hooks_skip_ancestors_of.iter())
.map(|attr| attr.params().hooks_skip_ancestors_of.iter())
.flatten()
.cloned()
.collect();

View File

@ -129,7 +129,8 @@ impl<'op> CreateBookmarkOp<'op> {
.check_kind(infinitepush_params, self.bookmark)?;
self.auth
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)?;
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)
.await?;
self.affected_changesets
.check_restrictions(

View File

@ -92,7 +92,8 @@ impl<'op> DeleteBookmarkOp<'op> {
.check_kind(infinitepush_params, self.bookmark)?;
self.auth
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)?;
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)
.await?;
if kind == BookmarkKind::Scratch || bookmark_attrs.is_fast_forward_only(self.bookmark) {
// Cannot delete scratch or fast-forward-only bookmarks.

View File

@ -110,7 +110,8 @@ impl<'op> PushrebaseOntoBookmarkOp<'op> {
.check_kind(infinitepush_params, self.bookmark)?;
self.auth
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)?;
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)
.await?;
if pushrebase_params.block_merges {
let any_merges = self

View File

@ -28,7 +28,7 @@ pub(crate) enum BookmarkMoveAuthorization<'params> {
}
impl<'params> BookmarkMoveAuthorization<'params> {
pub(crate) fn check_authorized(
pub(crate) async fn check_authorized(
&'params self,
ctx: &CoreContext,
bookmark_attrs: &BookmarkAttrs,
@ -53,7 +53,10 @@ impl<'params> BookmarkMoveAuthorization<'params> {
let user = ctx.metadata().unix_name().unwrap_or("svcscm");
// TODO: clean up `is_allowed_user` to avoid this clone.
if !bookmark_attrs.is_allowed_user(&Some(user.to_string()), bookmark) {
if !bookmark_attrs
.is_allowed_user(&Some(user.to_string()), ctx.metadata(), bookmark)
.await?
{
return Err(BookmarkMovementError::PermissionDeniedUser {
user: user.to_string(),
bookmark: bookmark.clone(),

View File

@ -182,7 +182,8 @@ impl<'op> UpdateBookmarkOp<'op> {
.check_kind(infinitepush_params, self.bookmark)?;
self.auth
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)?;
.check_authorized(ctx, bookmark_attrs, self.bookmark, kind)
.await?;
self.update_policy
.check_update_permitted(

View File

@ -1476,6 +1476,7 @@ fn test_verify_integrity_fast_failure(fb: FacebookInit) {
hooks: vec!["verify_integrity".into()],
only_fast_forward: false,
allowed_users: None,
allowed_hipster_group: None,
allow_only_external_sync: None,
rewrite_dates: None,
hooks_skip_ancestors_of: vec![],
@ -1504,6 +1505,7 @@ fn test_load_hooks_bad_rust_hook(fb: FacebookInit) {
hooks: vec!["hook1".into()],
only_fast_forward: false,
allowed_users: None,
allowed_hipster_group: None,
allow_only_external_sync: None,
rewrite_dates: None,
hooks_skip_ancestors_of: vec![],
@ -1559,6 +1561,7 @@ fn test_load_disabled_hooks_referenced_by_bookmark(fb: FacebookInit) {
hooks: vec!["hook1".into()],
only_fast_forward: false,
allowed_users: None,
allowed_hipster_group: None,
allow_only_external_sync: None,
rewrite_dates: None,
hooks_skip_ancestors_of: vec![],

View File

@ -912,6 +912,7 @@ mod test {
hooks: vec!["hook1".to_string(), "rust:rusthook".to_string()],
only_fast_forward: false,
allowed_users: Some(Regex::new("^(svcscm|twsvcscm)$").unwrap().into()),
allowed_hipster_group: None,
allow_only_external_sync: None,
rewrite_dates: None,
hooks_skip_ancestors_of: vec![],
@ -921,6 +922,7 @@ mod test {
hooks: vec![],
only_fast_forward: false,
allowed_users: None,
allowed_hipster_group: None,
allow_only_external_sync: None,
rewrite_dates: None,
hooks_skip_ancestors_of: vec![],

View File

@ -169,6 +169,7 @@ impl Convert for RawBookmarkConfig {
.map(|re| Regex::new(&re))
.transpose()?
.map(ComparableRegex::new);
let allowed_hipster_group = self.allowed_hipster_group;
let allow_only_external_sync = self.allow_only_external_sync;
let rewrite_dates = self.rewrite_dates;
let hooks_skip_ancestors_of = self
@ -183,6 +184,7 @@ impl Convert for RawBookmarkConfig {
hooks,
only_fast_forward,
allowed_users,
allowed_hipster_group,
allow_only_external_sync,
rewrite_dates,
hooks_skip_ancestors_of,

View File

@ -9,9 +9,12 @@ license = "GPLv2+"
anyhow = "1.0"
ascii = "1.0"
bookmarks_types = { version = "0.1.0", path = "../../bookmarks/bookmarks_types" }
fbinit = { version = "0.1.0", git = "https://github.com/facebookexperimental/rust-shed.git", branch = "master" }
mononoke_types = { version = "0.1.0", path = "../../mononoke_types" }
permission_checker = { version = "0.1.0", path = "../../permission_checker" }
regex = "1.4.2"
scuba = { version = "0.1.0", git = "https://github.com/facebookexperimental/rust-shed.git", branch = "master" }
serde = { version = "=1.0.118", features = ["derive", "rc"] }
serde_derive = "1.0"
sql = { version = "0.1.0", git = "https://github.com/facebookexperimental/rust-shed.git", branch = "master" }
sshrelay = { version = "0.1.0", path = "../../sshrelay" }

View File

@ -26,7 +26,9 @@ use std::{
use ascii::AsciiString;
use bookmarks_types::BookmarkName;
use fbinit::FacebookInit;
use mononoke_types::{BonsaiChangeset, MPath, PrefixTrie, RepositoryId};
use permission_checker::{BoxMembershipChecker, MembershipCheckerBuilder};
use regex::Regex;
use scuba::ScubaValue;
use serde_derive::Deserialize;
@ -36,6 +38,7 @@ use sql::mysql_async::{
prelude::{ConvIr, FromValue},
FromValueError, Value,
};
use sshrelay::Metadata;
/// A Regex that can be compared against other Regexes.
///
@ -362,42 +365,80 @@ impl From<Regex> for BookmarkOrRegex {
}
}
/// Attributes for a single bookmark
pub struct SingleBookmarkAttr {
params: BookmarkParams,
membership: Option<BoxMembershipChecker>,
}
impl SingleBookmarkAttr {
fn new(params: BookmarkParams, membership: Option<BoxMembershipChecker>) -> Self {
Self { params, membership }
}
/// Bookmark parameters from config
pub fn params(&self) -> &BookmarkParams {
&self.params
}
/// Membership checker
pub fn membership(&self) -> &Option<BoxMembershipChecker> {
&self.membership
}
}
/// Collection of all bookmark attribtes
#[derive(Clone)]
pub struct BookmarkAttrs {
bookmark_params: Arc<Vec<BookmarkParams>>,
bookmark_attrs: Arc<Vec<SingleBookmarkAttr>>,
}
impl BookmarkAttrs {
/// create bookmark attributes from bookmark params vector
pub fn new(bookmark_params: impl Into<Arc<Vec<BookmarkParams>>>) -> Self {
Self {
bookmark_params: bookmark_params.into(),
pub async fn new(
fb: FacebookInit,
bookmark_params: impl IntoIterator<Item = BookmarkParams>,
) -> Result<Self, Error> {
let mut v = vec![];
for params in bookmark_params {
let membership_checker = match params.allowed_hipster_group {
Some(ref hipster_group) => {
Some(MembershipCheckerBuilder::for_group(fb, &hipster_group).await?)
}
None => None,
};
v.push(SingleBookmarkAttr::new(params, membership_checker));
}
Ok(Self {
bookmark_attrs: Arc::new(v),
})
}
/// select bookmark params matching provided bookmark
pub fn select<'a>(
&'a self,
bookmark: &'a BookmarkName,
) -> impl Iterator<Item = &'a BookmarkParams> {
self.bookmark_params
) -> impl Iterator<Item = &'a SingleBookmarkAttr> {
self.bookmark_attrs
.iter()
.filter(move |params| params.bookmark.matches(bookmark))
.filter(move |attr| attr.params().bookmark.matches(bookmark))
}
/// check if provided bookmark is fast-forward only
pub fn is_fast_forward_only(&self, bookmark: &BookmarkName) -> bool {
self.select(bookmark).any(|params| params.only_fast_forward)
self.select(bookmark)
.any(|attr| attr.params().only_fast_forward)
}
/// Check if a bookmark config overrides whether date should be rewritten during pushrebase.
/// Return None if there are no bookmark config overriding rewrite_dates.
pub fn should_rewrite_dates(&self, bookmark: &BookmarkName) -> Option<bool> {
for params in self.select(bookmark) {
for attr in self.select(bookmark) {
// NOTE: If there are multiple patterns matching the bookmark, the first match
// overrides others. It might not be the most desired behavior, though.
if let Some(rewrite_dates) = params.rewrite_dates {
if let Some(rewrite_dates) = attr.params().rewrite_dates {
return Some(rewrite_dates);
}
}
@ -405,15 +446,42 @@ impl BookmarkAttrs {
}
/// check if provided unix name is allowed to move specified bookmark
pub fn is_allowed_user(&self, user: &Option<String>, bookmark: &BookmarkName) -> bool {
pub async fn is_allowed_user(
&self,
user: &Option<String>,
metadata: &Metadata,
bookmark: &BookmarkName,
) -> Result<bool, Error> {
match user {
None => true,
None => Ok(true),
Some(user) => {
// NOTE: `Iterator::all` combinator returns `true` if selected set is empty
// which is consistent with what we want
self.select(bookmark)
.flat_map(|params| &params.allowed_users)
.all(|re| re.is_match(user))
for attr in self.select(bookmark) {
let maybe_allowed_users = attr
.params()
.allowed_users
.as_ref()
.map(|re| re.is_match(user));
let maybe_member = if let Some(membership) = &attr.membership {
Some(membership.is_member(&metadata.identities()).await?)
} else {
None
};
// Check if either is user is allowed to access it
// or that they are a member of hipster group.
let allowed = match (maybe_allowed_users, maybe_member) {
(Some(x), Some(y)) => x || y,
(Some(x), None) | (None, Some(x)) => x,
(None, None) => true,
};
if !allowed {
return Ok(false);
}
}
Ok(true)
}
}
}
@ -421,7 +489,7 @@ impl BookmarkAttrs {
/// Check if a bookmark is only updatable by an external sync process.
pub fn is_only_external_sync_allowed(&self, bookmark: &BookmarkName) -> bool {
self.select(bookmark)
.any(|params| params.allow_only_external_sync.unwrap_or(false))
.any(|attr| attr.params().allow_only_external_sync.unwrap_or(false))
}
}
@ -436,8 +504,12 @@ pub struct BookmarkParams {
pub only_fast_forward: bool,
/// Whether to rewrite dates for pushrebased commits or not
pub rewrite_dates: Option<bool>,
/// Only users matching this pattern will be allowed to move this bookmark
/// Only users matching this pattern or hipster group will be allowed to
/// move this bookmark
pub allowed_users: Option<ComparableRegex>,
/// Only users matching this pattern or hipster group will be allowed to
/// move this bookmark
pub allowed_hipster_group: Option<String>,
/// Only the external sync job is allowed to modify this bookmark
pub allow_only_external_sync: Option<bool>,
/// Skip hooks for changesets that are already ancestors of these

View File

@ -30,7 +30,8 @@ impl RepoWriteContext {
self.check_method_permitted("create_bookmark")?;
let bookmark = BookmarkName::new(bookmark)?;
let bookmark_attrs = BookmarkAttrs::new(self.config().bookmarks.clone());
let bookmark_attrs =
BookmarkAttrs::new(self.ctx().fb, self.config().bookmarks.clone()).await?;
let lca_hint: Arc<dyn LeastCommonAncestorsHint> = self.skiplist_index().clone();

View File

@ -28,7 +28,8 @@ impl RepoWriteContext {
self.check_method_permitted("delete_bookmark")?;
let bookmark = BookmarkName::new(bookmark)?;
let bookmark_attrs = BookmarkAttrs::new(self.config().bookmarks.clone());
let bookmark_attrs =
BookmarkAttrs::new(self.ctx().fb, self.config().bookmarks.clone()).await?;
// We need to find out where the bookmark currently points to in order
// to delete it. Make sure to bypass any out-of-date caches.

View File

@ -38,7 +38,8 @@ impl RepoWriteContext {
self.check_method_permitted("land_stack")?;
let bookmark = BookmarkName::new(bookmark)?;
let bookmark_attrs = BookmarkAttrs::new(self.config().bookmarks.clone());
let bookmark_attrs =
BookmarkAttrs::new(self.ctx().fb, self.config().bookmarks.clone()).await?;
let lca_hint: Arc<dyn LeastCommonAncestorsHint> = self.skiplist_index().clone();

View File

@ -34,7 +34,8 @@ impl RepoWriteContext {
self.check_method_permitted("move_bookmark")?;
let bookmark = BookmarkName::new(bookmark)?;
let bookmark_attrs = BookmarkAttrs::new(self.config().bookmarks.clone());
let bookmark_attrs =
BookmarkAttrs::new(self.ctx().fb, self.config().bookmarks.clone()).await?;
// We need to find out where the bookmark currently points to in order
// to move it. Make sure to bypass any out-of-date caches.

View File

@ -82,4 +82,8 @@ impl MembershipCheckerBuilder {
pub async fn for_admin_group(_fb: FacebookInit) -> Result<BoxMembershipChecker> {
Ok(Self::never_member())
}
pub async fn for_group(fb: FacebookInit, group_name: &str) -> Result<BoxMembershipChecker> {
Ok(Self::never_member())
}
}

View File

@ -173,7 +173,7 @@ impl MononokeRepo {
}
// TODO: Update Metaconfig so we just have this in config:
let bookmark_attrs = BookmarkAttrs::new(repo.config().bookmarks.clone());
let bookmark_attrs = BookmarkAttrs::new(fb, repo.config().bookmarks.clone()).await?;
Ok(Self {
repo,