Refactor acl checker to use solid-permissions lib #443

dmitrizagidulin · 2016-09-15T16:33:19Z

No description provided.

dmitrizagidulin · 2016-09-15T16:59:17Z

package.json

    "rimraf": "^2.5.0",
    "run-waterfall": "^1.1.3",
    "solid-namespace": "^0.1.0",
+    "solid-permissions": "git://github.com/solid/solid-permissions.git#dz_allows_helpers",


Temporary (for this pr review only), until PR solid-contrib/solid-permissions#3 gets merged.

dmitrizagidulin · 2016-09-15T17:00:07Z

@dan-f @nicola - ready for review.

nicola · 2016-09-18T14:29:03Z

lib/acl-checker.js

    var possibleACLs = ACLChecker.possibleACLs(resource, this.suffix)
    // If this is an ACL, Control mode must be present for any operations
-    if (this.isAcl(resource)) {
+    if (this.isAcl(resource, this.suffix)) {


Can this.suffix not be used directly in this.isAcl?
In the rest of the code this.isAcl doesn't take any second parameter

Yeah, missed this bit when I was refactoring. Thanks!

nicola · 2016-09-18T14:34:20Z

lib/acl-checker.js

+      rdf: rdf,
+      strictOrigin: this.strictOrigin,
+      isAcl: (uri) => { return this.isAcl(uri) },
+      aclUrlFor: (uri) => { return this.aclUrlFor(uri) }


at this point, can we not pass the entire ACL instance?
(maybe not)

We could? I'd rather be explicit, though..

nicola · 2016-09-18T14:34:49Z

lib/acl-checker.js

+    let aclOptions = {
+      aclSuffix: this.suffix,
+      graph: graph,
+      host: options.host,


how is this used?
where is host set and what is this set to?

host gets set in the allow handler, here.
It's used by the 'enforce strict origin' code in the permissions lib.

perfect! works with me!

dan-f

A couple requests

Could you help me with my API questions?
It looks like you introduced some error handling in findRule. Could you add tests to cover that error handling?

Looks good other than that!

dan-f · 2016-09-19T14:32:32Z

lib/acl-checker.js

        }
-        return callback(null)
+      })
+      .catch(err => {


Could you clarify the distinction between a rejected promise and a resolved promise with a falsy value in checkAccess?

Sure thing. checkAccess() resolves with either a truthy value (which means the user has access), or a falsy value (which means the user does not have access). A rejected promise means an error (parsing error, argument error, network errors, etc).

dan-f · 2016-09-19T14:34:35Z

lib/acl-checker.js

-  }
-
+  /**
+   * @method findRule


Awesome JSDoc, but what does this method do?

Heh, yeah, I s'pose it wouldn't hurt to add an actual description.

dan-f · 2016-09-19T14:35:49Z

lib/acl-checker.js

+      .then(hasAccess => {
+        if (hasAccess) {
+          debug(`${mode} access permitted to ${user}`)
+          return callback()


Since this method is called findRule, I think I assumed that the callback function would get passed the access rule/policy for the given user.

Yeah, now that it's refactored the findRule name doesn't make as much sense. I'll rename it to something like checkAccess.

dmitrizagidulin · 2016-09-19T16:44:08Z

It looks like you introduced some error handling in findRule. Could you add tests to cover that error handling?

I don't think there's any additional error handling introduced. The existing test suite should cover the various cases.

dan-f · 2016-09-19T18:14:16Z

I don't think there's any additional error handling introduced. The existing test suite should cover the various cases.

I don't see tests for the "parsing error, argument error, network errors" cases.

dmitrizagidulin · 2016-09-19T18:15:30Z

I don't see tests for the "parsing error, argument error, network errors" cases.

Those belong in the permissions library, though, not here.

dan-f · 2016-09-19T18:18:04Z

Those belong in the permissions library, though, not here.

... which I just checked and does not have tests for such cases.

dmitrizagidulin · 2016-09-19T18:19:58Z

I'm not sure I understand your point. This PR is a net increase in test coverage, in this acl checking code.

dan-f · 2016-09-19T18:23:52Z

I'm not sure I understand your point. This PR is a net increase in test coverage, in this acl checking code.

My point is that checkAccess is supposed to catch network (and other?) errors, and return false, that the user does not have access in such cases. But those cases are not covered either in node-solid-server nor in the permissions library. We should have a test case that demonstrates that the error handling actually works as expected, since we don't want to accidentally give permissions when the network fails, for example.

dmitrizagidulin · 2016-09-23T16:58:25Z

@dan-f so I've been looking at this issue (testing the .catch() clause of checkAccess) for a bit.
I'm still not really seeing what I can do here.
Am I testing the Promise error behavior? (That if there's an error inside a promise, it'll go into the .catch(err) clause?) That doesn't seem right - that's up to the Node foundation etc.
Am I testing that if checkAccess() calls the callback() with an error, the user is denied access? The rest of the existing suite checks that.
I don't really see what else I can do here.

dan-f · 2016-09-26T14:08:19Z

@dmitrizagidulin the code path I'm thinking of is where the call to acls.checkAccess(resource, user, mode) returns a Promise which rejects. You told me above that your library rejects a promise in the case of "parsing error, argument error, network errors, etc," so shouldn't we verify that this consumer code handles permissions correctly when one of those errors arise?

dmitrizagidulin · 2016-09-28T18:55:53Z

@dan-f Added proxyquire-based unit tests for acl-checker.checkAccess().

dan-f · 2016-09-29T14:21:33Z

Looks awesome, @dmitrizagidulin. Thanks! 👍

dmitrizagidulin added 2 commits August 23, 2016 18:18

Add docstring to findRule()

bf1c4b2

Refactor acl checker to use solid-permissions lib

24fe64f

deiu added the in progress label Sep 15, 2016

dmitrizagidulin commented Sep 15, 2016

View reviewed changes

dmitrizagidulin added ready for review and removed in progress labels Sep 15, 2016

Pass in server's acl uri deriving functions

80e0675

dmitrizagidulin force-pushed the external_permissions_lib branch from 4cf58ce to 80e0675 Compare September 15, 2016 17:56

nicola suggested changes Sep 18, 2016

View reviewed changes

dan-f suggested changes Sep 19, 2016

View reviewed changes

Rename findRule() to checkAccess(), add comments

3b0bc9f

Bump solid permissions dep

cfb71d1

nicola approved these changes Sep 19, 2016

View reviewed changes

Remove invalid comment

3f4a7bd

dmitrizagidulin added 3 commits September 28, 2016 14:50

Add proxyquire package

a831439

allow handler - format cleanup

8930736

Add acl-checker unit tests

5b17170

dan-f approved these changes Sep 29, 2016

View reviewed changes

dmitrizagidulin merged commit 99c2cb1 into master Sep 29, 2016

dmitrizagidulin deleted the external_permissions_lib branch September 29, 2016 16:32

Refactor acl checker to use solid-permissions lib #443

Refactor acl checker to use solid-permissions lib #443

Uh oh!

Conversation

dmitrizagidulin commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin commented Sep 15, 2016

Uh oh!

nicola Sep 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin commented Sep 19, 2016

Uh oh!

dan-f commented Sep 19, 2016

Uh oh!

dmitrizagidulin commented Sep 19, 2016

Uh oh!

dan-f commented Sep 19, 2016

Uh oh!

dmitrizagidulin commented Sep 19, 2016

Uh oh!

dan-f commented Sep 19, 2016

Uh oh!

dmitrizagidulin commented Sep 23, 2016

Uh oh!

dan-f commented Sep 26, 2016

Uh oh!

dmitrizagidulin commented Sep 28, 2016

Uh oh!

dan-f commented Sep 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicola Sep 18, 2016 •

edited

Loading