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

refactor(permissions): split up Descriptor into Allow, Deny, and Query #25508

Merged
merged 41 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
95a96a7
refactor: split up Descriptor into Allow, Deny, and Query
dsherret Sep 6, 2024
e546ad1
more work
dsherret Sep 6, 2024
883e04a
before revert to store more in permissions container
dsherret Sep 7, 2024
ac63de1
update
dsherret Sep 7, 2024
4987a2b
start moving to parser being in permissions container
dsherret Sep 7, 2024
6e33f81
More progress
dsherret Sep 7, 2024
05a9176
more work
dsherret Sep 8, 2024
90c5446
compiling
dsherret Sep 8, 2024
9e5647c
some fixes
dsherret Sep 8, 2024
6f8a564
Add way to prompt for name, but not really
dsherret Sep 8, 2024
5a29cba
match names for deny queries
dsherret Sep 8, 2024
25203ec
fixed more runtime/permissions tests
dsherret Sep 8, 2024
8ca817a
fixes and more tests
dsherret Sep 8, 2024
93107cf
all passing permissions tests
dsherret Sep 9, 2024
dffeb9c
start on clippy
dsherret Sep 9, 2024
bb49285
clippy
dsherret Sep 9, 2024
2673bfe
clippy
dsherret Sep 9, 2024
3c4b237
finish clippy
dsherret Sep 9, 2024
49797fe
update
dsherret Sep 9, 2024
6950392
use allow all enum
dsherret Sep 9, 2024
5b3e8b0
make worker tests work
dsherret Sep 9, 2024
f6d8ff9
Merge branch 'main' into refactor_permissions
dsherret Sep 9, 2024
d0c1504
do not resolve absolute paths when querying with which
dsherret Sep 9, 2024
f52fd78
fix windows issues
dsherret Sep 9, 2024
b22bc8e
clippy
dsherret Sep 9, 2024
b9d1551
fix failing windows test
dsherret Sep 9, 2024
e93276b
Merge branch 'main' into refactor_permissions
dsherret Sep 10, 2024
16d7306
update
dsherret Sep 10, 2024
8253cbb
maybe fix ci
dsherret Sep 10, 2024
48d41d5
Merge branch 'main' into refactor_permissions
dsherret Sep 11, 2024
5aaf4a0
fix
dsherret Sep 11, 2024
f47ae35
Merge branch 'main' into refactor_permissions
dsherret Sep 14, 2024
e075e3a
move structs together
dsherret Sep 14, 2024
c829704
to_permissions_options doesn't need to be a result
dsherret Sep 14, 2024
f6ac563
switch to moving methods to query
dsherret Sep 15, 2024
0fce3bf
update doc string
dsherret Sep 15, 2024
3184127
Only have query trait
dsherret Sep 15, 2024
dc924cd
Merge branch 'main' into refactor_permissions
dsherret Sep 16, 2024
2521bef
Lucas review
dsherret Sep 16, 2024
42f3084
Merge branch 'main' into refactor_permissions
dsherret Sep 16, 2024
9722059
fix lint
dsherret Sep 16, 2024
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
103 changes: 10 additions & 93 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ use deno_core::normalize_path;
use deno_core::resolve_url_or_path;
use deno_core::url::Url;
use deno_graph::GraphKind;
use deno_runtime::colors;
use deno_runtime::deno_permissions::parse_sys_kind;
use deno_runtime::deno_permissions::PermissionsOptions;
use log::debug;
Expand Down Expand Up @@ -661,107 +660,25 @@ impl PermissionFlags {
|| self.deny_write.is_some()
}

