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

More permissions prompt options #1926

Merged
merged 9 commits into from Mar 18, 2019
Merged

Conversation

afinch7
Copy link
Contributor

@afinch7 afinch7 commented Mar 13, 2019

Added some new permissions prompt options, and gave better names to existing options. The existing options when you receive a permission prompt are: y for allow always(I thought this meant yes once for some time) and n for no once(give the program a permission denied error this time). This change adds two new options and renames the existing allow always option the new set of options are:

  • a/A for allow always for this permission type
  • y/Y for allow once
  • n/N for deny once
  • d/D for deny always for this permission type

It might also make sense to remove the revoke commands, since this solves the same problems in a way that doesn't conflict with --no-prompt.

@afinch7 afinch7 changed the title [WIP]: More permissions prompt options More permissions prompt options Mar 14, 2019
@hayd
Copy link
Contributor

hayd commented Mar 14, 2019

Would this allow using a String rather than Boolean one day? E.g. allowing web by domain? #1639

@zekth
Copy link
Contributor

zekth commented Mar 14, 2019

E.g. allowing web by domain

Great idea

Also allowing read for certain files / pattern? like --allow-read-pattern /config.json --allow-read-pattern /assets/**/*

src/permissions.rs Show resolved Hide resolved
@afinch7
Copy link
Contributor Author

afinch7 commented Mar 17, 2019

Did a refactor of the permissions prompt tests. They are now semi-procedural I.E. test_yes_yes("read"). This should make them a little easier to maintain and update. I'm not completely sure if the run() call is synchronous, but it seems like it behaves just like the other calls in testing.

@afinch7
Copy link
Contributor Author

afinch7 commented Mar 18, 2019

This is ready for review.

tools/permission_prompt_test.py Outdated Show resolved Hide resolved
tools/permission_prompt_test.ts Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - very nice improvement

FYI the core integration patch #1938 introduces a lot of instability. In particular, I've had to disable to permission_prompt_test (among others) in order to get it to go green. I'm going to nevertheless land it today (after this one goes in).

def test(self):
for test_type in self.test_types:
test_name_base = "test_" + test_type
wrap_test(test_name_base + "_allow_flag", self.test_allow_flag,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be clearer as a second for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might look cleaner, but I think from a debugging perspective it's not worth removing that much explicitness from a test. This would likely only save a few lines. I will add some comments to wrap_test.

Copy link
Contributor

Choose a reason for hiding this comment

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

How so? if you debug then you can access the variable name.

@afinch7
Copy link
Contributor Author

afinch7 commented Mar 18, 2019

Not sure what happened to CI on that last one, but I made some changes based on @hayd's feedback. Hopefully it passes this time.

@ry ry merged commit 08a674b into denoland:master Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants