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

Add fs read permission (don't squash) #1689

Merged
merged 6 commits into from Feb 8, 2019

Conversation

4 participants
@sh7dm
Copy link
Contributor

sh7dm commented Feb 5, 2019

Fixes #1225;

  • Add tests
    /cc @ry

@sh7dm sh7dm force-pushed the sh7dm:read-permission branch from 0e15f1e to fc8c083 Feb 5, 2019

Show resolved Hide resolved tools/unit_tests.py

@fewf fewf referenced this pull request Feb 5, 2019

Closed

Add allow read #1693

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Feb 6, 2019

@ry please don't squash, @fewf should be credited for his work!

@sh7dm sh7dm changed the title Add fs read permission Add fs read permission (don't squash) Feb 6, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 7, 2019

We're getting this error in CI:

$ ./tools/test_format.py
/Users/travis/build/denoland/deno/target/release/deno --allow-read --allow-run tools/format.ts
Cannot resolve module "--allow-read --allow-run" from "."
@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Feb 7, 2019

@ry fixed

Show resolved Hide resolved js/read_dir_test.ts Outdated
@fewf

This comment has been minimized.

Copy link
Contributor

fewf commented Feb 7, 2019

I have a fix for what @J2P caught in a PR into this branch sh7dm#2 .

I think it could be misleading that after giving permission to a prompt that says Deno requests read access to {filename}. Grant?, I've now given permission to the script to read all of my filesystem. (This applies to the write permission prompt too). To resolve this, granting permission through the prompt could give a one-time permission to the script, or the prompt could read something like "Deno requests read access to your filesystem. Grant?"

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 7, 2019

To resolve this, granting permission through the prompt could give a one-time permission to the script, or the prompt could read something like "Deno requests read access to your filesystem. Grant?"

I agree - but let's fix that in a separate PR - because the same applies to allow-write too

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 7, 2019

@fewf I've pushed your patch

@ry ry force-pushed the sh7dm:read-permission branch from 522c781 to 3546fa5 Feb 7, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 8, 2019

Hoping to land this one tomorrow - but it needs a few more fixes to go green.

sh7dm and others added some commits Feb 5, 2019

Add --allow-read
Co-authored-by: Greg Altman <g.s.altman@gmail.com>

@ry ry force-pushed the sh7dm:read-permission branch from 236a7f4 to 7454431 Feb 8, 2019

@sh7dm sh7dm force-pushed the sh7dm:read-permission branch from 7406631 to 7454431 Feb 8, 2019

Fix
@ry

ry approved these changes Feb 8, 2019

Copy link
Collaborator

ry left a comment

LGTM - very nice work @sh7dm and @fewf !

I'm sure this is going to break a lot of code - but it's better to do this now than 6 months from now.

@ry ry merged commit 9ab0338 into denoland:master Feb 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

ry added a commit to ry/deno that referenced this pull request Feb 9, 2019

v0.2.11
- Add deps to --info output (denoland#1720)
- Add --allow-read (denoland#1689)
- Add deno.isTTY() (denoland#1622)
- Add emojis to permission prompts (denoland#1684)
- Add basic WebAssembly support (denoland#1677)
- Add `NO_COLOR` support https://no-color.org/ (denoland#1716)
- Add color exceptions (denoland#1698)
- Fix: do not load cache files when recompile flag is set (denoland#1695)
- Upgrade V8 to 7.4.98 (denoland#1640)

ry added a commit that referenced this pull request Feb 9, 2019

v0.2.11
- Add deps to --info output (#1720)
- Add --allow-read (#1689)
- Add deno.isTTY() (#1622)
- Add emojis to permission prompts (#1684)
- Add basic WebAssembly support (#1677)
- Add `NO_COLOR` support https://no-color.org/ (#1716)
- Add color exceptions (#1698)
- Fix: do not load cache files when recompile flag is set (#1695)
- Upgrade V8 to 7.4.98 (#1640)

@sh7dm sh7dm deleted the sh7dm:read-permission branch Feb 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment