-
Notifications
You must be signed in to change notification settings - Fork 26
feat!: authorization and registration flows #1059
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
Conversation
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.
Had to change so it could be run in browsers
| it('should set current space', async function () { | ||
| const agent = await Agent.create() | ||
| const space = await agent.createSpace('test') | ||
| const authorization = await space.createAuthorization(agent, { |
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 wouldn't mind if we left it so that Agent#createSpace automatically sets up authz, importing the space, and setting current, which would be a lot more back compat with what we've had. Not sure what's best here, but I'm worried that changing the behavior of createSpace too much might lead to headaches for end-users. I'd be inclined to try to keep all the current side effects for the same method (if easy), and only introduce new APIs/sideEffects in new methods that end-users can opt into over time.
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 think that was a really bad design choice because it effectively creates a space that is not usable and throws away the key so it never can be made usable. New API intentionally does not save anything to the agent so you have to arrange recovery beforehand.
I think doing breaking changes before we actually launch is probably better than shipping a footgun for the users, especially given that only users right now are us
And jchris who's been running into this problem and keeps asking us to fix it.
If you insist I will create another method and leave createSpace alone, but I think it would be a mistake given that our API is not yet stable, has issues and it's essentially fixing our apps.
packages/access-client/src/access.js
Outdated
| * @param {API.Agent} source.agent | ||
| * @param {API.DID} source.audience | ||
| * @param {API.ProviderDID} source.provider | ||
| * @param {number} source.expiration |
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.
what kind of number? e.g. is it number of second or milliseconds since epoch? something else?
(should it be a Date?)
gobengo
left a comment
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.
Looks good to me so far. I left a few comments above.
It's definitely worth changing at least the part I commented on where there are still some registerSpace stuff commented out.
alanshaw
left a comment
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 is great, it fits really nicely and seems pretty easy to understand. I've left some non-blocking suggestions.
We need a docs update for w3up-client but we can handle separately...
packages/access-client/src/access.js
Outdated
| delegation.facts.some((fact) => `${fact['access/request']}` === `${request}`) | ||
|
|
||
| /** | ||
| * Maps access object that follows new UCAN spec inspired by recap style layout |
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.
What is recap style - can you put a link?
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 think this is it https://github.com/spruceid/EIPs/blob/master/EIPS/eip-5573.md, but it's not worth mentioning a recap at all, it's more of the latest UCAN specs https://github.com/ucan-wg/spec#251-examples
| * | ||
| * @returns {Promise<API.Result<API.Delegation[], API.InvocationError|API.AccessClaimFailure|RequestExpired>>} | ||
| */ | ||
| async poll() { |
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.
How should this class be used? Should I call poll or claim on instances I receive? The docs above say I can use this to poll for the requested delegation but I think I would need to call claim not poll for that behaviour?
It seems back to front to me that poll sends a single request, but claim sends multiple requests. Perhaps poll should be a private method?
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.
My thinking was unless you do want to poll manually you could call .claim() which will keep polling until it's succeeds or fails. But if you have a reason to poll yourself you could use this. I'll add doc comment to this effect.
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Implements flows described in #945
High level overview
Accounts
This pull request starts differentiating between two kinds of authorizations
Most core functionality has been added in
Accountmodule which exposes following functionalityAccount.login(client, email)- Request account level access thoughaccess/authorizeand awaits until access is granted (through email link) or request expires.It returns an
Accountobject which is just a view over UCAN delegations and an agent. That is convenience wrapper allowing us to add e.g..provision(space)method (for provisioning spaces) and.save()method that will save account proofs into agent store.Account.list()- Lists all account (sessions) client has in storage. It essentially goes over proofs and wraps ones that are "account authorization"s intoAccountobjects.Above functionality should help us answer question whether user has already logged-in or not ? Previously we had no good way to answer such questions.
Access
This fixes issues around authorization flows, where requests would succeed but access was not gained. Issue was caused by imperfect heuristics that tried to match received delegation with a requested authorization.
To address this problem
access/authorizeandaccess/confirmcapabilities and their handler had been extended to capture request CIDs. This allows client polling for delegations to detect whether received delegation is corresponds to the request more precisely.I have added new
Accessmodule that attempts to do the client side work without much configurability.Access.request- will sendaccess/authorizerequest and returnPendingAccessRequestobject, which in turn could be used to poll for the response via.claim()method..claim()internally uses.poll()until it gets the delegation, session expires or request is aborted.Access.claim- sendsaccess/claimrequest and returnsGrantedAccessobject that wraps received delegations. It also (just likeAccount) has.save()method which can be used to save delegations into client's store.Space
I have added new
Spacemodule that has hasSpace.generate()function to generate new spaces,Space.fromDelegation,Space.fromMnemonicand more. General idea here also is thatclient.createSpace()will give youOwnedSpaceobject that has bunch of methods like.toMnemonic,.createAuthorization,.createRecoveryetc...Notion of not registered spaces had been removed. If you create a space you have an object, you can save it in your client using
client.addSpace(await space. createAuthorization(agent)).Attestation
I have moved
ucan/attestcapability definition fromaccesstoucanas former place was confusing and made no sense.The account.test.js attempts to capture authorization / provisioning flows from the event diagram.
There is one unfortunate problem right now, that is new spaces added will not show in other devices unless you re-authorize. Fixing that requires storacha/ucanto#326 and some more work here. I think it's best to do it as a followup.
I had to rebase because things upstream have changed, unfortunately I failed to do it cleanly so there is lot of strange changes which I don't exactly know how they happened. I'll try to remove undo them tomorrow