The middleware functions of this package expect their first argument to be an IOriginMetadata object. The objects intended usage is made clear by these lines in providerMiddlewareFunction. Specifically, it provides metadata about the external domain that the middleware is bound to.
We can see that our usage was incorrect, because we defaulted to the IOriginMetadata object's id property as the metadata.id property of the IPermissionsRequest object, which represents a pending permission. If the IOriginMetadata object did not have an id, it was set. Since the IOriginMetadata object is bound to a middleware, once this id was set, it forced the same metadata.id value for all pending permissions request for that domain. This breaks both methods internal to rpc-cap, like removePermissionsRequest, and functionality in the MetaMask Extension.
Changes
Rename requestPermissionsMiddlewareIOriginMetadata parameter from metadata to domain, in line with other middleware methods
I believe that this misnaming of the parameter may have contributed to its incorrect use here and elsewhere
For IPermissionsRequest.metadata.id values, default to the req.id value, and generate a uuid() as a fallback
Type IPermissionsRequest.metadata.id values as string | number
This is because the types from JsonRpcEngine (which are in line with the JSON-RPC 2.0 spec), allow string and numberid values
I would like to restrict the id types to string only, but that would require changes to json-rpc-engine and @metamask/inpage-provider. That can be done in follow-ups.
Rationalize related types
Remove id property from IOriginMetadata
The origin is the id, and the id was never set beyond the case removed in this PR
Remove top-level origin property from IPermissionsRequest
The metadata property already has the origin, and that's also what's used in practice by the MetaMask extension
Add relevant test case
json-rpc-engine IDs
json-rpc-engine generates its IDs in this file. We use its ID remap middleware in @metamask/inpage-provider to ensure that every request has a unique ID. I mention this just to note that the ID can take value 0, and that the probability of collision is sufficiently low for our purposes.
Summary
Our generation of
idvalues for pending permissions requests inrequestPermissionsMiddlewarewas incorrect. This PR fixes it.This is a prerequisite for solving: https://github.com/MetaMask/metamask-extension/issues/8919
Details
The middleware functions of this package expect their first argument to be an
IOriginMetadataobject. The objects intended usage is made clear by these lines inproviderMiddlewareFunction. Specifically, it provides metadata about the external domain that the middleware is bound to.We can see that our usage was incorrect, because we defaulted to the
IOriginMetadataobject'sidproperty as themetadata.idproperty of theIPermissionsRequestobject, which represents a pending permission. If theIOriginMetadataobject did not have an id, it was set. Since theIOriginMetadataobject is bound to a middleware, once thisidwas set, it forced the samemetadata.idvalue for all pending permissions request for that domain. This breaks both methods internal torpc-cap, likeremovePermissionsRequest, and functionality in the MetaMask Extension.Changes
requestPermissionsMiddlewareIOriginMetadataparameter frommetadatatodomain, in line with other middleware methodsIPermissionsRequest.metadata.idvalues, default to thereq.idvalue, and generate auuid()as a fallbackIPermissionsRequest.metadata.idvalues asstring | numberJsonRpcEngine(which are in line with the JSON-RPC 2.0 spec), allowstringandnumberidvaluesidtypes tostringonly, but that would require changes tojson-rpc-engineand@metamask/inpage-provider. That can be done in follow-ups.idproperty fromIOriginMetadataoriginis theid, and theidwas never set beyond the case removed in this PRoriginproperty fromIPermissionsRequestmetadataproperty already has theorigin, and that's also what's used in practice by the MetaMask extensionjson-rpc-engineIDsjson-rpc-enginegenerates its IDs in this file. We use its ID remap middleware in@metamask/inpage-providerto ensure that every request has a unique ID. I mention this just to note that the ID can take value0, and that the probability of collision is sufficiently low for our purposes.