ACL Implementation - initial commit - still a WIP

[cicorias]

[mattpodwysocki]

Is any VS Code specific files necessary here?

[cicorias]

removing..

[yaronyg]

Reviewed 11 of 31 files at r1, 2 of 16 files at r2, 5 of 6 files at r3.
Review status: 13 of 41 files reviewed at latest revision, 21 unresolved discussions.

---

README.md, line 2 [r1] (raw file):
You went to all the trouble of creating a stand alone depot, might as well put in some kind of Readme to tell the poor reader what this thing is and what it's meant to do.

---

lib/acl-example.js, line 1 [r3] (raw file):
Yeah, I meant jsdoc. :) So we can get rid of this.

---

lib/acl-schema.json, line 1 [r3] (raw file):
Per previous we don't need this.

---

lib/fauxton.js, line 1 [r3] (raw file):
Shouldn't this be in sample?

---

lib/index.js, line 14 [r1] (raw file):
Didn't you just calculate this in the value of itExists? Why are you calling indexOf twice with identical arguments?

---

lib/index.js, line 8 [r3] (raw file):
I'm confused as to what this function does. Near as I can tell it is exactly equivalent to 'return fast.indexOf(subject, target, key, 0)'.

---

lib/index.js, line 10 [r3] (raw file):
Seriously, I don't even care if it's faster. This is not readable code and at least requires a comment. Can you please just check if the result is > -1?

---

lib/index.js, line 21 [r3] (raw file):
/agasint/against an/

---

lib/index.js, line 22 [r3] (raw file):
Saying it's an object sorta defeats the purpose. You can use jsdoc to create stand alone type definitions and then refer to them in the param's type

---

lib/index.js, line 27 [r3] (raw file):
The use of the term role is actually confusing. I don't believe you mean role which is usually a group definition (e.g. admins are a role, mike is an admin). I think what you actually mean is user or identity. This distinction matters because eventually we do want to add in support for roles.

I also don't think we want to have path.role.verb because it requires us to redefine allowed verbs for every user. E.g. if both Joe and Jane have access to the same paths and verbs we have to enter them as two separate roles so their identities will match.

Also having path.role.verb turns out to be difficult because it doesn't let us define all the rights for a user in one place. We have to define the splattered across the various paths. This makes adding or removing a user really painful because we have to change values in a ton of places.

I would suggest a different approach. How about role.path.verb?

I would also add a translation step, a group object. When we look up an identity we will submit it to the group object array that is allowed to return exactly one (or no) value. That is their role.

We then look up role['pullSync'].path['/'].verb['GET'].

Now we can define all the paths and verbs for a specific role in one place at one time. We can easily separate definitions for different roles. And we can add and remove users from a role by changing their value in the separate group array.

---

lib/index.js, line 42 [r3] (raw file):
If I'm reading this correctly then wouldn't it allow matches against /okpath/notokpath? I think the check you are looking for is req.path.slice(-1) === '/'.

Also wouldn't it simplify the code to, before calling mapIt, instead check if req.path contains a '/' as the last character and if it does then remove and then for the first time call mapIt?

---

lib/index.js, line 48 [r3] (raw file):
I'm confused. You ran exactly the same mapIt function with the same arguments on line 37 or so. Why are you repeating it here?

---

lib/index.js, line 63 [r3] (raw file):
Why isn't this just roles[roleExists].verbs? You already calculated the roles value in line 56?

---

lib/index.js, line 64 [r3] (raw file):
Per https://tools.ietf.org/html/rfc7230#section-3.1.1 HTTP methods are case sensitive so we must not change the casing of the method.

---

lib/indexOf.js, line 32 [r1] (raw file):
Why would you need secure comparison for any of the scenarios in this project? The issue is that secure-compare does constant time comparisons which is important to prevent exposing passwords but we aren't doing that in this scenario. We are just comparing paths and in this case the path is set by the caller so there is nothing for us to expose. So a '===' comparison should be fine.

---

lib/indexOf.js, line 33 [r1] (raw file):
Shouldn't this be if (!secure)?

---

lib/indexOf.js, line 46 [r1] (raw file):
Why bother with fromIndex when none of your code uses it? Simpler is better!

---

lib/pouchdb.js, line 1 [r3] (raw file):
I'm pretty sure this isn't necessary for the module so it shouldn't be in lib, right?

---

lib/pouchdb.js, line 11 [r3] (raw file):
? Is this for a test?

---

Comments from the review on Reviewable.io

