-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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(runtime/permissions): rewrite permission handling #9367
Conversation
# Conflicts: # cli/tests/integration_tests.rs
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.
How does the name
, description
mechanism work for the [de]serialising in worker_host.rs
? I don't think anything should be #[serde(skip)]
ed here.
# Conflicts: # std/fs/empty_dir_test.ts # std/fs/exists_test.ts
well it creates a new instance of |
Co-authored-by: Nayeem Rahman <[email protected]>
@@ -1,4 +1,4 @@ | |||
[WILDCARD]error: Uncaught PermissionDenied: read access to "non-existent", run again with the --allow-read flag | |||
[WILDCARD]error: Uncaught PermissionDenied: Access to read "non-existent" required, run again with read permission |
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.
The message should contain the flag suggestion. The wording is a bit off for the global case as well. I sense that the capitalisation of the permission name is a problem? How about just:
[WILDCARD]error: Uncaught PermissionDenied: Access to read "non-existent" required, run again with read permission | |
[WILDCARD]error: Uncaught PermissionDenied: Requires read access to "non-existent", run again with --allow-read. |
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.
I agree with Nayeem, the message should stay as is and ideally we could conditionally skip the part with flag to address #9002
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.
@bartlomieju is it better now?
Access to read "non-existent" required, run again with the --allow-read flag
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.
IMO they are still less concise/uniform than the original messages.
Access to read "foo" required
Access to write to "foo" required
🤮Access to network for "foo:80" required
Access to run a subprocess required
vs original:
read access to "foo" required
write access to "foo" required
network access to "foo:80" required
access to run a subprocess required
My suggestion:
Requires read access to "foo"
Requires write access to "foo"
Requires network access to "foo:80"
Requires subprocess access
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.
I agree with Nayeem
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.
Requires read access to "foo"
sounds a bit off to me, as there is no subject for Requires
. I know deno
is implied, but it still sounds weird
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.
Requires read access to "foo"
sounds a bit off to me, as there is no subject forRequires
. I knowdeno
is implied, but it still sounds weird
The implied subject is the operation that failed. I think plenty of error messages are structured like that.
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.
I forgot for a moment that error message in JS would be PermissionDenied: Requires read access to "foo", run again with --allow-read.
, so it would actually work. will rework the messages.
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.
Alright, error messages now are
Requires net access to "google.com", run again with the --allow-net flag
Requires read access to "./test.ts", run again with the --allow-read flag
Requires write access to "./test.ts", run again with the --allow-write flag
Requires env access, run again with the --allow-env flag
Requires run access, run again with the --allow-run flag
Requires plugin access, run again with the --allow-plugin flag
and debug log messages are
Granted net access to "www.google.com"
Granted read access to "./test.ts"
Granted write access to "./test.ts"
Granted env access
Granted run access
Granted plugin access
will this do?
# Conflicts: # cli/tests/059_fs_relative_path_perm.ts.out # cli/tests/error_015_dynamic_import_permissions.out # runtime/ops/fs.rs # runtime/ops/os.rs # runtime/ops/tls.rs # runtime/permissions.rs
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.
Great work @crowlKats! I have a few small nitpicks regarding the implementation. Before landing I will wait for another review.
runtime/permissions.rs
Outdated
impl UnaryPermission<NetPermission> { | ||
pub fn query<T: AsRef<str>>( | ||
&self, | ||
host: &Option<&(T, Option<u16>)>, |
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.
This type looks funky, why use reference to option and then reference to tuple?
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.
This is how it currently already is implemented. Not sure how simple it would be to change
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.
changed to Option<&(T, Option<u16>)>
runtime/permissions.rs
Outdated
if let Some(host) = host { | ||
let state = self.query_net(&Some(host)); | ||
let state = self.query(&Some(host)); | ||
if state == PermissionState::Prompt { | ||
let host_string = format_host(host); | ||
if permission_prompt(&format!( | ||
"Deno requests network access to \"{}\"", | ||
host_string, | ||
)) { | ||
let host = NetPermission::new(host); |
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.
Nitpick: it's quite hard to read so many nested conditionals, maybe you could reorganize the code to return early instead?
runtime/permissions.rs
Outdated
self.env = PermissionState::Denied; | ||
pub fn revoke<T: AsRef<str>>( | ||
&mut self, | ||
host: &Option<&(T, Option<u16>)>, |
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.
Ditto
#[serde(skip)] | ||
pub name: &'static str, | ||
#[serde(skip)] | ||
pub description: &'static str, |
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.
Why serde skip? Is the Deserialize trait even used?
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.
because those are just for internal use, for the likes of error messages or prompts
yes, for worker permissions
Just to be clear - there's no change in behavior beyond some reworking of error messages? |
indeed. still works the same, just used differently. as proof you can see that the tests havent changed |
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.
LGTM - nice clean up @crowlKats and sorry for the delay.
No description provided.