Skip to content

Commit

Permalink
backend: add error variant for access denied, handle when diffing
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
martinvonz committed May 29, 2024
1 parent 3090adf commit ae4f5f2
Show file tree
Hide file tree
Showing 10 changed files with 338 additions and 14 deletions.
1 change: 1 addition & 0 deletions cli/src/config/colors.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
42 changes: 33 additions & 9 deletions cli/src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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()?;
Expand Down Expand Up @@ -967,15 +991,15 @@ pub fn show_diff_stat(
tree_diff: TreeDiffStream,
path_converter: &RepoPathUiConverter,
display_width: usize,
) -> Result<(), DiffRenderError> {
) -> Result<(), CommandError> {
let mut stats: Vec<DiffStat> = vec![];
let mut max_path_width = 0;
let mut max_diffs = 0;

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)?;
Expand All @@ -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()?;

Expand Down
1 change: 1 addition & 0 deletions cli/tests/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
112 changes: 112 additions & 0 deletions cli/tests/test_acls.rs
Original file line number Diff line number Diff line change
@@ -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
}
6 changes: 6 additions & 0 deletions lib/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ pub enum BackendError {
hash: String,
source: Box<dyn std::error::Error + Send + Sync>,
},
#[error("Access denied to read object {hash} of type {object_type}")]
ReadAccessDenied {
object_type: &'static str,
hash: String,
source: Box<dyn std::error::Error + Send + Sync>,
},
#[error("Could not write object of type {object_type}")]
WriteObject {
object_type: &'static str,
Expand Down
2 changes: 2 additions & 0 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/src/local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 9 additions & 0 deletions lib/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
crate::secret_backend::SecretBackend::name(),
Box::new(|settings, store_path| {
Ok(Box::new(crate::secret_backend::SecretBackend::load(
settings, store_path,
)?))
}),
);

// OpStores
factories.add_op_store(
Expand Down
Loading

0 comments on commit ae4f5f2

Please sign in to comment.