refactor(runtime/permissions): rewrite permission handling by crowlKats · Pull Request #9367 · denoland/deno

crowlKats · 2021-02-02T09:22:48Z

No description provided.

# Conflicts: # cli/tests/integration_tests.rs

nayeemrmn

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.

runtime/permissions.rs

# Conflicts: # std/fs/empty_dir_test.ts # std/fs/exists_test.ts

crowlKats · 2021-02-02T11:33:29Z

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.

well it creates a new instance of Permissions and merges it with the existing one. name & description are for handling of prompts, logs & errors. so these shouldn't be settable by the user.

runtime/permissions.rs

Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>

nayeemrmn · 2021-02-02T12:00:46Z

cli/tests/059_fs_relative_path_perm.ts.out

@@ -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


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:

Suggested change

[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.

I agree with Nayeem, the message should stay as is and ideally we could conditionally skip the part with flag to address #9002

@bartlomieju is it better now?
Access to read "non-existent" required, run again with the --allow-read flag

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

I agree with Nayeem

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

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

The implied subject is the operation that failed. I think plenty of error messages are structured like that.

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.

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: # runtime/permissions.rs

# 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

bartlomieju

Great work @crowlKats! I have a few small nitpicks regarding the implementation. Before landing I will wait for another review.

bartlomieju · 2021-03-16T14:14:35Z

runtime/permissions.rs

+impl UnaryPermission<NetPermission> {
+  pub fn query<T: AsRef<str>>(
+    &self,
+    host: &Option<&(T, Option<u16>)>,


This type looks funky, why use reference to option and then reference to tuple?

This is how it currently already is implemented. Not sure how simple it would be to change

changed to Option<&(T, Option<u16>)>

runtime/permissions.rs

bartlomieju · 2021-03-16T14:15:41Z

runtime/permissions.rs

    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);


Nitpick: it's quite hard to read so many nested conditionals, maybe you could reorganize the code to return early instead?

bartlomieju · 2021-03-16T14:15:57Z

runtime/permissions.rs

-        self.env = PermissionState::Denied;
+  pub fn revoke<T: AsRef<str>>(
+    &mut self,
+    host: &Option<&(T, Option<u16>)>,


runtime/permissions.rs

runtime/ops/worker_host.rs

ry · 2021-03-16T15:58:07Z

runtime/permissions.rs

+  #[serde(skip)]
+  pub name: &'static str,
+  #[serde(skip)]
+  pub description: &'static str,


Why serde skip? Is the Deserialize trait even used?

because those are just for internal use, for the likes of error messages or prompts
yes, for worker permissions

ry · 2021-03-16T15:59:14Z

Just to be clear - there's no change in behavior beyond some reworking of error messages?

crowlKats · 2021-03-16T16:00:31Z

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

ry

LGTM - nice clean up @crowlKats and sorry for the delay.

crowlKats added 12 commits January 30, 2021 12:33

work

b26d3f0

work

e167abc

work

1f4b383

work

ee7562b

Merge branch 'master' into permission_rewrite

2ba9d1d

fix tests

927ef76

fix tests

7c12e9c

lint

aa5357f

fix tests

7307dd4

fix tests

a6a185e

Merge branch 'master' into permission_rewrite

114f358

# Conflicts: # cli/tests/integration_tests.rs

remove "all" parameter in new unary functions

6ec8c3f

nayeemrmn reviewed Feb 2, 2021

View reviewed changes

runtime/permissions.rs Outdated Show resolved Hide resolved

runtime/permissions.rs Outdated Show resolved Hide resolved

Merge branch 'master' into permission_rewrite

4160181

# Conflicts: # std/fs/empty_dir_test.ts # std/fs/exists_test.ts

nayeemrmn reviewed Feb 2, 2021

View reviewed changes

runtime/permissions.rs Outdated Show resolved Hide resolved

crowlKats and others added 3 commits February 2, 2021 12:44

Apply suggestions from code review

3eb1d23

Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>

fix change to static

ba1153a

rename state to global_state

4805645

nayeemrmn reviewed Feb 2, 2021

View reviewed changes

crowlKats mentioned this pull request Feb 2, 2021

feat(runtime/permissions): prompt fallback #9376

Merged

crowlKats added 3 commits February 2, 2021 20:52

Merge branch 'master' into permission_rewrite

630ec7f

# Conflicts: # runtime/permissions.rs

clean up

a94aa96

clean up

c06a85d

Base automatically changed from master to main February 19, 2021 14:59

crowlKats added 2 commits March 11, 2021 18:09

Merge branch 'master' into permission_rewrite

e7a3a9e

# 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

change error message

292baa5

bartlomieju reviewed Mar 16, 2021

View reviewed changes

ry reviewed Mar 16, 2021

View reviewed changes

runtime/ops/worker_host.rs Outdated Show resolved Hide resolved

fix imports

9098162

ry reviewed Mar 16, 2021

View reviewed changes

crowlKats added 3 commits March 17, 2021 10:44

update error and log messages

4a1e190

change &Option<&T> to Option<&T> for perm functions

b3602d2

fix tests

b2f23ed

ry approved these changes Mar 17, 2021

View reviewed changes

ry merged commit 0e70d9e into denoland:main Mar 17, 2021

crowlKats deleted the permission_rewrite branch March 17, 2021 22:22

crowlKats restored the permission_rewrite branch March 18, 2021 11:26

crowlKats deleted the permission_rewrite branch March 18, 2021 13:45

		@@ -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

Conversation

crowlKats commented Feb 2, 2021

Uh oh!

nayeemrmn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crowlKats commented Feb 2, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crowlKats Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ry commented Mar 16, 2021

Uh oh!

crowlKats commented Mar 16, 2021

Uh oh!

ry left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crowlKats Mar 16, 2021 •

edited

Loading