-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trust check & installation improvements #38
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ use std::path::{Path, PathBuf}; | |
|
||
use anyhow::{bail, Context}; | ||
use fs_err::File; | ||
use itertools::{Either, Itertools}; | ||
use once_cell::unsync::OnceCell; | ||
|
||
use crate::auth::AuthManifest; | ||
|
@@ -19,7 +20,7 @@ use crate::tool_id::ToolId; | |
use crate::tool_name::ToolName; | ||
use crate::tool_source::{Asset, GitHubSource, Release}; | ||
use crate::tool_spec::ToolSpec; | ||
use crate::trust::{TrustCache, TrustMode}; | ||
use crate::trust::{TrustCache, TrustMode, TrustStatus}; | ||
|
||
pub struct ToolStorage { | ||
pub storage_dir: PathBuf, | ||
|
@@ -135,15 +136,35 @@ impl ToolStorage { | |
} | ||
|
||
/// Install all tools from all reachable manifest files. | ||
pub fn install_all(&self, trust: TrustMode) -> anyhow::Result<()> { | ||
pub fn install_all(&self, trust: TrustMode, skip_untrusted: bool) -> anyhow::Result<()> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
let current_dir = current_dir().context("Failed to get current working directory")?; | ||
let manifests = Manifest::discover(&self.home, ¤t_dir)?; | ||
|
||
for manifest in manifests { | ||
for (alias, tool_id) in manifest.tools { | ||
self.install_exact(&tool_id, trust)?; | ||
self.link(&alias)?; | ||
} | ||
// Installing all tools is split into multiple steps: | ||
// 1. Trust check, which may prompt the user and yield if untrusted | ||
// 2. Installation of trusted tools, which will be in parallel in the future | ||
// 3. Reporting of installation trust errors, unless trust errors are skipped | ||
|
||
let (trusted_tools, trust_errors): (Vec<_>, Vec<_>) = manifests | ||
.iter() | ||
.flat_map(|manifest| &manifest.tools) | ||
.partition_map( | ||
|(alias, tool_id)| match self.trust_check(tool_id.name(), trust) { | ||
Ok(_) => Either::Left((alias, tool_id)), | ||
Err(e) => Either::Right(e), | ||
}, | ||
); | ||
|
||
for (alias, tool_id) in &trusted_tools { | ||
self.install_exact(tool_id, trust)?; | ||
self.link(alias)?; | ||
} | ||
|
||
if !trust_errors.is_empty() && !skip_untrusted { | ||
bail!( | ||
"Installation trust check failed for the following tools:\n{}", | ||
trust_errors.iter().map(|e| format!(" {e}")).join("\n") | ||
) | ||
} | ||
|
||
Ok(()) | ||
|
@@ -320,10 +341,9 @@ impl ToolStorage { | |
} | ||
|
||
fn trust_check(&self, name: &ToolName, mode: TrustMode) -> anyhow::Result<()> { | ||
let trusted = TrustCache::read(&self.home)?; | ||
let is_trusted = trusted.tools.contains(name); | ||
let status = self.trust_status(name)?; | ||
|
||
if !is_trusted { | ||
if status == TrustStatus::NotTrusted { | ||
if mode == TrustMode::Check { | ||
// If the terminal isn't interactive, tell the user that they | ||
// need to open an interactive terminal to trust this tool. | ||
|
@@ -344,11 +364,10 @@ impl ToolStorage { | |
.interact_opt()?; | ||
|
||
if let Some(false) | None = proceed { | ||
eprintln!( | ||
"Skipping installation of {} and exiting with an error.", | ||
name | ||
bail!( | ||
"Tool {name} is not trusted. \ | ||
Run `aftman trust {name}` in your terminal to trust it.", | ||
Comment on lines
+367
to
+369
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we no longer exit the process here because that would make "greedy" installation of as many trusted tools as possible, impossible. We still bail and that will prevent any callers of |
||
); | ||
std::process::exit(1); | ||
} | ||
} | ||
|
||
|
@@ -358,6 +377,16 @@ impl ToolStorage { | |
Ok(()) | ||
} | ||
|
||
fn trust_status(&self, name: &ToolName) -> anyhow::Result<TrustStatus> { | ||
let trusted = TrustCache::read(&self.home)?; | ||
let is_trusted = trusted.tools.contains(name); | ||
if is_trusted { | ||
Ok(TrustStatus::Trusted) | ||
} else { | ||
Ok(TrustStatus::NotTrusted) | ||
} | ||
} | ||
Comment on lines
+380
to
+388
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method essentially does the same thing as |
||
|
||
fn install_executable(&self, id: &ToolId, mut contents: impl Read) -> anyhow::Result<()> { | ||
let output_path = self.exe_path(id); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a way to word this better? I'm not super familiar with what the use case was as the related issue doesn't contain much detail about use cases.