Skip to content

Commit

Permalink
fix(KeychainLock): support address args with any casing (#4164)
Browse files Browse the repository at this point in the history
### Description

[Viem's
account.address](https://github.com/valora-inc/wallet/blob/main/src/viem/getLockableWallet.ts#L59)
returns checksummed address, which doesn't work with KeychainLock, which
uses lowercased addresses. This PR updates KeychainLock to support
address args with any casing.
We could also update the viem action to pass lowercased addresses, but
seems safest to do it in KeychainLock

### Test plan

Unit tests

### Related issues

- Part of ACT-786

### Backwards compatibility

Yes
  • Loading branch information
satish-ravi authored Sep 7, 2023
1 parent 089d1a8 commit 85f4a99
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 38 deletions.
105 changes: 75 additions & 30 deletions src/web3/KeychainLock.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { normalizeAddress } from '@celo/utils/lib/address'
import { KeychainLock, clearStoredAccounts } from 'src/web3/KeychainLock'
import * as mockedKeychain from 'test/mockedKeychain'
import { mockAccount } from 'test/values'
import { mockAccount2 } from 'test/values'

describe(clearStoredAccounts, () => {
it('only clears the stored accounts', async () => {
Expand Down Expand Up @@ -31,38 +31,83 @@ describe('KeychainLock', () => {
date = new Date()
mockedKeychain.clearAllItems()
})
it('isUnlocked returns false if the account has not been added', () => {
expect(lock.isUnlocked(mockAccount)).toBe(false)
})
it('isUnlocked returns false if the account has been added but not unlocked', () => {
lock.addAccount({ address: mockAccount, createdAt: date })
expect(lock.isUnlocked(mockAccount)).toBe(false)
})
it('isUnlocked returns false if the account has been added and unlocked but the duration has passed', async () => {
lock.addAccount({ address: mockAccount, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAccount)}`]: {
password: 'password',
},

const mockAddress = mockAccount2
const mockAddressInUpperCase = `0x${mockAccount2.substring(2).toUpperCase()}`
const mockAddressInLowerCase = mockAccount2.toLowerCase()

describe('isUnlocked', () => {
it('returns false if the account has not been added', () => {
expect(lock.isUnlocked(mockAddress)).toBe(false)
})
await lock.unlock(mockAccount, 'password', -1) // spoofing duration passed by using a negative duration
expect(lock.isUnlocked(mockAccount)).toBe(false)
})
it('isUnlocked returns true if the account has been added and unlocked and the duration has not passed', async () => {
lock.addAccount({ address: mockAccount, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAccount)}`]: {
password: 'password',
},
it('returns false if the account has been added but not unlocked', () => {
lock.addAccount({ address: mockAddress, createdAt: date })
expect(lock.isUnlocked(mockAddress)).toBe(false)
})
it('returns false if the account has been added and unlocked but the duration has passed', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAddress)}`]: {
password: 'password',
},
})
await lock.unlock(mockAddress, 'password', -1) // spoofing duration passed by using a negative duration
expect(lock.isUnlocked(mockAddress)).toBe(false)
})
it('returns true if the account has been added and unlocked and the duration has not passed', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAddress)}`]: {
password: 'password',
},
})
await lock.unlock(mockAddress, 'password', 100)
expect(lock.isUnlocked(mockAddress)).toBe(true)
// check for different casing
expect(lock.isUnlocked(mockAddressInLowerCase)).toBe(true)
expect(lock.isUnlocked(mockAddressInUpperCase)).toBe(true)
})
await lock.unlock(mockAccount, 'password', 100)
expect(lock.isUnlocked(mockAccount)).toBe(true)
})
it('unlock returns false if the account has not been added', async () => {
expect(await lock.unlock(mockAccount, 'password', 100)).toBe(false)

describe('unlock', () => {
it('returns false if the account has not been added', async () => {
expect(await lock.unlock(mockAddress, 'password', 100)).toBe(false)
})
it('throws if the account has been added but private key is not in the keychain', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
await expect(lock.unlock(mockAddress, 'password', 100)).rejects.toThrow()
})
it('returns true if account is unlocked with all uppercase address', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAddress)}`]: {
password: 'password',
},
})
expect(await lock.unlock(mockAddressInUpperCase, 'password', 100)).toBe(true)
})
})
it('unlock throws if has been added but private key is not in the keychain', async () => {
lock.addAccount({ address: mockAccount, createdAt: date })
await expect(lock.unlock(mockAccount, 'password', 100)).rejects.toThrow()

describe('updatePassphrase', () => {
it('returns false if the account has not been added', async () => {
expect(await lock.updatePassphrase(mockAddress, 'password', 'new-password')).toBe(false)
})

it('throws if the account has been added but private key is not in the keychain', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
await expect(lock.updatePassphrase(mockAddress, 'password', 'new-password')).rejects.toThrow()
})

it('returns true if the account and key is present and address is passed in different case', async () => {
lock.addAccount({ address: mockAddress, createdAt: date })
mockedKeychain.setItems({
[`account--${date.toISOString()}--${normalizeAddress(mockAddress)}`]: {
password: 'password',
},
})
expect(await lock.updatePassphrase(mockAddressInUpperCase, 'password', 'new-password')).toBe(
true
)
})
})
})
19 changes: 11 additions & 8 deletions src/web3/KeychainLock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,24 +196,25 @@ export class KeychainLock {
> = new Map()

addAccount(account: KeychainAccount) {
this.locks.set(account.address, { account })
this.locks.set(normalizeAddressWith0x(account.address), { account })
}

/**
* Unlocks an account for a given duration
* A duration of 0 means the account is unlocked indefinitely
* */
async unlock(address: string, passphrase: string, duration: number) {
const normalizedAddress = normalizeAddressWith0x(address)
Logger.debug(`${TAG}@unlock`, `Unlocking keychain for ${address} for ${duration} seconds`)
if (!this.locks.has(address)) {
if (!this.locks.has(normalizedAddress)) {
return false
}
const account = this.locks.get(address)!.account
const account = this.locks.get(normalizedAddress)!.account
const privateKey = await getStoredPrivateKey(account, passphrase)
if (!privateKey) {
return false
}
this.locks.set(address, {
this.locks.set(normalizedAddress, {
account,
unlockTime: Date.now(),
unlockDuration: duration,
Expand All @@ -222,10 +223,11 @@ export class KeychainLock {
}

isUnlocked(address: string): boolean {
if (!this.locks.has(address)) {
const normalizedAddress = normalizeAddressWith0x(address)
if (!this.locks.has(normalizedAddress)) {
return false
}
const { unlockTime, unlockDuration } = this.locks.get(address)!
const { unlockTime, unlockDuration } = this.locks.get(normalizedAddress)!
if (unlockDuration === undefined || unlockTime === undefined) {
return false
}
Expand All @@ -245,10 +247,11 @@ export class KeychainLock {
* @returns whether the update was successful
*/
async updatePassphrase(address: string, oldPassphrase: string, newPassphrase: string) {
if (!this.locks.has(address)) {
const normalizedAddress = normalizeAddressWith0x(address)
if (!this.locks.has(normalizedAddress)) {
return false
}
const account = this.locks.get(address)!.account
const account = this.locks.get(normalizedAddress)!.account
const privateKey = await getStoredPrivateKey(account, oldPassphrase)
if (!privateKey) {
return false
Expand Down

0 comments on commit 85f4a99

Please sign in to comment.