[yaronyg]

@cicorias some initial comments to chew on. Please assign the bug back to me when it's ready for final review.

[cicorias]

Review status: 7 of 41 files reviewed at latest revision, 21 unresolved discussions.

---

LICENSE, line 3 [r1] (raw file):
Done.

---

lib/acl-schema.json, line 1 [r3] (raw file):
I think it helps. I'd like to leave it.

---

lib/index.js, line 14 [r1] (raw file):
lingering code. fixed

---

lib/index.js, line 8 [r3] (raw file):
yep - lingering code - will fix

---

lib/index.js, line 10 [r3] (raw file):
modified

---

lib/index.js, line 21 [r3] (raw file):
updated

---

lib/index.js, line 22 [r3] (raw file):
at this point if we want to get to some level of 'done' well come back to this.

---

lib/index.js, line 27 [r3] (raw file):
in my testing as for PouchDB replication the "path" is the primary resource that requires permission. And this is REST so the verbs are really the permissions.

So, the simplest configuration that i came up with is 'path.role.verb. And there would be "1" per DB.

As for Identity vs. Role - the use of the term Identity is something that carries forward from PskIdentity. However, our permission model is seems categorizes into 3 (really 2 for this add-in) "buckets". So, identities would really map to a "role" - and that's how the permission is articulated. If it were Identity, there would be a far larger dataset of ACLs.

As for adding a group object, again, until we fully integrate and validate it won't work, then we can address.

For your example of "pullSync" - not sure what that applies from. A "PullSync" would actually entail GET/PUT/POST - which is how at the layer we're at, the PouchDB client talks to the PouchDB server. If we go to "pullSync" - well, that would break out into the verbs as they are now. So, they have to be represented somewhere.

To move forward on this, i' going to need better articulated acceptance testing as the initial discussion was with PouchDB replication as the acceptance test.

---

lib/index.js, line 42 [r3] (raw file):
the code has been simplified.

---

lib/index.js, line 48 [r3] (raw file):
no, one has a trailing slash, the other doesn't

---

lib/index.js, line 63 [r3] (raw file):
roleExists is an integer (the index) of the array/object offset.

---

lib/index.js, line 64 [r3] (raw file):
updated, however, we're deep in the Express/Nodejs http layer and quite frankly, any HTTP request that was malformed in that it used lowercase GET or other verbs, wouldn't have gotten here. The responsibility is in NodeJS-http . The lowercase was there to make the JSON ACL file readable. However, it's been updated for Verbs to Uppercase. And any comparison will not do case comparison.

---

lib/indexOf.js, line 32 [r1] (raw file):
i'm sorry, you had indicated to use this in the past during a conversation. If you don't want it i'm fine with that.

i've removed it.

---

lib/indexOf.js, line 33 [r1] (raw file):
Yes. updated.

---

lib/indexOf.js, line 46 [r1] (raw file):
yep, removed.

---

lib/acl-example.js, line 1 [r3] (raw file):
actually, i'd rather leave it. I think it's helpful note.

---

lib/fauxton.js, line 1 [r3] (raw file):
i'll refer to it in the Readme.md when I do that. But it's a version of the ACL JSON that will handle fauxton web site (_utils).

---

Comments from the review on Reviewable.io

[cicorias]

updated and pushed.

---

Review status: 7 of 41 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from the review on Reviewable.io

[yaronyg]

My assumption is that we don't need test-old so I'm not reviewing it. Please let me know if my assumption is wrong.

---

Reviewed 5 of 41 files at r1, 10 of 18 files at r2, 1 of 6 files at r3, 8 of 13 files at r4, 2 of 4 files at r5, 1 of 19 files at r6, 1 of 2 files at r8, 2 of 24 files at r9, 3 of 7 files at r10, 1 of 6 files at r12, 11 of 11 files at r14.
Review status: 46 of 55 files reviewed at latest revision, 36 unresolved discussions.

---

newtests.txt, line 2 [r14] (raw file):
Was this file really intended to be checked in?

---

lib/index.1.js, line 1 [r14] (raw file):
I don't believe anyone uses this file so can we delete it?

---

lib/index.js, line 13 [r14] (raw file):
Why is the same string value twice in the debug statement?

---

lib/index.js, line 16 [r14] (raw file):
Wouldn't this code be easier to read if it said

if (NOTFOUND === pathExists) {
   return false;
}

That way the rest of the code can be out-dented?

---

