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

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
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 13 additions & 7 deletions packages/access-api/src/service/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ import { providerAddProvider } from './provider-add.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 @@ -53,7 +47,19 @@ export function service(ctx) {
}
return accessDelegateProvider({
delegations: ctx.models.delegations,
hasStorageProvider,
hasStorageProvider: async (space) => {
/** @type {import('./access-delegate.js').HasStorageProvider} */
const registeredViaVoucherRedeem = async (space) =>
Boolean(await ctx.models.spaces.get(space))
/** @type {import('./access-delegate.js').HasStorageProvider} */
// eslint-disable-next-line unicorn/consistent-function-scoping
const registeredViaProviderAdd = (space) =>
ctx.models.provisions.hasStorageProvider(space)
return Boolean(
(await registeredViaProviderAdd(space)) ||
(await registeredViaVoucherRedeem(space))
)
},
})(...args)
},
},
Expand Down
96 changes: 96 additions & 0 deletions packages/access-api/test/provider-add.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,11 @@ for (const accessApiVariant of /** @type {const} */ ([
/** @type {{to:string, url:string}[]} */
const emails = []
const email = createEmail(emails)
const features = new Set(['provider/add', 'access/delegate'])
return {
spaceWithStorageProvider,
emails,
features,
...createTesterFromContext(
() =>
context({
Expand Down Expand Up @@ -97,6 +99,100 @@ for (const accessApiVariant of /** @type {const} */ ([
})
})
})

if (
['provider/add', 'access/delegate'].every((f) =>
accessApiVariant.features.has(f)
)
) {
it('provider/add allows for access/delegate', async () => {
const space = await principal.ed25519.generate()
const issuer = await accessApiVariant.issuer
const service = await accessApiVariant.audience
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

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)

capabilities: [
{
can: 'provider/add',
with: accountDid,
},
{
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.

},
],
})
const sessionProofs = [
await ucanto.delegate({
issuer: service,
audience: issuer,
capabilities: [
{
can: 'ucan/attest',
with: accountDid,
nb: {
// note: whole delegation is also included in 'proofs'
proof: serviceSessionAttest.cid,
},
},
],
}),
serviceSessionAttest,
]
const addStorageProvider = await ucanto
.invoke({
issuer,
audience: service,
capability: {
can: 'provider/add',
with: accountDid,
nb: {
provider: 'did:web:web3.storage:providers:w3up-alpha',
consumer: space.did(),
},
},
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

await ucanto.delegate({
issuer: space,
audience: issuer,
capabilities: [
{
can: 'provider/add',
with: accountDid,
},
],
}),
...sessionProofs,
],
})
.delegate()
const addStorageProviderResult = await accessApiVariant.invoke(
addStorageProvider
)
assertNotError(addStorageProviderResult)

// storage provider added. So we should be able to delegate now
const accessDelegate = await ucanto
.invoke({
issuer,
audience: service,
capability: {
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

},
},
proofs: [...sessionProofs],
})
.delegate()
const accessDelegateResult = await accessApiVariant.invoke(accessDelegate)
assertNotError(accessDelegateResult)
})
}
}

/**
Expand Down