Skip to content
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

Permissions refactor #1864

Merged
merged 6 commits into from Mar 2, 2019

Conversation

3 participants
@afinch7
Copy link
Contributor

afinch7 commented Mar 1, 2019

Refactored permissions to be assignable on a per-isolate basis, and added a fix for #1858 to op_fetch_module_meta_data.

afinch7 added some commits Mar 1, 2019

fmt

@afinch7 afinch7 changed the title [WIP]: Permissions refactor Permissions refactor Mar 1, 2019

@@ -54,6 +58,7 @@ impl Worker {
pub fn spawn(

This comment has been minimized.

@ry

ry Mar 1, 2019

Collaborator

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?

@@ -195,6 +169,7 @@ impl Isolate {
snapshot: libdeno::deno_buf,
state: Arc<IsolateState>,
dispatch: Dispatch,
permissions: Option<DenoPermissions>,

This comment has been minimized.

@ry

ry Mar 1, 2019

Collaborator

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?

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

This comment has been minimized.

@ry

ry Mar 1, 2019

Collaborator

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.

@ry
Copy link
Collaborator

ry left a 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_data if they don't have read access

@afinch7

This comment has been minimized.

Copy link
Contributor Author

afinch7 commented Mar 1, 2019

I made the requested changes by making permissions a required parameter, and added 3 different tests for op_fetch_module_meta_data.

fmt
@@ -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) {

This comment has been minimized.

@bartlomieju

bartlomieju Mar 1, 2019

Contributor

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.

This comment has been minimized.

@afinch7

afinch7 Mar 1, 2019

Author Contributor

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.

This comment has been minimized.

@bartlomieju

bartlomieju Mar 2, 2019

Contributor

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

@ry

ry approved these changes Mar 2, 2019

Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit 8c310d3 into denoland:master Mar 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.