refactor(permissions): add PermissionsContainer struct for internal mutability #17134

bartlomieju · 2022-12-19T23:29:52Z

Turns out we were cloning permissions which after prompting were discarded,
so the state of permissions was never preserved. To handle that we need to store
all permissions behind "Arc<Mutex<>>" (because there are situations where we
need to send them to other thread).

Testing and benching code still uses "Permissions" in most places - it's undesirable
to share the same permission set between various test/bench files - otherwise
granting or revoking permissions in one file would influence behavior of other test
files.

bartlomieju · 2022-12-20T15:07:34Z

cli/cache/mod.rs

+ let permissions = if is_dynamic {
 self.dynamic_permissions.clone()
 } else {
 self.root_permissions.clone()


This is fixing actual fix of the issue linked in this PR - because we were passing a mutable reference to a future the updated state of permissions was never preserved - because we were cloning the struct here and never updating it after future resolves.

ry · 2023-01-04T12:38:53Z

runtime/permissions/mod.rs

@@ -34,6 +36,8 @@ pub use prompter::PromptCallback;
 static DEBUG_LOG_ENABLED: Lazy<bool> =
 Lazy::new(|| log::log_enabled!(log::Level::Debug));

+pub type PermissionsContainer = Arc<Mutex<Permissions>>;


If every usage of Permissions is replaced with PermissionsContainer, then why not just include that in the actual struct. IE

struct Permissions(Arc<Mutex<Inner>>)

Not every usage is replaced with PermissionsContainer - usages in ext/ crates are using Arc<Mutex<P>> which depend on traits from various crates. I tried the approach you suggest but it doesn't worked.

usages in ext/ crates are using Arc<Mutex
> which depend on traits from various crates

what's an example?

Examples:

ext/net/ops.rs

ext/net/ops_tls.rs

ext/fetch/lib.rs

These ops don't have access to Permissions struct (because runtime/ crate depends on ext/net and ext/fetch) and thus to PermissionsContainer. Extension crates define traits for permission checks that have matching signatures that are later implemented in runtime/ for Permissions struct (and thus PermissionsContainer since it derefs to Permissions). This is somewhat optimal solution because it doesn't introduce tight coupling between ext/ crates and runtime/ thanks to Rust's trait system.

I don't get it. Let's take op_net_send_udp for example. Here it gets a NetPermissions object from the state:

deno/ext/net/ops.rs

Lines 166 to 169 in 319f607

s.borrow_mut::<NP>().check_net(

&(&addr.hostname, Some(addr.port)),

"Deno.DatagramConn.send()",

)?;

How is changing Permissions to be struct Permissions(Arc<Mutex<Inner>>) going to affect that?

Yeah, let’s make this a struct that buries this type (could make PermissionsContainer a struct). Lots of noise.

There's still a PermissionsContainer tho?

To reduce scenarios where we acquire a lock multiple times, I believe we still need one type for the unlocked state then one for the locked state.

One thing we could maybe do is hide the mutex within the unlocked state type's interface, but I'm not sure it's worth it.

There's still a PermissionsContainer tho?

Yes, there are still situations where you need to get to the inner structure, eg. in CLI - I'd much rather handle that via Permissions struct than some Inner.

cli/file_fetcher.rs

dsherret · 2023-01-06T21:51:25Z

runtime/permissions/mod.rs

@@ -34,6 +36,8 @@ pub use prompter::PromptCallback;
 static DEBUG_LOG_ENABLED: Lazy<bool> =
 Lazy::new(|| log::log_enabled!(log::Level::Debug));

+pub type PermissionsContainer = Arc<Mutex<Permissions>>;


Yeah, let’s make this a struct that buries this type (could make PermissionsContainer a struct). Lots of noise.

dsherret

LGTM

dsherret · 2023-01-07T16:17:18Z

runtime/ops/os/mod.rs

@@ -87,7 +85,7 @@ fn op_set_env(
 key: String,
 value: String,
 ) -> Result<(), AnyError> {
- state.borrow_mut::<Permissions>().env.check(&key)?;
+ state.borrow_mut::<PermissionsContainer>().check_env(&key)?;


I guess these don't really need to be borrow_mut anymore. I don't think it matters though.

Ah true. I'll change that to regular borrows

Actually I will leave it as is - traits from various ext/ crates require mutable reference (because they don't know that we're storing this behind Arc<Mutex<>> and it feels better to have consistent signature (also suggests that it can be internally mutated).

…utability (#17134) Turns out we were cloning permissions which after prompting were discarded, so the state of permissions was never preserved. To handle that we need to store all permissions behind "Arc<Mutex<>>" (because there are situations where we need to send them to other thread). Testing and benching code still uses "Permissions" in most places - it's undesirable to share the same permission set between various test/bench files - otherwise granting or revoking permissions in one file would influence behavior of other test files.

bartlomieju added 5 commits December 20, 2022 00:28

refactor(permissions): store Permissions in Arc<Mutex<>>

f197bcf

handle missed borrows

a0f4c27

add arc mutex to generics

9097cc2

add arc mutex to generics 2

12f04aa

add arc mutex to generics 3

808d46a

bartlomieju commented Dec 20, 2022

View reviewed changes

add PermissionsContainer helper type

67fa88a

bartlomieju mentioned this pull request Dec 20, 2022

npm: PermissionDenied: Reading package.json is not allowed when using npm:[email protected] #15845

Closed

bartlomieju changed the title ~~[WIP] refactor(permissions): store Permissions in Arc<Mutex<>>~~ [WIP] refactor(permissions): add PermissionsContainer struct for internal mutability Dec 21, 2022

bartlomieju changed the title ~~[WIP] refactor(permissions): add PermissionsContainer struct for internal mutability~~ refactor(permissions): add PermissionsContainer struct for internal mutability Dec 22, 2022

bartlomieju force-pushed the permissions_refactor branch from 6363ee9 to 5d5892c Compare December 22, 2022 04:42

bartlomieju added 2 commits December 22, 2022 05:49

Merge branch 'main' into permissions_refactor

3a36833

test & bench files don't share permissions

eee7b55

bartlomieju force-pushed the permissions_refactor branch from 5d5892c to eee7b55 Compare December 22, 2022 04:49

bartlomieju added 2 commits December 22, 2022 05:53

add regression test

27c2c44

Merge branch 'main' into permissions_refactor

286ee62

bartlomieju mentioned this pull request Dec 28, 2022

fix(npm): allow to read package.json if permissions are granted #17209

Merged

bartlomieju added 2 commits December 29, 2022 10:58

Merge branch 'main' into permissions_refactor

0a98d71

update

b0dfff8

bartlomieju mentioned this pull request Dec 29, 2022

Weird behavior when requesting network permissions before using fetch #17153

Open

ry reviewed Jan 4, 2023

View reviewed changes

dsherret reviewed Jan 6, 2023

View reviewed changes

bartlomieju added 5 commits January 6, 2023 23:11

Merge branch 'main' into permissions_refactor

4c2a7fc

revert ext/ crates

2b1b5cd

wip

1f78f38

runtime

1c54bc2

cli

e0a2b09

bartlomieju force-pushed the permissions_refactor branch from c541b23 to e0a2b09 Compare January 7, 2023 14:18

bartlomieju added 2 commits January 7, 2023 15:23

Merge branch 'main' into permissions_refactor

02b1ba7

smaller diff

cfdabd4

dsherret approved these changes Jan 7, 2023

View reviewed changes

dsherret reviewed Jan 7, 2023

View reviewed changes

bartlomieju merged commit fac6447 into denoland:main Jan 7, 2023

bartlomieju deleted the permissions_refactor branch January 7, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(permissions): add PermissionsContainer struct for internal mutability #17134

refactor(permissions): add PermissionsContainer struct for internal mutability #17134

bartlomieju commented Dec 19, 2022 •

edited

Loading

bartlomieju Dec 20, 2022

ry Jan 4, 2023

bartlomieju Jan 4, 2023

ry Jan 4, 2023

bartlomieju Jan 4, 2023

ry Jan 4, 2023

dsherret Jan 6, 2023

bartlomieju Jan 7, 2023

ry Jan 7, 2023

dsherret Jan 7, 2023

bartlomieju Jan 7, 2023

dsherret Jan 6, 2023

dsherret left a comment

dsherret Jan 7, 2023 •

edited

Loading

bartlomieju Jan 7, 2023

bartlomieju Jan 7, 2023

	s.borrow_mut::<NP>().check_net(
	&(&addr.hostname, Some(addr.port)),
	"Deno.DatagramConn.send()",
	)?;

refactor(permissions): add PermissionsContainer struct for internal mutability #17134

refactor(permissions): add PermissionsContainer struct for internal mutability #17134

Conversation

bartlomieju commented Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret left a comment

Choose a reason for hiding this comment

dsherret Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Dec 19, 2022 •

edited

Loading

dsherret Jan 7, 2023 •

edited

Loading