Strange behaviour when creating a policy for ACP

renyuneyun commented 7 months ago

Search terms you've used

ACP
getResourcePolicyAll
getPolicyUrlAll

Bug description

I'm following the official document to create a policy (rule) in ACP. The policy was successfully created, and seems to be interpreted by Solid (CSS). However, if I look into the .acr document, things are not as expected.

There are three strange behaviours:

Unnamed nodes from existing policy are unwrapped and given a blank node name (a very long one rather than a compact one);
It creates a new node (#defaultAccessControl) for acp:AccessControl instead of using an existing one (because this line refers to a function which should reuse an existing acp:AccessControl);
The created new node does not have a acp:AccessControl for its type.

To Reproduce

Create a .acr with some policy (e.g. the MRE below)
Follow the steps in the official document to create a new access control policy for it

Minimal reproduction

See this gist for the example .acr file: https://gist.github.com/renyuneyun/834eb5ee542e06a2cc3ee1a57712ca3f. To reproduce, remove , <#defaultAccessControl> at line 6 and remove line 25-31, and put it to the .acr file.

Expected result

Unnamed nodes stay the same;
Existing acp:AccessControl node being used;
(Or) the created new node has acp:AccessControl as its type.

Actual result

See comment under the gist for the actual .acr file after the operation.

Environment

  System:
    OS: Linux 6.6 Arch Linux
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
    Memory: 1.90 GB / 15.40 GB
    Container: Yes
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.6.1 - ~/node_modules/.bin/node
    Yarn: 1.22.21 - /usr/bin/yarn
    npm: 10.4.0 - /usr/bin/npm
  Browsers:
    Chromium: 122.0.6261.57
  npmPackages:
    @comunica/query-sparql: ^2.8.1 => 2.10.0 
    @inrupt/solid-client: ^2.0.0 => 2.0.0 
    @inrupt/solid-client-authn-browser: ^2.0.0 => 2.0.0 
    @inrupt/vocab-common-rdf: ^1.0.5 => 1.0.5 
    @inrupt/vocab-solid: ^1.0.4 => 1.0.4 
    @mdi/font: ^7.1.96 => 7.2.96 
    @renyuneyun/solid-helper: ^0.0.3 => 0.0.3 
    @rushstack/eslint-patch: ^1.1.4 => 1.3.2 
    @tsconfig/node20: ^20.1.0 => 20.1.0 
    @types/n3: ^1.10.4 => 1.16.0 
    @types/node: ^20.4.4 => 20.4.4 
    @vitejs/plugin-vue: ^4.0.0 => 4.2.3 
    @vue/eslint-config-prettier: ^8.0.0 => 8.0.0 
    @vue/eslint-config-typescript: ^11.0.0 => 11.0.3 
    @vue/tsconfig: ^0.4.0 => 0.4.0 
    eslint: ^8.22.0 => 8.45.0 
    eslint-plugin-vue: ^9.3.0 => 9.15.1 
    n3: ^1.16.3 => 1.17.2 
    npm-run-all: ^4.1.5 => 4.1.5 
    path-browserify: ^1.0.1 => 1.0.1 
    pinia: ^2.1.4 => 2.1.4 
    prettier: ^3.0.0 => 3.0.0 
    solid-helper-vue: ^0.1.4 => 0.1.4 
    typescript: ^5.1.6 => 5.1.6 
    vite: ^4.0.0 => 4.4.6 
    vue: ^3.3.4 => 3.3.4 
    vue-tsc: ^1.0.12 => 1.8.6 
    vuetify: ^3.3.9 => 3.3.9 
  npmGlobalPackages:
    @quasar/cli: 2.3.0
    @renyuneyun/solid-helper: 0.0.3
    @renyuneyun/parse-link-header-ts: 1.0.1
    orchestrator-app: 0.1.0
    solid-helper-vue: 0.2.0
    solid-shell: 2.0.8
    vercel: 32.5.5

Additional information

NSeydoux commented 7 months ago

Hi @renyuneyun , thanks for reaching out.

I understand your concerns, but I think it is correct that a given Access Control Resource may have several Access Controls, and having the acp:AccessControl type is implicit. For instance, it is omitted in https://solidproject.org/TR/acp#example-access-control.

Is it accurate to say that the graph isn't isomorphic to what you would prefer, but it is semantically equivalent and results in access policies being applied accurately?

renyuneyun commented 7 months ago

Hi @NSeydoux , thanks for looking into this.

Yes, you are right, it is semantically equivalent, and the policies are applied accurately.

I understand that they may have implicit types, and actually would be more inclined to not requiring an explicit type (because it should be inferrable in principle). Although, 1) the solid-client library actually assumes explicit typing and relies on that heavily for ACP; 2) for the manipulation, I would believe it is a better idea to explicitly type every new node the library creates.

Apart from that, the main issue I see is the (seemingly) inconsistency between the code and the actual behaviour, particularly the item 2 and this line of the library (which is called by acp_ess_2.setResourcePolicy()). The code seems to suggest that no new acp:AccessControl node should be created, while in reality it creates a new node (without explicitly giving it a type acp:AccessControl), and named it #defaultAccessControl. The behaviour seems to be more aligned with this addPolicyUrl() function in policy/addPolicyUrl.ts, rather than the imported control.ts#addPolicyUrl().

So, again, this routes back to my question (in the other recent issues) on the relation between these same-name similar-purpose different-implementation functions under different files / directories, and the possibility of them messing up the code.

inrupt / solid-client-js