lib/index.js, line 37 [r14] (raw file):
I would suggest either putting in a proper definition or just deleting this JSdoc. It's not nice to tease the devs. :)

---

lib/index.js, line 68 [r14] (raw file):
NB: Wouldn't it be better to reject the request if no role is set? It's always dangerous in security to default to allowing something as this means that someone forgetting a line of code can break the security of the whole system. I do realize that the fix is to just to ever use the role 'public' but still, in security treating nothing as something tends to lead to tears.

---

lib/index.js, line 119 [r14] (raw file):
Where is the code that validates that if we get a /local/thali:id that the public key hash after 'thali_' matches the validated identity of the caller?

---

lib/index.js, line 121 [r14] (raw file):
Isn't this comment wrong since there is already a check and we know the request is _local?

---

lib/index.js, line 122 [r14] (raw file):
When lookupPath is calculated it looks for the last / and then goes to the next character and returns all of that and then appends IDTOKEN. So wouldn't this make a mess of /thali_:id since it would find the / before thali_ and then effectively remove thali_ and replace it with :id?

---

lib/indexOf.js, line 39 [r6] (raw file):
@cicorias Could you please answer this one? I honestly don't know why this function isn't just ' return source === target'?

---

lib/indexOf.js, line 47 [r14] (raw file):
Don't we need to remove fromIndex from the jsdoc?

---

sample/acl-example.js, line 11 [r14] (raw file):
I don't think anyone uses this file anymore

---

sample/fauxton.js, line 86 [r14] (raw file):
Do we need these comments?

---

sample/key-cert.pem, line 1 [r14] (raw file):
With the latest fix do we need this?

---

sample/key.pem, line 1 [r14] (raw file):
With the latest fix do we need this?

---

sample/package.json, line 12 [r14] (raw file):
Since we use nodemon shouldn't it be at least a dev dependency?

---

sample/pouchdb.js, line 38 [r14] (raw file):
Are these comments still needed?

---

sample/server.js, line 37 [r14] (raw file):
This call confuses me since I thought ACL rquired two arguments, a dbname and the actual acl. But the acl variable only returns an acl, not a db name.

---

sample/app/validate.js, line 36 [r14] (raw file):
I've seen this function in at least two places. Also there are standard NPM libraries that handle GUIDs. In either case I think we should either use one of the standard libraries or we should put this code into a central place and require it.

---

test/acl-block.1.js, line 6 [r14] (raw file):
If the goal is to replicate our standard rules then shouldn't there be a rule for '/' by itself?

---

test/acl-get-multipleusers.js, line 1 [r14] (raw file):
This file is used by a bunch of tests but I believe all of them are in test-old so wouldn't it be good to move this to test-old as well?

---

test/acl-get-user.js, line 1 [r14] (raw file):
Since this is only used by no-test-acl-get-user.js which is in test-old wouldn't it be good to move this to test-old as well?

---

test/acl-path-types.js, line 1 [r14] (raw file):
Since this is only used by no-test-acl-pathtypes.js which itself is in test-old wouldn't it be good to move this to test-old as well?

---

test/handlers.js, line 7 [r14] (raw file):
Since every array entry is identical why do we need three? Why not just use one?

---

test/jshint.spec.js, line 1 [r14] (raw file):
Wouldn't it also be good to lint the tests themselves?

---

test/template.js, line 1 [r14] (raw file):
Does this need to be checked in given all the examples we now have of this?

---

test/test-core-resources-local.js, line 15 [r14] (raw file):
I think it's confusing to have a require variable (e.g. path = require('path')) with the same name as a local variable, in this case, also path.

---

test/test-core-resources-local.js, line 15 [r14] (raw file):
Wouldn't it have been easier to shrink these tests down both here and in places like test-core-resources or test-core-db or test-core-bulkget, by just creating a test object of the form:
{ path: dbName + '/_local/1234',
methodResults: [{ method = 'GET', result = 200 },
{ method = 'POST', result = 401}]
}

That way we could have a single test rig that read in the object and ran all the tests. It would also make it much easier to see exactly what the test was doing.

---

test/test-core-resources-local.js, line 25 [r14] (raw file):
I was expecting to see a test for thali_

---

test/test-core-resources.js, line 42 [r14] (raw file):
It would be good to have a test for '/' by itself

---

test/test-noaclfile.js, line 37 [r14] (raw file):
What are these comments for? They don't seem consistent with this test.

---

