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

Add --deny flag support #1591

Closed
wants to merge 11 commits into from

Conversation

4 participants
@sh7dm
Copy link
Contributor

sh7dm commented Jan 26, 2019

Fixes #1580.
Testing:
python tools/test.py
To test:
This should throw an error: deno https://deno.land/x/http/file_server.ts --deny
This should work: deno https://deno.land/x/http/file_server.ts --allow-net --deny
This should also work: deno https://deno.land/x/http/file_server.ts -A --deny

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Jan 26, 2019

This should work: deno https://deno.land/x/http/file_server.ts --allow-net --deny
This should also work: deno https://deno.land/x/http/file_server.ts -A --deny

It defeats purpose of explicit permissions. Why would I enable access to net and deny all and still get net permission?

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Jan 26, 2019

@bartlomieju Fixed

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Jan 26, 2019

I actually meant that I see no purpose for this flag, but let's wait for opinion of others.

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Jan 26, 2019

@ry what did you mean in #1580? Which one is correct: 9360400 or 2e99561?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Jan 26, 2019

Okay in light of #1580 I know see its purpose - if explicit permission is not given, then program shouldn't display prompt.

So instead of this:

pub fn check_run(&self) -> DenoResult<()> {
    if self.deny {
      return Err(permission_denied());
    };
    if self.allow_run.load(Ordering::SeqCst) {
      return Ok(());
    };
    ...
}

it should be:

pub fn check_run(&self) -> DenoResult<()> {
    if self.allow_run.load(Ordering::SeqCst) {
      return Ok(());
    };
    if self.deny {
      return Err(permission_denied());
    };
    ...
}
@ry
Copy link
Collaborator

ry left a comment

Cool! Needs tests somehow

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Jan 26, 2019

@ry maybe create a -D alias to --deny?

Show resolved Hide resolved src/permissions.rs Outdated
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Jan 26, 2019

sh7dm added some commits Jan 27, 2019

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented on tools/permission_prompt_test.py in 9de7d50 Jan 27, 2019

Should this be not in stderr ?

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Feb 1, 2019

@ry @hayd I've added unit tests, could you please review?

sh7dm added some commits Feb 1, 2019

@ry
Copy link
Collaborator

ry left a comment

Nice work!
Just one nit otherwise LGTM

Show resolved Hide resolved src/permissions.rs Outdated
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Feb 1, 2019

Should we test e.g. that --allow-net --deny on a net test passes?

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Feb 1, 2019

@hayd I'll do it!

@sh7dm

This comment has been minimized.

Copy link
Contributor Author

sh7dm commented Feb 1, 2019

@hayd done

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Apr 14, 2019

This is an old PR. We have --no-prompt which handles this situation now.

Thanks @sh7dm. Closing.

@ry ry closed this Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.