Implement Ad Hoc Group listing support (acl:agentGroup) by dmitrizagidulin · Pull Request #15 · solid-contrib/solid-permissions

dmitrizagidulin · 2017-01-05T16:22:46Z

See https://github.com/solid/web-access-control-spec#groups-of-agents for specs

The usage is mostly unchanged; the main difference is that now the consuming code should pass a fetchGraph() function in the options parameter of ps.checkAccess(), so that it can load (local or remote) group listings.

- Implement agentGroup parsing and serializing - Implement group.initFromGraph(), add tests - Add checkAccess() support with loading group listings

dmitrizagidulin · 2017-01-06T18:20:38Z

@dan-f / @timbl / @nicola - ready for review.

Fixes issue #12.

elf-pavlik · 2017-01-07T23:29:10Z

    let target = null
-    let base = null
+    let base = this.aclUrl
    return new Promise((resolve, reject) => {


this should solve 2/2 in #16

elf-pavlik · 2017-01-07T23:29:38Z

    "babel-cli": "^6.14.0",
    "babel-preset-es2015": "^6.14.0",
-    "rdflib": "^0.12.1",
+    "rdflib": "^0.13.0",


this should solve 1/2 in #16

elf-pavlik · 2017-01-08T01:34:44Z

-    var ns = vocab(rdf)
-    var statements = [
+    let statement
+    let fragment = rdf.namedNode('#' + this.hashFragment())


Shouldn't NamedNode use full IRI instead of just fragment? Just as PermissionSet constructor accepts aclUrl Authorization should also accept aclUrl and use it to construct NamedNode with full IRI. I mentioned in the chatroom that currently .toNT() will produce invalid N-Triples

this commit based on branch from this PR implements that elf-pavlik@e439300

If single Authorization doesn't have to represent itself as RDF graph, possibly code from Authorization#rdfStatements() could move to PermissionSet#buildGraph() which would not require passing aclUrl to constructor of Authorization. It seems that currently original IRIs of authorizations from parsed ACL document will not get preserved and Authorization#rdfStatements() will always generate new IRI for each Authorization. Plus Authorization#mergeWith suggests that it doesn't really exactly represent instances of acl:Authorization (not "for a single resource and for a single webId (either agent or group)"). Actually it seems that PermissionSet should have responsibility to create RDF instances of acl:Authorization just as PermissionSet#initFromGraph() converts them into solid-permissions.js Authorization objects (!= acl:Authorization)

I captured above in dedicated issue #17 since this PR doesn't really introduce that problem and it can get addressed independently after merning this PR.

Good point. I'll throw in my $.02 that the aclUrl could alternatively be passed into the rdfStatements method so that an authorization can be reserialized for different graphs.

nicola

Looks great to me, this has still the group issue, right?

dmitrizagidulin · 2017-01-08T18:49:21Z

@nicola Yeah, this does still have the group issue. But, as I point out in the discussion at https://github.com/solid/web-access-control-spec#infinite-request-loops-in-group-listings -- this is an acceptable risk, and at worst results in a few extra requests on the server. We can address it later if it becomes a major problem.

nicola · 2017-01-09T04:17:46Z

hey @dmitrizagidulin although it is a great idea to mention,
I think would create an infinite loop of requests that might never end.

Doing the call just once, would basically a few (less than hundred?) times might crash the server since you will end up running out of memory. As soon as one finds a publicly writable space, they can attack the server by creating the files tha will cause the infinite loop.

I recall that the simples solution was that the server, when updating an ACL, will make sure that the new updated ACL does not reference a group where the acl to access that group is the ACL itself. I think this check might fix the issue. Before this trick is in place, we should not add this to release.

We can add it, but disabling external fetching of groups

dmitrizagidulin · 2017-01-09T14:09:03Z

@nicola Sure, we can discuss the various strategies of mitigating the group fetch loop. This is not the PR to be concerned about, however -- there will be a corresponding PR to node-solid-server that will be using this group support, so we can discuss the strategies there.

dan-f

Group permissions are exciting! I've got a bunch of thoughts on this PR.

dan-f · 2017-01-10T19:12:04Z

-    var ns = vocab(rdf)
-    var statements = [
+    let statement
+    let fragment = rdf.namedNode('#' + this.hashFragment())


Good point. I'll throw in my $.02 that the aclUrl could alternatively be passed into the rdfStatements method so that an authorization can be reserialized for different graphs.

dan-f · 2017-01-10T19:14:40Z

        ns.acl('Authorization'))
    ]
-    if (this.agent) {
+    if (this.isAgent()) {


To nitpick, i'm finding the is* methods (isPublic, isGroup, isAgent) to be a bit confusing. An authorization isn't a group nor is it an agent, but an authorization. Maybe something like isFor* instead?

dan-f · 2017-01-10T19:16:27Z

   * @method rdfStatements
-   * @return {Array<Statement>} List of RDF statements representing this Auth,
+   * @param rdf {RDF} RDF Library
+   * @return {Array<Triple>} List of RDF statements representing this Auth,


This has nothing to do with this PR, but I really wish we had an RDF library that could pass around immutable indexed graphs and merge/intersect/union etc. them. Arrays of statements feels a bit awkward.

dan-f · 2017-01-10T20:02:37Z

  /**
-   * Sets the agent WebID for this authorization. Implemented as `setAgent()`
-   * setter method to enforce mutual exclusivity with `group` property, until
-   * ES6 setter methods become available.


ES6 setter methods are available.

Another thing is to detect the type of agent (foaf:Agent or vcard:Group) and choose the appropriate predicate.

dan-f · 2017-01-10T20:20:23Z

+  constructor (options = {}) {
+    this.uri = options.uri
+    this.uid = options.uid
+    this.members = options.members || {}


https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

Set would not be appropriate here -- the members object is used as a dictionary, for key-based lookups. (Using Set would require using .find() to achieve equivalent functionality.)

Your use case is exactly what sets are for. O(1) membership lookups.

And that way you don't have to map webIds to booleans. The webId is either in the set or it isn't.

I think you're thinking of Maps or something. Sets do not have O(1) lookups.

Are you referring to the has()? Are those operations O(1)?

Membership checks are the sole use case for a set. They're implemented in almost every language using a hash table.

Ah, my bad, I didn't realize that. Sure, I can change that part to use Sets instead.

dan-f · 2017-01-11T17:07:39Z

-   * @param agent {String|Quad} Agent URL (or `acl:agent` RDF triple).
+   * @param agent {string|Quad|GroupListing} Agent URL (or `acl:agent` RDF triple).
   */
  setAgent (agent) {


I'm concerned that these setAgent/setGroup methods are adding a fair bit of complexity for not much payoff. Why should permissions be mutable? Can't we handle the modification of an ACL at the PermissionSet level?

Another way to put this is that the current rules for mutating an Authorization's agent/group is difficult to reason about. I think that part of the problem is that at a conceptual level, mutating a single authorization is a semantically broad operation. An Authorization is currently a tuple of modes, agent/group, allowed origins, etc. With the current set of setter methods you can modify an Authorization to the point that it's not remotely the same authorization anymore, e.g. by changing the access modes and the agent/group.

However if you wish to stick with this approach I'd like to see the agent be abstracted away from the concerns of this method. Could we define an Agent class/superclass which represents either a group, individual agent, or "everyone"? That way we'd only need a single setAgent method, and the concern of whether the agent is an individual, group, or everyone could be handled during serialization using a duck typing approach.

dan-f · 2017-01-11T17:26:16Z

+const vocab = require('solid-namespace')
+
+/**
+ * Ad Hoc ACL Group Listing


What about group permissions is ad hoc?

dan-f · 2017-01-11T17:32:01Z

+    let result = false
+    let membershipMatches = this.groupsForMember(agentId)
+    membershipMatches.forEach(groupWebId => {
+      if (this.checkAccessForAgent(resourceUrl, groupWebId, accessMode)) {


This could be more efficient using .find().

dan-f · 2017-01-11T17:32:59Z


+  /**
+   * Returns a list of webIds of groups to which this agent belongs.
+   * Note: Only checks loaded groups (assumes a previous `loadGroups()` call).


You know what they say about assumptions...

I think the loading of groups should be transparent to the caller. It should not be the responsibility of the client to handle/understand the internal implementation of the library.

dan-f · 2017-01-11T17:36:55Z

+    let fetchGraph = options.fetchGraph
+    let rdf = options.rdf || this.rdf
+    if (!fetchGraph) {
+      return Promise.reject(new Error('Cannot load groups, fetchGraph() not supplied'))


I'm not a fan of this practice of passing fetchGraph / rdf / webclient objects around to methods which need them. At the very, very least there should be a default implementation that's provided by the library.

elf-pavlik · 2017-03-07T13:48:03Z

It looks like two small fixes needed for #16 got stuck in this PR which seems not active for almost a month now :(

dmitrizagidulin · 2017-03-07T15:18:32Z

@elf-pavlik indeed. I'll see if I can add those two fixes in a separate PR.

dmitrizagidulin added the in progress label Jan 5, 2017

dmitrizagidulin force-pushed the dz_groups branch 2 times, most recently from 43ec1f7 to 54a227d Compare January 5, 2017 22:47

Implement Ad Hoc Group listing support

b1ce882

- Implement agentGroup parsing and serializing - Implement group.initFromGraph(), add tests - Add checkAccess() support with loading group listings

dmitrizagidulin force-pushed the dz_groups branch from 929637d to b1ce882 Compare January 6, 2017 16:02

checkAccess() now calls loadGroups() if needed

fdac218

dmitrizagidulin force-pushed the dz_groups branch from 6a51d95 to fdac218 Compare January 6, 2017 18:17

dmitrizagidulin added ready for review and removed in progress labels Jan 6, 2017

Make ps.addAuthorizationFor() chainable

a5c9567

Fixes issue #12.

elf-pavlik reviewed Jan 7, 2017

View reviewed changes

elf-pavlik mentioned this pull request Jan 7, 2017

PermissionSet#serialize breaks with rdflib.js 0.13.0 #16

Closed

2 tasks

elf-pavlik reviewed Jan 8, 2017

View reviewed changes

nicola approved these changes Jan 8, 2017

View reviewed changes

elf-pavlik mentioned this pull request Jan 8, 2017

Authorization objects don't represent instances of acl:Authorization #17

Closed

dan-f suggested changes Jan 11, 2017

View reviewed changes

elf-pavlik mentioned this pull request May 15, 2018

Implement Group access in the ACL system nodeSolidServer/node-solid-server#673

Closed

kjetilk added this to the nss-5.0.0 milestone Jun 26, 2018

kjetilk merged commit a5c9567 into master Jul 13, 2018

dmitrizagidulin deleted the dz_groups branch August 8, 2018 13:35

Conversation

dmitrizagidulin commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrizagidulin commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik Jan 8, 2017 • 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!

elf-pavlik Jan 8, 2017 • 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!

nicola left a comment

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin commented Jan 8, 2017

Uh oh!

nicola commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrizagidulin commented Jan 9, 2017

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik commented Mar 7, 2017

dmitrizagidulin commented Jan 5, 2017 •

edited

Loading

elf-pavlik Jan 8, 2017 •

edited

Loading

elf-pavlik Jan 8, 2017 •

edited

Loading

nicola commented Jan 9, 2017 •

edited

Loading