foster/denoland-deno
1
0
Fork 0
mirror of https://github.com/denoland/deno.git synced 2024-11-21 15:04:11 -05:00
Code Issues Packages 1 Wiki Activity

refactor(permissions): remove FromStr implementations, add ::parse methods (#25473)

Browse source 
The `.parse()` calls in permission code are only making it more
confusing, verbosity
is encouraged and welcome in this code even at the cost of not being
concise.

Left a couple TODOs to not use `AnyError`.
This commit is contained in:
Bartek Iwańczuk 2024-09-06 10:28:53 +01:00 committed by GitHub
parent 73ab32c551
commit 5dedb49ac4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 60 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
cli/args/flags_net.rs
View file

@ -50,7 +50,7 @@ pub fn parse(paths: Vec<String>) -> clap::error::Result<Vec<String>> {
out.push(format!("{}:{}", host, port.0)); out.push(format!("{}:{}", host, port.0));
} }
} else { } else {
host_and_port.parse::<NetDescriptor>().map_err(|e| { NetDescriptor::parse(&host_and_port).map_err(|e| {
clap::Error::raw(clap::error::ErrorKind::InvalidValue, format!("{e:?}")) clap::Error::raw(clap::error::ErrorKind::InvalidValue, format!("{e:?}"))
})?; })?;
out.push(host_and_port) out.push(host_and_port)

7
runtime/ops/permissions.rs
View file

@ -1,6 +1,7 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
use ::deno_permissions::parse_sys_kind; use ::deno_permissions::parse_sys_kind;
use ::deno_permissions::NetDescriptor;
use ::deno_permissions::PermissionState; use ::deno_permissions::PermissionState;
use ::deno_permissions::PermissionsContainer; use ::deno_permissions::PermissionsContainer;
use deno_core::error::custom_error; use deno_core::error::custom_error;
@ -63,7 +64,7 @@ pub fn op_query_permission(
"net" => permissions.net.query( "net" => permissions.net.query(
match args.host.as_deref() { match args.host.as_deref() {
None => None, None => None,
Some(h) => Some(h.parse()?), Some(h) => Some(NetDescriptor::parse(h)?),
} }
.as_ref(), .as_ref(),
), ),
@ -97,7 +98,7 @@ pub fn op_revoke_permission(
"net" => permissions.net.revoke( "net" => permissions.net.revoke(
match args.host.as_deref() { match args.host.as_deref() {
None => None, None => None,
Some(h) => Some(h.parse()?), Some(h) => Some(NetDescriptor::parse(h)?),
} }
.as_ref(), .as_ref(),
), ),
@ -131,7 +132,7 @@ pub fn op_request_permission(
"net" => permissions.net.request( "net" => permissions.net.request(
match args.host.as_deref() { match args.host.as_deref() {
None => None, None => None,
Some(h) => Some(h.parse()?), Some(h) => Some(NetDescriptor::parse(h)?),
} }
.as_ref(), .as_ref(),
), ),

3
runtime/permissions/clippy.toml Normal file
View file

@ -0,0 +1,3 @@
disallowed-methods = [
{ path = "std::str::FromStr", reason = "Don't want to have stuff like `'0.0.0.0'.parse().unwrap()`. Instead implement ConcreteType::parse methods." },
]

98
runtime/permissions/lib.rs
View file

@ -29,7 +29,6 @@ use std::net::IpAddr;
use std::net::Ipv6Addr; use std::net::Ipv6Addr;
use std::path::Path; use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
use std::str::FromStr;
use std::string::ToString; use std::string::ToString;
use std::sync::Arc; use std::sync::Arc;
@ -683,10 +682,9 @@ pub enum Host {
Ip(IpAddr), Ip(IpAddr),
} }
impl FromStr for Host { impl Host {
type Err = AnyError; // TODO(bartlomieju): rewrite to not use `AnyError` but a specific error implementations
fn parse(s: &str) -> Result<Self, AnyError> {
fn from_str(s: &str) -> Result<Self, Self::Err> {
if s.starts_with('[') && s.ends_with(']') { if s.starts_with('[') && s.ends_with(']') {
let ip = s[1..s.len() - 1] let ip = s[1..s.len() - 1]
.parse::<Ipv6Addr>() .parse::<Ipv6Addr>()
@ -708,14 +706,23 @@ impl FromStr for Host {
} else { } else {
Cow::Owned(s.to_ascii_lowercase()) Cow::Owned(s.to_ascii_lowercase())
}; };
let fqdn = FQDN::from_str(&lower) let fqdn = {
.with_context(|| format!("invalid host: '{s}'"))?; use std::str::FromStr;
FQDN::from_str(&lower)
.with_context(|| format!("invalid host: '{s}'"))?
};
if fqdn.is_root() { if fqdn.is_root() {
return Err(uri_error(format!("invalid empty host: '{s}'"))); return Err(uri_error(format!("invalid empty host: '{s}'")));
} }
Ok(Host::Fqdn(fqdn)) Ok(Host::Fqdn(fqdn))
} }
} }
#[cfg(test)]
#[track_caller]
fn must_parse(s: &str) -> Self {
Self::parse(s).unwrap()
}
} }
#[derive(Clone, Eq, PartialEq, Hash, Debug)] #[derive(Clone, Eq, PartialEq, Hash, Debug)]
@ -750,10 +757,9 @@ impl Descriptor for NetDescriptor {
} }
} }
impl FromStr for NetDescriptor { // TODO(bartlomieju): rewrite to not use `AnyError` but a specific error implementations
type Err = AnyError; impl NetDescriptor {
pub fn parse(hostname: &str) -> Result<Self, AnyError> {
fn from_str(hostname: &str) -> Result<Self, Self::Err> {
// If this is a IPv6 address enclosed in square brackets, parse it as such. // If this is a IPv6 address enclosed in square brackets, parse it as such.
if hostname.starts_with('[') { if hostname.starts_with('[') {
if let Some((ip, after)) = hostname.split_once(']') { if let Some((ip, after)) = hostname.split_once(']') {
@ -784,7 +790,7 @@ impl FromStr for NetDescriptor {
Some((host, port)) => (host, port), Some((host, port)) => (host, port),
None => (hostname, ""), None => (hostname, ""),
}; };
let host = host.parse::<Host>()?; let host = Host::parse(host)?;
let port = if port.is_empty() { let port = if port.is_empty() {
None None
@ -1222,7 +1228,7 @@ impl UnaryPermission<NetDescriptor> {
let host = url let host = url
.host_str() .host_str()
.ok_or_else(|| type_error(format!("Missing host in url: '{}'", url)))?; .ok_or_else(|| type_error(format!("Missing host in url: '{}'", url)))?;
let host = host.parse::<Host>()?; let host = Host::parse(host)?;
let port = url.port_or_known_default(); let port = url.port_or_known_default();
let descriptor = NetDescriptor(host, port); let descriptor = NetDescriptor(host, port);
self.check_desc(Some(&descriptor), false, api_name, || { self.check_desc(Some(&descriptor), false, api_name, || {
@ -1865,7 +1871,7 @@ impl PermissionsContainer {
host: &(T, Option<u16>), host: &(T, Option<u16>),
api_name: &str, api_name: &str,
) -> Result<(), AnyError> { ) -> Result<(), AnyError> {
let hostname = host.0.as_ref().parse::<Host>()?; let hostname = Host::parse(host.0.as_ref())?;
let descriptor = NetDescriptor(hostname, host.1); let descriptor = NetDescriptor(hostname, host.1);
self.0.lock().net.check(&descriptor, Some(api_name)) self.0.lock().net.check(&descriptor, Some(api_name))
} }
@ -1914,7 +1920,7 @@ fn parse_net_list(
) -> Result<HashSet<NetDescriptor>, AnyError> { ) -> Result<HashSet<NetDescriptor>, AnyError> {
if let Some(v) = list { if let Some(v) = list {
v.iter() v.iter()
.map(|x| NetDescriptor::from_str(x)) .map(|x| NetDescriptor::parse(x))
.collect::<Result<HashSet<NetDescriptor>, AnyError>>() .collect::<Result<HashSet<NetDescriptor>, AnyError>>()
} else { } else {
Ok(HashSet::new()) Ok(HashSet::new())
@ -2480,7 +2486,7 @@ mod tests {
]; ];
for (host, port, is_ok) in domain_tests { for (host, port, is_ok) in domain_tests {
let host = host.parse().unwrap(); let host = Host::parse(host).unwrap();
let descriptor = NetDescriptor(host, Some(port)); let descriptor = NetDescriptor(host, Some(port));
assert_eq!( assert_eq!(
is_ok, is_ok,
@ -2522,7 +2528,7 @@ mod tests {
]; ];
for (host_str, port) in domain_tests { for (host_str, port) in domain_tests {
let host = host_str.parse().unwrap(); let host = Host::parse(host_str).unwrap();
let descriptor = NetDescriptor(host, Some(port)); let descriptor = NetDescriptor(host, Some(port));
assert!( assert!(
perms.net.check(&descriptor, None).is_ok(), perms.net.check(&descriptor, None).is_ok(),
@ -2563,7 +2569,7 @@ mod tests {
]; ];
for (host_str, port) in domain_tests { for (host_str, port) in domain_tests {
let host = host_str.parse().unwrap(); let host = Host::parse(host_str).unwrap();
let descriptor = NetDescriptor(host, Some(port)); let descriptor = NetDescriptor(host, Some(port));
assert!( assert!(
perms.net.check(&descriptor, None).is_err(), perms.net.check(&descriptor, None).is_err(),
@ -2835,14 +2841,14 @@ mod tests {
assert_eq!(perms4.ffi.query(Some(Path::new("/foo/bar"))), PermissionState::Denied); assert_eq!(perms4.ffi.query(Some(Path::new("/foo/bar"))), PermissionState::Denied);
assert_eq!(perms4.ffi.query(Some(Path::new("/bar"))), PermissionState::Granted); assert_eq!(perms4.ffi.query(Some(Path::new("/bar"))), PermissionState::Granted);
assert_eq!(perms1.net.query(None), PermissionState::Granted); assert_eq!(perms1.net.query(None), PermissionState::Granted);
assert_eq!(perms1.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), None))), PermissionState::Granted); assert_eq!(perms1.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), None))), PermissionState::Granted);
assert_eq!(perms2.net.query(None), PermissionState::Prompt); assert_eq!(perms2.net.query(None), PermissionState::Prompt);
assert_eq!(perms2.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)))), PermissionState::Granted); assert_eq!(perms2.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)))), PermissionState::Granted);
assert_eq!(perms3.net.query(None), PermissionState::Prompt); assert_eq!(perms3.net.query(None), PermissionState::Prompt);
assert_eq!(perms3.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)))), PermissionState::Denied); assert_eq!(perms3.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)))), PermissionState::Denied);
assert_eq!(perms4.net.query(None), PermissionState::GrantedPartial); assert_eq!(perms4.net.query(None), PermissionState::GrantedPartial);
assert_eq!(perms4.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)))), PermissionState::Denied); assert_eq!(perms4.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)))), PermissionState::Denied);
assert_eq!(perms4.net.query(Some(&NetDescriptor("192.168.0.1".parse().unwrap(), Some(8000)))), PermissionState::Granted); assert_eq!(perms4.net.query(Some(&NetDescriptor(Host::must_parse("192.168.0.1"), Some(8000)))), PermissionState::Granted);
assert_eq!(perms1.env.query(None), PermissionState::Granted); assert_eq!(perms1.env.query(None), PermissionState::Granted);
assert_eq!(perms1.env.query(Some("HOME")), PermissionState::Granted); assert_eq!(perms1.env.query(Some("HOME")), PermissionState::Granted);
assert_eq!(perms2.env.query(None), PermissionState::Prompt); assert_eq!(perms2.env.query(None), PermissionState::Prompt);
@ -2896,9 +2902,9 @@ mod tests {
prompt_value.set(true); prompt_value.set(true);
assert_eq!(perms.ffi.request(None), PermissionState::Denied); assert_eq!(perms.ffi.request(None), PermissionState::Denied);
prompt_value.set(true); prompt_value.set(true);
assert_eq!(perms.net.request(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), None))), PermissionState::Granted); assert_eq!(perms.net.request(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), None))), PermissionState::Granted);
prompt_value.set(false); prompt_value.set(false);
assert_eq!(perms.net.request(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)))), PermissionState::Granted); assert_eq!(perms.net.request(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)))), PermissionState::Granted);
prompt_value.set(true); prompt_value.set(true);
assert_eq!(perms.env.request(Some("HOME")), PermissionState::Granted); assert_eq!(perms.env.request(Some("HOME")), PermissionState::Granted);
assert_eq!(perms.env.query(None), PermissionState::Prompt); assert_eq!(perms.env.query(None), PermissionState::Prompt);
@ -2967,9 +2973,9 @@ mod tests {
assert_eq!(perms.ffi.revoke(Some(Path::new("/foo/bar"))), PermissionState::Prompt); assert_eq!(perms.ffi.revoke(Some(Path::new("/foo/bar"))), PermissionState::Prompt);
assert_eq!(perms.ffi.query(Some(Path::new("/foo"))), PermissionState::Prompt); assert_eq!(perms.ffi.query(Some(Path::new("/foo"))), PermissionState::Prompt);
assert_eq!(perms.ffi.query(Some(Path::new("/foo/baz"))), PermissionState::Granted); assert_eq!(perms.ffi.query(Some(Path::new("/foo/baz"))), PermissionState::Granted);
assert_eq!(perms.net.revoke(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(9000)))), PermissionState::Prompt); assert_eq!(perms.net.revoke(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(9000)))), PermissionState::Prompt);
assert_eq!(perms.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), None))), PermissionState::Prompt); assert_eq!(perms.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), None))), PermissionState::Prompt);
assert_eq!(perms.net.query(Some(&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)))), PermissionState::Granted); assert_eq!(perms.net.query(Some(&NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)))), PermissionState::Granted);
assert_eq!(perms.env.revoke(Some("HOME")), PermissionState::Prompt); assert_eq!(perms.env.revoke(Some("HOME")), PermissionState::Prompt);
assert_eq!(perms.env.revoke(Some("hostname")), PermissionState::Prompt); assert_eq!(perms.env.revoke(Some("hostname")), PermissionState::Prompt);
assert_eq!(perms.run.revoke(Some("deno")), PermissionState::Prompt); assert_eq!(perms.run.revoke(Some("deno")), PermissionState::Prompt);
@ -3004,7 +3010,7 @@ mod tests {
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)),
None None
) )
.is_ok()); .is_ok());
@ -3012,31 +3018,31 @@ mod tests {
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)),
None None
) )
.is_ok()); .is_ok());
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8001)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8001)),
None None
) )
.is_err()); .is_err());
assert!(perms assert!(perms
.net .net
.check(&NetDescriptor("127.0.0.1".parse().unwrap(), None), None) .check(&NetDescriptor(Host::must_parse("127.0.0.1"), None), None)
.is_err()); .is_err());
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("deno.land".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("deno.land"), Some(8000)),
None None
) )
.is_err()); .is_err());
assert!(perms assert!(perms
.net .net
.check(&NetDescriptor("deno.land".parse().unwrap(), None), None) .check(&NetDescriptor(Host::must_parse("deno.land"), None), None)
.is_err()); .is_err());
#[allow(clippy::disallowed_methods)] #[allow(clippy::disallowed_methods)]
@ -3121,7 +3127,7 @@ mod tests {
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)),
None None
) )
.is_err()); .is_err());
@ -3129,21 +3135,21 @@ mod tests {
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8000)),
None None
) )
.is_err()); .is_err());
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8001)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8001)),
None None
) )
.is_ok()); .is_ok());
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("deno.land".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("deno.land"), Some(8000)),
None None
) )
.is_ok()); .is_ok());
@ -3151,14 +3157,14 @@ mod tests {
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("127.0.0.1".parse().unwrap(), Some(8001)), &NetDescriptor(Host::must_parse("127.0.0.1"), Some(8001)),
None None
) )
.is_ok()); .is_ok());
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("deno.land".parse().unwrap(), Some(8000)), &NetDescriptor(Host::must_parse("deno.land"), Some(8000)),
None None
) )
.is_ok()); .is_ok());
@ -3286,24 +3292,24 @@ mod tests {
perms perms
.net .net
.check( .check(
&NetDescriptor("allowed.domain.".parse().unwrap(), None), &NetDescriptor(Host::must_parse("allowed.domain."), None),
None, None,
) )
.unwrap(); .unwrap();
perms perms
.net .net
.check(&NetDescriptor("1.1.1.1".parse().unwrap(), None), None) .check(&NetDescriptor(Host::must_parse("1.1.1.1"), None), None)
.unwrap(); .unwrap();
assert!(perms assert!(perms
.net .net
.check( .check(
&NetDescriptor("denied.domain.".parse().unwrap(), None), &NetDescriptor(Host::must_parse("denied.domain."), None),
None None
) )
.is_err()); .is_err());
assert!(perms assert!(perms
.net .net
.check(&NetDescriptor("2.2.2.2".parse().unwrap(), None), None) .check(&NetDescriptor(Host::must_parse("2.2.2.2"), None), None)
.is_err()); .is_err());
} }
@ -3599,7 +3605,7 @@ mod tests {
]; ];
for (host_str, expected) in hosts { for (host_str, expected) in hosts {
assert_eq!(host_str.parse::<Host>().ok(), *expected, "{host_str}"); assert_eq!(Host::parse(host_str).ok(), *expected, "{host_str}");
} }
} }
@ -3665,7 +3671,7 @@ mod tests {
]; ];
for (input, expected) in cases { for (input, expected) in cases {
assert_eq!(input.parse::<NetDescriptor>().ok(), *expected, "'{input}'"); assert_eq!(NetDescriptor::parse(input).ok(), *expected, "'{input}'");
} }
} }
} }