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: access-api handles provider/add invocations #462

Merged
merged 30 commits into from
Mar 4, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
1c5d196
add voucher/claim test that invokes the delegation in email
gobengo Feb 28, 2023
679e3c2
fix lint
gobengo Feb 28, 2023
b8849a4
access/authorize confirmation stores ucan/attest not ./update
gobengo Feb 28, 2023
d15d0db
add provider add capability
gobengo Feb 28, 2023
6da1479
add provider/add capability parser
gobengo Feb 28, 2023
25c4e0e
start createProviderAddHandler
gobengo Feb 28, 2023
da6adb6
start testing providerAdd handler
gobengo Feb 28, 2023
2f64d27
test provider/add as did:mailto
gobengo Feb 28, 2023
624b355
lint
gobengo Feb 28, 2023
bd05cbd
typo fix
gobengo Feb 28, 2023
fbb0ce4
provider-add.test asserts storage provider using new testing/space-st…
gobengo Feb 28, 2023
27acf17
provider/add stores StorageProvision (in-memory only)
gobengo Mar 1, 2023
0c62dbc
fix caps test after provider/add nb.consumer parser change
gobengo Mar 1, 2023
e537211
reenable important assertion even tho failing
gobengo Mar 1, 2023
d9768ad
StorageProvision backed by kysely
gobengo Mar 2, 2023
6c1d1be
Merge branch 'main' into 459-register-space-provider-add
gobengo Mar 3, 2023
49004e8
Merge branch 'main' into 459-register-space-provider-add
gobengo Mar 3, 2023
e403bb3
Merge branch 'main' into 459-register-space-provider-add
gobengo Mar 3, 2023
237d188
fix capabilities test
gobengo Mar 3, 2023
e62faaa
rename StorageProvisions model to just Provisions
gobengo Mar 3, 2023
2e90305
mv HelperTestContext interface into new access-api/test/helpers/types…
gobengo Mar 3, 2023
cd7dc57
fix: tests for provider add (#477)
Gozala Mar 3, 2023
184c55a
add comment to id col of migration 0006
gobengo Mar 3, 2023
754463d
rename issuer to sponsor in provisions table
gobengo Mar 3, 2023
4a48928
Merge branch 'main' into 459-register-space-provider-add
gobengo Mar 3, 2023
a199c11
provider capabilities dont use derive
gobengo Mar 3, 2023
6903891
remove provider/add ./update test that wasnt useful
gobengo Mar 4, 2023
40a4f16
provisions table has cid column as pk
gobengo Mar 4, 2023
c1e37f9
rename StoreProvisionCreation per review
gobengo Mar 4, 2023
9b9a14d
rename Provisions ProvisionsStorage
gobengo Mar 4, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
-- Migration number: 0006 2023-03-02T16:40:04.407Z

/*
goal: add a table to keep track of the storage provider for a space.
to enable https://github.com/web3-storage/w3protocol/issues/459
*/

CREATE TABLE
-- provision: the action of providing or supplying something for use
-- use case: representing the registration of a storage provider to a space
IF NOT EXISTS provisions (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In the past when I had to do more DB work using plural names for tables was considered a bad practice, stack overflow still seems to think the singular is a the way to go with bunch of reasons there.

I'm not going to get upset if we use plural, but might be worth considering singular names instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in general I don't mind singular at all, and maybe even slightly prefer it, but here I made it plural because delegations accounts spaces already was.

I don't have any objection to batch changing them to singular in one big other PR

id INTEGER NOT NULL PRIMARY KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding comment also what is the id here ? I'll probably discover as I read more of the pr, but comment here would be helpful

Copy link
Contributor Author

@gobengo gobengo Mar 3, 2023

Choose a reason for hiding this comment

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

It's meant to just be an auto id that other tables could join to.
I considered not even having it. wdyt about having it at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment. 184c55a

fwiw it will not be autoincrementing it will be a 64-bit ROWID https://www.sqlite.org/lang_createtable.html#rowid

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still add the cid field per our discussion in slack. We can drop it later if we decide it's not needed, but I think we'll want to tie these to the invocations that provisioned the provider. If we don't write those now it would be really hard to trace them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- DID of the actor that is consuming the provider. e.g. a space DID
consumer TEXT NOT NULL,
-- DID of the provider e.g. a storage provider
provider TEXT NOT NULL,
-- DID of the actor that issued this provision
issuer TEXT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this, is it supposed to be an account here ? If so I think issuer is not a great name. It also may not be an issuer it's the with field that matters. I think I would call it either account or subscriber but something like sponsor or patron could also probably work.

Suggested change
-- DID of the actor that issued this provision
issuer TEXT NOT NULL,
-- DID of the actor that is billed by the provider
subscriber TEXT NOT NULL,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gozala The issuer isn't necessarily going to be billed, e.g. if there are several provisions for the same space.

I don't want to attach any semantics to this other than 'the id of the thing that created this'. In ucan parlance that's issuer so that's why I used it. I think because when I used author you didn't like it, and I considered using attributedTo but wanted to avoid the camelCase.

It also may not be an issuer it's the with field that matters.

I can see how that might lead to confusion since the actor to attribute the capability to is not necessarily the final issuer.

I think I would call it either account or subscriber

The thing is I tried to make this table generic enough to support this field not necessarily being an account did nor a subscriber...

but something like sponsor or patron could also probably work.

I also considered sponsor so maybe we can agree there... I will update to use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to sponsor 754463d

inserted_at TEXT NOT NULL DEFAULT (strftime ('%Y-%m-%dT%H:%M:%fZ', 'now')),
updated_at TEXT NOT NULL DEFAULT (strftime ('%Y-%m-%dT%H:%M:%fZ', 'now'))
);
5 changes: 3 additions & 2 deletions packages/access-api/src/bindings.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ export interface RouteContext {
url: URL
email: Email
models: {
spaces: Spaces
validations: Validations
accounts: Accounts
delegations: Delegations
spaces: Spaces
storageProvisions: StorageProvisions
validations: Validations
}
uploadApi: ConnectionView
}
Expand Down
99 changes: 99 additions & 0 deletions packages/access-api/src/models/provisions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/* eslint-disable no-void */

/**
* @typedef {import("../types/provisions").StorageProvisions} StorageProvisions
*/

/**
* @param {Array<import("../types/provisions").StorageProvisionCreation>} storage
* @returns {StorageProvisions}
*/
export function createStorageProvisions(storage = []) {
/** @type {StorageProvisions['hasStorageProvider']} */
const hasStorageProvider = async (consumerId) => {
const hasRowWithSpace = storage.some(({ space }) => space === consumerId)
return hasRowWithSpace
}
/** @type {StorageProvisions['putMany']} */
const putMany = async (...items) => {
storage.push(...items)
}
/** @type {StorageProvisions['count']} */
const count = async () => {
return BigInt(storage.length)
}
return {
count,
putMany,
hasStorageProvider,
}
}

/**
* @typedef ProvsionsRow
* @property {string} consumer
* @property {string} provider
* @property {string} issuer - did of actor who did the provisioning
*/

/**
* @typedef {import("../types/database").Database<{ provisions: ProvsionsRow }>} ProvisionsDatabase
*/

/**
* StorageProvisions backed by a kyseli database (e.g. sqlite or cloudflare d1)
*/
export class DbStorageProvisions {
/** @type {ProvisionsDatabase} */
#db

/**
* @param {ProvisionsDatabase} db
*/
constructor(db) {
this.#db = db
this.tableNames = {
provisions: /** @type {const} */ ('provisions'),
}
void (/** @type {StorageProvisions} */ (this))
}

/** @type {StorageProvisions['count']} */
async count(...items) {
const { size } = await this.#db
.selectFrom(this.tableNames.provisions)
.select((e) => e.fn.count('provider').as('size'))
.executeTakeFirstOrThrow()
return BigInt(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it otherwise a string ? Can you plz add a comment explaining why this needs to be a BigInt.

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 made it a BigInt to match the interface. I used bigint in the interface for consistency with the count() method on delegations, which I made return a BigInt per @alanshaw review. #427 (comment)

}

/** @type {StorageProvisions['putMany']} */
async putMany(...items) {
if (items.length === 0) {
return
}
/** @type {ProvsionsRow[]} */
const rows = items.map((item) => {
return {
consumer: item.space,
provider: item.provider,
issuer: item.account,
}
})
await this.#db
.insertInto(this.tableNames.provisions)
.values(rows)
.executeTakeFirstOrThrow()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to have method that gives you providers you have on space in shape of { [provider]: [subscriber, ...subscriber[]] }. Which is more or less what I was asking to expose from the space/info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#480 (comment)

By subscriber I assume you mean the account DIDs?

Copy link
Contributor

Choose a reason for hiding this comment

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

By subscriber I assume you mean the account DIDs?

Yes

/** @type {StorageProvisions['hasStorageProvider']} */
async hasStorageProvider(consumerDid) {
const { provisions } = this.tableNames
const { size } = await this.#db
.selectFrom(provisions)
.select((e) => e.fn.count('provider').as('size'))
.where(`${provisions}.consumer`, '=', consumerDid)
.executeTakeFirstOrThrow()
return size > 0
}
}
35 changes: 32 additions & 3 deletions packages/access-api/src/service/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as ucanto from '@ucanto/core'
import * as Ucanto from '@ucanto/interface'
import * as Server from '@ucanto/server'
import { Failure } from '@ucanto/server'
import * as Space from '@web3-storage/capabilities/space'
Expand All @@ -13,6 +14,7 @@ import * as uploadApi from './upload-api-proxy.js'
import { accessAuthorizeProvider } from './access-authorize.js'
import { accessDelegateProvider } from './access-delegate.js'
import { accessClaimProvider } from './access-claim.js'
import { providerAddProvider } from './provider-add.js'

/**
* @param {import('../bindings').RouteContext} ctx
Expand All @@ -22,6 +24,12 @@ import { accessClaimProvider } from './access-claim.js'
* }
*/
export function service(ctx) {
/**
* @param {Ucanto.DID<'key'>} uri
*/
const hasStorageProvider = async (uri) => {
return Boolean(await ctx.models.spaces.get(uri))
}
return {
store: uploadApi.createStoreProxy(ctx),
upload: uploadApi.createUploadProxy(ctx),
Expand All @@ -45,12 +53,21 @@ export function service(ctx) {
}
return accessDelegateProvider({
delegations: ctx.models.delegations,
hasStorageProvider: async (uri) => {
return Boolean(await ctx.models.spaces.get(uri))
},
hasStorageProvider,
})(...args)
},
},

provider: {
add: (...args) => {
// disable until hardened in test/staging
if (ctx.config.ENV === 'production') {
throw new Error(`provider/add invocation handling is not enabled`)
}
return providerAddProvider(ctx)(...args)
},
},

voucher: {
claim: voucherClaimProvider(ctx),
redeem: voucherRedeemProvider(ctx),
Expand Down Expand Up @@ -181,6 +198,18 @@ export function service(ctx) {
fail() {
throw new Error('test fail')
},
/**
* @param {Ucanto.Invocation<Ucanto.Capability<'testing/space-storage', Ucanto.DID<'key'>, Ucanto.Failure>>} invocation
*/
'space-storage': async (invocation) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just amend space/info and add providers: DID[] to it ? We would need that for store/* capabilities anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. I thought about it but didn't want to extend the public api without discussion.

In this case I am very supportive because that will also be helpful to upload-api and things like storacha/w3infra#134 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can I do it as a fast follow once I have verified that the space registration via provider/add works on staging at all?
#480

const spaceId = invocation.capabilities[0].with
const hasStorageProvider =
await ctx.models.storageProvisions.hasStorageProvider(spaceId)
return {
hasStorageProvider,
foo: 'ben',
}
},
},
}
}
58 changes: 58 additions & 0 deletions packages/access-api/src/service/provider-add.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import * as Ucanto from '@ucanto/interface'
import * as Server from '@ucanto/server'
import { Provider } from '@web3-storage/capabilities'
import * as validator from '@ucanto/validator'

/**
* @typedef {import('@web3-storage/capabilities/types').ProviderAdd} ProviderAdd
* @typedef {import('@web3-storage/capabilities/types').ProviderAddSuccess} ProviderAddSuccess
* @typedef {import('@web3-storage/capabilities/types').ProviderAddFailure} ProviderAddFailure
*/

/**
* @callback ProviderAddHandler
* @param {Ucanto.Invocation<import('@web3-storage/capabilities/types').ProviderAdd>} invocation
* @returns {Promise<Ucanto.Result<ProviderAddSuccess, ProviderAddFailure>>}
*/

/**
* @param {object} options
* @param {import('../types/provisions').StorageProvisions} options.storageProvisions
* @returns {ProviderAddHandler}
*/
export function createProviderAddHandler(options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This obviously follows the style of code that is in the repo already, but I do find amount of indirection adds so much unnecessary complexity e.g. I would much rather have something like:

/**
 * @param {object} input
 * @param {Ucanto.Invocation<import('@web3-storage/capabilities/types').ProviderAdd>} input.invocation
 * @param {{ storageProvisions: import('../types/provisions').StorageProvisions }} input.context
 */
export const add ({ invocation, context: { storageProvisions } }) => {
   // ...
}

And in the service defs something like

Server.provide(Provider.add, ({ invocation }) => add({ invocation, context })

Than all this layers and closures all over.

Also for what it's worth I'm going to add context to the ucanto provider handles to remove the need for all those closures.

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 a good idea and I'll try it out across all the relevant things I wrote in that style to use the less-closures style you suggest. #481

/** @type {ProviderAddHandler} */
return async (invocation) => {
const [providerAddCap] = invocation.capabilities
const {
nb: { consumer, provider },
with: accountDID,
} = providerAddCap
if (!validator.DID.match({ method: 'mailto' }).is(accountDID)) {
return {
error: true,
name: 'Unauthorized',
message: 'Issuer must be a mailto DID',
}
}
await options.storageProvisions.putMany({
space: consumer,
provider,
account: accountDID,
})
return {}
}
}

/**
* @param {object} ctx
* @param {Pick<import('../bindings').RouteContext['models'], 'storageProvisions'>} ctx.models
*/
export function providerAddProvider(ctx) {
return Server.provide(Provider.add, async ({ invocation }) => {
const handler = createProviderAddHandler({
storageProvisions: ctx.models.storageProvisions,
})
return handler(/** @type {Ucanto.Invocation<ProviderAdd>} */ (invocation))
})
}
30 changes: 30 additions & 0 deletions packages/access-api/src/types/provisions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as Ucanto from '@ucanto/interface'

export type AlphaStorageProvider = 'did:web:web3.storage:providers:w3up-alpha'

/**
* action which results in provisionment of a space consuming a storage provider
*/
export interface StorageProvisionCreation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would prefer if we did not had Storage prefix, protocol is meant to allow you to add capability providers which can be for storage or other things sometimes for multiple things. It is true that did:web:web3.storage:providers:w3up-alpha is mostly for storage, yet it's not just storage it's also upload/* and access/*.

Copy link
Contributor Author

@gobengo gobengo Mar 3, 2023

Choose a reason for hiding this comment

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

At this layer of the types I wanted to model this particular use case really explicitly (e.g. with space and account vs consumer and issuer). In practice those get translated into a sql table that I think can accomodate the more general thing you're talking about over time. We could have many of these narrower types that mode domain event like this that write into the same underlying thing.

protocol is meant to allow you to add capability providers which can be for storage or other things sometimes for multiple things.

Because the underlying table schema accomodates this, I don't want to change this particular action type now. I'm not against revising it or adding other events in subsequent PR. or e.g. renaming the model to not have storage prefix but still maybe having this particular event be specific to adding a storage provider.

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 removed 'Storage' prefix from the model object itself e62faaa

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I find naming to be very confusing, can we simply call this

Suggested change
export interface StorageProvisionCreation {
export interface ProvisionModel {

Or better yet

Suggested change
export interface StorageProvisionCreation {
export interface Provision {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

space: Ucanto.DID<'key'>
account: Ucanto.DID<'mailto'>
provider: AlphaStorageProvider
}

/**
* stores instances of a storage provider being consumed by a consumer
*/
export interface StorageProvisions {
hasStorageProvider: (consumer: Ucanto.DID<'key'>) => Promise<boolean>
/**
* write several items into storage
*
* @param items - provisions to store
*/
putMany: (...items: StorageProvisionCreation[]) => Promise<void>

/**
* get number of stored items
*/
count: () => Promise<bigint>
}
2 changes: 2 additions & 0 deletions packages/access-api/src/utils/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { createUploadApiConnection } from '../service/upload-api-proxy.js'
import { DID } from '@ucanto/core'
import { DbDelegationsStorage } from '../models/delegations.js'
import { createD1Database } from './d1.js'
import { DbStorageProvisions } from '../models/provisions.js'

/**
* Obtains a route context object.
Expand Down Expand Up @@ -68,6 +69,7 @@ export function getContext(request, env, ctx) {
spaces: new Spaces(config.DB),
validations: new Validations(config.VALIDATIONS),
accounts: new Accounts(config.DB),
storageProvisions: new DbStorageProvisions(createD1Database(config.DB)),
},
email,
uploadApi: createUploadApiConnection({
Expand Down
10 changes: 8 additions & 2 deletions packages/access-api/src/utils/email.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
export const debug = () => new DebugEmail()

/**
* @typedef ValidationEmailSend
* @property {string} to
* @property {string} url
*/

/**
* @param {{token:string, sender?:string}} opts
*/
Expand All @@ -23,7 +29,7 @@ export class Email {
/**
* Send validation email with ucan to register
*
* @param {{ to: string; url: string }} opts
* @param {ValidationEmailSend} opts
*/
async sendValidation(opts) {
const rsp = await fetch('https://api.postmarkapp.com/email/withTemplate', {
Expand Down Expand Up @@ -90,7 +96,7 @@ export class DebugEmail {
/**
* Send validation email with ucan to register
*
* @param {{ to: string; url: string }} opts
* @param {ValidationEmailSend} opts
*/
async sendValidation(opts) {
try {
Expand Down
32 changes: 1 addition & 31 deletions packages/access-api/test/access-delegate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import * as delegationsResponse from '../src/utils/delegations-response.js'
import {
assertNotError,
createTesterFromContext,
createTesterFromHandler,
warnOnErrorResult,
} from './helpers/ucanto-test-utils.js'

Expand Down Expand Up @@ -210,37 +211,6 @@ for (const variant of /** @type {const} */ ([
})
}

/**
* @template {Ucanto.Capability} Capability
* @template Result
* @typedef {object} InvokeTester
* @property {(invocation: Ucanto.Invocation<Capability>) => Promise<Result>} invoke
* @property {Resolvable<Ucanto.Signer<Ucanto.DID<'key'>>>} issuer
* @property {Resolvable<Ucanto.Verifier<Ucanto.DID>>} audience
*/

/**
* Tests using simple function invocation -> result
*
* @template {Ucanto.Capability} Capability
* @template Result
* @param {() => (invocation: Ucanto.Invocation<Capability>) => Promise<Result>} createHandler
* @returns {InvokeTester<Capability, Result>}
*/
function createTesterFromHandler(createHandler) {
const issuer = principal.ed25519.generate()
const audience = principal.ed25519.generate()
/**
* @param {Ucanto.Invocation<Capability>} invocation
*/
const invoke = async (invocation) => {
const handle = createHandler()
const result = await handle(invocation)
return result
}
return { issuer, audience, invoke }
}

/**
* a value that can be passed to Promise.resolve() to get Promise<T>
*
Expand Down
Loading