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: define access/confirm handler and use it in ucanto-test-utils registerSpaces + validate-email handler #530

Merged
merged 11 commits into from
Mar 14, 2023

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Mar 11, 2023

Previously a lot of logic to handle access/confirm was in the validate-email flow, since that is the most common place we'd receive that invocation (after clicking email sent by access/authorize handler).

However, this logic can be expressed as a ServiceMethod on invocation of access/confirm, and validate-email can call that.
This allows us to also self-issue access/confirm in some tests, e.g. in ucanto-test-utils registerSpaces and send it to our service to handle, which is only enabled when node env is TEST for now.

Benefits:

  • wherever we use ucanto-test-utils registerSpaces, we'll be accurately testing the access/confirm + provider/add flow (not old/deprecated voucher/redeem)

@gobengo gobengo requested a review from Gozala March 11, 2023 01:50
@gobengo gobengo requested a review from travis March 11, 2023 01:56
@gobengo gobengo changed the title define access/confirm handler and use it in ucanto-test-utils registerSpaces + validate-email handler feat: define access/confirm handler and use it in ucanto-test-utils registerSpaces + validate-email handler Mar 11, 2023
@gobengo gobengo self-assigned this Mar 11, 2023
@gobengo gobengo added this to the w3up phase 3 milestone Mar 11, 2023
Copy link
Contributor

@travis travis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly just clarifying questions, but a couple of requests that would be good to address

