Merge pull request #143 from Cardinal-Cryptography/A0-3946-remember-password-not-working

A0-3946: Fix password saving function not working properly

Author: rAskVAL

packages/extension-base/src/background/handlers/Extension.spec.ts

@@ -33,6 +33,11 @@ const authUrls = {
 
 chromeStub.windows.getAll.resolves([]);
 
+// @ts-expect-error the "sinon-chrome" mocking library does not provide stubs for session storage, so we have to append them ourselves
+chromeStub.storage.session = {
+  get: () => Promise.resolve({})
+};
+
 const stubChromeStorage = (data: Record<string, unknown> = {}) => chromeStub.storage.local.get.resolves({
   authUrls,
   ...data

packages/extension-base/src/background/handlers/Extension.ts

@@ -18,13 +18,12 @@ import { accounts as accountsObservable } from '@polkadot/ui-keyring/observable/
 import { assert, isHex, u8aToHex } from '@polkadot/util';
 import { keyExtractSuri, mnemonicGenerate, mnemonicValidate } from '@polkadot/util-crypto';
 
+import chromeStorage from './chromeStorage';
 import { POPUP_CREATE_WINDOW_DATA } from './consts';
 import { openCenteredWindow } from './helpers';
 import State from './State';
 import { createSubscription, unsubscribe } from './subscriptions';
 
-type CachedUnlocks = Record<string, number>;
-
 type GetContentPort = (tabId: number) => chrome.runtime.Port
 
 const SEED_DEFAULT_LENGTH = 12;
@@ -40,12 +39,9 @@ function isJsonPayload (value: SignerPayloadJSON | SignerPayloadRaw): value is S
 }
 
 export default class Extension {
-  readonly #cachedUnlocks: CachedUnlocks;
-
   readonly #state: State;
 
   constructor (state: State) {
-    this.#cachedUnlocks = {};
     this.#state = state;
   }
 
@@ -137,20 +133,26 @@ export default class Extension {
     return true;
   }
 
-  private refreshAccountPasswordCache (pair: KeyringPair): number {
+  private async refreshAccountPasswordCache (pair: KeyringPair): Promise<{remainingTime: number, cachedPassword?: string}> {
     const { address } = pair;
 
-    const savedExpiry = this.#cachedUnlocks[address] || 0;
-    const remainingTime = savedExpiry - Date.now();
+    const savedPasswords = await chromeStorage.getItem('password-cache') ?? {};
+    const expiresAt = savedPasswords[address]?.expiresAt ?? 0;
+    const remainingTime = expiresAt - Date.now();
+    const cachedPassword = savedPasswords[address]?.password;
 
-    if (remainingTime < 0) {
-      this.#cachedUnlocks[address] = 0;
-      pair.lock();
+    if (remainingTime > 0) {
+      return { remainingTime, cachedPassword };
+    }
 
-      return 0;
+    if (address in savedPasswords) {
+      delete savedPasswords[address];
+      await chromeStorage.setItem('password-cache', () => (savedPasswords));
     }
 
-    return remainingTime;
+    pair.lock();
+
+    return { remainingTime: 0 };
   }
 
   private accountsShow ({ address, isShowing }: RequestAccountShow): boolean {
@@ -334,7 +336,7 @@ export default class Extension {
     };
   }
 
-  private async signingApprovePassword ({ id, password, savePass }: RequestSigningApprovePassword, getContentPort: GetContentPort): Promise<void> {
+  private async signingApprovePassword ({ id, password: inputPassword, savePass }: RequestSigningApprovePassword, getContentPort: GetContentPort): Promise<void> {
     const queued = await this.#state.getSignRequest(id);
 
     assert(queued, 'Unable to find request');
@@ -351,10 +353,12 @@ export default class Extension {
       throw error;
     }
 
-    this.refreshAccountPasswordCache(pair);
+    const { cachedPassword, remainingTime } = await this.refreshAccountPasswordCache(pair);
+
+    const password = cachedPassword ?? inputPassword;
 
     // if the keyring pair is locked, the password is needed
-    if (pair.isLocked && !password) {
+    if (!password) {
       const error = new Error('Password needed to unlock the account');
 
       await this.#state.removeSignRequest(id);
@@ -408,13 +412,14 @@ export default class Extension {
         .createType('ExtrinsicPayload', payload, { version: payload.version })
         .sign(pair);
 
-    if (savePass) {
+    if (savePass && remainingTime <= 0) {
       // unlike queued.account.address the following
       // address is encoded with the default prefix
       // which what is used for password caching mapping
-      this.#cachedUnlocks[pair.address] = Date.now() + PASSWORD_EXPIRY_MS;
-    } else {
-      pair.lock();
+
+      await chromeStorage.setItem('password-cache', (savedPasswords) => (
+        { ...savedPasswords, [pair.address]: { password, expiresAt: Date.now() + PASSWORD_EXPIRY_MS } }
+      ));
     }
 
     await this.#state.removeSignRequest(id);
@@ -449,11 +454,11 @@ export default class Extension {
 
     assert(pair, 'Unable to find pair');
 
-    const remainingTime = this.refreshAccountPasswordCache(pair);
+    const { remainingTime } = await this.refreshAccountPasswordCache(pair);
 
     return {
-      isLocked: pair.isLocked,
-      remainingTime
+      remainingTime,
+      isLocked: remainingTime <= 0
     };
   }
 

packages/extension-base/src/background/handlers/chromeStorage.ts

@@ -0,0 +1,32 @@
+import { z } from 'zod';
+
+// use namespaces to add typization for your storage data
+const namespaces = { 'password-cache': z.record(z.string(), z.object({ password: z.string(), expiresAt: z.number() })) } as const satisfies Record<string, z.ZodSchema>;
+
+export const setItem = async <N extends keyof typeof namespaces>(namespace: N, updater: (currData?: z.TypeOf<typeof namespaces[N]>) => z.TypeOf<typeof namespaces[N]>) => {
+  const currentData = await getItem(namespace);
+
+  await chrome.storage.session.set({ [namespace]: updater(currentData) });
+};
+
+export const getItem = async <N extends keyof typeof namespaces>(namespace: N): Promise<z.infer<typeof namespaces[N]> | undefined> => {
+  const { [namespace]: cachedData } = await chrome.storage.session.get(namespace);
+
+  try {
+    return namespaces[namespace].parse(cachedData);
+  } catch {
+    return undefined;
+  }
+};
+
+export const removeItem = async <N extends keyof typeof namespaces>(namespace: N) => {
+  await chrome.storage.session.remove(namespace);
+};
+
+const chromeStorage = {
+  getItem,
+  setItem,
+  removeItem
+};
+
+export default chromeStorage;

packages/extension-base/src/background/types.ts

@@ -306,8 +306,8 @@ export interface RequestSigningIsLocked {
 }
 
 export interface ResponseSigningIsLocked {
-  isLocked: boolean;
   remainingTime: number;
+  isLocked: boolean;
 }
 
 export type RequestSigningSubscribe = null;

packages/extension-ui/src/Popup/Signing/Request/SignArea.tsx

@@ -45,7 +45,9 @@ function SignArea({ buttonText, className, error, isExternal, isFirst, isLast, s
 
           // if the account was unlocked check the remember me
           // automatically to prolong the unlock period
-          !isLocked && setSavePass(true);
+          if(remainingTime > 0) {
+            setSavePass(true);
+          }
         })
         .catch((error: Error) => console.error(error));
 
@@ -92,20 +94,15 @@ function SignArea({ buttonText, className, error, isExternal, isFirst, isLast, s
 
   const StyledCheckbox = styled(Checkbox)`
     margin-left: 8px;
-`;
+  `;
 
   const RememberPasswordCheckbox = () => (
     <StyledCheckbox
       checked={savePass}
-      label={
-        isLocked
-          ? t<string>('Remember password for {{expiration}} minutes', {
-              replace: { expiration: PASSWORD_EXPIRY_MIN }
-            })
-          : t<string>('Extend the period without password by {{expiration}} minutes', {
-              replace: { expiration: PASSWORD_EXPIRY_MIN }
-            })
-      }
+      disabled={!isLocked}
+      label={t<string>('Remember password for {{expiration}} minutes', {
+        replace: { expiration: PASSWORD_EXPIRY_MIN }
+      })}
       onChange={setSavePass}
     />
   );

packages/extension-ui/src/components/Checkbox.tsx

@@ -9,6 +9,7 @@ import Subtract from '../assets/subtract.svg';
 
 interface Props {
   checked: boolean;
+  disabled?: boolean;
   indeterminate?: boolean;
   className?: string;
   label: ReactNode;
@@ -20,6 +21,7 @@ interface Props {
 function Checkbox({
   checked,
   className,
+  disabled,
   indeterminate,
   label,
   onChange,
@@ -49,6 +51,7 @@ function Checkbox({
         {label}
         <input
           checked={checked && !indeterminate}
+          disabled={disabled}
           onBlur={() => setIsFocusVisible(false)}
           onChange={_onChange}
           onClick={_onClick}
@@ -102,7 +105,7 @@ const Label = styled.label<{ isOutlined: boolean; variant: NonNullable<Props['va
     cursor: pointer;
     user-select: none;
     padding-left: ${({ variant }) => variantToStyles[variant].paddingLeft};;
-    padding-top: 1px;
+    padding-top: 2px;
     color: ${({ theme }) => theme.subTextColor};
     font-size: ${({ theme }) => theme.fontSize};
     line-height: ${({ theme }) => theme.lineHeight};
@@ -115,6 +118,10 @@ const Label = styled.label<{ isOutlined: boolean; variant: NonNullable<Props['va
       outline-style: auto;
     }
 
+    :has(input:disabled) {
+      color: ${({ theme }) => theme.disabledTextColor}
+    }
+
     /* :has selector is the pure css solution to this problem, but doesn't have (so far) enough support ;( */
     ${({ isOutlined }) => (isOutlined ? 'outline-style: auto;' : '')}
 `;
@@ -188,6 +195,15 @@ export default styled(Checkbox)(
         mask-size: cover;
       }
     }
+
+    input:disabled ~ .checkbox-ui {
+      background: ${theme.disabledTextColor};
+      box-shadow: 0 0 0 1px ${theme.disabledTextColor};
+
+      &:hover {
+        box-shadow: 0 0 0 1px ${theme.disabledTextColor};
+      }
+    }
   }
 
   &:hover {