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

feat!: authorization and registration flows #1059

Merged
merged 26 commits into from
Nov 6, 2023
Merged

feat!: authorization and registration flows #1059

merged 26 commits into from
Nov 6, 2023

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Nov 3, 2023

Implements flows described in #945

High level overview

Accounts

This pull request starts differentiating between two kinds of authorizations

  1. Space authorization, allowing access to a space.
  2. Account authorization, allowing account management and space provisioning.

Most core functionality has been added in Account module which exposes following functionality

  • Account.login(client, email) - Request account level access though access/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 into Account 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 and access/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 send access/authorize request and return PendingAccessRequest 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 - sends access/claim request and returns GrantedAccess object that wraps received delegations. It also (just like Account) has .save() method which can be used to save delegations into client's store.

Space

I have added new Space module that has has Space.generate() function to generate new spaces, Space.fromDelegation, Space.fromMnemonic and more. General idea here also is that client.createSpace() will give you OwnedSpace 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 from access to ucan 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 web3-storage/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

Copy link
Contributor Author

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, {
Copy link
Contributor

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.

Copy link
Contributor Author

@Gozala Gozala Nov 4, 2023

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

image

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.

* @param {API.Agent} source.agent
* @param {API.DID} source.audience
* @param {API.ProviderDID} source.provider
* @param {number} source.expiration
Copy link
Contributor

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?)

Copy link
Contributor

@gobengo gobengo left a 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.

@Gozala Gozala requested a review from gobengo November 4, 2023 08:54
Copy link
Member

@alanshaw alanshaw left a 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 Show resolved Hide resolved
delegation.facts.some((fact) => `${fact['access/request']}` === `${request}`)

/**
* Maps access object that follows new UCAN spec inspired by recap style layout
Copy link
Member

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?

Copy link
Contributor Author

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() {
Copy link
Member

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?

Copy link
Contributor Author

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.

packages/access-client/src/agent.js Outdated Show resolved Hide resolved
packages/access-client/src/space.js Outdated Show resolved Hide resolved
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
@Gozala Gozala merged commit b1f860b into main Nov 6, 2023
14 checks passed
@Gozala Gozala deleted the feat/auth branch November 6, 2023 18:55
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.

None yet

3 participants