refactor: streamline token management and credential updates#250
refactor: streamline token management and credential updates#250
Conversation
| const tokenScheduler = new TokenScheduler(5, tokens, onUserUnauthorized); | ||
| export async function createTokenScheduleWithRetry() { | ||
| const { newToken } = getCredentials(); | ||
| const tokenScheduler = new TokenScheduler(5, newToken, onUserUnauthorized); |
There was a problem hiding this comment.
This parameter defines how many days in advance the token should be renewed. It was previously set to 5 days. Do you think that's too long, or is it reasonable?
There was a problem hiding this comment.
Its fine its just that we shouldnt use magic numbers, we need to give it context so that we can know properly the justification of the number so that we dont forget
| updateCredentials({ newToken }); | ||
|
|
||
| return right([token, newToken]); | ||
| return right([newToken]); |
There was a problem hiding this comment.
Is there any particular reason we need to return it as array? we are only passing one item
There was a problem hiding this comment.
There is no reason to leave it like that. Before, an array was returned because the newToken and bearerToken were delivered.
| } | ||
|
|
||
| try { | ||
| if (!safeStorage.isEncryptionAvailable()) { |
There was a problem hiding this comment.
On Linux, this can happen in at least two relevant scenarios:
-
No keyring/secret store is available —
safeStorageon Linux relies on a backend such as GNOME Keyring or KWallet. If neither is installedisEncryptionAvailable()will returnfalse. -
Missing
libsecretdependency — Electron useslibsecretunder the hood to communicate with the keyring. On minimal Linux distributions where this library is not installed, encryption operations will fail entirely, regardless of whether a keyring daemon is running.
This guard ensures the application handles those cases gracefully instead of throwing an unhandled error at runtime.
There was a problem hiding this comment.
Maybe we can add a ticket on this topic so that we dont move out of the scope this pr, but maybe it could be nice to have some sort of check at startup so that if a user can access safeStorage, we push an error (Pretty much the same as the compatibility with nautilus, libfuse..etc) since this issue is about compatibility
What do you think?
…and clean up updateCredentials tests
|
2 participants
What is Changed / Added
Everything related to legacyToken is removed, and the mnemonic is encrypted, solving a security issue.
Why