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: move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩ fast ⏩ #449

Merged
merged 13 commits into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
2 changes: 1 addition & 1 deletion packages/access-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"toucan-js": "^2.7.0"
},
"devDependencies": {
"@cloudflare/workers-types": "^3.18.0",
"@cloudflare/workers-types": "^3.19.0",
"@databases/split-sql-query": "^1.0.3",
"@databases/sql": "^3.2.0",
"@sentry/cli": "2.7.0",
Expand Down
8 changes: 8 additions & 0 deletions packages/access-api/src/bindings.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ import { DelegationsStorage as Delegations } from './types/delegations.js'

export {}

export type {
DurableObjectNamespace,
DurableObjectState,
Response as WorkerResponse,
} from '@cloudflare/workers-types'

// CF Analytics Engine types not available yet
export interface AnalyticsEngine {
writeDataPoint: (event: AnalyticsEngineEvent) => void
Expand Down Expand Up @@ -52,6 +58,7 @@ export interface Env {
SPACES: KVNamespace
VALIDATIONS: KVNamespace
W3ACCESS_METRICS: AnalyticsEngine
SPACE_VERIFIERS: DurableObjectNamespace
// eslint-disable-next-line @typescript-eslint/naming-convention
__D1_BETA__: D1Database
}
Expand All @@ -69,6 +76,7 @@ export interface RouteContext {
delegations: Delegations
}
uploadApi: ConnectionView
spaceVerifiers: DurableObjectNamespace
}

export type Handler = _Handler<RouteContext>
Expand Down
131 changes: 131 additions & 0 deletions packages/access-api/src/durable-objects/space-verifier.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { stringToDelegation } from '@web3-storage/access/encoding'

/**
*
* @template {import('@ucanto/interface').Capabilities} [T=import('@ucanto/interface').Capabilities]
* @param {import('../bindings').DurableObjectNamespace} spaceVerifiers
* @param {string} space
* @param {import('@web3-storage/access/src/types').EncodedDelegation<T>} ucan
*/
export async function sendDelegationToSpaceVerifier(
spaceVerifiers,
space,
ucan
) {
const durableObjectID = spaceVerifiers.idFromName(space)
const durableObject = spaceVerifiers.get(durableObjectID)
// hostname is totally ignored by the durable object but must be set so set it to example.com
const response = await durableObject.fetch('https://example.com/delegation', {
method: 'PUT',
body: ucan,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does body need to be a string or can we pass binary data ? If we could pass binary it might be a better option.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good question, I suspect binary would work? noted and a good change for the next version of this I think

})
if (response.status === 400) {
throw new Error(response.statusText)
}
}

/**
* @template {import('@ucanto/interface').Capabilities} [T=import('@ucanto/interface').Capabilities]
* @param {WebSocket} server
* @param {import('@web3-storage/access/src/types').EncodedDelegation<T>} ucan
*/
function sendDelegation(server, ucan) {
server.send(
JSON.stringify({
type: 'delegation',
delegation: ucan,
})
)
server.close()
}

/**
* SpaceVerifier
*/
export class SpaceVerifier {
/**
* @param {import('../bindings').DurableObjectState} state
*/
constructor(state) {
this.state = state
// `blockConcurrencyWhile()` ensures no requests are delivered until
// initialization completes.
this.state.blockConcurrencyWhile(async () => {
this.ucan = await this.state.storage.get('ucan')
})
}

cleanupServer() {
this.server = undefined
}

async cleanupUCAN() {
this.ucan = undefined
await this.state.storage.put('ucan', '')
}

/**
* @param {Request} req
*/
async fetch(req) {
Copy link
Contributor

@gobengo gobengo Mar 10, 2023

Choose a reason for hiding this comment

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

I would expect to see an authorization check somewhere to make we don't give the ucan to someone who shouldn't have it?

If someone else tries to connects first before the client we expect, what would happen? (It's been a long day, but I read it like that would deny service to the desired user)

I wonder if (long term) maybe this should be like AgentVerifier and be keyed off of the agent did, not space did.
(or agent did + cid(authorizationRequest) even).

It seems like keying this off of space and only allowing one server/ucan at a time will prevent more than one of these flows happening at a time for a space - not really a problem for voucher but it makes this less useful with new access/authorize + access/claim flow which is not 1:1 with a single space.
(we dont have to optimize in this PR for the new access stuff, but this will come up in #395 or #433)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect to see an authorization check somewhere to make we don't give the ucan to someone who shouldn't have it?

hm, I would probably handle this further up the stack, but also I'm not sure we do this with the existing implementation and if we do I think we're still doing that for this flow?

tbh I'm not entirely sure whether we need to do additional auth here or not, but I'm not sure what that would look like concretely - do you have some sort of signature verification in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if (long term) maybe this should be like AgentVerifier and be keyed off of the agent did, not space did.
(or agent did + cid(authorizationRequest) even).

I think this is exactly the direction we should go with the new stuff coming in yep

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like keying this off of space and only allowing one server/ucan at a time will prevent more than one of these flows happening at a time for a space - not really a problem for voucher but it makes this less useful with new access/authorize + access/claim flow which is not 1:1 with a single space.

yep - keying off Agent will be the way to go for the path that handles the access/authorize flow

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 invocation CID might be better yet as agent in theory might start multiple authorization flows or maybe we'll want to deliver other messages over websocket.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm I don't totally follow, but seems reasonable as long as both sides of the equation (ie, the code calling registerSpace and the code handling the request from the email link) can both calculate the invocation CID

const path = new URL(req.url).pathname
if (req.method === 'GET' && path.startsWith('/validate-ws/')) {
const upgradeHeader = req.headers.get('Upgrade')
if (!upgradeHeader || upgradeHeader !== 'websocket') {
return new Response('Expected Upgrade: websocket', { status: 426 })
}
if (this.server) {
return new Response('Websocket already connected for this space.', {
status: 409,
})
}
const [client, server] = Object.values(new WebSocketPair())
// @ts-ignore
server.accept()
// if the user has already verified and set this.ucan here, send them the delegation

if (this.ucan) {
travis marked this conversation as resolved.
Show resolved Hide resolved
sendDelegation(
server,
/** @type {import('@web3-storage/access/src/types').EncodedDelegation} */ (
this.ucan
)
)
await this.cleanupUCAN()
} else {
this.server = server
}
return new Response(undefined, {
status: 101,
webSocket: client,
})
} else if (req.method === 'PUT' && path === '/delegation') {
const ucan = await req.text()
const delegation = stringToDelegation(ucan)

// it's only important to check expiration here - if we successfully validate before expiration
// here and a user connects to the websocket later after expiration we should still send the delegation
if (Date.now() < delegation.expiration * 1000) {
if (this.server) {
sendDelegation(this.server, ucan)
this.cleanupServer()
} else {
await this.state.storage.put('ucan', ucan)
this.ucan = ucan
}
return new Response(undefined, {
status: 200,
})
} else {
this.server?.close()
return new Response('Delegation expired', {
status: 400,
})
}
} else {
return new Response("SpaceVerifier can't handle this request", {
status: 404,
})
}
}
}
4 changes: 4 additions & 0 deletions packages/access-api/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { postRaw } from './routes/raw.js'
import { postRoot } from './routes/root.js'
import { preValidateEmail, validateEmail } from './routes/validate-email.js'
import { validateWS } from './routes/validate-ws.js'
import { validateWSDID } from './routes/validate-ws-did.js'
import { version } from './routes/version.js'
import { getContext } from './utils/context.js'

Expand All @@ -17,6 +18,7 @@ r.add('get', '/version', version)
r.add('get', '/validate-email', preValidateEmail)
r.add('post', '/validate-email', validateEmail)
r.add('get', '/validate-ws', validateWS)
r.add('get', '/validate-ws/:did', validateWSDID)
r.add('post', '/', postRoot)
r.add('post', '/raw', postRaw)

Expand All @@ -39,4 +41,6 @@ const worker = {
},
}

export { SpaceVerifier } from './durable-objects/space-verifier.js'

export default worker
17 changes: 16 additions & 1 deletion packages/access-api/src/models/validations.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { stringToDelegation } from '@web3-storage/access/encoding'
import { sendDelegationToSpaceVerifier } from '../durable-objects/space-verifier.js'

/**
* Validations
Expand All @@ -7,9 +8,11 @@ export class Validations {
/**
*
* @param {KVNamespace} kv
* @param {import('../bindings').DurableObjectNamespace | undefined} spaceVerifiers
*/
constructor(kv) {
constructor(kv, spaceVerifiers) {
this.kv = kv
this.spaceVerifiers = spaceVerifiers
}

/**
Expand All @@ -22,10 +25,22 @@ export class Validations {
stringToDelegation(ucan)
)

// TODO: remove this KV stuff once we have the durable objects stuff in production
await this.kv.put(delegation.audience.did(), ucan, {
expiration: delegation.expiration,
})

const cap =
/** @type import('@ucanto/interface').InferInvokedCapability<typeof import('@web3-storage/capabilities/voucher').redeem> */ (
delegation.capabilities[0]
)
if (this.spaceVerifiers && cap.nb?.space) {
await sendDelegationToSpaceVerifier(
this.spaceVerifiers,
cap.nb.space,
ucan
)
}
return delegation
}

Expand Down
17 changes: 17 additions & 0 deletions packages/access-api/src/routes/validate-ws-did.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @param {import('@web3-storage/worker-utils/router').ParsedRequest} req
* @param {import('../bindings.js').RouteContext} env
*/
export async function validateWSDID(req, env) {
const durableObjectID = env.spaceVerifiers.idFromName(req.params.did)
const durableObject = env.spaceVerifiers.get(durableObjectID)
/** @type {import('../bindings.js').WorkerResponse} */
const response = await durableObject.fetch(req)
// wrap the response because it's not possible to set headers on the response we get back from the durable object
travis marked this conversation as resolved.
Show resolved Hide resolved
return new Response(response.body, {
status: response.status,
statusText: response.statusText,
headers: response.headers,
webSocket: response.webSocket,
})
}
10 changes: 5 additions & 5 deletions packages/access-api/src/service/voucher-claim.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,16 @@ export function voucherClaimProvider(ctx) {
.delegate()

const encoded = delegationToString(inv)
// For testing
if (ctx.config.ENV === 'test') {
return encoded
}

const url = `${ctx.url.protocol}//${ctx.url.host}/validate-email?ucan=${encoded}`

await ctx.email.sendValidation({
to: capability.nb.identity.replace('mailto:', ''),
url,
})

// For testing
if (ctx.config.ENV === 'test') {
return encoded
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is this really necessary now hat we can provide debug email thing for tests ? Because ucan is already in the query param. I have changed it in other placed, must have missed it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good question and I think no - I had originally been planning to rework the tests to get rid of this, but paused on that because we'll probably end up porting all of this over to the new session stuff, and it will make sense to rework tests and generally clean all of this up at that point.

})
}
3 changes: 2 additions & 1 deletion packages/access-api/src/utils/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export function getContext(request, env, ctx) {
models: {
delegations: new DbDelegationsStorage(createD1Database(config.DB)),
spaces: new Spaces(config.DB),
validations: new Validations(config.VALIDATIONS),
validations: new Validations(config.VALIDATIONS, env.SPACE_VERIFIERS),
accounts: new Accounts(config.DB),
},
email,
Expand All @@ -75,5 +75,6 @@ export function getContext(request, env, ctx) {
url: new URL(config.UPLOAD_API_URL),
fetch: globalThis.fetch.bind(globalThis),
}),
spaceVerifiers: env.SPACE_VERIFIERS,
}
}
2 changes: 1 addition & 1 deletion packages/access-api/test/helpers/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ dotenv.config({
})

/**
* @typedef {Omit<import('../../src/bindings').Env, 'SPACES'|'VALIDATIONS'|'__D1_BETA__'>} AccessApiBindings - bindings object expected by access-api workers
* @typedef {Omit<import('../../src/bindings').Env, 'SPACES'|'VALIDATIONS'|'__D1_BETA__'|'SPACE_VERIFIERS'>} AccessApiBindings - bindings object expected by access-api workers
*/

/**
Expand Down
Loading