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

treaty: add desk permissions check #215

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

ryjm
Copy link

@ryjm ryjm commented Aug 17, 2023

This PR introduces changes to %treaty to support the surfacing of desk permissions. The changes are as follows:

  1. pact mold: +$ pact [=treaty =crew:clay]
  2. pact-from-docket gate has been added. Takes a desk and docket:docket as arguments and returns a pact. It fetches the permissions from Clay and constructs a crew based on those permissions. A treaty is 'signed' before being passed along a subscription wire if the ship specified in the wire is in the pact's crew.

desk/sur/treaty.hoon Outdated Show resolved Hide resolved
this exposes a gall bug that triggers a kick-rewatch loop on the
subscriber sides. gall does not register the mark conversion failure
from treaty-0 to treaty-1 as an error and does not send a watch-nack, so
the subscriber will keep trying to watch the publisher's treaty even
though it immediately gets kicked.
@ryjm
Copy link
Author

ryjm commented Aug 20, 2023

this exposes a gall bug that triggers a kick-rewatch loop on the
subscriber sides. gall does not register the mark conversion failure
from treaty-0 to treaty-1 as an error and does not send a watch-nack, so
the subscriber will keep trying to watch the publisher's treaty even
though it immediately gets kicked.

any change to treaty will trigger this and i don't see an easy fix without implementing some versioning scheme - otherwise old versions of treaty will be ceaselessly spammed by new ones.

@ryjm
Copy link
Author

ryjm commented Aug 20, 2023

(this will happen regardless of whether or not the treaty-1 mark is used, i put it there to make the problem explicit)

@vcavallo
Copy link

vcavallo commented Sep 7, 2023

Any updates on this? Does Tlon need further input from us?

@arthyn
Copy link
Member

arthyn commented Sep 7, 2023

It didn't look finished since it's still a draft and mentioned issues that didn't seem resolved. If we could chat together that would be good because it's not clear from this PR what the effects are.

@arthyn arthyn requested a review from Fang- September 7, 2023 16:05
@Fang-
Copy link
Member

Fang- commented Sep 7, 2023

I'm gonna want to see at least some justification/use case description for this before I properly review it. Want to know why this is needed and what it can be used for. Kind of hard to judge otherwise.

Knowing what I know about the project you guys are working on, I can make a good guess: you want to show users that an app exists, and that they cannot presently install it, instead of letting the install attempt "silently" hang.

(@arthyn's suggestion is indeed better btw: in the blacklist case the crew is innumerably large. But not clear to me we need to expose this in the first place.)

Also, regardless of anything else: should've just put the =crew:clay inside the $treaty. Current approach forces you to essentially rename the core concept of this app...

#164 adds version negotiation to treaty, but: 1) that's intermingled with userspace permissions work, and 2) there is a version negotiation library in development that will hopefully deprecate all other, more ad-hoc implementations.
Maybe that will alleviate some of the upgrade problems you saw, since it shouldn't depend on mark conversions anymore.

@ryjm
Copy link
Author

ryjm commented Sep 7, 2023

In some ways this fixes a bug - if you :treaty|publish a desk and then make it private, it will still show up as installable but just hang. So treaty needs some way of communicating whether you have permission to install the desk or not, irrespective of what the use case for future permission schemes are.

Also, regardless of anything else: should've just put the =crew:clay inside the $treaty. Current approach forces you to essentially rename the core concept of this app...

Not entirely true... treaties are still the thing that goes over the network. I separated out crew from treaty because we don't want to expose the set of ships that are whitelisted/blacklisted. instead the publishing ship checks the crew list in the pact when a requesting ship asks, and then updates the signed flag so the requester knows it can't install it.

EDIT: Also pacts only matter to sovereign publishers - the treaty map state is unchanged.

@ryjm
Copy link
Author

ryjm commented Sep 7, 2023

(@arthyn's suggestion is indeed better btw: in the blacklist case the crew is innumerably large. But not clear to me we need to expose this in the first place.)

maybe you were looking at an old comment? I did fix that.

@Fang-
Copy link
Member

Fang- commented Sep 7, 2023

Oh, you'll have to forgive me for speedreading the diff then.

instead the publishing ship checks the crew list in the pact when a requesting ship asks, and then updates the signed flag so the requester knows it can't install it.

Except this is only done during +on-watch and not during subsequent updates (+give:so, +give:tr(?)).

If we want to go down the route of personalized responses, we should do it properly: let people subscribe on /whatever/treaties/path/~sampel, force that only ~sampel can subscribe on that path, and then when giving updates, iterate over all active subscribers to give them their personalized updates.

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.

4 participants