test/test-noaclfile.js, line 58 [r14] (raw file):
Since public is anyway the default role (although I do hope that is changed) why do you need to set this? Shouldn't the test pass without it?

---

test/test-parmchecks.js, line 5 [r14] (raw file):
Request, express and colors aren't being used so why are they required?

---

test/test-parmchecks.js, line 11 [r14] (raw file):
We really shouldn't need to use path.join here. Just a simple '../lib/index' should work. That is how we do everything in Thali. This would also mean we can remove the path require.

---

test/test-parmchecks.js, line 13 [r14] (raw file):
This function isn't called so why is it in the file?

---

Comments from Reviewable

[cicorias]

Reviewed 3 of 41 files at r1, 3 of 18 files at r2, 1 of 24 files at r9.
Review status: 46 of 55 files reviewed at latest revision, 37 unresolved discussions.

---

newtests.txt, line 2 [r14] (raw file):
Done.

---

lib/index.js, line 42 [r5] (raw file):
this library doesn't handle the Admin scenario as that was done in another library. For the public the PSK key would need to be known. And this library wouldn't do anything other than perhaps check what the PSK identity is.

---

lib/index.js, line 13 [r14] (raw file):
Done.

---

lib/index.js, line 16 [r14] (raw file):
Done.

---

lib/index.js, line 37 [r14] (raw file):
Done.

---

lib/index.js, line 68 [r14] (raw file):
Done.

---

lib/index.js, line 119 [r14] (raw file):
since both /local/thali:id and /_local/:id exist, and they both are GET only, and we have no definition as to what ID can be, then the /local/thali:id path is irrelevant as /_local/zthali would be valid, in fact anything /_local/zzzz is valid. articulate how you'd differentiate for /local/thali:id vs /_local/:id then i can implement. but since :id is undetermine it could in face even be thali_zzzz

---

lib/index.js, line 121 [r14] (raw file):
Done.

---

lib/index.js, line 122 [r14] (raw file):
see note prior on the fact there is no way nor reason to ditingguish between /_local/:id vs /_local/thali_:id

The ACL for /_local/:id is the same as /_local/thali_:id

---

lib/indexOf.js, line 39 [r6] (raw file):
Done.

---

lib/indexOf.js, line 47 [r14] (raw file):
Done.

---

sample/acl-example.js, line 11 [r14] (raw file):
Done.

---

sample/fauxton.js, line 86 [r14] (raw file):
Done.

---

sample/key-cert.pem, line 1 [r14] (raw file):
I have to rewrite/update the sample - so I'm creating another Issue for that. #4

---

sample/key.pem, line 1 [r14] (raw file):
to be addressed by #4

---

sample/package.json, line 12 [r14] (raw file):
I'm no sure where nodemon is being used in the sample.

---

sample/pouchdb.js, line 38 [r14] (raw file):
Done.

---

sample/server.js, line 37 [r14] (raw file):
sample to be addressed by #4

---

sample/app/validate.js, line 36 [r14] (raw file):
it will be addressed in #4

---

test/acl-block.1.js, line 6 [r14] (raw file):
where is that identified? the '/' by itself? I can add a test, along with the ACL - is it "GET", etc.

---

test/acl-get-multipleusers.js, line 1 [r14] (raw file):
Done.

---

test/acl-get-user.js, line 1 [r14] (raw file):
Done.

---

test/acl-path-types.js, line 1 [r14] (raw file):
Done.

---

test/handlers.js, line 7 [r14] (raw file):
not used anymore

---

test/jshint.spec.js, line 1 [r14] (raw file):
no as with mocha you get errors like 'it' is not defined. (W117)' etc.

---

test/template.js, line 1 [r14] (raw file):
Done.

---

test/test-core-resources-local.js, line 15 [r14] (raw file):
renamed the require('path') var.

---

test/test-core-resources-local.js, line 15 [r14] (raw file):
at some future point.

---

test/test-core-resources-local.js, line 25 [r14] (raw file):
see notes regarding the fact that there is no way or reason to distinguish /_local/:id vs. /_local/thali_:id as the ACL is the same and the former could be anything for :ID

---

test/test-core-resources.js, line 42 [r14] (raw file):
Done.

---

test/test-noaclfile.js, line 37 [r14] (raw file):
Done.

---

test/test-noaclfile.js, line 58 [r14] (raw file):
public has been removed.

---

test/test-parmchecks.js, line 5 [r14] (raw file):
Done.

---

test/test-parmchecks.js, line 11 [r14] (raw file):
i'll leave it as is for now.

