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

Add hasPaid(agent) function to ACL check #1577

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add hasPaid(agent) function to ACL check #1577

wants to merge 16 commits into from

Conversation

michielbdejong
Copy link
Member

No description provided.

@michielbdejong
Copy link
Member Author

Linked to nodeSolidServer/acl-check#38

@michielbdejong
Copy link
Member Author

Now seeing:

solid:ACL accessDenied: modeURIorReasons: ["User Unauthorized","Paying Would Help"] +0ms

in the logs so that's good progress so far.

@michielbdejong
Copy link
Member Author

I'm thinking the ACL checks are getting too complicated if we add this the way I originally thought we could.

So instead:

  • run the ACL check as normal, but keep track of whether any of the authorisations that did not apply would have applied if either the user would (authenticate and) pay (i.e. was the hasPaid(agent) callback called at least once).
  • if so and access was denied (401 or 403) then re-run the ACL check as if the agent had (authenticated and) paid. Careful not to cause any side-effects there.
  • if so, respond with 402 instead of 401 / 403.
  • if not (e.g. paying would give read access but the attempt is to control), then still give the 401/403 we would originally.

@michielbdejong
Copy link
Member Author

Got an approximation now where if hasPaid was checked and it was empty, and the response otherwise is a 401 or a 403, then it becomes a 402. This is too often, because it could be that you pay to get more access, but then that access will still not be enough to do what you were trying to do. But it's a reasonable first approximation.

I can now look at the integration of koa-ilp first, and the resolve this small inaccuracy last.

@michielbdejong
Copy link
Member Author

Hm, seems that https://github.com/interledgerjs/ilp-fetch supports both psk2 and stream (ILP/STREAM being the newer one) but https://github.com/interledgerjs/koa-ilp/blob/master/index.js only does psk2

@michielbdejong
Copy link
Member Author

https://interledger.org/rfcs/0025-pre-shared-key-2/ even explicitly says PSK2 is deprecated. So that's no good, then. Will see if I can find some code that creates a Pay header with stream.

@michielbdejong
Copy link
Member Author

Not sure how to add the Pay header into the response. https://github.com/solid/node-solid-server/blob/v5.6.2/lib/handlers/allow.js#L73 seems to be where the response status is passed on from the acl check, but not sure if we could add additional headers through that too

@michielbdejong
Copy link
Member Author

@michielbdejong
Copy link
Member Author

michielbdejong commented Aug 31, 2021

Failing test from monetization-tests:
● Read-Paying › Gives a 402 with accessTo Read access on non-container resource

This was because I hadn't npm-linked to the corresponding PR on acl-check. All good.
Now we just need to add a call to a 'hasPaid' oracle.

@michielbdejong
Copy link
Member Author

It's now making the call to the oracle and checking if the response body equals 'ok'.
The monetization-tests seem to be making some calls with a null agent, which I don't understand.
Should also maybe return the payment details from the oracle?

@michielbdejong michielbdejong changed the title [WiP] Add hasPaid(agent) function to ACL check Add hasPaid(agent) function to ACL check Sep 2, 2021
@michielbdejong michielbdejong marked this pull request as ready for review September 2, 2021 11:46
@michielbdejong
Copy link
Member Author

michielbdejong commented Sep 2, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant