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 permissions.request #3296

Merged
merged 4 commits into from Nov 11, 2019

Conversation

@kt3k
Copy link
Contributor

kt3k commented Nov 8, 2019

This PR implements permissions.request API based on WICG's permissions-request spec. This PR follows #3200

closes #2767 (though it's already closed).

@kt3k kt3k force-pushed the kt3k:feature/perm-req branch from 13991a1 to 82d03c8 Nov 8, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Nov 8, 2019

I have a side question: so with recent changes we don't show prompt by default (but instead fail with PermissionsDenied) but you can use permissions.request() to display prompt? Isn't it step backwards compared to --no-prompt?

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Nov 8, 2019

@kt3k I'm happy to have this feature - but it needs some test. We removed the interactive prompt tests in 8f571ef. Ideally this would be done in Rust using something like nix::pty::openpty().

});
set_prompt_result(false);
assert_eq!(perms1.request_hrtime(), PermissionAccessorState::Deny);
}

This comment has been minimized.

Copy link
@ry

ry Nov 8, 2019

Collaborator

Sorry - I didn't see these tests when I wrote my previous comment.

But these still don't seem to be using openpty... I need to look into it more.

This comment has been minimized.

Copy link
@kt3k

kt3k Nov 8, 2019

Author Contributor

These tests replace permission_prompt with stub function and its returning values are controlled by set_prompt_result calls.

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Nov 8, 2019

@bartlomieju I think this isn't simply step backward compared to the previous API with --no-prompt option because with the previsou API, the prompt is automatically displayed whenever the feature is used. But with the current API, the prompt appears only when the program author intentionally tries to ask the permissions.
With the previous API, there was no way to avoid prompt when using the feature except passing --no-prompt, but with the current API, the program can avoid displaying prompt by simply avoiding calling permissions.request().

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Nov 8, 2019

@bartlomieju I think this isn't simply step backward compared to the previous API with --no-prompt option because with the previsou API, the prompt is automatically displayed whenever the feature is used. But with the current API, the prompt appears only when the program author intentionally tries to ask the permissions.
With the previous API, there was no way to avoid prompt when using the feature except passing --no-prompt, but with the current API, the program can avoid displaying prompt by simply avoiding calling permissions.request().

Thanks for the answer, let me rephrase my concern: with permissions.request() API is there a way to make it fail fast without displaying prompt? In production setting I would definitely not like for a program to display a prompt but fail hard instead.

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Nov 8, 2019

@bartlomieju

with permissions.request() API is there a way to make it fail fast without displaying prompt?

No such way as long as the program is started from interactive terminal.

In production setting I would definitely not like for a program to display a prompt but fail hard instead.

The prompt function checks the tty of stdin and stderr. So if we start the program from a non-tty environment, the program fails without displaying a prompt. I think most production envrionments don't have a tty terminal, so I don't think this cause that problem (stuck at prompt) such often.

async request(desc: PermissionDescriptor): Promise<PermissionStatus> {
const { state } = sendSync(dispatch.OP_REQUEST_PERMISSION, desc);
return new PermissionStatus(state);
}

This comment has been minimized.

Copy link
@ry

ry Nov 10, 2019

Collaborator

This needs to be added to lib.deno_runtime.d.ts

This comment has been minimized.

Copy link
@ry

ry Nov 10, 2019

Collaborator

Slight error in the example too. I got this when trying to run it:

 ️⚠️  Deno requests to access to environment variables.. Grant? [g/d (g = grant, d = deny)] g
error: Uncaught ReferenceError: home is not defined
► file:///Users/rld/src/deno/x.js:3:8

3   home = Deno.homeDir();
         ^

    at file:///Users/rld/src/deno/x.js:3:8

This comment has been minimized.

Copy link
@kt3k

kt3k Nov 11, 2019

Author Contributor

Thank you for catching!

/** Requests the permission.
* const status = await Deno.permissions.request({ name: "env" });
* if (status.state === "granted") {
* home = Deno.homeDir();

This comment has been minimized.

Copy link
@ry

ry Nov 11, 2019

Collaborator

If you make this line console.log(Deno.homeDir()) then this snippet of code becomes copy/paste able

This comment has been minimized.

Copy link
@kt3k

kt3k Nov 11, 2019

Author Contributor

OK. Makes sense. I'll update!

@ry
ry approved these changes Nov 11, 2019
Copy link
Collaborator

ry left a comment

LGTM

@ry
ry approved these changes Nov 11, 2019
Copy link
Collaborator

ry left a comment

LGTM

@ry ry merged commit 3111506 into denoland:master Nov 11, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.