feat: add rate limiting support to controllers leveraging the accounts API #6066
Conversation
mathieuartu
added
the
team-identity
…ller auth subpath exports
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
packages/assets-controllers/src/multi-chain-accounts-service/multi-chain-accounts.ts
Show resolved
Hide resolved
| AuthenticationControllerGetBearerToken['handler'] | ||
| > { | ||
| return await this.messagingSystem.call( | ||
| 'AuthenticationController:getBearerToken', |
There was a problem hiding this comment.
If all these controllers ultimately call the same action / method, why can't this just be the default where needed in a central accounts API service to avoid all these breaking changes and new dependencies?
Maybe in @metamask/utils or @metamask/controller-utils to avoid a new dependency?
Or even a util such as fetchWithBearerToken?
Or could the clients just implement a proxy to inject this token automatically to avoid any core changes at all and decouple all this from the controllers?
There was a problem hiding this comment.
Great callout. Do we only use the Accounts API in the accounts controller?
There was a problem hiding this comment.
I think multiple controllers have their own partial abstractions over the accounts API.
My fear is if we keep casually adding peer dependencies between the controllers, we make the release process harder, breaking changes more common, and eventually we'll just have one interdependent bundle meaning no single controller can have a major release without it cascading through the others.
There was a problem hiding this comment.
@matthewwalsh0 I'm not sure I understand your comment correctly 😄 AFAIK, AccountsController does not make Accounts API requests, but each of those controllers directly do in their own "bubble". They all have their own logic and wrappers around fetch and also often attach their own headers to these requests. This is why we added this logic at this level.
LMK if that clarifies it - or if I completely missed your point!
There was a problem hiding this comment.
Apologies, I initially mentioned the AccountsController by mistake, but updated it to clarify how we could abstract this complexity in various ways.
There was a problem hiding this comment.
Exactly, right now it's a lot of duplication and new dependencies on three new controllers, to ultimately do the same logic. So a central mechanism would be great, either client proxy, util, or shared service.
There was a problem hiding this comment.
Thanks everyone for your input!
I think I see the pattern here, and I agree. Nobody wants a new peerDep.
It's just unfortunate that I'll need to juggle around 3 different fetch implementations 😅
handleFetchforTokenDetectionControllersuccessfulFetchforTransactionControllerfetch: window.fetch.bind(window)forMultichainNetworkController(as seen on extension)
There was a problem hiding this comment.
How do we feel about the proxy / hook idea, to automatically inject this from any client request, meaning no controller changes needed at all? And rather the clients themselves are conceptually "authorised" and so any requests they generate are automatically authorised also?
There was a problem hiding this comment.
What would automatically injecting the token from any client request look like? Why would it mean no controller changes?
There was a problem hiding this comment.
Instead of modifying individual service/controller functions. We can also use a rule-based fetch interceptor that automatically adds authentication headers to matching URLs.
Note: The interceptor below only works with globalThis.fetch, so it has limitations when used with other HTTP clients. Additionally, it may need to be adapted to support both extension and mobile environments. We should also consider addressing known issues related to differences in polyfill timing, global scope variations, and Metro bundler behavior. The example is only for idea sharing.
// packages/controller-utils/src/fetch-interceptor.ts
type AuthRule = {
pattern: string | RegExp;
getToken: () => Promise<string>;
headerName?: string;
enabled?: boolean;
};
class FetchInterceptor {
private authRules: AuthRule[] = [];
private originalFetch: typeof globalThis.fetch;
private isSetup = false;
constructor() {
this.originalFetch = globalThis.fetch;
}
setup() {
if (this.isSetup) return;
globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => {
const url = this.extractUrl(input);
const matchedRule = this.findMatchingRule(url);
if (matchedRule && matchedRule.enabled !== false) {
try {
const token = await matchedRule.getToken();
const headerName = matchedRule.headerName || 'Authorization';
const headerValue = headerName === 'Authorization' ? `Bearer ${token}` : token;
init = {
...init,
headers: {
...init?.headers,
[headerName]: headerValue,
},
};
} catch (error) {
console.warn(`Failed to get auth token for ${url}:`, error);
// Continue with original request
}
}
return this.originalFetch(input, init);
};
this.isSetup = true;
}
addAuthRule(rule: AuthRule) {
this.authRules.push(rule);
}
teardown() {
if (this.isSetup) {
globalThis.fetch = this.originalFetch;
this.isSetup = false;
}
}
private extractUrl(input: RequestInfo | URL): string {
if (typeof input === 'string') return input;
if (input instanceof URL) return input.href;
return input.url || '';
}
private findMatchingRule(url: string): AuthRule | null {
return this.authRules.find(rule => {
if (typeof rule.pattern === 'string') {
return url.includes(rule.pattern);
}
return rule.pattern.test(url);
}) || null;
}
}
export const fetchInterceptor = new FetchInterceptor();That we can use in the clients side
import { fetchInterceptor } from '@metamask/controller-utils';
export async function initializeControllers() {
// Initialize auth controller first
const authController = new AuthenticationController({...});
// Setup authentication rules
fetchInterceptor.addAuthRule({
pattern: 'accounts.api.cx.metamask.io',
getToken: () => authController.getBearerToken(),
});
// Activate the interceptor
fetchInterceptor.setup();
// All subsequent fetch calls are automatically authenticated
const tokenDetectionController = new TokenDetectionController({...});
}
|
What's the reasoning behind using |
| @@ -2616,10 +2617,10 @@ describe('TokenDetectionController', () => { | |||
| properties: { | |||
| tokens: [`${sampleTokenA.symbol} - ${sampleTokenA.address}`], | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
| // eslint-disable-next-line @typescript-eslint/naming-convention | |||
There was a problem hiding this comment.
Was this a mistake? It looks like the line below would still violate this lint rule.
If the rule doesn't apply for some reason, we should ensure the accompanying TODO comment is also removed (as well as this empty line).
There was a problem hiding this comment.
autofixOnSave was the culprit here 🕵️ Fixed with 0ce01be
mathieuartu
dismissed stale reviews from fabiobozzo and Prithpal-Sooriya
via
0ce01be
packages/assets-controllers/src/TokenDetectionController.test.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/TokenDetectionController.test.ts
Outdated
Show resolved
Hide resolved
| @@ -879,10 +918,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | |||
| tokens: eventTokensDetails, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
There was a problem hiding this comment.
Should we remove this comment too?
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. |
| @@ -879,10 +918,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | |||
| tokens: eventTokensDetails, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
| // eslint-disable-next-line @typescript-eslint/naming-convention | |||
|
|
|||
| token_standard: ERC20, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
There was a problem hiding this comment.
Should we remove this comment too?
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. |
| @@ -1009,10 +1049,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | |||
| tokens: eventTokensDetails, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
There was a problem hiding this comment.
Should we remove this comment too?
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. |
| @@ -1009,10 +1049,10 @@ export class TokenDetectionController extends StaticIntervalPollingController<To | |||
| properties: { | |||
| tokens: eventTokensDetails, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
| // eslint-disable-next-line @typescript-eslint/naming-convention | |||
|
|
|||
| token_standard: ERC20, | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
There was a problem hiding this comment.
Should we remove this comment too?
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. |
Explanation
@MetaMask/identity recently deployed a
profileIdbased (with IP fallback) rate limiting service for the Accounts API.This PR adds the correct Authentication headers to everywhere in the code the Accounts API was called.
It modifies three controllers, and adds
@metamask/profile-sync-controllerto their respectivepackage.jsonfile as apeerDependency.The three controllers are:
TokenDetectionControllerMultichainNetworkControllerTransactionControllerThose are breaking changes, since we need clients to make changes in order to accommodate for this new logic.
An extension test-drive PR can be found below, and act as a reference on how to adapt client code for those breaking changes:
References
Changelog
Checklist