feat(run): allow/deny-all permission prompts

[iuioiua]

Previously, the user would be prompted every time a new entry needed to be added to either the granted or denied list for a given unary permission. This would result in the following output:

Download ⠴ https://deno.land/x/aws_api/services/sts/mod.ts
Warning Implicitly using latest version (v0.7.0) for https://deno.land/x/aws_api/services/sts/mod.ts
✅ Granted env access to "AWS_REGION". // Enter `y`
✅ Granted env access to "AWS_ACCESS_KEY_ID". // Enter `y`
✅ Granted env access to "AWS_SECRET_ACCESS_KEY". // Enter `y`
...

This change introduces two new options for unary runtime permission prompts; Y to allow all or N to deny all. This will result in the following output:

Download ⠴ https://deno.land/x/aws_api/services/sts/mod.ts
Warning Implicitly using latest version (v0.7.0) for https://deno.land/x/aws_api/services/sts/mod.ts
✅ Granted all env access. // Enter `Y`

Todo:

• [x] Implement for remaining unary permissions
• [x] Tests (any guidance involving user input such as Y and N?)

Closes #16550.

[bartlomieju]

@iuioiua this PR looks good and I would love to land it for 1.31. Could you rebase the PR?

[iuioiua]

Will do! However, I'd like to have a test for each permission before landing. Do you agree?

[bartlomieju]

Yes, that sounds reasonable

[iuioiua]

Hm... Why are other tests such as _090_run_permissions_request passing in CI? They shouldn't be, as they should be asking for Y/N (uppercase) input options. Am I missing something?

[iuioiua]

Aside from requiring a rebase, I tested the branch and indeed both upper and lower case Y and N work. This is I believe by design and also how it works with current release Deno CLI.

Rebased. Did you mean Y/N allow/deny all permissions? If so, Y and N behaviour now haven't been affected by this PR.

Also, I tried the a behaviour, and it seems to work within expectations. Using a yields the following:

asher@Ashers-MacBook-Air deno % cargo run run /Users/asher/GitHub/deno/cli/tests/testdata/run/permissions_prompt_allow_deny_all.ts
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s
     Running `target/debug/deno run /Users/asher/GitHub/deno/cli/tests/testdata/run/permissions_prompt_allow_deny_all.ts`
✅ Granted all run access.
✅ Granted all read access.
✅ Granted all write access.
✅ Granted all net access.
✅ Granted all env access.
✅ Granted all sys access.
✅ Granted all ffi access.

Using d yields the following:

asher@Ashers-MacBook-Air deno % cargo run run /Users/asher/GitHub/deno/cli/tests/testdata/run/permissions_prompt_allow_deny_all.ts
    Finished dev [unoptimized + debuginfo] target(s) in 0.24s
     Running `target/debug/deno run /Users/asher/GitHub/deno/cli/tests/testdata/run/permissions_prompt_allow_deny_all.ts`
❌ Denied all run access.
❌ Denied all run access.
❌ Denied all read access.
❌ Denied all read access.
❌ Denied all write access.
❌ Denied all write access.
❌ Denied all net access.
❌ Denied all net access.
❌ Denied all env access.
❌ Denied all env access.
❌ Denied all sys access.
❌ Denied all sys access.
❌ Denied all ffi access.
❌ Denied all ffi access.

Note: permission statuses are printed twice per permission use here. I didn't specifically enable this functionality, so I assume it's the default behaviour (wherever that code is). The fact that the output of this manual test varies from the output of integration::run::permissions_prompt_deny_all, yet it still passes, makes me question whether the test is working as it should.

[iuioiua]

Ah, my bad, @aapoalas - I get you now. Yeah, I see that issue with tools/format.js... I'll see what's up with that.

[iuioiua]

I've got a few more things to do with this PR before it's ready. I aim to have it ready for 1.31.0.

[bartlomieju]

I've got a few more things to do with this PR before it's ready. I aim to have it ready for 1.31.0.

@iuioiua do you need help with anything?

[iuioiua]

@iuioiua do you need help with anything?

I think I have it figured out now, but I'll let you know if I do 🤙🏾

[iuioiua]

Previously, only Deno.permissions.query() calls were covered. Now, prompt interactions are covered. Preliminary tests on my local machine look good.

This is done by adding is_all: bool to the return value of PermissionState.check(), which resembles whether the permission given in the prompt interaction is for one permission value (y or n) or all (a or d) of the same permission type (env, read, etc.)

Is this a sound approach?

[bartlomieju]

Tested locally, seems to be working very well.

Previously, only Deno.permissions.query() calls were covered. Now, prompt interactions are covered. Preliminary tests on my local machine look good.

This is done by adding is_all: bool to the return value of PermissionState.check(), which resembles whether the permission given in the prompt interaction is for one permission value (y or n) or all (a or d) of the same permission type (env, read, etc.)

Is this a sound approach?

I'm not fully grepping it - is_all seems very ambigious, what are the situations where this flag would be false?

We should probably bike shed a bit on the prompt; I'm thinking that instead of Allow? [y/n/a/d] (y = yes, allow; n = no, deny; a = yes to all, allow all; d = no to all, deny all) > we could do Allow? [y/n/a/d] (y = yes, allow; n = no, deny; a = allow all; d = deny all) >

[lucacasonato]

I find the wording of the question a bit confusing. We need to make clear that a = allow all env vars for example, not all permissions in general. Anyone have ideas to clarify?

[iuioiua]

Current:

[y/n/a/d] (y = yes, allow; n = no, deny; a = yes to all, allow all; d = no to all, deny all)

Suggestion (80 characters max. for write permissions):

[y/n/a/d] (y = yes; n = no; a = allow all write perms; d = deny all write perms)

[iuioiua]

I'm not fully grepping it - is_all seems very ambigious, what are the situations where this flag would be false?

I've renamed is_all to is_for_all_perm_values. It'd be false when the user enters y for a single permission value.

[bartlomieju]

Hey @iuioiua we discussed this PR during today's CLI working group meeting. Since denying permissions in 99% will mean that program terminates, we want to remove the option to "deny all" and only leave "allow all".

Other suggestions:

• use "A" (capital "A") for "allow all"
• prompt:

Allow? [y/n/A] (y = yes, allow; n = no, deny; A = allow all read/write/env... permissions) >

[iuioiua]

Hey @iuioiua we discussed this PR during today's CLI working group meeting. Since denying permissions in 99% will mean that program terminates, we want to remove the option to "deny all" and only leave "allow all".

Other suggestions:

• use "A" (capital "A") for "allow all"
• prompt:

Allow? [y/n/A] (y = yes, allow; n = no, deny; A = allow all read/write/env... permissions) >

Sounds good. I'll do that.