Skip to content
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

Merged
merged 4 commits into from
Jan 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ dialoguer = "0.9.0"
dirs = "3.0.2"
env_logger = "0.9.0"
fs-err = "2.6.0"
itertools = "0.10.5"
log = "0.4.14"
once_cell = "1.9.0"
reqwest = { version = "0.11.4", features = ["blocking"] }
Expand Down
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,15 @@ aftman add rojo-rbx/rojo@6.2.0 rojo6
Usage:

```bash
aftman install [--no-trust-check]
aftman install [--no-trust-check] [--skip-untrusted]
```

Install all tools listed in `aftman.toml` files based on your current directory.

If `--no-trust-check` is given, all tools will be installed, regardless of whether they are known. This should generally only be used in CI environments. To trust a specific tool before running `aftman install`, use `aftman trust <tool>` instead.

If `--skip-untrusted` is given, only already trusted tools will be installed, others will be skipped and not emit any errors.
Copy link
Contributor Author

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.


### `aftman self-install`
Usage:

Expand Down
5 changes: 4 additions & 1 deletion src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ pub struct InstallSubcommand {
/// recommended to only run this on CI machines.
#[clap(long)]
pub no_trust_check: bool,
/// Skip / don't error if a tool was not trusted during install.
#[clap(long)]
pub skip_untrusted: bool,
}

impl InstallSubcommand {
Expand All @@ -165,7 +168,7 @@ impl InstallSubcommand {
TrustMode::Check
};

tools.install_all(trust)
tools.install_all(trust, self.skip_untrusted)
}
}

Expand Down
57 changes: 43 additions & 14 deletions src/tool_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skip_untrusted flag might be better handled by giving back the vec of errors to the caller of install_all here, but it didn't really look great when I tried and would leave some room for not properly handling those errors.

let current_dir = current_dir().context("Failed to get current working directory")?;
let manifests = Manifest::discover(&self.home, &current_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(())
Expand Down Expand Up @@ -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.
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 trust_check from using it incorrectly. I assume this was done for safety reasons but since there are lints for unhandled Result types it should be hard to miss if anything needs to use trust_check in the future.

);
std::process::exit(1);
}
}

Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method essentially does the same thing as trust_check did at the top before, but in the same spirit of TrustMode having a TrustStatus here instead of a bool makes things more explicit and allows for easier expansion of trust behavior in the future.


fn install_executable(&self, id: &ToolId, mut contents: impl Read) -> anyhow::Result<()> {
let output_path = self.exe_path(id);

Expand Down
6 changes: 6 additions & 0 deletions src/trust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ pub enum TrustMode {
NoCheck,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum TrustStatus {
Trusted,
NotTrusted,
}

#[derive(Debug)]
pub struct TrustCache {
pub tools: BTreeSet<ToolName>,
Expand Down