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.

Sign up for GitHub

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

Jump to bottom

fix: access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate #483

Merged
merged 6 commits into from
Mar 7, 2023
Merged

fix: access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate #483

merged 6 commits into from
Mar 7, 2023

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Mar 7, 2023 •
edited
Loading

Motivation:

This makes it so that access/delegate will detect that a storage provider has been registered with a space via provider/add. Previously it would only allow spaces who had completed a voucher/redeem flow. Now it will support both. In the long run we will disable the voucher/redeem part.

@gobengo gobengo requested a review from Gozala March 7, 2023 00:32
@gobengo gobengo changed the title access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate fix: access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate Mar 7, 2023
@gobengo gobengo force-pushed the access-delegate-hasstorageprovider branch from c390366 to dc0eb51 Compare March 7, 2023 00:37
@gobengo gobengo temporarily deployed to dev March 7, 2023 00:38 — with GitHub Actions Inactive
@gobengo gobengo requested review from alanshaw and travis March 7, 2023 00:45
@gobengo gobengo self-assigned this Mar 7, 2023
@gobengo gobengo mentioned this pull request Mar 7, 2023
Closed
7 tasks
Gozala
Gozala requested changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Changes look good, but tests seem off.

packages/access-api/test/provider-add.test.js Outdated
},
{
can: 'access/delegate',
with: space.did(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Issuer should not be able to delegate this capability, because they do not own or had been delegated access to space.

packages/access-api/test/provider-add.test.js Show resolved Hide resolved
packages/access-api/test/provider-add.test.js Outdated
},
},
proofs: [
// space says issuer can provider/add with this account
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... But space should have no authority over the account here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 good call
90afea4

packages/access-api/test/provider-add.test.js Show resolved Hide resolved
@heyjay44 heyjay44 added this to the w3up phase 3 milestone Mar 7, 2023
Gozala
Gozala reviewed Mar 7, 2023
View reviewed changes
packages/access-api/test/provider-add.test.js Outdated
const accountDid = /** @type {const} */ ('did:mailto:example.com:foo')
const serviceSessionAttest = await ucanto.delegate({
issuer: service,
audience: issuer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Swapped names here are pretty confusing btw

Copy link
Contributor Author

@gobengo gobengo Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. 6d31d28

(I want to rename the InvokeTester#issuer to tester.agent or something too, but will do in standalone PR)

@gobengo gobengo temporarily deployed to dev March 7, 2023 22:04 — with GitHub Actions Inactive
@gobengo gobengo requested a review from Gozala March 7, 2023 22:07
@gobengo gobengo temporarily deployed to dev March 7, 2023 22:08 — with GitHub Actions Inactive
@gobengo gobengo temporarily deployed to dev March 7, 2023 22:16 — with GitHub Actions Inactive
Gozala
Gozala requested changes Mar 7, 2023
View reviewed changes
packages/access-api/test/provider-add.test.js
capabilities: [
{
with: 'ucan:*',
can: '*',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you could keep original capability set here, not implying you should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted. I considered that but did it this way on purpose

packages/access-api/test/provider-add.test.js
can: 'access/delegate',
with: space.did(),
nb: {
delegations: {},
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 supposed to be empty ? because if it is you're not delegating anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the specific delegation isn't important. Just want to show that the access/delegate invocation is accepted sans error

packages/access-api/test/provider-add.test.js
Comment on lines +173 to +183
// space says agent can access/delegate with space
await ucanto.delegate({
issuer: space,
audience: agent,
capabilities: [
{
can: 'access/delegate',
with: space.did(),
},
],
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used by anyhing as far as I can tell. And sessionProofs should be enough to call access/delegate with space.did(), so motivation here is unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would session proofs be enough to act on with=space.did()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this proof the test fails because agent is now allowed to access/delegate with space

Copy link
Contributor Author

@gobengo gobengo Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we spoke in person and as a result I realized that the sessionProofs weren't needed since we have delegation directly from space to agent, and we removed them 940d24c

@gobengo gobengo temporarily deployed to dev March 7, 2023 22:59 — with GitHub Actions Inactive
@gobengo gobengo requested a review from Gozala March 7, 2023 23:00
@gobengo
Copy link
Contributor Author

gobengo commented Mar 7, 2023

we addressed last feedback in person and got to a place of mutual understanding that this is a useful test for the original goal: to ensure that access/delegate can be invoked on a space that has had a storage provider registered via provider/add

@gobengo gobengo merged commit f4c640d into main Mar 7, 2023
@gobengo gobengo deleted the access-delegate-hasstorageprovider branch March 7, 2023 23:04
@it-dag-house it-dag-house mentioned this pull request Mar 7, 2023
Closed
This was referenced Mar 21, 2023
Closed
Merged
alanshaw pushed a commit that referenced this pull request Mar 23, 2023
@it-dag-house
chore(main): release access-api 5.0.0 (#588)
c0fb10a 
🤖 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 -> 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).
@it-dag-house it-dag-house mentioned this pull request Mar 23, 2023
Closed
gobengo added a commit that referenced this pull request Apr 11, 2023
@gobengo
fix: access/delegate checks hasStorageProvider(space) in a way that p…
1d3d562 
…rovider/add allows access/delegate (#483)

Motivation:
* #393
*
[discovered](https://filecoinproject.slack.com/archives/C02BZPRS9HP/p1678146419356999?thread_ts=1678146205.801039&cid=C02BZPRS9HP)
while testing https://observablehq.com/d/95bfec64fbb3c2d1@421

This makes it so that `access/delegate` will detect that a storage
provider has been registered with a space via `provider/add`. Previously
it would only allow spaces who had completed a `voucher/redeem` flow.
Now it will support both. In the long run we will disable the
`voucher/redeem` part.
gobengo pushed a commit that referenced this pull request Apr 11, 2023
@it-dag-house
chore(main): release access-api 5.0.0 (#588)
1230e7e 
🤖 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 -> 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).
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
@travis
feat: use new faster auth function (storacha#483)
372f249 
* update `@web3-storage/access` dep to make the minimum version the one
that has this new auth function
* use new polling-based auth function - auth feels muuuuuch faster in
local testing
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
@github-actions
chore(main): release keyring-core 4.1.0 (storacha#486)
0e34c3c 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@keyring-core-v4.0.0...keyring-core-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([372f249](storacha/w3ui@372f249))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
@github-actions @travis
chore(main): release react-keyring 5.1.0 (storacha#487)
3f63cdf 
🤖 I have created a release *beep* *boop*
---


##
[5.1.0](storacha/w3ui@react-keyring-v5.0.0...react-keyring-v5.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([372f249](storacha/w3ui@372f249))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
@github-actions @travis
chore(main): release vue-keyring 4.1.0 (storacha#484)
17be8b6 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@vue-keyring-v4.0.0...vue-keyring-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([372f249](storacha/w3ui@372f249))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
@github-actions @travis
chore(main): release solid-keyring 4.1.0 (storacha#485)
5d96b76 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@solid-keyring-v4.0.0...solid-keyring-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([372f249](storacha/w3ui@372f249))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 29, 2025
@travis
feat: use new faster auth function (storacha#483)
f100ec2 
* update `@web3-storage/access` dep to make the minimum version the one
that has this new auth function
* use new polling-based auth function - auth feels muuuuuch faster in
local testing
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 29, 2025
@github-actions
chore(main): release keyring-core 4.1.0 (storacha#486)
3f8a903 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@keyring-core-v4.0.0...keyring-core-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([f100ec2](storacha/w3ui@f100ec2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 29, 2025
@github-actions @travis
chore(main): release react-keyring 5.1.0 (storacha#487)
279d1b1 
🤖 I have created a release *beep* *boop*
---


##
[5.1.0](storacha/w3ui@react-keyring-v5.0.0...react-keyring-v5.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([f100ec2](storacha/w3ui@f100ec2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 29, 2025
@github-actions @travis
chore(main): release vue-keyring 4.1.0 (storacha#484)
2bebb04 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@vue-keyring-v4.0.0...vue-keyring-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([f100ec2](storacha/w3ui@f100ec2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 29, 2025
@github-actions @travis
chore(main): release solid-keyring 4.1.0 (storacha#485)
28f28be 
🤖 I have created a release *beep* *boop*
---


##
[4.1.0](storacha/w3ui@solid-keyring-v4.0.0...solid-keyring-v4.1.0)
(2023-03-30)


### Features

* use new faster auth function
([storacha#483](storacha/w3ui#483))
([f100ec2](storacha/w3ui@f100ec2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Travis Vachon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanshaw alanshaw Awaiting requested review from alanshaw

@travis travis Awaiting requested review from travis

@Gozala Gozala Awaiting requested review from Gozala

Assignees

@gobengo gobengo

Labels
None yet
Projects
None yet
Milestone
w3up phase 3
Development

Successfully merging this pull request may close these issues.
3 participants
@gobengo @Gozala @heyjay44