feat!: introduce new administrative capabilities #832
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As described in https://hackmd.io/@tvpl/H1whErCI3, introduce a set of capabilities that will: 1) allow providers to rate limit (especially fully limit, ie block) users 2) allow providers to get information about subscriptions and consumers TODO -[] write tests for capabilities (need to pick @Gozala's brain) -[] implement in-memory RateLimitsStorage -[] use it to write `upload-api` tests -[] move capabilities spec from hackmd to https://github.com/web3-storage/w3up/blob/main/spec/capabilities.md -[] port 0eea3d1 and d230033 to this commit
originally implemented in #801, this allows us to block space allocation for a particular space or account creation for a particular email or domain
mostly just cargo-culting the existing type setups, but basing these types on the designs proposed in https://hackmd.io/@tvpl/H1whErCI3
also update existing Consumer.has types to match our normal patterns
it really wasn't happy with getCustomer potentially returning `null` and I think that should use a `Ucanto.Failure` anyway so I elected to make a backwards-incompatible change here since I think this work will merit a breaking change anyway
this eliminates the specter of partial failure
Remove it from the interface - I thought that IN queries would be supported by Dynamo but they aren't. I was already on the fence about this interface method and this pushed me into the "not worth it" camp.
I thought we were doing this, but the implementation of `allocateSpace` in the `AccessVerifier` never actually called into `Space.allocate`, it just used `Space.info` to make sure the space existed, which, until now, fair! I'm not sure this is the right way to call the `Space.allocate` provider - maybe it would be better to invoke and execute the actual capability rather than just calling the provider function?
travis
added a commit
to storacha/w3infra
that referenced
this pull request
Jul 28, 2023
Implement the new RateLimitStorage interface from storacha/w3up#832 and integrate it into the upload-api. This gives us the ability to block uploads to a space and to block an email or domain name from authorizing with our service.
1 task
this gets basic testing in place for the new rate limit capability handlers the test implementations are a bit wordier and boilerplate-inclined than I'm strictly happy with, but I don't have any easy plans to clean them up - need to think and prototype a little
The AccessVerifier was an artifact of the dual-service architecture we had previously, and its implementation in tests both hid bugs and made it more difficult to test the whole system. I've removed it and gotten the tests running!
so we can use it in w3infra
id is used more consistently in w3infra tests and they are identical
Gozala
reviewed
Aug 9, 2023
Gozala
reviewed
Aug 9, 2023
Gozala
reviewed
Aug 9, 2023
Gozala
reviewed
Aug 9, 2023
Gozala
approved these changes
Aug 9, 2023
|
|
||
| if (allocated.error) { | ||
| return allocated | ||
| const hasProviderResult = await hasProvider( |
There was a problem hiding this comment.
Do we not want to check the block list here ?
I also would recommend command style API over defensive style API that is instead of having hasProvider you could have useCapability so that you don't have to have one branch for propagating an error and other branch for creating an error if answer to question was false.
That is also why we had allocateSpace as opposed to `hasSpace.
P.S. It is very same idea as with Parse, don’t validate (a good read if you have not come across it before).
There was a problem hiding this comment.
oo I like this, and I also like being able to re-use allocate the way we were - I've pushed a new commit that makes this happen, and drops the size input on space/allocate - we weren't using it for anything and the fact that we don't have a size in the upload/add provider was the main reason I moved away from it.
I suspect we may want to re-introduce it later, either as an optional parameter that triggers a check against a space's allocation or as a separate but similar capability, but this seems ok to me for now - let me know if you think it might be a problem!
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
ensure callers can't change subject, rate or id constraints
travis
changed the title
we err on the side of denying a user the ability to authorize or provision a space - they can retry and we really don't want to let people do things unless we're sure they're allowed to
per feedback from @Gozala, move to a style where we always return an `{ok: {}}` result if there is no rate limit above a given threshold. use this instead of an explicit "block" checker
per feedback from @Gozala, explain the reasoning behind having a separate, opaque identifier for rate limits.
this ensures people can't upload to a blocked space, and makes it more consistent. this also gets rid of the `size` parameter in `space/allocate` because: a) we weren't using it for anything b) we don't have size information in `upload/add` I think we'll want to add this back in later, possibly as an optional parameter that triggers a check against provisioned allocation, or we'll want to split space/allocate into two capabilities.
This was referenced Aug 9, 2023
travis
pushed a commit
that referenced
this pull request
Aug 9, 2023
🤖 I have created a release *beep* *boop* --- ## [9.0.0](capabilities-v8.0.0...capabilities-v9.0.0) (2023-08-09) ### ⚠ BREAKING CHANGES * introduce new administrative capabilities ([#832](#832)) ### Features * introduce new administrative capabilities ([#832](#832)) ([7b8037a](7b8037a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
travis
added a commit
that referenced
this pull request
Aug 9, 2023
🤖 I have created a release *beep* *boop* --- ## [5.0.0](upload-api-v4.1.0...upload-api-v5.0.0) (2023-08-09) ### ⚠ BREAKING CHANGES * introduce new administrative capabilities ([#832](#832)) ### Features * introduce new administrative capabilities ([#832](#832)) ([7b8037a](7b8037a)) ### Bug Fixes * run format for upload-api ([#825](#825)) ([59dc765](59dc765)) --- 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: Travis Vachon <travis.vachon@protocol.ai>
olizilla
reviewed
Aug 11, 2023
| }) | ||
|
|
||
| /** | ||
| * Capability can be invoked by the provider are an authorized delegate to remove rate limits from a subject. |
There was a problem hiding this comment.
Capability can be invoked by the provider **or** an authorized delegate
olizilla
reviewed
Aug 11, 2023
| > | ||
| export type RateLimitRemoveSuccess = Unit | ||
|
|
||
| export interface RateLimitsNotFound extends Ucanto.Failure { |
There was a problem hiding this comment.
nit: RateLimits is plural here but singular in all other types
olizilla
reviewed
Aug 11, 2023
| if (rateLimitResult.error) { | ||
| return { | ||
| error: { | ||
| name: 'InsufficientStorage', |
There was a problem hiding this comment.
InsufficientStorage doens't feel like the right name here. It's more of an AccountBlocked kind of error.
travis
added a commit
to storacha/w3infra
that referenced
this pull request
Aug 11, 2023
Implement the new RateLimitStorage interface from storacha/w3up#832 and integrate it into the upload-api. This gives us the ability to block uploads to a space and to block an email or domain name from authorizing with our service. Note that we leave a couple of the new capabilities unimplemented for now since we don't need them urgently. We are actively hoping to block some abusive users ASAP, so I'm getting the blocking part of this work in now and will then turn my attention to the remaining capabilities. TODO - [x] remove file dependencies from package.json and point at latest versions of deps once storacha/w3up#832 is merged and released
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As described in https://hackmd.io/@tvpl/H1whErCI3, introduce a set of capabilities that will:
I've taken this opportunity to remove the
AccessVerifierfrom the service and now simply use the real access service implementation for space provisioning.I'm not entirely happy with the verbosity of the tests for either capabilities or invocation providers, but I'd like to address that in a separate line of work - I'm taking notes!
TODO
upload-apitests