// Send delegations to the client through a websocket
await env.models.validations.putSession(authorization, agent.did())
if (confirmResult.error) {
throw new Error('error confirming')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget, does passing cause to this do anything? is confirmResult.error swallowed if we don't pass it along or does it get printed/preserved somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does passing cause to this do anything?

Which cause?

is confirmResult.error swallowed if we don't pass it along

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoop sorry, meant to say "does passing this along as cause do anything?"

like:

throw new Error('error confirming', {cause: confirmError.result})

I think I've seen this used in one or another of our codebases but hadn't confirmed it actually does anything useful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a new js feature. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
And yeah I should probably use it here

Copy link
Contributor Author

@gobengo gobengo Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travis one thing to keep in mind is that the version of typescript in use needs to have updated builtin Error types to allow passing error.cause. I think most of our repos are upgraded to it now, but some may not be (yet)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thx!

audience = service,
lifetimeInSeconds = 60 * 15
) {
const registerSpace = await Access.confirm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be named registerAccount ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty b4fc3c7

audience,
with: service.did(),
lifetimeInSeconds,
// We link to the authorization request so that this attestation can
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We link to the authorization request

what's this referring to? I see the issuer set to be the account, the audience to the agent, and the attestations set to full open, but can't figure out in what sense this is a "link to the authorization request"

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 don't think it makes sense either. It's from copypasta. Good catch. i removed it

await ctx.models.delegations.putMany(delegation, attestation)

const authorization = delegationsToString([delegation, attestation])
// Send delegations to the client through a websocket
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is not accurate - all this does is stash the delegation in kv. can probably just drop this comment or rephrase to "send delegations to validations model where it will be made available to claimers"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same as the comment was before.
I agree that would be a more accurate comment. Open to accepting a suggestion here, or anyone changing it as a followup.

with: env.signer.did(),
nb: { proof: delegation.cid },
expiration: Infinity,
const confirm = provide(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this all going to be backwards compatible with the current implementation? backwards compatibility isn't necessarily a hard requirement, but I am interested in tracking what's likely to break for older clients

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 understanding is yes, it will be. All the old tests pass.
(this whole path is only exercised when clicking confirmation email sent by access/authorize, so it will be backward compatible with the not-very-many implementations of the client side of that, e.g. the observable I made)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH right, the separate codepath for this should make it all work fine, cool!

Comment on lines +53 to +56
// only needed in tests
if (ctx.config.ENV !== 'test') {
throw new Error(`access/confirm is disabled`)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not limit this to tests only.

packages/capabilities/src/types.ts Show resolved Hide resolved
@gobengo gobengo requested a review from travis March 14, 2023 18:26
@gobengo gobengo merged commit b1bbc90 into main Mar 14, 2023
@gobengo gobengo deleted the accessconfirmregisterspaces branch March 14, 2023 18:31
gobengo added a commit that referenced this pull request Mar 14, 2023
… registerSpaces + validate-email handler (#530)

Previously a lot of logic to handle `access/confirm` was in the
`validate-email` flow, since that is the most common place we'd receive
that invocation (after clicking email sent by `access/authorize`
handler).

However, this logic can be expressed as a `ServiceMethod` on invocation
of `access/confirm`, and validate-email can call that.
This allows us to also self-issue `access/confirm` in some tests, e.g.
in ucanto-test-utils `registerSpaces` and send it to our service to
handle, which is only enabled when node env is TEST for now.

Benefits:
* wherever we use ucanto-test-utils `registerSpaces`, we'll be
accurately testing the `access/confirm` + `provider/add` flow (not
old/deprecated `voucher/redeem`)

---------

Co-authored-by: Travis Vachon <travis@dag.house>
travis pushed a commit that referenced this pull request Mar 20, 2023
🤖 I have created a release *beep* *boop*
---


##
[4.0.0](capabilities-v3.2.0...capabilities-v4.0.0)
(2023-03-17)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))

### Features

* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([b1bbc90](b1bbc90))
* implement new account-based multi-device flow
([#433](#433))
([1ddc6a0](1ddc6a0))
* provision provider type is now the DID of the w3s service
([#528](#528))
([6a72855](6a72855))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
travis pushed a commit that referenced this pull request Mar 20, 2023
🤖 I have created a release *beep* *boop*
---


##
[11.0.0-rc.0](access-v10.0.0...access-v11.0.0-rc.0)
(2023-03-20)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))

### Features

* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([b1bbc90](b1bbc90))
* implement new account-based multi-device flow
([#433](#433))
([1ddc6a0](1ddc6a0))
* move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩
fast ⏩ ([#449](#449))
([02d7552](02d7552))
* space/info will not error for spaces that have had storage provider
added via provider/add
([#510](#510))
([ea4e872](ea4e872))


### Miscellaneous Chores

* **access-client:** release 11.0.0-rc.0
([#573](#573))
([be4386d](be4386d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
alanshaw pushed a commit that referenced this pull request Mar 23, 2023
🤖 I have created a release *beep* *boop*
---


##
[5.0.0](access-api-v4.11.0...access-api-v5.0.0)
(2023-03-23)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))
* upgrade capabilities to latest ucanto
([#463](#463))

### Features

* access-api handles provider/add invocations
([#462](#462))
([5fb56f7](5fb56f7))
* access-api serves access/claim invocations
([#456](#456))
([baacf35](baacf35))
* access/authorize confirmation email click results in a delegation back
to the issuer did:key so that access/claim works
([#460](#460))
([a466a7d](a466a7d))
* allow multiple providers
([#595](#595))
([96c5a2e](96c5a2e))
* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([b1bbc90](b1bbc90))
* handle access/delegate invocations without error
([#427](#427))
([4f0bd1c](4f0bd1c))
* if POST /validate-email?mode=authorize catches error w/ too big qr
code ([#516](#516))
([d0df525](d0df525))
* implement new account-based multi-device flow
([#433](#433))
([1ddc6a0](1ddc6a0))
* includes proofs chains in the delegated authorization chain
([#467](#467))
([5144293](5144293))
* move access-api delegation bytes out of d1 and into r2
([#578](#578))
([4510c9a](4510c9a))
* move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩
fast ⏩ ([#449](#449))
([02d7552](02d7552))
* provision provider type is now the DID of the w3s service
([#528](#528))
([6a72855](6a72855))
* space/info will not error for spaces that have had storage provider
added via provider/add
([#510](#510))
([ea4e872](ea4e872))
* upgrade capabilities to latest ucanto
([#463](#463))
([2d786ee](2d786ee))
* upgrade to new ucanto
([#498](#498))
([dcb41a9](dcb41a9))
* write invocations and receipts into ucan log
([#592](#592))
([754bf52](754bf52))


### Bug Fixes

* access/delegate checks hasStorageProvider(space) in a way that
provider/add allows access/delegate
([#483](#483))
([f4c640d](f4c640d))
* adjust migration 0005 to keep delegations table but create new used
delegations_v2
([#469](#469))
([a205ad1](a205ad1))
* adjust migration 0005 to not do a drop table and instead rename
delegations -&gt; delegations_old and create a new delegations
([#468](#468))
([6c8242d](6c8242d))
* allow injecting email
([#466](#466))
([e19847f](e19847f))
* DbDelegationsStorage#find throws UnexpectedDelegation w/ { row } if
failed bytesToDelegations
([#476](#476))
([a6dafcb](a6dafcb))
* DbProvisionsStorage putMany doesnt error on cid col conflict
([#517](#517))
([c1fea63](c1fea63))
* delegations model tries to handle if row.bytes is Array not Buffer
(e.g. cloudflare)
([#478](#478))
([030e7b7](030e7b7))


### Miscellaneous Chores

* **access-client:** release 11.0.0-rc.0
([#573](#573))
([be4386d](be4386d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo added a commit that referenced this pull request Apr 11, 2023
… registerSpaces + validate-email handler (#530)

Previously a lot of logic to handle `access/confirm` was in the
`validate-email` flow, since that is the most common place we'd receive
that invocation (after clicking email sent by `access/authorize`
handler).

However, this logic can be expressed as a `ServiceMethod` on invocation
of `access/confirm`, and validate-email can call that.
This allows us to also self-issue `access/confirm` in some tests, e.g.
in ucanto-test-utils `registerSpaces` and send it to our service to
handle, which is only enabled when node env is TEST for now.

Benefits:
* wherever we use ucanto-test-utils `registerSpaces`, we'll be
accurately testing the `access/confirm` + `provider/add` flow (not
old/deprecated `voucher/redeem`)

---------

Co-authored-by: Travis Vachon <travis@dag.house>
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[4.0.0](capabilities-v3.2.0...capabilities-v4.0.0)
(2023-03-17)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))

### Features

* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([a08b513](a08b513))
* implement new account-based multi-device flow
([#433](#433))
([6152e55](6152e55))
* provision provider type is now the DID of the w3s service
([#528](#528))
([4cd6cd9](4cd6cd9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[11.0.0-rc.0](access-v10.0.0...access-v11.0.0-rc.0)
(2023-03-20)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))

### Features

* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([a08b513](a08b513))
* implement new account-based multi-device flow
([#433](#433))
([6152e55](6152e55))
* move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩
fast ⏩ ([#449](#449))
([3868d97](3868d97))
* space/info will not error for spaces that have had storage provider
added via provider/add
([#510](#510))
([362024f](362024f))


### Miscellaneous Chores

* **access-client:** release 11.0.0-rc.0
([#573](#573))
([29daa02](29daa02))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[5.0.0](access-api-v4.11.0...access-api-v5.0.0)
(2023-03-23)


### ⚠ BREAKING CHANGES

* implement new account-based multi-device flow
([#433](#433))
* upgrade capabilities to latest ucanto
([#463](#463))

### Features

* access-api handles provider/add invocations
([#462](#462))
([46da0df](46da0df))
* access-api serves access/claim invocations
([#456](#456))
([2ec16e9](2ec16e9))
* access/authorize confirmation email click results in a delegation back
to the issuer did:key so that access/claim works
([#460](#460))
([fc62691](fc62691))
* allow multiple providers
([#595](#595))
([aba57b3](aba57b3))
* define `access/confirm` handler and use it in ucanto-test-utils
registerSpaces + validate-email handler
([#530](#530))
([a08b513](a08b513))
* handle access/delegate invocations without error
([#427](#427))
([db01d07](db01d07))
* if POST /validate-email?mode=authorize catches error w/ too big qr
code ([#516](#516))
([ab83b19](ab83b19))
* implement new account-based multi-device flow
([#433](#433))
([6152e55](6152e55))
* includes proofs chains in the delegated authorization chain
([#467](#467))
([743a72f](743a72f))
* move access-api delegation bytes out of d1 and into r2
([#578](#578))
([3029e4a](3029e4a))
* move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩
fast ⏩ ([#449](#449))
([3868d97](3868d97))
* provision provider type is now the DID of the w3s service
([#528](#528))
([4cd6cd9](4cd6cd9))
* space/info will not error for spaces that have had storage provider
added via provider/add
([#510](#510))
([362024f](362024f))
* upgrade capabilities to latest ucanto
([#463](#463))
([e375ae4](e375ae4))
* upgrade to new ucanto
([#498](#498))
([790750d](790750d))
* write invocations and receipts into ucan log
([#592](#592))
([d52a281](d52a281))


### Bug Fixes

* access/delegate checks hasStorageProvider(space) in a way that
provider/add allows access/delegate
([#483](#483))
([1d3d562](1d3d562))
* adjust migration 0005 to keep delegations table but create new used
delegations_v2
([#469](#469))
([d90825a](d90825a))
* adjust migration 0005 to not do a drop table and instead rename
delegations -&gt; delegations_old and create a new delegations
([#468](#468))
([89f2acd](89f2acd))
* allow injecting email
([#466](#466))
([b4b0173](b4b0173))
* DbDelegationsStorage#find throws UnexpectedDelegation w/ { row } if
failed bytesToDelegations
([#476](#476))
([660f773](660f773))
* DbProvisionsStorage putMany doesnt error on cid col conflict
([#517](#517))
([8c6dea8](8c6dea8))
* delegations model tries to handle if row.bytes is Array not Buffer
(e.g. cloudflare)
([#478](#478))
([02c0c28](02c0c28))


### Miscellaneous Chores

* **access-client:** release 11.0.0-rc.0
([#573](#573))
([29daa02](29daa02))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

3 participants