-
-
Notifications
You must be signed in to change notification settings - Fork 242
feat: persist access token and metadata access token in seedless controller #6060
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
Conversation
828ed0b
to
473382e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access token and Metadata access token are being added but not used anywhere.
I'm assuming they will be used in a future version of the toprf client.
I think the purpose of this PR would be clearer if we first update the client and then add the new tokens. (This way we can make sure that the integration is right.)
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
accessToken?: string; | ||
metadataAccessToken?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it optional here but then in many/most/all? places we require this to be defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here: 62be127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a breaking change then.
how would this change be reflected in the frontend code? where would the new tokens be retrieved from?
it is usually required to create a draft PR in at least one of the frontend repositories (extension/mobile) to ensure a proper integration is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing will break even if you don't pass these tokens, so its technically not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you add a required parameter to an existing API, that is a breaking change!?
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
Yes that's correct, access_token will be used by profile sync team in auth controller. Metadata access token will be used by sdk. |
I think a draft PR in extension would help to understand how this change is integrated. I am thinking that we should release it as a separate version to unblock the merging of the other seedless onboarding PRs that rely on controller v2 and the store keyring key functionality. |
- Modify AuthenticationController to get accessToken from SeedlessOnboardingController state - Add pairing logic that runs non-blocking during signIn - Add state tracking for pairing status to prevent duplicate attempts - Add PAIR_SOCIAL_IDENTIFIER endpoint for social pairing API - Ensure pairing failures don't affect other authentication flows Based on PR #6048 but using accessToken from SeedlessOnboardingController (PR #6060) instead of injecting a separate socialPairingToken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
This PR is a prerequisite for the backup & sync pairing.
But @matthiasgeihs is right. For proper integration testing we do also require equivalent PRs in the clients
@metamaskbot publish-preview |
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
@metamaskbot publish-preview |
@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.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Token Refresh Causes Infinite Recursion
The fetchMetadataAccessCreds
method causes infinite recursion: an expired token triggers refreshAuthTokens
, which calls authenticate
, potentially leading to TOPRF operations that re-trigger fetchMetadataAccessCreds
via the TOPRF client.
Additionally, accessToken
and metadataAccessToken
are handled unsafely. In fetchMetadataAccessCreds
, newMetadataAccessToken
is retrieved from state after refresh and unsafely cast as string
without validation, potentially returning undefined
as a string. In refreshAuthTokens
, these tokens from the #refreshJWTToken
result are passed directly to authenticate
without validation, risking undefined
or null
values.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L219-L245
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 219 to 245 in 4f1f11b
async fetchMetadataAccessCreds(): Promise<{ | |
metadataAccessToken: string; | |
}> { | |
const { metadataAccessToken } = this.state; | |
if (!metadataAccessToken) { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InvalidMetadataAccessToken, | |
); | |
} | |
// Check if token is expired and refresh if needed | |
const decodedToken = decodeJWTToken(metadataAccessToken); | |
if (decodedToken.exp < Math.floor(Date.now() / 1000)) { | |
// Token is expired, refresh it | |
await this.refreshAuthTokens(); | |
// Get the new token after refresh | |
const { metadataAccessToken: newMetadataAccessToken } = this.state; | |
return { | |
metadataAccessToken: newMetadataAccessToken as string, | |
}; | |
} | |
return { metadataAccessToken }; | |
} |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L1707-L1719
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 1707 to 1719 in 4f1f11b
}); | |
const { idTokens, accessToken, metadataAccessToken } = res; | |
// re-authenticate with the new id tokens to set new node auth tokens | |
await this.authenticate({ | |
idTokens, | |
accessToken, | |
metadataAccessToken, | |
authConnection: this.state.authConnection, | |
authConnectionId: this.state.authConnectionId, | |
groupedAuthConnectionId: this.state.groupedAuthConnectionId, | |
userId: this.state.userId, | |
skipLock: true, | |
}); |
Bug: Vault Access Broken by Token Change
The SeedlessOnboardingController
's vault validation and parsing (#assertIsValidVaultData
, #parseVaultData
) were updated to require accessToken
and no longer accept authTokens
(or nodeAuthTokens
). This breaks backward compatibility, preventing existing users from unlocking their vaults and causing loss of account access, as no migration path is provided.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L1670-L1689
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 1670 to 1689 in 4f1f11b
*/ | |
#assertIsValidVaultData(value: unknown): asserts value is VaultData { | |
// value is not valid vault data if any of the following conditions are true: | |
if ( | |
!value || // value is not defined | |
typeof value !== 'object' || // value is not an object | |
!('toprfEncryptionKey' in value) || // toprfEncryptionKey is not defined | |
typeof value.toprfEncryptionKey !== 'string' || // toprfEncryptionKey is not a string | |
!('toprfPwEncryptionKey' in value) || // toprfPwEncryptionKey is not defined | |
typeof value.toprfPwEncryptionKey !== 'string' || // toprfPwEncryptionKey is not a string | |
!('toprfAuthKeyPair' in value) || // toprfAuthKeyPair is not defined | |
typeof value.toprfAuthKeyPair !== 'string' || // toprfAuthKeyPair is not a string | |
!('revokeToken' in value) || // revokeToken is not defined | |
typeof value.revokeToken !== 'string' || // revokeToken is not a string | |
!('accessToken' in value) || // accessToken is not defined | |
typeof value.accessToken !== 'string' // accessToken is not a string | |
) { | |
throw new Error(SeedlessOnboardingControllerErrorMessage.VaultDataError); | |
} | |
} |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L1521-L1565
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 1521 to 1565 in 4f1f11b
*/ | |
#parseVaultData(data: unknown): { | |
toprfEncryptionKey: Uint8Array; | |
toprfPwEncryptionKey: Uint8Array; | |
toprfAuthKeyPair: KeyPair; | |
revokeToken: string; | |
accessToken: string; | |
} { | |
if (typeof data !== 'string') { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InvalidVaultData, | |
); | |
} | |
let parsedVaultData: unknown; | |
try { | |
parsedVaultData = JSON.parse(data); | |
} catch { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InvalidVaultData, | |
); | |
} | |
this.#assertIsValidVaultData(parsedVaultData); | |
const rawToprfEncryptionKey = base64ToBytes( | |
parsedVaultData.toprfEncryptionKey, | |
); | |
const rawToprfPwEncryptionKey = base64ToBytes( | |
parsedVaultData.toprfPwEncryptionKey, | |
); | |
const parsedToprfAuthKeyPair = JSON.parse(parsedVaultData.toprfAuthKeyPair); | |
const rawToprfAuthKeyPair = { | |
sk: BigInt(parsedToprfAuthKeyPair.sk), | |
pk: base64ToBytes(parsedToprfAuthKeyPair.pk), | |
}; | |
return { | |
toprfEncryptionKey: rawToprfEncryptionKey, | |
toprfPwEncryptionKey: rawToprfPwEncryptionKey, | |
toprfAuthKeyPair: rawToprfAuthKeyPair, | |
revokeToken: parsedVaultData.revokeToken, | |
accessToken: parsedVaultData.accessToken, | |
}; | |
} |
Bug: Token Assignment Inconsistency
The authenticate
method's accessToken
and metadataAccessToken
parameters are typed as required string
s, but their assignment to the controller state is conditional. This inconsistency can lead to falsy string values being ignored and is a breaking change for existing callers who now must provide these parameters.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L313-L320
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 313 to 320 in 4f1f11b
} | |
if (accessToken) { | |
state.accessToken = accessToken; | |
} | |
if (metadataAccessToken) { | |
state.metadataAccessToken = metadataAccessToken; | |
} | |
}); |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L264-L277
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 264 to 277 in 4f1f11b
*/ | |
async authenticate(params: { | |
idTokens: string[]; | |
accessToken: string; | |
metadataAccessToken: string; | |
authConnection: AuthConnection; | |
authConnectionId: string; | |
userId: string; | |
groupedAuthConnectionId?: string; | |
socialLoginEmail?: string; | |
refreshToken?: string; | |
revokeToken?: string; | |
skipLock?: boolean; | |
}) { |
Bug: Inconsistent Auth Validation Causes Failures
The authenticate
method now requires accessToken
and metadataAccessToken
parameters, a breaking change for existing callers. Additionally, metadataAccessToken
is strictly validated as required for all authenticated users, which may cause authentication failures for existing users. This introduces inconsistent validation, as accessToken
is required for vault creation but not consistently asserted for authenticated users.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L1608-L1627
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 1608 to 1627 in 4f1f11b
) { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InsufficientAuthToken, | |
); | |
} | |
if (!('refreshToken' in value) || typeof value.refreshToken !== 'string') { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InvalidRefreshToken, | |
); | |
} | |
if ( | |
!('metadataAccessToken' in value) || | |
typeof value.metadataAccessToken !== 'string' | |
) { | |
throw new Error( | |
SeedlessOnboardingControllerErrorMessage.InvalidMetadataAccessToken, | |
); | |
} | |
} |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts#L264-L277
core/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Lines 264 to 277 in 4f1f11b
*/ | |
async authenticate(params: { | |
idTokens: string[]; | |
accessToken: string; | |
metadataAccessToken: string; | |
authConnection: AuthConnection; | |
authConnectionId: string; | |
userId: string; | |
groupedAuthConnectionId?: string; | |
socialLoginEmail?: string; | |
refreshToken?: string; | |
revokeToken?: string; | |
skipLock?: boolean; | |
}) { |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Explanation
This update introduces two new tokens, accessToken and metadataAccessToken, to the SeedlessOnboardingController.
These tokens are essential for pairing with the profile sync auth service and accessing the metadata service before the vault is created or unlocked.
The changes include updates to the controller state, methods for handling these tokens, and corresponding tests to ensure proper functionality and error handling when tokens are missing.
Additionally, the vault data structure has been modified to include the new accessToken. Tests have been added to verify the correct behavior of the controller with respect to these tokens.
Removed unused nodeAuthTokens from vault.
Toprf sdk updated to use metadataAccessToken
References
Changelog
Checklist