-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat!: authorization and registration flows #1059
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
?)
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.
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 <[email protected]>
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
Account
module which exposes following functionalityAccount.login(client, email)
- Request account level access thoughaccess/authorize
and awaits until access is granted (through email link) or request expires.It returns an
Account
object 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 intoAccount
objects.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/authorize
andaccess/confirm
capabilities 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
Access
module that attempts to do the client side work without much configurability.Access.request
- will sendaccess/authorize
request and returnPendingAccessRequest
object, 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/claim
request and returnsGrantedAccess
object 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
Space
module that has hasSpace.generate()
function to generate new spaces,Space.fromDelegation
,Space.fromMnemonic
and more. General idea here also is thatclient.createSpace()
will give youOwnedSpace
object that has bunch of methods like.toMnemonic
,.createAuthorization
,.createRecovery
etc...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/attest
capability definition fromaccess
toucan
as 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