refactor(permissions): split up Descriptor into Allow, Deny, and Query

[dsherret]

This makes the permission system more versatile.

[lowlighter]

Any chance taking a look at #14668 during this refactor ?

[dsherret]

@lowlighter no, that would be too many changes for this PR.

[bartlomieju]

What's wrong with that?

[dsherret]

It's exposing a mutex, it's having code that shouldn't be done/exposed at this level (parsing descriptors), and it's coupling what could be its private api to this part of the code.

[bartlomieju]

Open an issue to keep track of it? Or are you planning to address it before landing?

[dsherret]

#25634

[bartlomieju]

I want to give runtime/permissions/lib.rs one more pass tomorrow

[bartlomieju]

This is a bit confusing - effectively there are circular dependencies between these three types. Do you see any chance we could decouple that with a helper type?

[bartlomieju]

Also could you add some docstrings around these traits?

[dsherret]

I got rid of the additional traits and now there's only a QueryDescriptor.

[bartlomieju]

Why are we using .matches() instead of .stronger_than() now?

[dsherret]

There are multiple specific methods. I've grouped them all on query now.

[bartlomieju]

Could you reword this docstring? I'm having trouble understanding it - what does "This variant can't be used to grant permissions" mean?

[dsherret]

I updated it a bit. The variant gets created when the command path can't be resolved, so it will prompt the users for permissions about it, but it doesn't actually do anything when the user grants permissions. It's to prevent someone trying to probe for permissions to find out what's on the path.

[bartlomieju]

Can you open an issue about this one?

[dsherret]

I put it in #25634

[satyarohith]

why is this change required? I assume it's unrelated?

[dsherret]

undefined isn't valid JSON, but null is (it does a JSON.parse below). I ran into this issue while developing where I introduced a bug, then it was undefined, then the error message was not great.

[lucacasonato]

Did you benchmark these changes?

From what I can tell, we do a lot more canonicalization for FS ops now (maybe including more getcwd calls?) - I wonder if sync FS ops will slow down due to this. From my reading of the code, this difference would only be visible in non --allow-all contexts.

[lucacasonato]

In a follow up we should change the return value here to an enum or struct. This is unreadable.

[dsherret]

From what I can tell, we do a lot more canonicalization for FS ops now (maybe including more getcwd calls?)

It should be doing the same or less now with --allow-all. It doesn't canonicalize like before.

[lucacasonato]

I meant without --allow-all

[dsherret]

Yeah should be the same without allow-run. It was doing the cwd call before as well.

[dsherret]

Some benchmarks:

$ cat main.ts
for (var i = 0; i < 100_000; i++) {
  const text = Deno.readTextFileSync("./main.ts");
  console.log(text.length);
}
$ hyperfine --warmup=3 "./deno_main run --allow-read=main.ts main.ts" "./deno_new run --allow-read=main.ts main.ts"
Benchmark 1: ./deno_main run --allow-read=main.ts main.ts
  Time (mean ± σ):      1.955 s ±  0.018 s    [User: 0.513 s, System: 1.446 s]
  Range (min … max):    1.931 s …  1.989 s    10 runs
 
Benchmark 2: ./deno_new run --allow-read=main.ts main.ts
  Time (mean ± σ):      1.932 s ±  0.011 s    [User: 0.495 s, System: 1.441 s]
  Range (min … max):    1.920 s …  1.953 s    10 runs
 
Summary
  ./deno_new run --allow-read=main.ts main.ts ran
    1.01 ± 0.01 times faster than ./deno_main run --allow-read=main.ts main.ts
$ hyperfine --warmup=3 "./deno_main run --allow-all main.ts" "./deno_new run --allow-all main.ts"
Benchmark 1: ./deno_main run --allow-all main.ts
  Time (mean ± σ):      1.907 s ±  0.015 s    [User: 0.468 s, System: 1.442 s]
  Range (min … max):    1.882 s …  1.933 s    10 runs
 
Benchmark 2: ./deno_new run --allow-all main.ts
  Time (mean ± σ):      1.893 s ±  0.014 s    [User: 0.462 s, System: 1.435 s]
  Range (min … max):    1.876 s …  1.914 s    10 runs
 
Summary
  ./deno_new run --allow-all main.ts ran
    1.01 ± 0.01 times faster than ./deno_main run --allow-all main.ts

[bartlomieju]

LGTM