Permissions refactor #1864
Permissions refactor #1864
Conversation
| @@ -54,6 +58,7 @@ impl Worker { | |||
| pub fn spawn( | |||
There was a problem hiding this comment.
Please add some triple slash docs here.
What does it mean when permissions is None? How is that different than having the Permission struct present but with all the fields set to false?
src/isolate.rs
Outdated
| @@ -195,6 +169,7 @@ impl Isolate { | |||
| snapshot: libdeno::deno_buf, | |||
| state: Arc<IsolateState>, | |||
| dispatch: Dispatch, | |||
| permissions: Option<DenoPermissions>, | |||
There was a problem hiding this comment.
Please add some triple slash docs here.
What does it mean when permissions is None? How is that different than having the Permission struct present but with all the fields set to false?
src/isolate.rs
Outdated
| @@ -208,6 +183,8 @@ impl Isolate { | |||
| let libdeno_isolate = unsafe { libdeno::deno_new(config) }; | |||
| // This channel handles sending async messages back to the runtime. | |||
| let (tx, rx) = mpsc::channel::<(usize, Buf)>(); | |||
| let isolate_perms = | |||
| Arc::new(permissions.unwrap_or(DenoPermissions::new(&state.flags))); | |||
There was a problem hiding this comment.
I think this is strange that the permissions come from state.flags or from this extra argument. I'd rather explicitly have the caller create the permission struct and always pass it, than this implicit logic.
There was a problem hiding this comment.
Looks good - thanks!
-
I think the Permissions argument to Isolate::new should not be optional. What do you think?
-
Also there should be a test showing the user gets PermissionDenied when attempting to call
op_fetch_module_meta_dataif they don't have read access
|
I made the requested changes by making permissions a required parameter, and added 3 different tests for |
| @@ -365,11 +374,28 @@ fn op_fetch_module_meta_data( | |||
| let specifier = inner.specifier().unwrap(); | |||
| let referrer = inner.referrer().unwrap(); | |||
|
|
|||
| assert_eq!(state.dir.root.join("gen"), state.dir.gen, "Sanity check"); | |||
| if !isolate.permissions.allow_read.load(Ordering::SeqCst) { | |||
There was a problem hiding this comment.
If I understand correctly we only want to give "compiler" isolate access to this op. Some comment would be welcomed here, as all other ops prompt for missing permissions.
There was a problem hiding this comment.
Since I have no way to know at this point what file is going to be accessed or who is making the call to op_fetch_module_meta_data, any isolate that doesn't have the permissions required to do the operations that would be performed by this call should just be given a permission denied. The permissions in question are read and net, because a module request can perform a http/https get request or a file system read depending on the url of said module.
There was a problem hiding this comment.
Yeah I get it, it's just kind of "special" case - all other ops prompt for permission. I just wanted one-line comment on this special case :)
Refactored permissions to be assignable on a per-isolate basis, and added a fix for #1858 to
op_fetch_module_meta_data.