Split trust check out when installing multiple tools #30
Conversation
| @@ -74,7 +76,9 @@ impl ToolStorage { | |||
| } | |||
|
|
|||
| pub fn run(&self, id: &ToolId, args: Vec<String>) -> anyhow::Result<i32> { | |||
| self.install_exact(id, TrustMode::Check)?; | |||
| self.trust_check(id.name(), TrustMode::Check)?; | |||
There was a problem hiding this comment.
Is there any times when self.trust_check is called with TrustMode::NoCheck? I wouldn't think so...
There was a problem hiding this comment.
It doesn't seem that way right now, but I'm wondering if the enum is there to make room for more different kinds of trust checks in the future? There's probably a reason why TrustMode was chosen with that name and also as an enum and not a bool. Maybe @LPGhatguy could provide some feedback on this.
There was a problem hiding this comment.
This API uses an enum instead of a bool to make what the function does much more clear. If it were a bool, it'd be really easy to miss some part of Aftman skipping a trust check when that's a very important thing to do.
TrustMode::NoCheck is used when passed from the user:
Line 163 in f69ee9f
I had a branch at some point that refactored all of the trust checking code to better guarantee that a tool is trusted before it can be installed. This branch feels like it's moving slightly in the opposite direction.
I'd be interested to see how we could make install_inexact and install_exact query information to ensure that the thing they're installing has already been marked as trusted. We should not remove the trust checking from those functions, because then they become a security liability. At worst, we could force the user to manually trust tools beforehand by making those functions fail if any tool they encounter is not trusted.
I think this PR should also consider the feature request described in #17 which came from the Rojo VS Code extension.
Maybe we can make the trust check function return a token that indicates that it is safe to install or run a specific tool. We could take the list of tools, try to check each of them, install the ones that passed, and then depending on the flags, return a collection of errors if any of them weren't trusted.
There was a problem hiding this comment.
This is all great feedback, thank you for taking the time to elaborate! It can be a bit tricky to reason about design choices of other projects but I think I understand the way Aftman thinks about trust better now.
So, I'm thinking trust could be split out into something like trust_prompt and trust_check where the former calls the latter, and if we are in an interactive shell, prompts for trust when it does not exist. Install methods would stay untouched. This would decouple user input from the install process in a neater way, which was the main point of the original PR commit, while keeping trust modes intact. It should also align better with #17.
I will close this PR and start off on a new branch since I think this one was a bit misguided from the start.
No description provided.