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
[NEXT] Adapt to UCAN 0.9 #532
Conversation
|
Looking the linking API: // authority
const program = await odd.program(config, "authority")
program.access.provide()
program.access.on("query", ...)
// delegate
const program = await odd.program(config, "delegate", {
request: [ odd.access.fileSystem("read", path) ]
})
program.access.request()
program.access.on("challenge", ...)Love how this is coming along, "request... access" reads great to me. ❤️ One question here is whether or not this supports a Would that work with this API? Does putting the request into the program initialization make it difficult to declare who we are requesting access from? |
This hierarchy sounds great for supporting interoperability. 💯
Nice! Like the "connected" naming.
Is there a risk for race conditions here? Say a user registers a name, and a second user comes along and wants the same name. If we are waiting for DNS to propagate, could the username check fail? This might be alright, as I imagine registration itself would fail.
Yes, bon voyage 👋 . When we have a rough timeline for next, we should announce this in Discord to give folks a heads up. |
@bgins Yeah good point. I don't know if you saw the recent discussion about that on Discord. But I'd like some well-defined use cases for this, otherwise I don't know really how to shape the API. But maybe we opt to make it generic instead? I'd like to avoid roping the initialisation into this, but not sure how. With these changes when you run your program in There's something here about specific use cases. We assume there's an "authority" which requires full read & write access to the file system and a "delegate" which could operate with a very small subset of that (eg. only read portion of file system). But is that always the case? 🤷♂️ Maybe we could do something like this instead? const program = await odd.program(config)
// authority
await program.account.register()
await program.account.login()
await program.account.isConnected()
// NOTE: Could check if UCAN has been delegated with full write access and if we can update the data root.
// Also should potentially check if the configured account system has all the capabilities it needs.
// delegate
await program.access.request({
from: ...,
query: [ ... ]
})
await program.access.isGranted(query)Thoughts?
Blaine is setting up some DNS magic which will make it look up the values in the database directly, so propagation will be faster I assume. That said, I think it'll be fine too, the registration itself would indeed fail. |
Yeah, it would be nice to have use cases. Not sure what they are, and I can see how this makes it more challenging to design an API.
I'm agreed that there is a fundamental difference between an authority and a delegate, and I think it's a good design to only expose what one or the other needs.
Are there cases where a delegate could start with a local-only file system that later gets merged into a file system granted by an authority? If so, I think The way these would function differently is the method of requesting authority. The authority gets it by registering an account, and the delegate gets it from the authority. const program = await odd.program(config)
// authority
await program.account.register()
await program.account.login()
await program.account.isConnected()
// NOTE: Could check if UCAN has been delegated with full write access and if we can update the data root.
// Also should potentially check if the configured account system has all the capabilities it needs.
// delegate
await program.access.request({
from: ...,
query: [ ... ]
})
await program.access.isGranted(query)Yes, I think this has potential. It would make the request for access more flexible. It might be possible to retain the distinction between authority and delegate? An authority will never need to request access, and a delegate won't need to register or login. |
|
I was discussing a use cases for multiple levels of subdelegation with @QuinnWilton today, and I think we have a good example. Suppose we have the following apps:
Diffuse and Notify request access from the auth lobby to access MetaFixer is a secondary app that is accessible from Diffuse and Notify. It doesn't do much on its own, but on requesting access from Diffuse or Notify, it will look up metadata and store it with your music. Diffuse and Notify have a button that starts linking directly with MetaFixer, subdelegating access to the music directories they have access to. Alternatively, the user could be redirected to the MetaFixer where they request permission from the auth lobby. The disadvantage to this approach is the user loses context when they request access from the auth lobby. Also, MetaFixer could request access to directories that it doesn't need. The user may not be aware that MetaFixer is asking for more, whereas requesting from Diffuse or Notify guarantees MetaFixer can only request what it needs. Note also that MetaFixer will get access on an as needed basis. A user can start by applying metadata to music in |
|
👋 I just read through this thread too, and I have one more thought on a possible use-case for sub-delegation:
I actually don't think this is necessarily always the case, even if in the current world it is! Baked into this sentence is the assumption that WNFS is the only service for which an authority may delegate access to, but there's a possible future where apps built using OddSDK may integrate with other UCAN enabled services too, for things like message buses, email, or SMS. If these services are third-party, then the Fission auth lobby isn't likely to serve as an authority for all of them, and so some applications may require delegations from multiple different authorities. This means that it actually becomes possible for individual applications to hold more capabilities than any individual authority, and so the ability to sub-delegate from that rights-amplified application reduces the friction involved in sharing those capabilities with other applications. In practice, this might show up as something like a Heroku or AWS style dashboard that accumulates capabilities from disparate, possibly third-party services, and can then further attenuate those capabilities to other applications. Anyway, this is just me spitballing about another instance where sub-delegation shows up, and I understand that this isn't how things work today, so this isn't me suggesting that it's a use-case to be targeted by you in this refactor. |
Thanks you two! And for picking an example I can relate to ❤️ Sounds like a great use case.
Makes sense yeah. I'm actually getting very close to this, so I'll start working in that direction for the API 👍 |
I think this information could still be useful to developers for understanding authorization flows. They may want to log the agent DID to track which agent they are working in, and we might want to show this information in the browser extension. We will eventually extend the browser extension to display UCANs, so this information will be good context for that as well.
Yep, agreed it will be better to require developers to handle this explicitly. 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have looked through all the components. A bit more to go, but getting there. 😃
Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com>
Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com>
Co-authored-by: Brian Ginsburg <7957636+bgins@users.noreply.github.com>
|
@bgins Added back the DID shorthand methods, except for the sharing one, because that still needs to be figured out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM left a few comments but nothing blocking we should merge asap and work of small PRs
src/common/event-emitter.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should look into using https://github.com/sindresorhus/emittery for a more robust event emitter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be worth adding a dependency if we don't need the additional functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some additional context. We considered using Emittery when we initially implemented the event emitter.
Our thought process was: This is simple enough. We can implement it ourselves and save on bundle size while reducing the risk of supply chain attacks by having one less dependency.
Our plan was to extend it as needed, but at a certain point it may not be worth our time and effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that approach with a few exceptions one being event emitter because they are very very tricky and are prone to memory leaks and uncaught exceptions.
emittery defends against a bunch of weird stuff ppl do inside handlers and adds a couple of nice to haves, it net positive IMO.
https://pkg-size.dev/emittery its small and has 0 deps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be down for this change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's change it. One request, we should make sure the type information is not degraded making the switch.
src/common/fission.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i add did dns resolver to iso-did for this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. Would it allow you to customise how the DNS resolving is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would do DoH by default with round robin over like 5 options and allow for optional custom resolver
| canRegister: (formValues: Record<string, string>) => Promise< | ||
| { canRegister: true } | { canRegister: false; reason: string } | ||
| > | ||
|
|
||
| /** | ||
| * How to register an account with this account system. | ||
| */ | ||
| register: (formValues: Record<string, string>, identifierUcan: Ucan) => Promise< | ||
| { registered: true; ucans: Ucan[] } | { registered: false; reason: string } | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we normalize the return types to a generic Result type { result?: T, error: Error }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugomrdias a bit of discussion around this here: #532 (comment)
| const response = await fetch(Fission.apiUrl(endpoints, "/user"), { | ||
| method: "PUT", | ||
| headers: { | ||
| authorization: `Bearer ${token}`, | ||
| "content-type": "application/json", | ||
| }, | ||
| body: JSON.stringify(formValues), | ||
| }) | ||
|
|
||
| if (response.status < 300) { | ||
| return { | ||
| registered: true, | ||
| ucans: [ | ||
| // TODO: This should be done by the server | ||
| await Ucan.build({ | ||
| audience: identifierUcan.payload.iss, | ||
| issuer: await Ucan.keyPair(dependencies.agent), | ||
| proofs: [Ucan.encode(identifierUcan)], | ||
|
|
||
| facts: [{ username }], | ||
| }), | ||
| ], | ||
| // TODO: We need some UCANs here. We should get capabilities from the Fission server. | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| registered: false, | ||
| reason: `Server error: ${response.statusText}`, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should extract code like this to a fission server client and also export from that odd sdk components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea yeah. Not sure if we'd ever make use of that outside of the SDK though, since the CLI will be written in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it allows for easier and faster iteration on server logic without messing with the SDK and we extract Fission related components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can already iterate on that without messing with the SDK though, thanks to component system:
odd.program({
account: fissionAccountComponentWithAdjustments
})| }).then((response: Response) => { | ||
| method: "PUT", | ||
| }).then((response: Response): { updated: true } | { updated: false; reason: string } => { | ||
| if (response.status < 300) dependencies.manners.log("🪴 DNSLink updated:", cid) | ||
| else dependencies.manners.log("🔥 Failed to update DNSLink for:", cid) | ||
| return { success: response.status < 300 } | ||
|
|
||
| return response.ok ? { updated: true } : { updated: false, reason: response.statusText } | ||
| }).catch(err => { | ||
| dependencies.manners.log("🔥 Failed to update DNSLink for:", cid) | ||
| console.error(err) | ||
| return { success: false } | ||
|
|
||
| return { updated: false, reason: err } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these return types should be normalised IMO and should return something other than a boolean, either cid or an Error. Especially reason should only be of one single type.
| /** | ||
| * This goes hand in hand with the DID `keyTypes` record from the crypto component. | ||
| */ | ||
| keyAlgorithm: () => Promise<KeyType> | ||
|
|
||
| /** | ||
| * The JWT algorithm string for agent UCANs. | ||
| */ | ||
| ucanAlgorithm: () => Promise<SignatureAlgorithm> | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these async ? they should be pretty much constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keeping all the options open, doesn't hurt since all the functions that depend on these are async anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its creating micro tasks in a bunch of places to return a constant string
| /** | ||
| * DID associated with this agent. | ||
| */ | ||
| did: () => Promise<string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this async ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as with the agent methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why didnt you use the RSA Signer from iso-signature here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, totally forgot about that. I wrote this before you released that, will change soon.
| return { | ||
| did: async () => DIDKey.fromPublicKey("RSA", exportedKey).toString(), | ||
| sign: async data => WebCryptoAPIAgent.sign(data, signingKey), | ||
| ucanAlgorithm: async () => "RS256", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here why not just use Signer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

This PR aims to bring the UCAN 0.9 to the SDK. The prototype version of UCAN we currently have is mixed into a lot aspects of the SDK. Because of this I have disabled and removed a lot functionality, such as most importantly, device linking, but more on that later.
After merging this, the
nextbranch will be able to make somewhat functioning apps again. You can't make a Fission account yet though. You can load the file system without having an account and play around with it, like so:Goals of this PR
UCAN
Ideally we'd upgrade directly to UCAN v0.10. But we can't do that just yet (waiting on libs to upgrade), so I opted for 0.9 for now. That said, it should be quite straightforward to upgrade to the next version.
File System
The
nextbranch is already using rs-wnfs primarily, but you can't make an app with it yet. This PR builds further on that and connects various pieces. Most importantly here is the private node mounting system. Which is where you take a private ref to load a private node using rs-wnfs. These secrets need to be passed from device to device in order to gain access to various parts of the private file system.Linking
The SDK also has this delegation system (auth lobby). We've gradually abstracted this lobby system more and more, and this PR takes another step further. Because device linking and app-to-app linking is so similar, we've decided to merge the APIs and the channel over which the UCANs and secrets are transferred.
I've completed the initial API surface which you can see in various places, but it's not finished just yet. The idea is to do this:
This'll be implemented using two components: authority & channel
Other things to check out:
src/authority/query.tsAccounts & Identifiers
The account system and identity layer have also been abstracted. We currently have usernames mixed in everywhere, this PR gets rid of that. Also very important, @hugomrdias had the great idea to always delegate from the identifier to the agent, which this PR implements. I'm using the naming "identifier" here instead of "identity" because that's @bmann his preferred naming which I agree with. Humans have multiple identifiers, not identities.
This makes implementing various identifier systems much easier. There will always be a delegation from the identifier to agent, and the agent delegates to the account system. Account and identifier systems can be implemented using their relevant components (account & identifier). The first account system to be implemented will be the new Fission server.
Registering an account has changed a bit because of the abstraction:
If either
okis false, there will bereasonattached to it so you know why it failed.You can also add additional methods to the implementation that should be made available on the
program.accountobject. This is called theannex. So for example, the fission account implementation will most likely have a method to request a code via email which is required for the registration.Depot
I've added a
flushmethod to theDepotcomponent. This is part of separating the data from the account layer. This should make it possible to for example use Web3Storage as the file system block storage (or in addition to Fission's storage).The way this works is, you have two components that are called when a mutation is made in the file system (after a short debounce). You have the account system which is responsible for the data root and the depot, which is responsible for the IPLD block storage. Whenever the data root is going to be updated,
flushis called. So you could use this to boot up your IPFS node in the browser, setting up a bitswap connection. Or to gather the changed IPLD blocks, package them up in a CAR file and push them to Web3Storage.Cabinet
I've renamed the UCAN repository to the "cabinet". That is where your "ucan tickets" and access keys (file system secrets) are stored. The idea here is that this would be the place where you go look to check if you have access to a particular resource, be it related to your account or your file system.
New flow
With all these changes, the flow to work with a program has changed to:
From here you'd link a device or provide partial access to another entity (as mentioned above).
Smaller Changes
program.access.isGranted()commoncode.debuganddebuggingconfig params have been merged.To do
In this PR
loginbehaviour/implementationindexIn other PRs
Things that are missing, need to re-implemented or still need to be fixed: