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.

Sign up for GitHub

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

Jump to bottom

No prompts flag for non interactive environments. #1913

Merged
merged 8 commits into from
Mar 13, 2019
Merged

No prompts flag for non interactive environments. #1913

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9160e8d
No prompts flag for non interactive environments.
afinch7 Mar 12, 2019
ddd043d
Applied a little DRY to no prompts checks in permissions.
afinch7 Mar 12, 2019
57c362f
Not sure what I was thinking with that last commit
afinch7 Mar 12, 2019
4af2845
Added some tests
afinch7 Mar 12, 2019
03665ef
Forgot the most important part
afinch7 Mar 13, 2019
0e4ee2d
Added a comment to make ci run again
afinch7 Mar 13, 2019
7625d11
Wrap this comment
afinch7 Mar 13, 2019
82dea1d
fmt
afinch7 Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ fn lazy_start(parent_state: &Arc<IsolateState>) -> Resource {
allow_env: AtomicBool::new(false),
allow_net: AtomicBool::new(true),
allow_run: AtomicBool::new(false),
..Default::default()
};
let rid = cell.get_or_insert_with(|| {
let resource = workers::spawn(
Expand Down
5 changes: 5 additions & 0 deletions src/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub struct DenoFlags {
pub allow_net: bool,
pub allow_env: bool,
pub allow_run: bool,
pub no_prompts: bool,
pub types: bool,
pub prefetch: bool,
pub info: bool,
Expand Down Expand Up @@ -108,6 +109,9 @@ fn set_recognized_flags(
flags.allow_read = true;
flags.allow_write = true;
}
if matches.opt_present("no-prompt") {
flags.no_prompts = true;
}
if matches.opt_present("types") {
flags.types = true;
}
Expand Down Expand Up @@ -149,6 +153,7 @@ pub fn set_flags(
opts.optflag("", "allow-env", "Allow environment access");
opts.optflag("", "allow-run", "Allow running subprocesses");
opts.optflag("A", "allow-all", "Allow all permissions");
opts.optflag("", "no-prompt", "Do not use prompts");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--deny ?
#1580

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No prompt shouldn't be limited to permissions, so in that respect it's different than --deny. Either your there to answer for prompts or you aren't as a user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other prompts are there?

I don't understand the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess for the time being it only affects permissions, but If we add any other prompts in the future it should be used to allow disabling those as well.

Copy link
Member

@ry ry Mar 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--deny would be confusing when combined with, say, --allow-read

> deno  foo.ts --allow-read --deny

What is intended is: "run deno with foo.ts as the main script, allow fs read access, but do not prompt for permission"
But people might interpret this as: "run deno with foo.ts as the main script, allow fs read access, deny all access (overriding previous?)"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--no-prompt is kinda long.... Can we quickly just throw out a few other suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other suggestions it seems. I can't think of anything better...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about --no (as analogous to --yes e.g. of unix tools)

opts.optflag("", "recompile", "Force recompilation of TypeScript code");
opts.optflag("h", "help", "Print this message");
opts.optflag("D", "log-debug", "Log debug output");
Expand Down
4 changes: 4 additions & 0 deletions src/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,6 +1876,7 @@ mod tests {
allow_env: AtomicBool::new(true),
allow_net: AtomicBool::new(true),
allow_run: AtomicBool::new(true),
..Default::default()
};
let isolate = Isolate::new(
IsolateInit {
Expand Down Expand Up @@ -1921,6 +1922,7 @@ mod tests {
allow_env: AtomicBool::new(true),
allow_net: AtomicBool::new(true),
allow_run: AtomicBool::new(true),
..Default::default()
};
let isolate = Isolate::new(
IsolateInit {
Expand Down Expand Up @@ -1966,6 +1968,7 @@ mod tests {
allow_env: AtomicBool::new(true),
allow_net: AtomicBool::new(false),
allow_run: AtomicBool::new(true),
..Default::default()
};
let isolate = Isolate::new(
IsolateInit {
Expand Down Expand Up @@ -2011,6 +2014,7 @@ mod tests {
allow_env: AtomicBool::new(false),
allow_net: AtomicBool::new(true),
allow_run: AtomicBool::new(false),
..Default::default()
};
let isolate = Isolate::new(
IsolateInit {
Expand Down
26 changes: 20 additions & 6 deletions src/permissions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub struct DenoPermissions {
pub allow_net: AtomicBool,
pub allow_env: AtomicBool,
pub allow_run: AtomicBool,
pub no_prompts: AtomicBool,
}

impl DenoPermissions {
Expand All @@ -28,6 +29,7 @@ impl DenoPermissions {
allow_env: AtomicBool::new(flags.allow_env),
allow_net: AtomicBool::new(flags.allow_net),
allow_run: AtomicBool::new(flags.allow_run),
no_prompts: AtomicBool::new(flags.no_prompts),
}
}

Expand All @@ -36,7 +38,7 @@ impl DenoPermissions {
return Ok(());
};
// TODO get location (where access occurred)
let r = permission_prompt("access to run a subprocess");
let r = self.try_permissions_prompt("access to run a subprocess");
if r.is_ok() {
self.allow_run.store(true, Ordering::SeqCst);
}
Expand All @@ -48,7 +50,8 @@ impl DenoPermissions {
return Ok(());
};
// TODO get location (where access occurred)
let r = permission_prompt(&format!("read access to \"{}\"", filename));;
let r =
self.try_permissions_prompt(&format!("read access to \"{}\"", filename));;
if r.is_ok() {
self.allow_read.store(true, Ordering::SeqCst);
}
Expand All @@ -60,7 +63,8 @@ impl DenoPermissions {
return Ok(());
};
// TODO get location (where access occurred)
let r = permission_prompt(&format!("write access to \"{}\"", filename));;
let r =
self.try_permissions_prompt(&format!("write access to \"{}\"", filename));;
if r.is_ok() {
self.allow_write.store(true, Ordering::SeqCst);
}
Expand All @@ -72,8 +76,10 @@ impl DenoPermissions {
return Ok(());
};
// TODO get location (where access occurred)
let r =
permission_prompt(&format!("network access to \"{}\"", domain_name));
let r = self.try_permissions_prompt(&format!(
"network access to \"{}\"",
domain_name
));
if r.is_ok() {
self.allow_net.store(true, Ordering::SeqCst);
}
Expand All @@ -85,13 +91,20 @@ impl DenoPermissions {
return Ok(());
};
// TODO get location (where access occurred)
let r = permission_prompt(&"access to environment variables");
let r = self.try_permissions_prompt(&"access to environment variables");
if r.is_ok() {
self.allow_env.store(true, Ordering::SeqCst);
}
r
}

fn try_permissions_prompt(&self, message: &str) -> DenoResult<()> {
if self.no_prompts.load(Ordering::SeqCst) {
return Err(permission_denied());
}
permission_prompt(message)
}

pub fn allows_run(&self) -> bool {
return self.allow_run.load(Ordering::SeqCst);
}
Expand Down Expand Up @@ -144,6 +157,7 @@ impl DenoPermissions {
allow_env: AtomicBool::new(false),
allow_net: AtomicBool::new(false),
allow_run: AtomicBool::new(false),
..Default::default()
}
}
}
Expand Down