From e5662dc0a39e38c964353b77e83301f3012a890d Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sat, 6 Jul 2024 19:55:39 +0100 Subject: [PATCH] feat: update auth of interal repos when changed in config Closes kemitix/git-next#100 --- .cargo/config.toml | 2 +- Cargo.toml | 2 + crates/config/Cargo.toml | 6 + crates/config/src/forge_alias.rs | 2 +- crates/config/src/git_dir.rs | 20 ++- crates/config/src/lib.rs | 5 + crates/config/src/newtype.rs | 2 - crates/config/src/remote_url.rs | 16 +++ crates/config/src/repo_alias.rs | 2 +- crates/config/src/tests.rs | 2 + crates/config/src/tests/url.rs | 28 ++++ crates/git/Cargo.toml | 1 + crates/git/src/commit.rs | 4 +- crates/git/src/repo_details.rs | 109 ++++++++++++++- crates/git/src/repository/factory.rs | 15 ++- crates/git/src/repository/mod.rs | 17 ++- crates/git/src/repository/open/mod.rs | 4 +- crates/git/src/repository/open/oreal.rs | 19 ++- crates/git/src/repository/open/otest.rs | 4 +- crates/git/src/repository/open/tests/fetch.rs | 3 +- .../open/tests/find_default_remote.rs | 25 ++-- crates/git/src/repository/tests/factory.rs | 124 +++++++++++++++--- crates/git/src/repository/tests/mod.rs | 4 + crates/git/src/repository/tests/validate.rs | 14 +- crates/git/src/tests.rs | 34 ++++- crates/git/src/validation/remotes.rs | 28 ++-- crates/git/src/validation/tests.rs | 20 +-- crates/repo-actor/src/handlers/clone_repo.rs | 3 +- crates/repo-actor/src/messages.rs | 2 +- crates/repo-actor/src/tests/given.rs | 7 +- .../src/tests/handlers/clone_repo.rs | 11 +- .../src/tests/handlers/validate_repo.rs | 1 + crates/repo-actor/src/tests/mod.rs | 2 +- 33 files changed, 424 insertions(+), 114 deletions(-) create mode 100644 crates/config/src/remote_url.rs create mode 100644 crates/config/src/tests/url.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index 947a94e..27ec85c 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -13,5 +13,5 @@ debug = 0 strip = "debuginfo" [env] -RUST_LOG = "hyper=warn,debug" +RUST_LOG = "hyper=warn,git_url_parse=warn,debug" RUSTFLAGS = "--cfg tokio_unstable" diff --git a/Cargo.toml b/Cargo.toml index 3639f50..7a3990f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,6 +61,7 @@ gix = { version = "0.63", features = [ "blocking-http-transport-reqwest-rust-tls", ] } async-trait = "0.1" +git-url-parse = "0.4" # fs/network kxio = { version = "1.2" } @@ -91,6 +92,7 @@ derive_more = { version = "1.0.0-beta.6", features = [ ] } derive-with = "0.5" thiserror = "1.0" +pike = "0.1" # file watcher inotify = "0.10" diff --git a/crates/config/Cargo.toml b/crates/config/Cargo.toml index 387c63a..af274e1 100644 --- a/crates/config/Cargo.toml +++ b/crates/config/Cargo.toml @@ -23,6 +23,10 @@ toml = { workspace = true } # Secrets and Password secrecy = { workspace = true } +# Git +gix = { workspace = true } +git-url-parse = { workspace = true } + # Webhooks ulid = { workspace = true } @@ -30,12 +34,14 @@ ulid = { workspace = true } derive_more = { workspace = true } derive-with = { workspace = true } thiserror = { workspace = true } +pike = { workspace = true } [dev-dependencies] # # Testing assert2 = { workspace = true } rand = { workspace = true } pretty_assertions = { workspace = true } +test-log = { workspace = true } [lints.clippy] nursery = { level = "warn", priority = -1 } diff --git a/crates/config/src/forge_alias.rs b/crates/config/src/forge_alias.rs index cb362ae..a665b6f 100644 --- a/crates/config/src/forge_alias.rs +++ b/crates/config/src/forge_alias.rs @@ -1,4 +1,4 @@ -crate::newtype!(ForgeAlias: String, Hash, derive_more::Display, Default: "The name of a Forge to connect to"); +crate::newtype!(ForgeAlias: String, Hash, PartialOrd, Ord, derive_more::Display, Default: "The name of a Forge to connect to"); impl From<&ForgeAlias> for std::path::PathBuf { fn from(value: &ForgeAlias) -> Self { Self::from(&value.0) diff --git a/crates/config/src/git_dir.rs b/crates/config/src/git_dir.rs index a04b195..91aea9e 100644 --- a/crates/config/src/git_dir.rs +++ b/crates/config/src/git_dir.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use derive_more::Deref; +use pike::pike; /// The path to the directory containing the git repository. #[derive( @@ -24,8 +25,16 @@ impl GitDir { pub const fn pathbuf(&self) -> &PathBuf { &self.pathbuf } - pub const fn storage_path_type(&self) -> &StoragePathType { - &self.storage_path_type + pub const fn storage_path_type(&self) -> StoragePathType { + self.storage_path_type + } + pub fn as_fs(&self) -> kxio::fs::FileSystem { + pike! { + self + |> Self::pathbuf + |> PathBuf::clone + |> kxio::fs::new + } } } impl std::fmt::Display for GitDir { @@ -45,8 +54,13 @@ impl From<&GitDir> for PathBuf { value.to_path_buf() } } +impl From<&GitDir> for kxio::fs::FileSystem { + fn from(gitdir: &GitDir) -> Self { + gitdir.as_fs() + } +} -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum StoragePathType { Internal, External, diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index b972bf4..7f39a32 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -10,6 +10,7 @@ pub mod git_dir; mod host_name; mod newtype; mod registered_webhook; +mod remote_url; mod repo_alias; mod repo_branches; mod repo_config; @@ -33,6 +34,7 @@ pub use git_dir::GitDir; pub use git_dir::StoragePathType; pub use host_name::Hostname; pub use registered_webhook::RegisteredWebhook; +pub use remote_url::RemoteUrl; pub use repo_alias::RepoAlias; pub use repo_branches::RepoBranches; pub use repo_config::RepoConfig; @@ -43,3 +45,6 @@ pub use user::User; pub use webhook::auth::WebhookAuth; pub use webhook::forge_notification::ForgeNotification; pub use webhook::id::WebhookId; + +// re-export +pub use pike::{pike, pike_opt, pike_res}; diff --git a/crates/config/src/newtype.rs b/crates/config/src/newtype.rs index 63f2f01..cacf317 100644 --- a/crates/config/src/newtype.rs +++ b/crates/config/src/newtype.rs @@ -28,8 +28,6 @@ macro_rules! newtype { derive_more::From, PartialEq, Eq, - PartialOrd, - Ord, derive_more::AsRef, derive_more::Deref, $($derive),* diff --git a/crates/config/src/remote_url.rs b/crates/config/src/remote_url.rs new file mode 100644 index 0000000..273e46e --- /dev/null +++ b/crates/config/src/remote_url.rs @@ -0,0 +1,16 @@ +crate::newtype!(RemoteUrl: git_url_parse::GitUrl, derive_more::Display: "The URL of a remote repository"); +impl RemoteUrl { + pub fn parse(url: impl Into) -> Option { + Some(Self::new(::git_url_parse::GitUrl::parse(&url.into()).ok()?)) + } +} +impl TryFrom for RemoteUrl { + type Error = (); + + fn try_from(url: gix::Url) -> Result { + let pass = url.password().map(|p| p.to_owned()); + let mut parsed = ::git_url_parse::GitUrl::parse(&url.to_string()).map_err(|_| ())?; + parsed.token = pass; + Ok(Self(parsed)) + } +} diff --git a/crates/config/src/repo_alias.rs b/crates/config/src/repo_alias.rs index 39f0f0b..bd42984 100644 --- a/crates/config/src/repo_alias.rs +++ b/crates/config/src/repo_alias.rs @@ -2,6 +2,6 @@ use derive_more::Display; use crate::newtype; -newtype!(RepoAlias: String, Display, Default: r#"The alias of a repo. +newtype!(RepoAlias: String, Display, Default, PartialOrd, Ord: r#"The alias of a repo. This is the alias for the repo within `git-next-server.toml`."#); diff --git a/crates/config/src/tests.rs b/crates/config/src/tests.rs index 810025c..16e337e 100644 --- a/crates/config/src/tests.rs +++ b/crates/config/src/tests.rs @@ -12,6 +12,8 @@ use crate::server::ServerStorage; use crate::server::Webhook; use crate::webhook::push::Branch; +mod url; + type Result = core::result::Result>; type TestResult = Result<()>; diff --git a/crates/config/src/tests/url.rs b/crates/config/src/tests/url.rs new file mode 100644 index 0000000..72c6a77 --- /dev/null +++ b/crates/config/src/tests/url.rs @@ -0,0 +1,28 @@ +use super::*; + +#[test_log::test] +fn url_parse_https_url() -> TestResult { + let url = "https://user:pass@git.host/user/repo.git"; + + let result = RemoteUrl::parse(url); + + tracing::debug!(?result); + assert!(result.is_some()); + + Ok(()) +} + +#[test_log::test] +// INFO: internal clones will never use this format +// INFO: only external clones COULD use this +// FIXME: Do we need to 'parse' external remote urls? +fn url_parse_ssh_url() -> TestResult { + let url = "git@git.host:user/repo.git"; + + let result = RemoteUrl::parse(url); + + tracing::debug!(?result); + assert!(result.is_some()); + + Ok(()) +} diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index 4da1fea..c63a6e1 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -58,6 +58,7 @@ mockall = { workspace = true } assert2 = { workspace = true } rand = { workspace = true } pretty_assertions = { workspace = true } +test-log = { workspace = true } [lints.clippy] diff --git a/crates/git/src/commit.rs b/crates/git/src/commit.rs index 3bbcc1c..045d223 100644 --- a/crates/git/src/commit.rs +++ b/crates/git/src/commit.rs @@ -37,8 +37,8 @@ impl From for Commit { } } -newtype!(Sha: String, Display, Hash: "The unique SHA for a git commit."); -newtype!(Message: String, Hash: "The commit message for a git commit."); +newtype!(Sha: String, Display, Hash,PartialOrd, Ord: "The unique SHA for a git commit."); +newtype!(Message: String, Hash, PartialOrd, Ord: "The commit message for a git commit."); #[derive(Clone, Debug)] pub struct Histories { diff --git a/crates/git/src/repo_details.rs b/crates/git/src/repo_details.rs index 73d982b..b8f1ea3 100644 --- a/crates/git/src/repo_details.rs +++ b/crates/git/src/repo_details.rs @@ -1,8 +1,13 @@ +use std::sync::{Arc, RwLock}; + use config::{ BranchName, ForgeAlias, ForgeConfig, ForgeDetails, GitDir, RepoAlias, RepoConfig, RepoPath, - ServerRepoConfig, + ServerRepoConfig, StoragePathType, }; -use git_next_config as config; +use git_next_config::{self as config, pike, RemoteUrl}; +use secrecy::Secret; + +use crate::repository::{OpenRepositoryLike, RealOpenRepository}; use super::{Generation, GitRemote}; @@ -55,6 +60,10 @@ impl RepoDetails { origin.into() } + pub const fn gitdir(&self) -> &GitDir { + &self.gitdir + } + pub fn git_remote(&self) -> GitRemote { GitRemote::new(self.forge.hostname().clone(), self.repo_path.clone()) } @@ -64,4 +73,100 @@ impl RepoDetails { self.forge = forge.with_hostname(hostname); self } + + // url is a secret as it contains auth token + pub fn url(&self) -> Secret { + let user = self.forge.user(); + use secrecy::ExposeSecret; + let token = self.forge.token().expose_secret(); + let hostname = self.forge.hostname(); + let repo_path = &self.repo_path; + format!("https://{user}:{token}@{hostname}/{repo_path}.git").into() + } + + #[allow(clippy::result_large_err)] + pub fn open(&self) -> Result { + let gix_repo = pike! { + self + |> Self::gitdir + |> GitDir::pathbuf + |> gix::ThreadSafeRepository::open + }?; + let repo = pike! { + gix_repo + |> RwLock::new + |> Arc::new + |> RealOpenRepository::new + }; + Ok(repo) + } + + pub fn remote_url(&self) -> Option { + use secrecy::ExposeSecret; + RemoteUrl::parse(self.url().expose_secret()) + } + + #[tracing::instrument] + pub fn assert_remote_url(&self, found: Option) -> crate::repository::Result<()> { + let Some(found) = found else { + tracing::debug!("No remote url found to assert"); + return Ok(()); + }; + let Some(expected) = self.remote_url() else { + tracing::debug!("No remote url to assert against"); + return Ok(()); + }; + if found != expected { + tracing::debug!(?found, ?expected, "urls differ"); + match self.gitdir.storage_path_type() { + StoragePathType::External => { + tracing::debug!("Refusing to update an external repo - user must resolve this"); + return Err(crate::repository::Error::MismatchDefaultFetchRemote { + found: Box::new(found), + expected: Box::new(expected), + }); + } + StoragePathType::Internal => { + tracing::debug!(?expected, "Need to update config with new url"); + self.write_remote_url(&expected)?; + } + } + } + Ok(()) + } + + #[tracing::instrument] + pub fn write_remote_url(&self, url: &RemoteUrl) -> Result<(), kxio::fs::Error> { + if self.gitdir.storage_path_type() != StoragePathType::Internal { + // return Err(Not an internal repo) + } + let fs = self.gitdir.as_fs(); + // load config file + let config_filename = &self.gitdir.join("config"); + let config_file = fs.file_read_to_string(config_filename)?; + let mut config_lines = config_file + .lines() + .map(|l| l.to_owned()) + .collect::>(); + tracing::debug!(?config_lines, "original file"); + let url_line = format!(r#" url = "{}""#, url); + if config_lines + .iter() + .any(|line| *line == r#"[remote "origin"]"#) + { + tracing::debug!("has an 'origin' remote"); + config_lines + .iter_mut() + .filter(|line| line.starts_with(r#" url = "#)) + .for_each(|line| line.clone_from(&url_line)); + } else { + tracing::debug!("has no 'origin' remote"); + config_lines.push(r#"[remote "origin"]"#.to_owned()); + config_lines.push(url_line); + } + tracing::debug!(?config_lines, "updated file"); + // write config file back out + fs.file_write(config_filename, config_lines.join("\n").as_str())?; + Ok(()) + } } diff --git a/crates/git/src/repository/factory.rs b/crates/git/src/repository/factory.rs index cb9f48a..e983aae 100644 --- a/crates/git/src/repository/factory.rs +++ b/crates/git/src/repository/factory.rs @@ -1,15 +1,14 @@ use super::RepoDetails; use super::Result; pub use crate::repository::open::OpenRepositoryLike; -use crate::repository::RealOpenRepository; +use crate::repository::{Direction, RealOpenRepository}; use derive_more::Deref as _; -use git_next_config::GitDir; use std::sync::{atomic::AtomicBool, Arc, RwLock}; #[mockall::automock] pub trait RepositoryFactory: std::fmt::Debug + Sync + Send { fn duplicate(&self) -> Box; - fn open(&self, gitdir: &GitDir) -> Result>; + fn open(&self, repo_details: &RepoDetails) -> Result>; fn git_clone(&self, repo_details: &RepoDetails) -> Result>; } @@ -26,9 +25,13 @@ struct RealRepositoryFactory; #[cfg(not(tarpaulin_include))] // requires network access to either clone new and/or fetch. impl RepositoryFactory for RealRepositoryFactory { - fn open(&self, gitdir: &GitDir) -> Result> { - let gix_repo = gix::ThreadSafeRepository::open(gitdir.to_path_buf())?.to_thread_local(); - let repo = RealOpenRepository::new(Arc::new(RwLock::new(gix_repo.into()))); + fn open(&self, repo_details: &RepoDetails) -> Result> { + let repo = repo_details + .open() + .map_err(|e| crate::repository::Error::Open(e.to_string()))?; + let found = repo.find_default_remote(Direction::Fetch); + repo_details.assert_remote_url(found)?; + Ok(Box::new(repo)) } diff --git a/crates/git/src/repository/mod.rs b/crates/git/src/repository/mod.rs index 6b2a285..7a1ba7f 100644 --- a/crates/git/src/repository/mod.rs +++ b/crates/git/src/repository/mod.rs @@ -3,8 +3,7 @@ use super::RepoDetails; use crate::repository::test::TestRepository; use crate::validation::remotes::validate_default_remotes; pub use factory::RepositoryFactory; -use git_next_config as config; -use git_next_config::GitDir; +use git_next_config::{GitDir, RemoteUrl}; pub use open::otest::OnFetch; pub use open::otest::OnPush; pub use open::OpenRepository; @@ -36,14 +35,13 @@ pub const fn test(fs: kxio::fs::FileSystem) -> TestRepository { pub fn open( repository_factory: &dyn factory::RepositoryFactory, repo_details: &RepoDetails, - gitdir: config::GitDir, ) -> Result> { - let open_repository = if !gitdir.exists() { + let open_repository = if !repo_details.gitdir.exists() { info!("Local copy not found - cloning..."); repository_factory.git_clone(repo_details)? } else { info!("Local copy found - opening..."); - repository_factory.open(&gitdir)? + repository_factory.open(repo_details)? }; info!("Validating..."); validate_default_remotes(&*open_repository, repo_details) @@ -89,6 +87,9 @@ pub enum Error { #[error("invalid git dir: {0}")] InvalidGitDir(git_next_config::GitDir), + #[error("kxiofs: {0}")] + KxioFs(#[from] kxio::fs::Error), + #[error("io: {0}")] Io(std::io::Error), @@ -112,6 +113,12 @@ pub enum Error { #[error("fake repository lock")] FakeLock, + + #[error("MismatchDefaultFetchRemote(found: {found:?}, expected: {expected:?})")] + MismatchDefaultFetchRemote { + found: Box, + expected: Box, + }, } mod gix_errors { diff --git a/crates/git/src/repository/open/mod.rs b/crates/git/src/repository/open/mod.rs index 3b3f978..5e501c3 100644 --- a/crates/git/src/repository/open/mod.rs +++ b/crates/git/src/repository/open/mod.rs @@ -12,7 +12,7 @@ use std::{ use crate as git; use git::repository::Direction; -use git_next_config as config; +use git_next_config::{self as config, RemoteUrl}; pub use oreal::RealOpenRepository; pub use otest::TestOpenRepository; @@ -53,7 +53,7 @@ pub fn test( pub trait OpenRepositoryLike: std::fmt::Debug + Sync { fn duplicate(&self) -> Box; fn remote_branches(&self) -> git::push::Result>; - fn find_default_remote(&self, direction: Direction) -> Option; + fn find_default_remote(&self, direction: Direction) -> Option; fn fetch(&self) -> Result<(), git::fetch::Error>; fn push( &self, diff --git a/crates/git/src/repository/open/oreal.rs b/crates/git/src/repository/open/oreal.rs index 2f9f6f7..c16d252 100644 --- a/crates/git/src/repository/open/oreal.rs +++ b/crates/git/src/repository/open/oreal.rs @@ -2,7 +2,7 @@ use crate::{self as git, repository::OpenRepositoryLike}; use config::BranchName; use derive_more::Constructor; -use git_next_config as config; +use git_next_config::{self as config, RemoteUrl}; use gix::bstr::BStr; use std::{ @@ -36,17 +36,24 @@ impl super::OpenRepositoryLike for RealOpenRepository { })?; Ok(refs) } - fn find_default_remote(&self, direction: git::repository::Direction) -> Option { + + #[tracing::instrument] + fn find_default_remote(&self, direction: git::repository::Direction) -> Option { let Ok(repository) = self.0.read() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure + tracing::debug!("no repository"); return None; }; let thread_local = repository.to_thread_local(); let Some(Ok(remote)) = thread_local.find_default_remote(direction.into()) else { #[cfg(not(tarpaulin_include))] // test is on local repo - should always have remotes + tracing::debug!("no remote"); return None; }; - remote.url(direction.into()).map(Into::into) + remote + .url(direction.into()) + .cloned() + .and_then(|url| RemoteUrl::try_from(url).ok()) } #[tracing::instrument(skip_all)] @@ -216,9 +223,9 @@ fn as_gix_error(branch: BranchName) -> impl FnOnce(String) -> git::commit::log:: |error| git::commit::log::Error::Gix { branch, error } } -impl From<&gix::Url> for git::GitRemote { - fn from(url: &gix::Url) -> Self { - let host = url.host().unwrap_or_default(); +impl From<&RemoteUrl> for git::GitRemote { + fn from(url: &RemoteUrl) -> Self { + let host = url.host.clone().unwrap_or_default(); let path = url.path.to_string(); let path = path.strip_prefix('/').map_or(path.as_str(), |path| path); let path = path.strip_suffix(".git").map_or(path, |path| path); diff --git a/crates/git/src/repository/open/otest.rs b/crates/git/src/repository/open/otest.rs index 9a6f5de..b040c9f 100644 --- a/crates/git/src/repository/open/otest.rs +++ b/crates/git/src/repository/open/otest.rs @@ -1,7 +1,7 @@ // use crate::{self as git, repository::OpenRepositoryLike}; use derive_more::{Constructor, Deref}; -use git_next_config as config; +use git_next_config::{self as config, RemoteUrl}; use std::{ path::Path, @@ -73,7 +73,7 @@ impl git::repository::OpenRepositoryLike for TestOpenRepository { self.real.remote_branches() } - fn find_default_remote(&self, direction: git::repository::Direction) -> Option { + fn find_default_remote(&self, direction: git::repository::Direction) -> Option { self.real.find_default_remote(direction) } diff --git a/crates/git/src/repository/open/tests/fetch.rs b/crates/git/src/repository/open/tests/fetch.rs index 78e96f8..0048c8f 100644 --- a/crates/git/src/repository/open/tests/fetch.rs +++ b/crates/git/src/repository/open/tests/fetch.rs @@ -9,6 +9,7 @@ fn should_fetch_from_repo() { cwd.join("../.."), config::git_dir::StoragePathType::External, ); - let_assert!(Ok(repo) = crate::repository::factory::real().open(&gitdir)); + let repo_details = given::repo_details(&given::a_filesystem()).with_gitdir(gitdir); + let_assert!(Ok(repo) = crate::repository::factory::real().open(&repo_details)); let_assert!(Ok(_) = repo.fetch()); } diff --git a/crates/git/src/repository/open/tests/find_default_remote.rs b/crates/git/src/repository/open/tests/find_default_remote.rs index 49d7fa9..3e19fa2 100644 --- a/crates/git/src/repository/open/tests/find_default_remote.rs +++ b/crates/git/src/repository/open/tests/find_default_remote.rs @@ -1,20 +1,13 @@ use super::*; -#[test] +#[test_log::test] fn should_find_default_push_remote() { - // uses the current repo - let_assert!(Ok(cwd) = std::env::current_dir()); - let gitdir = config::GitDir::new( - cwd.join("../.."), - config::git_dir::StoragePathType::External, - ); // from ./crate/git directory to the project root - let_assert!(Ok(repo) = git::repository::factory::real().open(&gitdir)); - let_assert!(Some(remote) = repo.find_default_remote(crate::repository::Direction::Push)); - assert_eq!( - remote, - git::GitRemote::new( - config::Hostname::new("git.kemitix.net"), - config::RepoPath::new("kemitix/git-next".to_string()) - ) - ) + let fs = given::a_filesystem(); + let gitdir = GitDir::new(fs.base().to_path_buf(), config::StoragePathType::External); + let repo_details = given::repo_details(&given::a_filesystem()).with_gitdir(gitdir); + let url = repo_details.url(); + given::a_bare_repo_with_url(fs.base(), url.expose_secret(), &fs); + let_assert!(Ok(repo) = git::repository::factory::real().open(&repo_details)); + let remote = repo.find_default_remote(crate::repository::Direction::Push); + assert!(remote.is_some()); } diff --git a/crates/git/src/repository/tests/factory.rs b/crates/git/src/repository/tests/factory.rs index 95dab81..76e06cc 100644 --- a/crates/git/src/repository/tests/factory.rs +++ b/crates/git/src/repository/tests/factory.rs @@ -1,4 +1,4 @@ -use git_next_config::GitDir; +use git_next_config::{ApiToken, GitDir, StoragePathType}; use super::*; @@ -7,46 +7,134 @@ use crate::tests::given; // clone - can't test are this required a remote server (git_clone only works with https origins) // open // - outside storage path +// - - auth matches #[test_log::test] -fn open_where_storage_external() -> TestResult { +fn open_where_storage_external_auth_matches() -> TestResult { //given let fs = given::a_filesystem(); - // create a bare repo - let _x = gix::prepare_clone_bare("https://user:auth@git.kemitix.net/user/repo.git", fs.base())?; - let gitdir = GitDir::new( - fs.base().to_path_buf(), - git_next_config::git_dir::Provenance::Internal, - ); + let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::External); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); + use secrecy::ExposeSecret; + let url = repo_details.url(); + let url = url.expose_secret(); + given::a_bare_repo_with_url(fs.base(), url, &fs); let factory = crate::repository::factory::real(); //when - let result = factory.open(&gitdir); + let result = factory.open(&repo_details); //then tracing::debug!(?result, "open"); assert!(result.is_ok()); + // verify origin in .git/config matches url + let config = fs.file_read_to_string(&fs.base().join("config"))?; + tracing::debug!(config=?config.lines().collect::>(), "auth"); + assert!(config.lines().any(|line| line.contains(url))); + Ok(()) +} +// - outside storage path +// - - NEW: auth does not match +// - - - do *NOT* change repo config +#[test_log::test] +fn open_where_storage_external_auth_differs_error() -> TestResult { + //given + let fs = given::a_filesystem(); + let repo_details = given::repo_details(&fs); + use secrecy::ExposeSecret; + let original_url = repo_details.url(); + let original_url = original_url.expose_secret(); + given::a_bare_repo_with_url(fs.base(), original_url, &fs); + let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::External); + let factory = crate::repository::factory::real(); + // create new authentication + let api_token = ApiToken::new(given::a_name().into()); + let forge_details = repo_details.forge.clone().with_token(api_token); + let repo_details = repo_details.with_gitdir(gitdir).with_forge(forge_details); + + //when + let_assert!(Err(err) = factory.open(&repo_details)); + + //then + tracing::debug!(?err, "open"); + assert!(matches!( + err, + git::repository::Error::MismatchDefaultFetchRemote { + found: _, + expected: _ + } + )); + + // verify origin in .git/config is unchanged + let config = fs.file_read_to_string(&fs.base().join("config"))?; + tracing::debug!(config=?config.lines().collect::>(), "auth"); + assert!(config.lines().any(|line| line.contains(original_url))); // the original urk Ok(()) } // - in storage path +// - - auth matches #[test_log::test] -fn open_where_storage_internal() -> TestResult { +fn open_where_storage_internal_auth_matches() -> TestResult { //given let fs = given::a_filesystem(); - // create a bare repo - let _x = gix::prepare_clone_bare("https://user:auth@git.kemitix.net/user/repo.git", fs.base())?; - let gitdir = GitDir::new( - fs.base().to_path_buf(), - git_next_config::git_dir::Provenance::Internal, - ); + let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); + use secrecy::ExposeSecret; + let url = repo_details.url(); + let url = url.expose_secret(); + // create a bare repg with the auth from the forge_config + given::a_bare_repo_with_url(fs.base(), url, &fs); let factory = crate::repository::factory::real(); //when - let result = factory.open(&gitdir); + let result = factory.open(&repo_details); //then tracing::debug!(?result, "open"); assert!(result.is_ok()); + // verify origin in .git/config matches url + let config = fs.file_read_to_string(&fs.base().join("config"))?; + tracing::debug!(config=?config.lines().collect::>(), "auth"); + assert!( + config.lines().any(|line| line.contains(url)), + "Mismatched URL should not be updated" + ); Ok(()) } -// - - auth matches +// - in storage path // - - NEW: auth does not match +// - - - CHANGE repo config to match updated auth +#[test_log::test] +fn open_where_storage_internal_auth_differs_update_config() -> TestResult { + //given + let fs = given::a_filesystem(); + let repo_details = given::repo_details(&fs); + use secrecy::ExposeSecret; + let original_url = repo_details.url(); + let original_url = original_url.expose_secret(); + given::a_bare_repo_with_url(fs.base(), original_url, &fs); + let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let factory = crate::repository::factory::real(); + // create new authentication + let api_token = ApiToken::new(given::a_name().into()); + let forge_details = repo_details.forge.clone().with_token(api_token); + let repo_details = repo_details.with_gitdir(gitdir).with_forge(forge_details); + let updated_url = repo_details.url(); + let updated_url = updated_url.expose_secret().to_lowercase(); + + //when + let result = factory.open(&repo_details); + + //then + assert!(result.is_ok()); + + // verify origin in .git/config is unchanged + let config = fs.file_read_to_string(&fs.base().join("config"))?; + tracing::debug!(config=?config.lines().collect::>(), "auth"); + assert!( + config + .lines() + .any(|line| line.to_lowercase().contains(&updated_url)), + "Updated URL should be written to config file" + ); + Ok(()) +} diff --git a/crates/git/src/repository/tests/mod.rs b/crates/git/src/repository/tests/mod.rs index 100149f..a4dec8b 100644 --- a/crates/git/src/repository/tests/mod.rs +++ b/crates/git/src/repository/tests/mod.rs @@ -1,3 +1,7 @@ use crate as git; +use assert2::let_assert; +mod factory; mod validate; + +type TestResult = Result<(), Box>; diff --git a/crates/git/src/repository/tests/validate.rs b/crates/git/src/repository/tests/validate.rs index 15cb705..5905c22 100644 --- a/crates/git/src/repository/tests/validate.rs +++ b/crates/git/src/repository/tests/validate.rs @@ -10,7 +10,7 @@ fn should_ok_a_valid_repo() { let mut open_repository = git::repository::open::mock(); open_repository .expect_find_default_remote() - .returning(move |_direction| Some(repo_details_mock.git_remote())); + .returning(move |_direction| repo_details_mock.remote_url()); let result = validate_default_remotes(&*open_repository, &repo_details); println!("{result:?}"); @@ -28,7 +28,7 @@ fn should_fail_where_no_default_push_remote() { .expect_find_default_remote() .returning(move |direction| match direction { Direction::Push => None, - Direction::Fetch => Some(repo_details_mock.git_remote()), + Direction::Fetch => repo_details_mock.remote_url(), }); let result = validate_default_remotes(&*open_repository, &repo_details); @@ -46,7 +46,7 @@ fn should_fail_where_no_default_fetch_remote() { open_repository .expect_find_default_remote() .returning(move |direction| match direction { - Direction::Push => Some(repo_details_mock.git_remote()), + Direction::Push => repo_details_mock.remote_url(), Direction::Fetch => None, }); @@ -65,8 +65,8 @@ fn should_fail_where_invalid_default_push_remote() { open_repository .expect_find_default_remote() .returning(move |direction| match direction { - Direction::Push => Some(given::a_git_remote()), - Direction::Fetch => Some(repo_details_mock.git_remote()), + Direction::Push => Some(given::a_remote_url()), + Direction::Fetch => repo_details_mock.remote_url(), }); let result = validate_default_remotes(&*open_repository, &repo_details); @@ -84,8 +84,8 @@ fn should_fail_where_invalid_default_fetch_remote() { open_repository .expect_find_default_remote() .returning(move |direction| match direction { - Direction::Push => Some(repo_details_mock.git_remote()), - Direction::Fetch => Some(given::a_git_remote()), + Direction::Push => repo_details_mock.remote_url(), + Direction::Fetch => Some(given::a_remote_url()), }); let result = validate_default_remotes(&*open_repository, &repo_details); diff --git a/crates/git/src/tests.rs b/crates/git/src/tests.rs index e8ea904..9bbe2f5 100644 --- a/crates/git/src/tests.rs +++ b/crates/git/src/tests.rs @@ -214,7 +214,7 @@ mod repo_details { } } pub mod given { - use std::path::PathBuf; + use std::path::{Path, PathBuf}; use crate::{self as git, repository::open::MockOpenRepositoryLike}; use config::{ @@ -270,9 +270,9 @@ pub mod given { pub fn a_forge_config() -> ForgeConfig { ForgeConfig::new( ForgeType::MockForge, - a_name(), - a_name(), - a_name(), + format!("hostname-{}", a_name()), + format!("user-{}", a_name()), + format!("token-{}", a_name()), Default::default(), // no repos ) } @@ -369,6 +369,32 @@ pub mod given { config::RepoPath::new(given::a_name()), ) } + + #[allow(clippy::unwrap_used)] + pub fn a_bare_repo_with_url(path: &Path, url: &str, fs: &kxio::fs::FileSystem) { + // create a basic bare repo + let repo = gix::prepare_clone_bare(url, fs.base()).unwrap(); + repo.persist(); + // load config file + let config_file = fs.file_read_to_string(&path.join("config")).unwrap(); + // add use are origin url + let mut config_lines = config_file.lines().collect::>(); + config_lines.push(r#"[remote "origin"]"#); + let url_line = format!(r#" url = "{}""#, url); + tracing::info!(?url, %url_line, "writing"); + config_lines.push(&url_line); + // write config file back out + fs.file_write(&path.join("config"), config_lines.join("\n").as_str()) + .unwrap(); + } + + #[allow(clippy::unwrap_used)] + pub fn a_remote_url() -> git_next_config::RemoteUrl { + let hostname = given::a_hostname(); + let owner = given::a_name(); + let repo = given::a_name(); + git_next_config::RemoteUrl::parse(format!("git@{hostname}:{owner}/{repo}.git")).unwrap() + } } pub mod then { use std::path::{Path, PathBuf}; diff --git a/crates/git/src/validation/remotes.rs b/crates/git/src/validation/remotes.rs index b13a71b..fb325dd 100644 --- a/crates/git/src/validation/remotes.rs +++ b/crates/git/src/validation/remotes.rs @@ -1,3 +1,4 @@ +use git_next_config::RemoteUrl; use tracing::info; use crate as git; @@ -13,18 +14,22 @@ pub fn validate_default_remotes( let fetch_remote = open_repository .find_default_remote(git::repository::Direction::Fetch) .ok_or_else(|| Error::NoDefaultFetchRemote)?; - let git_remote = repo_details.git_remote(); - info!(config = %git_remote, push = %push_remote, fetch = %fetch_remote, "Check remotes match"); - if git_remote != push_remote { + let Some(remote_url) = repo_details.remote_url() else { + return Err(git::validation::remotes::Error::UnableToOpenRepo( + "Unable to build forge url".to_string(), + )); + }; + info!(config = %remote_url, push = %push_remote, fetch = %fetch_remote, "Check remotes match"); + if remote_url != push_remote { return Err(Error::MismatchDefaultPushRemote { found: push_remote, - expected: git_remote, + expected: remote_url, }); } - if git_remote != fetch_remote { + if remote_url != fetch_remote { return Err(Error::MismatchDefaultFetchRemote { found: fetch_remote, - expected: git_remote, + expected: remote_url, }); } Ok(()) @@ -53,13 +58,16 @@ pub enum Error { #[error("MismatchDefaultPushRemote(found: {found}, expected: {expected})")] MismatchDefaultPushRemote { - found: git::GitRemote, - expected: git::GitRemote, + found: RemoteUrl, + expected: RemoteUrl, }, #[error("MismatchDefaultFetchRemote(found: {found}, expected: {expected})")] MismatchDefaultFetchRemote { - found: git::GitRemote, - expected: git::GitRemote, + found: RemoteUrl, + expected: RemoteUrl, }, + + #[error("Unable to open repo")] + GixOpen(#[from] gix::open::Error), } diff --git a/crates/git/src/validation/tests.rs b/crates/git/src/validation/tests.rs index 732cb89..d4a236b 100644 --- a/crates/git/src/validation/tests.rs +++ b/crates/git/src/validation/tests.rs @@ -20,6 +20,7 @@ mod repos { fn where_repo_has_no_push_remote() { let_assert!(Ok(fs) = kxio::fs::temp(), "temp fs"); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); let mut mock_open_repository = git::repository::open::mock(); mock_open_repository .expect_find_default_remote() @@ -29,9 +30,8 @@ mod repos { repository_factory .expect_open() .return_once(move |_| Ok(mock_open_repository)); - let repo_details = given::repo_details(&fs); let_assert!( - Ok(open_repository) = repository_factory.open(&gitdir), + Ok(open_repository) = repository_factory.open(&repo_details), "open repo" ); @@ -62,6 +62,7 @@ mod positions { fn where_fetch_fails_should_error() { let fs = given::a_filesystem(); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); let mut mock_open_repository = git::repository::open::mock(); mock_open_repository .expect_fetch() @@ -71,10 +72,9 @@ mod positions { .expect_open() .return_once(move |_| Ok(mock_open_repository)); let_assert!( - Ok(repository) = repository_factory.open(&gitdir), + Ok(repository) = repository_factory.open(&repo_details), "open repo" ); - let repo_details = given::repo_details(&fs); //.with_gitdir(gitdir); let repo_config = given::a_repo_config(); let result = validate_positions(&*repository, &repo_details, repo_config); @@ -91,6 +91,7 @@ mod positions { fn where_main_branch_is_missing_or_commit_log_is_empty_should_error() { let fs = given::a_filesystem(); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); let repo_config = given::a_repo_config(); let main_branch = repo_config.branches().main(); let mut mock_open_repository = git::repository::open::mock(); @@ -112,10 +113,9 @@ mod positions { .expect_open() .return_once(move |_| Ok(mock_open_repository)); let_assert!( - Ok(open_repository) = repository_factory.open(&gitdir), + Ok(open_repository) = repository_factory.open(&repo_details), "open repo" ); - let repo_details = given::repo_details(&fs); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); @@ -130,6 +130,7 @@ mod positions { fn where_next_branch_is_missing_or_commit_log_is_empty_should_error() { let fs = given::a_filesystem(); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); let repo_config = given::a_repo_config(); let next_branch = repo_config.branches().next(); let mut mock_open_repository = git::repository::open::mock(); @@ -151,10 +152,9 @@ mod positions { .expect_open() .return_once(move |_| Ok(mock_open_repository)); let_assert!( - Ok(open_repository) = repository_factory.open(&gitdir), + Ok(open_repository) = repository_factory.open(&repo_details), "open repo" ); - let repo_details = given::repo_details(&fs); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); @@ -169,6 +169,7 @@ mod positions { fn where_dev_branch_is_missing_or_commit_log_is_empty_should_error() { let fs = given::a_filesystem(); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); + let repo_details = given::repo_details(&fs).with_gitdir(gitdir); let repo_config = given::a_repo_config(); let dev_branch = repo_config.branches().dev(); let mut mock_open_repository = git::repository::open::mock(); @@ -190,10 +191,9 @@ mod positions { .expect_open() .return_once(move |_| Ok(mock_open_repository)); let_assert!( - Ok(open_repository) = repository_factory.open(&gitdir), + Ok(open_repository) = repository_factory.open(&repo_details), "open repo" ); - let repo_details = given::repo_details(&fs); let result = validate_positions(&*open_repository, &repo_details, repo_config); println!("{result:?}"); diff --git a/crates/repo-actor/src/handlers/clone_repo.rs b/crates/repo-actor/src/handlers/clone_repo.rs index 6fa143c..7e214be 100644 --- a/crates/repo-actor/src/handlers/clone_repo.rs +++ b/crates/repo-actor/src/handlers/clone_repo.rs @@ -14,8 +14,7 @@ impl Handler for actor::RepoActor { ) -> Self::Result { actor::logger(&self.log, "Handler: CloneRepo: start"); tracing::debug!("Handler: CloneRepo: start"); - let gitdir = self.repo_details.gitdir.clone(); - match git::repository::open(&*self.repository_factory, &self.repo_details, gitdir) { + match git::repository::open(&*self.repository_factory, &self.repo_details) { Ok(repository) => { actor::logger(&self.log, "open okay"); tracing::debug!("open okay"); diff --git a/crates/repo-actor/src/messages.rs b/crates/repo-actor/src/messages.rs index bd24c25..139c335 100644 --- a/crates/repo-actor/src/messages.rs +++ b/crates/repo-actor/src/messages.rs @@ -38,7 +38,7 @@ impl From for WebhookRegistered { } } -newtype!(MessageToken: u32, Copy, Default, Display: r#"An incremental token used to identify the current set of messages. +newtype!(MessageToken: u32, Copy, Default, Display, PartialOrd, Ord: r#"An incremental token used to identify the current set of messages. Primarily used by [ValidateRepo] to reduce duplicate messages. The token is incremented when a new Webhook message is received, marking that message the latest, and causing any previous messages still being processed to be dropped when diff --git a/crates/repo-actor/src/tests/given.rs b/crates/repo-actor/src/tests/given.rs index 583189c..84f0fc3 100644 --- a/crates/repo-actor/src/tests/given.rs +++ b/crates/repo-actor/src/tests/given.rs @@ -1,4 +1,5 @@ use config::git_dir::StoragePathType; +use git_next_config::RemoteUrl; // use super::*; @@ -10,15 +11,15 @@ pub fn has_all_valid_remote_defaults( has_remote_defaults( open_repository, HashMap::from([ - (Direction::Push, Some(repo_details.git_remote())), - (Direction::Fetch, Some(repo_details.git_remote())), + (Direction::Push, repo_details.remote_url()), + (Direction::Fetch, repo_details.remote_url()), ]), ); } pub fn has_remote_defaults( open_repository: &mut MockOpenRepositoryLike, - remotes: HashMap>, + remotes: HashMap>, ) { remotes.into_iter().for_each(|(direction, remote)| { open_repository diff --git a/crates/repo-actor/src/tests/handlers/clone_repo.rs b/crates/repo-actor/src/tests/handlers/clone_repo.rs index 3c1ac02..c953ae6 100644 --- a/crates/repo-actor/src/tests/handlers/clone_repo.rs +++ b/crates/repo-actor/src/tests/handlers/clone_repo.rs @@ -42,13 +42,8 @@ async fn should_clone() -> TestResult { async fn should_open() -> TestResult { //given let fs = given::a_filesystem(); - let (mut open_repository, /* mut */ repo_details) = given::an_open_repository(&fs); - // #[allow(clippy::unwrap_used)] - // let repo_config = repo_details.repo_config.take().unwrap(); - + let (mut open_repository, repo_details) = given::an_open_repository(&fs); given::has_all_valid_remote_defaults(&mut open_repository, &repo_details); - // handles_validate_repo_message(&mut open_repository, repo_config.branches()); - // factory opens a repository let mut repository_factory = MockRepositoryFactory::new(); let opened = Arc::new(RwLock::new(vec![])); @@ -139,7 +134,7 @@ async fn opened_repo_with_no_default_push_should_not_proceed() -> TestResult { &mut open_repository, HashMap::from([ (Direction::Push, None), - (Direction::Fetch, Some(repo_details.git_remote())), + (Direction::Fetch, repo_details.remote_url()), ]), ); @@ -167,7 +162,7 @@ async fn opened_repo_with_no_default_fetch_should_not_proceed() -> TestResult { given::has_remote_defaults( &mut open_repository, HashMap::from([ - (Direction::Push, Some(repo_details.git_remote())), + (Direction::Push, repo_details.remote_url()), (Direction::Fetch, None), ]), ); diff --git a/crates/repo-actor/src/tests/handlers/validate_repo.rs b/crates/repo-actor/src/tests/handlers/validate_repo.rs index 4f3e206..58fe4f2 100644 --- a/crates/repo-actor/src/tests/handlers/validate_repo.rs +++ b/crates/repo-actor/src/tests/handlers/validate_repo.rs @@ -354,6 +354,7 @@ async fn should_reject_message_with_expired_token() -> TestResult { } #[test_log::test(actix::test)] +// NOTE: failed then passed on retry: count = 1 async fn should_send_validate_repo_when_retryable_error() -> TestResult { //given let fs = given::a_filesystem(); diff --git a/crates/repo-actor/src/tests/mod.rs b/crates/repo-actor/src/tests/mod.rs index 6e9de50..d6a003d 100644 --- a/crates/repo-actor/src/tests/mod.rs +++ b/crates/repo-actor/src/tests/mod.rs @@ -14,7 +14,7 @@ use config::{ }; use git::{ repository::{factory::MockRepositoryFactory, open::MockOpenRepositoryLike, Direction}, - Generation, GitRemote, MockForgeLike, RepoDetails, + Generation, MockForgeLike, RepoDetails, }; use git_next_actor_macros::message; use git_next_config as config;