From 9741dccb80d1efeee2c74e91334fc0d90b192138 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 19 Mar 2024 10:56:31 -0700 Subject: [PATCH] backend: add error variant for access denied, handle when diffing Some backends, like the one we have at Google, can restrict access to certain files. For such files, if they return a regular `BackendError::ReadObject`, then that will terminate iteration in many cases (e.g. when diffing or listing files). This patch adds a new error variant for them to return instead, plus handling of such errors in diff output. In order to test the feature, I added a new commit backend that returns the new `ReadAccessDenied` error when the caller tries to read certain objects. --- cli/src/config/colors.toml | 1 + cli/src/diff_util.rs | 42 +++++++-- cli/tests/runner.rs | 1 + cli/tests/test_acls.rs | 112 ++++++++++++++++++++++++ lib/src/backend.rs | 6 ++ lib/src/lib.rs | 2 + lib/src/local_working_copy.rs | 2 +- lib/src/repo.rs | 11 ++- lib/src/secret_backend.rs | 159 ++++++++++++++++++++++++++++++++++ lib/src/store.rs | 18 +++- 10 files changed, 339 insertions(+), 15 deletions(-) create mode 100644 cli/tests/test_acls.rs create mode 100644 lib/src/secret_backend.rs diff --git a/cli/src/config/colors.toml b/cli/src/config/colors.toml index 4d95523c3e..286f7fad4b 100644 --- a/cli/src/config/colors.toml +++ b/cli/src/config/colors.toml @@ -81,6 +81,7 @@ "diff removed" = "red" "diff added" = "green" "diff modified" = "cyan" +"diff access-denied" = { bg = "red" } "op_log id" = "blue" "op_log user" = "yellow" diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 22febd37fa..a1887210fb 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -38,6 +38,7 @@ use thiserror::Error; use tracing::instrument; use unicode_width::UnicodeWidthStr as _; +use crate::command_error::{user_error_with_hint, CommandError}; use crate::config::CommandNameAndArgs; use crate::formatter::Formatter; use crate::merge_tools::{self, DiffGenerateError, ExternalMergeTool}; @@ -227,7 +228,7 @@ impl<'a> DiffRenderer<'a> { from_tree: &MergedTree, to_tree: &MergedTree, matcher: &dyn Matcher, - ) -> Result<(), DiffRenderError> { + ) -> Result<(), CommandError> { let repo = self.repo; let path_converter = self.path_converter; for format in &self.formats { @@ -277,7 +278,7 @@ impl<'a> DiffRenderer<'a> { formatter: &mut dyn Formatter, commit: &Commit, matcher: &dyn Matcher, - ) -> Result<(), DiffRenderError> { + ) -> Result<(), CommandError> { let from_tree = commit.parent_tree(self.repo)?; let to_tree = commit.tree()?; self.show_diff(ui, formatter, &from_tree, &to_tree, matcher) @@ -495,7 +496,20 @@ pub fn show_color_words_diff( async { while let Some((path, diff)) = diff_stream.next().await { let ui_path = path_converter.format_file_path(&path); - let (left_value, right_value) = diff?; + let (left_value, right_value) = match diff { + Err(jj_lib::backend::BackendError::ReadAccessDenied { + object_type: _, + hash: _, + source, + }) => { + writeln!( + formatter.labeled("access-denied"), + "Access denied to {ui_path}: {source}" + )?; + continue; + } + other => other?, + }; if left_value.is_absent() { let description = basic_diff_file_type(&right_value); writeln!( @@ -830,19 +844,29 @@ fn materialized_diff_stream<'a>( .buffered((store.concurrency() / 2).max(1)) } +fn map_access_denied(err: BackendError) -> CommandError { + match err { + BackendError::ReadAccessDenied { .. } => user_error_with_hint( + err, + "The `--color-words` format supports access-restricted paths.", + ), + other => other.into(), + } +} + pub fn show_git_diff( repo: &dyn Repo, formatter: &mut dyn Formatter, num_context_lines: usize, tree_diff: TreeDiffStream, -) -> Result<(), DiffRenderError> { +) -> Result<(), CommandError> { formatter.push_label("diff")?; let mut diff_stream = materialized_diff_stream(repo.store(), tree_diff); async { while let Some((path, diff)) = diff_stream.next().await { let path_string = path.as_internal_file_string(); - let (left_value, right_value) = diff?; + let (left_value, right_value) = diff.map_err(map_access_denied)?; if left_value.is_absent() { let right_part = git_diff_part(&path, right_value)?; formatter.with_label("file_header", |formatter| { @@ -895,7 +919,7 @@ pub fn show_git_diff( show_unified_diff_hunks(formatter, &left_part.content, &[], num_context_lines)?; } } - Ok::<(), DiffRenderError>(()) + Ok::<(), CommandError>(()) } .block_on()?; formatter.pop_label()?; @@ -967,7 +991,7 @@ pub fn show_diff_stat( tree_diff: TreeDiffStream, path_converter: &RepoPathUiConverter, display_width: usize, -) -> Result<(), DiffRenderError> { +) -> Result<(), CommandError> { let mut stats: Vec = vec![]; let mut max_path_width = 0; let mut max_diffs = 0; @@ -975,7 +999,7 @@ pub fn show_diff_stat( let mut diff_stream = materialized_diff_stream(repo.store(), tree_diff); async { while let Some((repo_path, diff)) = diff_stream.next().await { - let (left, right) = diff?; + let (left, right) = diff.map_err(map_access_denied)?; let path = path_converter.format_file_path(&repo_path); let left_content = diff_content(&repo_path, left)?; let right_content = diff_content(&repo_path, right)?; @@ -984,7 +1008,7 @@ pub fn show_diff_stat( max_diffs = max(max_diffs, stat.added + stat.removed); stats.push(stat); } - Ok::<(), DiffRenderError>(()) + Ok::<(), CommandError>(()) } .block_on()?; diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index 6387d1f4df..b0a207254d 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -9,6 +9,7 @@ fn test_no_forgotten_test_files() { } mod test_abandon_command; +mod test_acls; mod test_advance_branches; mod test_alias; mod test_branch_command; diff --git a/cli/tests/test_acls.rs b/cli/tests/test_acls.rs new file mode 100644 index 0000000000..9b1acf6008 --- /dev/null +++ b/cli/tests/test_acls.rs @@ -0,0 +1,112 @@ +// Copyright 2024 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::path::Path; + +use crate::common::{get_stderr_string, get_stdout_string, TestEnvironment}; + +fn switch_to_secret_backend(repo_path: &Path) { + std::fs::write( + repo_path + .join(".jj") + .join("repo") + .join("store") + .join("type"), + "secret", + ) + .unwrap(); +} + +#[test] +fn test_diff() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::create_dir(repo_path.join("dir")).unwrap(); + std::fs::write(repo_path.join("a-first"), "foo\n").unwrap(); + std::fs::write(repo_path.join("deleted-secret"), "foo\n").unwrap(); + std::fs::write(repo_path.join("dir").join("secret"), "foo\n").unwrap(); + std::fs::write(repo_path.join("modified-secret"), "foo\n").unwrap(); + std::fs::write(repo_path.join("z-last"), "foo\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("a-first"), "bar\n").unwrap(); + std::fs::remove_file(repo_path.join("deleted-secret")).unwrap(); + std::fs::write(repo_path.join("added-secret"), "bar\n").unwrap(); + std::fs::write(repo_path.join("dir").join("secret"), "bar\n").unwrap(); + std::fs::write(repo_path.join("modified-secret"), "bar\n").unwrap(); + std::fs::write(repo_path.join("z-last"), "bar\n").unwrap(); + + switch_to_secret_backend(&repo_path); + + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--color-words"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" + Modified regular file a-first: + 1 1: foobar + Access denied to added-secret: No access + Access denied to deleted-secret: No access + Access denied to dir/secret: No access + Access denied to modified-secret: No access + Modified regular file z-last: + 1 1: foobar + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--summary"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" + M a-first + A added-secret + D deleted-secret + M dir/secret + M modified-secret + M z-last + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--types"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" + FF a-first + -F added-secret + F- deleted-secret + FF dir/secret + FF modified-secret + FF z-last + "###); + let assert = test_env + .jj_cmd(&repo_path, &["diff", "--stat"]) + .assert() + .failure(); + insta::assert_snapshot!(get_stdout_string(&assert).replace('\\', "/"), @""); + insta::assert_snapshot!(get_stderr_string(&assert), @r###" + Error: Access denied to read object 5716ca5987cbf97d6bb54920bea6adde242d87e6 of type file + Caused by: No access + Hint: The `--color-words` format supports access-restricted paths. + "###); + let assert = test_env + .jj_cmd(&repo_path, &["diff", "--git"]) + .assert() + .failure(); + insta::assert_snapshot!(get_stdout_string(&assert).replace('\\', "/"), @r###" + diff --git a/a-first b/a-first + index 257cc5642c...5716ca5987 100644 + --- a/a-first + +++ b/a-first + @@ -1,1 +1,1 @@ + -foo + +bar + "###); + insta::assert_snapshot!(get_stderr_string(&assert), @r###" + Error: Access denied to read object 5716ca5987cbf97d6bb54920bea6adde242d87e6 of type file + Caused by: No access + Hint: The `--color-words` format supports access-restricted paths. + "###); + + // TODO: Test external tool +} diff --git a/lib/src/backend.rs b/lib/src/backend.rs index 1b88e9a60f..5b532037c2 100644 --- a/lib/src/backend.rs +++ b/lib/src/backend.rs @@ -193,6 +193,12 @@ pub enum BackendError { hash: String, source: Box, }, + #[error("Access denied to read object {hash} of type {object_type}")] + ReadAccessDenied { + object_type: &'static str, + hash: String, + source: Box, + }, #[error("Could not write object of type {object_type}")] WriteObject { object_type: &'static str, diff --git a/lib/src/lib.rs b/lib/src/lib.rs index e96d1d1788..1d01a2bf21 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -71,6 +71,8 @@ pub mod revset; pub mod revset_graph; mod revset_parser; pub mod rewrite; +#[cfg(feature = "testing")] +pub mod secret_backend; pub mod settings; pub mod signing; pub mod simple_op_heads_store; diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index d585c60416..8642086cbe 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -835,7 +835,7 @@ impl TreeState { let tree = self.current_tree().unwrap(); let tree_paths: HashSet<_> = tree .entries_matching(sparse_matcher.as_ref()) - .map(|(path, _)| path) + .filter_map(|(path, result)| result.is_ok().then_some(path)) .collect(); let file_states = self.file_states.all(); let state_paths: HashSet<_> = file_states.paths().map(|path| path.to_owned()).collect(); diff --git a/lib/src/repo.rs b/lib/src/repo.rs index e77b5464fb..d6b687d714 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -60,7 +60,7 @@ use crate::store::Store; use crate::submodule_store::SubmoduleStore; use crate::transaction::Transaction; use crate::view::View; -use crate::{backend, dag_walk, op_store, revset}; +use crate::{backend, dag_walk, op_store, revset, secret_backend}; pub trait Repo { fn store(&self) -> &Arc; @@ -383,6 +383,15 @@ impl Default for StoreFactories { GitBackend::name(), Box::new(|settings, store_path| Ok(Box::new(GitBackend::load(settings, store_path)?))), ); + #[cfg(feature = "testing")] + factories.add_backend( + secret_backend::SecretBackend::name(), + Box::new(|settings, store_path| { + Ok(Box::new(secret_backend::SecretBackend::load( + settings, store_path, + )?)) + }), + ); // OpStores factories.add_op_store( diff --git a/lib/src/secret_backend.rs b/lib/src/secret_backend.rs new file mode 100644 index 0000000000..d3262e9ffc --- /dev/null +++ b/lib/src/secret_backend.rs @@ -0,0 +1,159 @@ +// Copyright 2024 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Provides a backend for testing ACLs + +use std::any::Any; +use std::io::Read; +use std::path::Path; +use std::time::SystemTime; + +use async_trait::async_trait; + +use crate::backend::{ + Backend, BackendError, BackendLoadError, BackendResult, ChangeId, Commit, CommitId, Conflict, + ConflictId, FileId, SigningFn, SymlinkId, Tree, TreeId, +}; +use crate::git_backend::GitBackend; +use crate::index::Index; +use crate::object_id::ObjectId; +use crate::repo_path::RepoPath; +use crate::settings::UserSettings; + +const SECRET_CONTENTS_HEX: [&str; 2] = [ + "d97c5eada5d8c52079031eef0107a4430a9617c5", // "secret\n" + "536aca34dbae6b2b8af26bebdcba83543c9546f0", // "secret" +]; + +/// A commit backend that's completely compatible with the Git backend, except +/// that it refuses to read files and symlinks with the word "secret" in the +/// path, or "secret" or "secret\n" in the content. +#[derive(Debug)] +pub struct SecretBackend { + inner: GitBackend, +} + +impl SecretBackend { + /// "secret" + pub fn name() -> &'static str { + "secret" + } + + /// Loads the backend from the given path. + pub fn load(settings: &UserSettings, store_path: &Path) -> Result { + let inner = GitBackend::load(settings, store_path)?; + Ok(SecretBackend { inner }) + } +} + +#[async_trait] +impl Backend for SecretBackend { + fn as_any(&self) -> &dyn Any { + self + } + + fn name(&self) -> &str { + SecretBackend::name() + } + + fn commit_id_length(&self) -> usize { + self.inner.commit_id_length() + } + + fn change_id_length(&self) -> usize { + self.inner.change_id_length() + } + + fn root_commit_id(&self) -> &CommitId { + self.inner.root_commit_id() + } + + fn root_change_id(&self) -> &ChangeId { + self.inner.root_change_id() + } + + fn empty_tree_id(&self) -> &TreeId { + self.inner.empty_tree_id() + } + + fn concurrency(&self) -> usize { + 1 + } + + async fn read_file(&self, path: &RepoPath, id: &FileId) -> BackendResult> { + if path.as_internal_file_string().contains("secret") + || SECRET_CONTENTS_HEX.contains(&id.hex().as_ref()) + { + return Err(BackendError::ReadAccessDenied { + object_type: "file", + hash: id.hex(), + source: "No access".into(), + }); + } + self.inner.read_file(path, id).await + } + + fn write_file(&self, path: &RepoPath, contents: &mut dyn Read) -> BackendResult { + self.inner.write_file(path, contents) + } + + async fn read_symlink(&self, path: &RepoPath, id: &SymlinkId) -> BackendResult { + if path.as_internal_file_string().contains("secret") + || SECRET_CONTENTS_HEX.contains(&id.hex().as_ref()) + { + return Err(BackendError::ReadAccessDenied { + object_type: "symlink", + hash: id.hex(), + source: "No access".into(), + }); + } + self.inner.read_symlink(path, id).await + } + + fn write_symlink(&self, path: &RepoPath, target: &str) -> BackendResult { + self.inner.write_symlink(path, target) + } + + async fn read_tree(&self, path: &RepoPath, id: &TreeId) -> BackendResult { + self.inner.read_tree(path, id).await + } + + fn write_tree(&self, path: &RepoPath, contents: &Tree) -> BackendResult { + self.inner.write_tree(path, contents) + } + + fn read_conflict(&self, path: &RepoPath, id: &ConflictId) -> BackendResult { + self.inner.read_conflict(path, id) + } + + fn write_conflict(&self, path: &RepoPath, contents: &Conflict) -> BackendResult { + self.inner.write_conflict(path, contents) + } + + async fn read_commit(&self, id: &CommitId) -> BackendResult { + self.inner.read_commit(id).await + } + + fn write_commit( + &self, + contents: Commit, + sign_with: Option<&mut SigningFn>, + ) -> BackendResult<(CommitId, Commit)> { + self.inner.write_commit(contents, sign_with) + } + + fn gc(&self, index: &dyn Index, keep_newer: SystemTime) -> BackendResult<()> { + self.inner.gc(index, keep_newer) + } +} diff --git a/lib/src/store.rs b/lib/src/store.rs index bdd985f5b8..dc59443337 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -24,8 +24,8 @@ use std::time::SystemTime; use pollster::FutureExt; use crate::backend::{ - self, Backend, BackendResult, ChangeId, CommitId, ConflictId, FileId, MergedTreeId, SigningFn, - SymlinkId, TreeId, + self, Backend, BackendError, BackendResult, ChangeId, CommitId, ConflictId, FileId, + MergedTreeId, SigningFn, SymlinkId, TreeId, }; use crate::commit::Commit; use crate::index::Index; @@ -130,7 +130,12 @@ impl Store { return Ok(data); } } - let commit = self.backend.read_commit(id).await?; + let commit = match self.backend.read_commit(id).await { + Err(BackendError::ReadAccessDenied { .. }) => { + unimplemented!("ReacAccessDenied is not supported for commits"); + } + other => other?, + }; let data = Arc::new(commit); let mut write_locked_cache = self.commit_cache.write().unwrap(); write_locked_cache.insert(id.clone(), data.clone()); @@ -179,7 +184,12 @@ impl Store { return Ok(data); } } - let data = self.backend.read_tree(dir, id).await?; + let data = match self.backend.read_tree(dir, id).await { + Err(BackendError::ReadAccessDenied { .. }) => { + unimplemented!("ReacAccessDenied is not supported for trees"); + } + other => other?, + }; let data = Arc::new(data); let mut write_locked_cache = self.tree_cache.write().unwrap(); write_locked_cache.insert(key, data.clone());