---

test/test-parmchecks.js, line 13 [r14] (raw file):
Done.

---

Comments from Reviewable

[coveralls]

Changes Unknown when pulling 258beec on initial into * on master*.

[yaronyg]

Reviewed 1 of 13 files at r4, 1 of 2 files at r8, 2 of 11 files at r14, 20 of 29 files at r15.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

lib/index.js, line 16 [r14] (raw file):
Why is the else clause still there?

---

lib/index.js, line 119 [r14] (raw file):
What we mean is that IF ID === thali_* THEN we have to confirm that the value after the thali_ prefix matches the same public key hash associated with the authenticated caller. The easiest way to do this is to accept in the SALTI constructor a third argument that is a callback of the form:

validateThaliLocal(publicKeyHash, pskIdentifier) which would then return a Boolean. If false then the request is rejected. If true then it is accepted.

The first argument would be the ID minus the thali_ prefix (this should be a base 64 URL safe encoded hash of the callers public key) and the second value is req.connection.pskIdentifier.

With those two inputs it is the job of the function to either return true if the pskIdentifier value matches the base 64 URL encoded public key hash in the ID or not.

So basically this all boils down to adding a new argument to the SALTI constructor that takes this function and putting in a call when you get to this part of the code (e.g. local) and determine that the submitted ID value begins with the characters 'thali'. You would then call the argument in the function with the two values I specified and it would return true if you should continue or false if you should return a 401.

(Note, not that it matters since this is invisible to the SALTI code but inside of the callback function passed to the constructor will be a call to thaliNotificationServer.getPskIdToPublicKey which will translate the pskIdentifier argument to a public key Buffer. We then will pass that buffer through thaliNotificationBeacons.createPublicKeyHash which will then be passed through the urlsafe-base64 NPM package. The resulting string will then be compared to the value you passed in with the content after thali_. If the two strings match then true will be returned, otherwise false.)

---

lib/index.js, line 122 [r14] (raw file):
Please see previous comment and please let me know if the explanation made sense.

---

lib/indexOf.js, line 39 [r6] (raw file):
I don't see any change to the file content.

---

sample/acl-example.js, line 11 [r14] (raw file):
What do you mean by done? The file is still in the project. Shouldn't it be deleted?

---

sample/fauxton.js, line 86 [r14] (raw file):
The comments are still here.

---

sample/package.json, line 12 [r14] (raw file):
In the runit.sh file

---

test/acl-block.1.js, line 6 [r14] (raw file):
Yup it's just '/' with absolutely nothing else and it only supports the GET method.

---

test/test-core-resources-local.js, line 25 [r14] (raw file):
Hopefully my previous comments will clarify and we can throw in a test that detects thali_ and shows that it calls the function that was passed in on the constructor.

---

test/test-core-resources.js, line 42 [r14] (raw file):
It's actually supposed to succeed with our base ACL :)

---

test/test-noaclfile.js, line 37 [r14] (raw file):
Actually they still appear to be in the file

---

Comments from Reviewable

[cicorias]

Reviewed 1 of 18 files at r2, 1 of 13 files at r4, 1 of 4 files at r5, 1 of 29 files at r15.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

lib/index.js, line 16 [r14] (raw file):
Done.

---

lib/indexOf.js, line 39 [r6] (raw file):
i missed that.

---

sample/acl-example.js, line 11 [r14] (raw file):
anything related to Sample/ are in #4

---

sample/fauxton.js, line 86 [r14] (raw file):
anyting in sample/ will be handled in #4

---

sample/package.json, line 12 [r14] (raw file):
removed runit.sh

---

test/test-core-resources.js, line 42 [r14] (raw file):
Done.

---

test/test-noaclfile.js, line 37 [r14] (raw file):
Done.

---

Comments from Reviewable

[cicorias]

Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

a discussion (no related file):
What I would like to do is move all the /thali_:id related ACL items to a new Issue/bug and just get this PR completed.

---

lib/index.1.js, line 1 [r14] (raw file):
Done.

---

Comments from Reviewable

[coveralls]

Changes Unknown when pulling 2f69b90 on initial into * on master*.

[yaronyg]

Reviewed 5 of 6 files at r16.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[yaronyg]

---

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[cicorias]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

.vscode/tasks.json, line 1 [r1] (raw file):
Done.

---

Comments from Reviewable

[cicorias]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

.vscode/tasks.json, line 1 [r1] (raw file):
Done.

---

Comments from Reviewable