Feat/seedless refresh token #15
Conversation
tuna1207
requested review from
chaitanyapotti,
ieow,
himanshuchawla009 and
lwin-kyaw
tuna1207
force-pushed
the
feat/seedless-refresh-token
branch
from
aaad9ed to
6887c51
Compare
| @@ -110,6 +115,10 @@ const seedlessOnboardingMetadata: StateMetadata<SeedlessOnboardingControllerStat | |||
| persist: true, | |||
| anonymous: true, | |||
| }, | |||
| refreshToken: { | |||
There was a problem hiding this comment.
should it be stored inside the vault as encrypted?
There was a problem hiding this comment.
@lwin-kyaw ah yes, it is stored inside the vault, this is only stored in state for convienience of usage in multiple places, it's not persisted.
It's only restored to state after unlockVaultAndGetBackupEncKey similar to other vault secret
There was a problem hiding this comment.
From what I saw, only nodeAuthTokens (and authPubKey?) is stored inside both vault and state. I think we should remove it from the vault, if we intent to use as plain text.
And for the refreshToken, I don't see any places that is used without needing to decrypt the vault. So, I think it should be only inside the vault.
| }) { | ||
| return await this.#withControllerLock(async () => { | ||
| const doAuthenticate = async () => { |
There was a problem hiding this comment.
can we refactor this inner function to class method, (private maybe), for the better readibility?
There was a problem hiding this comment.
@lwin-kyaw hmm i'm not sure about this, since the method is already authenticate and keeping the logic in other methods would actually make it harder to follow i think, what do you think ?
There was a problem hiding this comment.
I assume that this is due to the skipLock param. May I know why it is needed?
| @@ -196,8 +212,10 @@ export class SeedlessOnboardingController<EncryptionKey> extends BaseController< | |||
| userId: string; | |||
| groupedAuthConnectionId?: string; | |||
| socialLoginEmail?: string; | |||
| refreshToken: string; | |||
| skipLock?: boolean; | |||
There was a problem hiding this comment.
@lwin-kyaw this one is for executeWithRefreshToken to authenticate again when refreshing the token since executeWithRefreshToken should already be inside a lock
There was a problem hiding this comment.
Is this method, executeWithRefreshToken in the controller?
Anyway, I don't think we should skip the lock, the lock state represents the wallet access and also synced with other controllers.
There was a problem hiding this comment.
Actually, we don't need to skip the lock. If the wallet is unlocked (user has logged in), the isLockUnlocked is already set to true.
And the refresh token can be retrieved from the vault (either with password or cachedEncryptionKey in state), and can proceed to the authenticateWithRefreshToken
There was a problem hiding this comment.
@lwin-kyaw yes, by default we don't skip the lock, we skip only when it's already inside another lock
| @@ -845,6 +900,7 @@ export class SeedlessOnboardingController<EncryptionKey> extends BaseController< | |||
| nodeAuthTokens: NodeAuthTokens; | |||
| toprfEncryptionKey: Uint8Array; | |||
| toprfAuthKeyPair: KeyPair; | |||
| refreshToken?: string; | |||
There was a problem hiding this comment.
May I know what's the use for the refreshToken here? Seems like it's never used.
And refreshToken can be retrieved from the encrypted vault?
There was a problem hiding this comment.
@lwin-kyaw yes it's in the vault, this function decrypt the vault and return the refreshToken from the vault
There was a problem hiding this comment.
Then we should remove this from the method params?
| @@ -1037,6 +1103,7 @@ export class SeedlessOnboardingController<EncryptionKey> extends BaseController< | |||
| authTokens: this.state.nodeAuthTokens, | |||
| toprfEncryptionKey, | |||
| toprfAuthKeyPair, | |||
| refreshToken: this.state.refreshToken, | |||
There was a problem hiding this comment.
This can be provided from the method params, so that we don't need to store it in the state.
There was a problem hiding this comment.
@lwin-kyaw we currently only receive refreshToken from authenticate method, and these methods are using refreshToken in #createNewVaultWithAuthData: createToprfKeyAndBackupSeedPhrase, fetchAllSeedPhrases, changePassword, syncLatestGlobalPassword
i'm not sure if it's a good idea to always redo #unlockVaultAndGetBackupEncKey in all of the above methods to pass refreshToken in again
| */ | ||
| async refreshNodeAuthTokens(): Promise<void> { | ||
| this.#assertIsUnlocked(); | ||
| const { refreshToken } = this.state; |
There was a problem hiding this comment.
Should it be retrieved from the vault?
There was a problem hiding this comment.
@lwin-kyaw yes, in this case the vault is decrypted and refreshToken is set in temporal state
Explanation
Add refresh token handling to SeedlessOnboardingController
References
Changelog
Checklist