pub fn to_options(
&self,
// will be None when `deno compile` can't resolve the cwd
initial_cwd: Option<&Path>,
) -> Result<PermissionsOptions, AnyError> {
fn convert_option_str_to_path_buf(
flag: &Option<Vec<String>>,
initial_cwd: Option<&Path>,
) -> Result<Option<Vec<PathBuf>>, AnyError> {
let Some(paths) = &flag else {
return Ok(None);
};

let mut new_paths = Vec::with_capacity(paths.len());
for path in paths {
if let Some(initial_cwd) = initial_cwd {
new_paths.push(initial_cwd.join(path))
} else {
let path = PathBuf::from(path);
if path.is_absolute() {
new_paths.push(path);
} else {
bail!("Could not resolve relative permission path '{}' when current working directory could not be resolved.", path.display())
}
}
}
Ok(Some(new_paths))
}

fn resolve_allow_run(
allow_run: &[String],
) -> Result<Vec<PathBuf>, AnyError> {
let mut new_allow_run = Vec::with_capacity(allow_run.len());
for command_name in allow_run {
if command_name.is_empty() {
bail!("Empty command name not allowed in --allow-run=...")
}
let command_path_result = which::which(command_name);
match command_path_result {
Ok(command_path) => new_allow_run.push(command_path),
Err(err) => {
log::info!(
"{} Failed to resolve '{}' for allow-run: {}",
colors::gray("Info"),
command_name,
err
);
}
}
}
Ok(new_allow_run)
}

let mut deny_write =
convert_option_str_to_path_buf(&self.deny_write, initial_cwd)?;
let allow_run = self
.allow_run
.as_ref()
.and_then(|raw_allow_run| match resolve_allow_run(raw_allow_run) {
Ok(resolved_allow_run) => {
if resolved_allow_run.is_empty() && !raw_allow_run.is_empty() {
None // convert to no permissions if now empty
} else {
Some(Ok(resolved_allow_run))
}
}
Err(err) => Some(Err(err)),
})
.transpose()?;
// add the allow_run list to deno_write
if let Some(allow_run_vec) = &allow_run {
if !allow_run_vec.is_empty() {
let deno_write = deny_write.get_or_insert_with(Vec::new);
deno_write.extend(allow_run_vec.iter().cloned());
}
}

Ok(PermissionsOptions {
pub fn to_options(&self) -> PermissionsOptions {
PermissionsOptions {
allow_all: self.allow_all,
allow_env: self.allow_env.clone(),
deny_env: self.deny_env.clone(),
allow_net: self.allow_net.clone(),
deny_net: self.deny_net.clone(),
allow_ffi: convert_option_str_to_path_buf(&self.allow_ffi, initial_cwd)?,
deny_ffi: convert_option_str_to_path_buf(&self.deny_ffi, initial_cwd)?,
allow_read: convert_option_str_to_path_buf(
&self.allow_read,
initial_cwd,
)?,
deny_read: convert_option_str_to_path_buf(&self.deny_read, initial_cwd)?,
allow_run,
allow_ffi: self.allow_ffi.clone(),
deny_ffi: self.deny_ffi.clone(),
allow_read: self.allow_read.clone(),
deny_read: self.deny_read.clone(),
allow_run: self.allow_run.clone(),
deny_run: self.deny_run.clone(),
allow_sys: self.allow_sys.clone(),
deny_sys: self.deny_sys.clone(),
allow_write: convert_option_str_to_path_buf(
&self.allow_write,
initial_cwd,
)?,
deny_write,
allow_write: self.allow_write.clone(),
deny_write: self.deny_write.clone(),
prompt: !resolve_no_prompt(self),
})
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions cli/args/import_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use deno_core::error::AnyError;
use deno_core::serde_json;
use deno_core::url::Url;
use deno_runtime::deno_permissions::PermissionsContainer;

use crate::file_fetcher::FileFetcher;

Expand All @@ -17,7 +16,7 @@ pub async fn resolve_import_map_value_from_specifier(
Ok(serde_json::from_str(&data_url_text)?)
} else {
let file = file_fetcher
.fetch(specifier, &PermissionsContainer::allow_all())
.fetch_bypass_permissions(specifier)
.await?
.into_text_decoded()?;
Ok(serde_json::from_str(&file.source)?)
Expand Down
7 changes: 3 additions & 4 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use deno_npm::npm_rc::NpmRc;
use deno_npm::npm_rc::ResolvedNpmRc;
use deno_npm::resolution::ValidSerializedNpmResolutionSnapshot;
use deno_npm::NpmSystemInfo;
use deno_runtime::deno_permissions::PermissionsContainer;
use deno_semver::npm::NpmPackageReqReference;
use import_map::resolve_import_map_value_from_specifier;

Expand Down Expand Up @@ -1082,7 +1081,7 @@ impl CliOptions {
let specifier = specifier.clone();
async move {
let file = file_fetcher
.fetch(&specifier, &PermissionsContainer::allow_all())
.fetch_bypass_permissions(&specifier)
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
.await?
.into_text_decoded()?;
Ok(file.source.to_string())
Expand Down Expand Up @@ -1501,8 +1500,8 @@ impl CliOptions {
&self.flags.permissions
}

pub fn permissions_options(&self) -> Result<PermissionsOptions, AnyError> {
self.flags.permissions.to_options(Some(&self.initial_cwd))
pub fn permissions_options(&self) -> PermissionsOptions {
self.flags.permissions.to_options()
}

pub fn reload_flag(&self) -> bool {
Expand Down
8 changes: 4 additions & 4 deletions cli/cache/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::args::CacheSetting;
use crate::errors::get_error_class_name;
use crate::file_fetcher::FetchNoFollowOptions;
use crate::file_fetcher::FetchOptions;
use crate::file_fetcher::FetchPermissionsOption;
use crate::file_fetcher::FileFetcher;
use crate::file_fetcher::FileOrRedirect;
use crate::npm::CliNpmResolver;
Expand All @@ -18,7 +19,6 @@ use deno_graph::source::CacheInfo;
use deno_graph::source::LoadFuture;
use deno_graph::source::LoadResponse;
use deno_graph::source::Loader;
use deno_runtime::deno_permissions::PermissionsContainer;
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
Expand Down Expand Up @@ -112,7 +112,7 @@ pub struct FetchCacher {
global_http_cache: Arc<GlobalHttpCache>,
npm_resolver: Arc<dyn CliNpmResolver>,
module_info_cache: Arc<ModuleInfoCache>,
permissions: PermissionsContainer,
permissions: FetchPermissionsOption,
cache_info_enabled: bool,
}

Expand All @@ -123,7 +123,7 @@ impl FetchCacher {
global_http_cache: Arc<GlobalHttpCache>,
npm_resolver: Arc<dyn CliNpmResolver>,
module_info_cache: Arc<ModuleInfoCache>,
permissions: PermissionsContainer,
permissions: FetchPermissionsOption,
) -> Self {
Self {
file_fetcher,
Expand Down Expand Up @@ -230,7 +230,7 @@ impl Loader for FetchCacher {
.fetch_no_follow_with_options(FetchNoFollowOptions {
fetch_options: FetchOptions {
specifier: &specifier,
permissions: &permissions,
permissions: permissions.as_ref(),
maybe_accept: None,
maybe_cache_setting: maybe_cache_setting.as_ref(),
},
Expand Down
53 changes: 39 additions & 14 deletions cli/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,13 @@ use deno_core::FeatureChecker;
use deno_runtime::deno_fs;
use deno_runtime::deno_node::DenoFsNodeResolverEnv;
use deno_runtime::deno_node::NodeResolver;
use deno_runtime::deno_permissions::Permissions;
use deno_runtime::deno_permissions::PermissionsContainer;
use deno_runtime::deno_tls::rustls::RootCertStore;
use deno_runtime::deno_tls::RootCertStoreProvider;
use deno_runtime::deno_web::BlobStore;
use deno_runtime::inspector_server::InspectorServer;
use deno_runtime::permissions::RuntimePermissionDescriptorParser;
use log::warn;
use node_resolver::analyze::NodeCodeTranslator;
use once_cell::sync::OnceCell;
Expand Down Expand Up @@ -181,6 +184,7 @@ struct CliFactoryServices {
node_code_translator: Deferred<Arc<CliNodeCodeTranslator>>,
node_resolver: Deferred<Arc<NodeResolver>>,
npm_resolver: Deferred<Arc<dyn CliNpmResolver>>,
permission_desc_parser: Deferred<Arc<RuntimePermissionDescriptorParser>>,
sloppy_imports_resolver: Deferred<Option<Arc<SloppyImportsResolver>>>,
text_only_progress_bar: Deferred<ProgressBar>,
type_checker: Deferred<Arc<TypeChecker>>,
Expand Down Expand Up @@ -708,6 +712,15 @@ impl CliFactory {
.await
}

pub fn permission_desc_parser(
&self,
) -> Result<&Arc<RuntimePermissionDescriptorParser>, AnyError> {
self.services.permission_desc_parser.get_or_try_init(|| {
let fs = self.fs().clone();
Ok(Arc::new(RuntimePermissionDescriptorParser::new(fs)))
})
}

pub fn feature_checker(&self) -> Result<&Arc<FeatureChecker>, AnyError> {
self.services.feature_checker.get_or_try_init(|| {
let cli_options = self.cli_options()?;
Expand Down Expand Up @@ -739,6 +752,17 @@ impl CliFactory {
))
}

pub fn create_permissions_container(
&self,
) -> Result<PermissionsContainer, AnyError> {
let desc_parser = self.permission_desc_parser()?.clone();
let permissions = Permissions::from_options(
desc_parser.as_ref(),
&self.cli_options()?.permissions_options(),
)?;
Ok(PermissionsContainer::new(desc_parser, permissions))
}

pub async fn create_cli_main_worker_factory(
&self,
) -> Result<CliMainWorkerFactory, AnyError> {
Expand All @@ -754,11 +778,17 @@ impl CliFactory {
};

Ok(CliMainWorkerFactory::new(
StorageKeyResolver::from_options(cli_options),
cli_options.sub_command().clone(),
npm_resolver.clone(),
node_resolver.clone(),
self.blob_store().clone(),
if cli_options.code_cache_enabled() {
Some(self.code_cache()?.clone())
} else {
None
},
self.feature_checker()?.clone(),
self.fs().clone(),
maybe_file_watcher_communicator,
self.maybe_inspector_server()?.clone(),
cli_options.maybe_lockfile().cloned(),
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
Box::new(CliModuleLoaderFactory::new(
cli_options,
if cli_options.code_cache_enabled() {
Expand All @@ -779,17 +809,12 @@ impl CliFactory {
self.parsed_source_cache().clone(),
self.resolver().await?.clone(),
)),
node_resolver.clone(),
npm_resolver.clone(),
self.permission_desc_parser()?.clone(),
self.root_cert_store_provider().clone(),
self.fs().clone(),
maybe_file_watcher_communicator,
self.maybe_inspector_server()?.clone(),
cli_options.maybe_lockfile().cloned(),
self.feature_checker()?.clone(),
if cli_options.code_cache_enabled() {
Some(self.code_cache()?.clone())
} else {
None
},
StorageKeyResolver::from_options(cli_options),
cli_options.sub_command().clone(),
self.create_cli_main_worker_options()?,
))
}
Expand Down
Loading