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

Allow inspection and revocation of permissions #1875

Merged
merged 5 commits into from Mar 4, 2019

Conversation

5 participants
@fd
Copy link
Contributor

fd commented Mar 3, 2019

This patch allows deno scripts to revoke permissions granted by the user. When the script, at a later stage, needs those permissions, a new prompt will be presented to the user.

The main use-case for this feature is to allow trusted bootstrap code to run with raised privileges without granting all permissions to untrusted code following the bootstrap fase.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 3, 2019

CLA assistant check
All committers have signed the CLA.

@afinch7 afinch7 referenced this pull request Mar 3, 2019

Closed

Capabilities of process #1862

@zekth

This comment has been minimized.

Copy link
Contributor

zekth commented Mar 3, 2019

So one of the main goal for revocation of permissions is for example:

  • Script is needing to load configuration from a file but do not need to read anything anymore
  • deno runtime allow-read
  • read the file
  • revoke permission

Right?

I really like this PR. (but frustrated was working on it too :D )

@fd

This comment has been minimized.

Copy link
Contributor Author

fd commented Mar 3, 2019

That's right. The only tricky part is that the import order must be deterministic and all bootstrap code must be synchronous for this to work. Dynamic import() will eventually fix that too as it will allow us to import unsafe code at after the first event-loop tick.

@afinch7

This comment has been minimized.

Copy link
Contributor

afinch7 commented Mar 3, 2019

Dynamic import wouldn't let us error before execution, but like any other import function It should give a reliable way of determining where the request comes from.

This one looks like it should work well. I can't say if it would get approved in the current state. Typically changes that add ops need to be pretty well justified to be accepted.

I'm also not so sure how I feel about the revoke part, since I expect it to go unused in most cases. I think that separate allow once and allow always options for the permissions request prompt might be more useful.

@ry
Copy link
Collaborator

ry left a comment

Looks good - well done.

Please clean up the docs and scan website/manual.md if there is any places this could be mentioned

for (let perm of Object.keys(revoked)) {
assertEqual(revoked[perm], false);
}
});

This comment has been minimized.

@ry

ry Mar 4, 2019

Collaborator

Cool - interesting test

This comment has been minimized.

@zekth

zekth Mar 4, 2019

Contributor

this can be refactored with

for (let perm in revoked) {

This comment has been minimized.

@afinch7

afinch7 Mar 4, 2019

Contributor

perm could also be a const

for (const perm in revoked) {

This comment has been minimized.

@fd

fd Mar 4, 2019

Author Contributor

done, it now reads:

for (const perm in revoked) {
import { assert } from "./util";

// Permissions as granted by the caller
export type Permissions = {

This comment has been minimized.

@ry

ry Mar 4, 2019

Collaborator

Use jsdoc for the comment

This comment has been minimized.

@fd

fd Mar 4, 2019

Author Contributor

done

export type Permission = keyof Permissions;

// Inspect granted permissions for the current program.
export function permissions(): Permissions {

This comment has been minimized.

@ry

ry Mar 4, 2019

Collaborator

Use jsdoc for the comment. Bonus points for a code example (ideally a working one that people can copy and paste)

This comment has been minimized.

@fd

fd Mar 4, 2019

Author Contributor

done

}

// Revoke a permission. When the permission was already revoked nothing changes
export function revokePermission(permission: Permission): void {

This comment has been minimized.

@ry

ry Mar 4, 2019

Collaborator

Jsdoc

This comment has been minimized.

@fd

fd Mar 4, 2019

Author Contributor

done

@ry

ry approved these changes Mar 4, 2019

Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit 77d7ad6 into denoland:master Mar 4, 2019

2 checks passed

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

@fd fd deleted the fd:revocable-permissions branch Mar 5, 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.