Remove primary key.

[yantakus]

Closes #409.

[yantakus]

flowcrypt-browser/extension/js/common/core/crypto/key.ts

Lines 75 to 80 in da921ab

	export interface KeyInfo extends PrvKeyInfo {
	// this cannot be Pubkey has it's being passed to localstorage
	public: string;
	fingerprint: string;
	primary: boolean;
	}

flowcrypt-browser/test/source/platform/store/key-store.ts

Lines 9 to 13 in a79419e

	export interface KeyInfo extends PrvKeyInfo {
	public: string;
	fingerprint: string;
	primary: boolean;
	}

Is there any particular reason for duplicating these?

[tomholub]

I would remove the one in flowcrypt-browser/test/source/platform/store/key-store.ts. Then you can import in in tests from test/source/core which is a symlink to the extension/js/common/core (make sure not to import from the extension folder directly from tests, it would mess up build).

Thanks for noticing

[yantakus]

@tomholub I'm not getting this:

This following one is probably the trickiest. First, you'll have to figure out if any code is opening that frame with primary as one of the longid strings. If yes, please make that calling code instead give it the longid of the first stored key.

Could you please explain this in detail?

[yantakus]

I'm signing off till Monday. Just removed the primary key from everywhere and pushed in case you want to see the code in order to help me with the above question.

[tomholub]

@tomholub I'm not getting this:

This following one is probably the trickiest. First, you'll have to figure out if any code is opening that frame with primary as one of the longid strings. If yes, please make that calling code instead give it the longid of the first stored key.

Could you please explain this in detail?

You found a code that you want to refactor which is the following:

    const allPrivateKeys = await KeyStore.get(this.acctEmail);
    this.myPrivateKeys = allPrivateKeys
      .filter(ki => this.longids.includes(ki.longid) || (ki.primary && this.longids.includes('primary')));

You no longer want to deal with having a primary string because you are refactoring key storage to have no concept of primary keys. You'd rather keys be identified directly by id.

This is in passphrase.ts which is the main script in passphrase.htm which itself is rendered inside other pages as a modal. Also, this.longids comes from url params:

this.longids = Assert.urlParamRequire.string(uncheckedUrlParams, 'longids').split(',');

Some page is hosting / opening / rendering that passphrase.htm page that's settings that value. Let's take a look:

Brings us to:

  public srcPassphraseDialog = (longids: string[] = [], type: PassphraseDialogType) => {
    return this.frameSrc(this.extUrl('chrome/elements/passphrase.htm'), { type, longids });
  }

Brings us to:

Example

  public dialogPassphrase = (longids: string[], type: PassphraseDialogType) => {
    return this.divDialog_DANGEROUS(this.iframe(this.srcPassphraseDialog(longids, type), ['medium'], { scrolling: 'no' }), 'dialog-passphrase'); // xss-safe-factory
  }

Notice the longids, so far it's just passing the values around, let's go deeper:

    BrowserMsg.addListener('passphrase_dialog', async ({ longids, type }: Bm.PassphraseDialog) => {
      if (!$('#cryptup_dialog').length) {
        const factory = new XssSafeFactory(this.acctEmail!, this.tabId);
        $('body').append(factory.dialogPassphrase(longids, type)); // xss-safe-factory
      }
    });

still not there, now look for the passphrase_dialog browser message - whoever calls that. BrowserMsg is how scripts from various frames communicate together in the context of a browser extension.

    passphraseDialog: (dest: Bm.Dest, bm: Bm.PassphraseDialog) => BrowserMsg.sendCatch(dest, 'passphrase_dialog', bm),

See who calls that:

Quite a few places (I'm just going along a single path here for example, you'll need to ivestigate all of the other code paths above where I'm just using one for demonstration).

If I randomly pick one of them, what I'll see is eg this:

BrowserMsg.send.passphraseDialog(this.view.parentTabId, { type: 'message', longids: result.longids.needPassphrase });

That looks fine, result.longids are really a list of longids. I'm looking for some code to say something like longids: ['primary']. Let's see:

At least I found one, but I'm only looking causally for this demo

This code:

    this.view.S.cached('prompt').find('a.action_open_passphrase_dialog').click(this.view.setHandler(() => {
      BrowserMsg.send.passphraseDialog(this.view.parentTabId, { type: 'draft', longids: ['primary'] });
    }));

You'd want to instead grab keys from local storage, and get the first one, then just say longids: [firstKey.longid]

You don't want the 'primary' string be co-mingled with actual ids.

[tomholub]

There's one more problem we may like to address while we're at it.

The KeyStore also takes a primary string. Since you are removing KeyInfo.primary, then this code will be affected too.

  public static get = async (acctEmail: string, fingerprints?: string[]): Promise<KeyInfo[]> => {
    const stored = await AcctStore.get(acctEmail, ['keys']);
    const keys: KeyInfo[] = stored.keys || [];
    if (!fingerprints) {
      return keys;
    }
    return keys.filter(ki => {
      if (fingerprints.includes('primary') && ki.primary) {
        return true;
      }
      if (fingerprints.includes(ki.fingerprint)) {
        return true;
      }
      return false;
    });
  }

Instead of all of the KeyStore.get(this.view.acctEmail, ['primary']); calls, you could add a method KeyStore.getFirst(this.view.acctEmail); (which is really what it did) and refactor the KeyStore.get method accordingly.

[tomholub]

I'm signing off till Monday. Just removed the primary key from everywhere and pushed in case you want to see the code in order to help me with the above question.

👍 good approach - definitely helps to see something specific. Enjoy your weekend.

[tomholub]

All looks good - I have added last few adjustments to make. This change snowballed quite a bit, but it's good that this work is getting done.

[tomholub]

I've added you to semaphore so you can also address the failing tests. https://flowcrypt.semaphoreci.com/workflows/3cceb1b5-98bd-43ba-b064-a27d9dcd564d

This is a result of the typing mismatch mentioned above.

[yantakus]

I've added you to semaphore so you can also address the failing tests. https://flowcrypt.semaphoreci.com/workflows/3cceb1b5-98bd-43ba-b064-a27d9dcd564d

This is a result of the typing mismatch mentioned above.

This error happens here:

flowcrypt-browser/extension/chrome/settings/modules/my_key.ts

Line 48 in a74b439

this.pubKey = await KeyUtil.parse(this.keyInfo.public);

This is the result of KeyStore.get, not the new method. The method's signature hasn't changed (it could return an empty array before as well and the above test could fail as well). What do I do in such a case?

[tomholub]

This is the result of KeyStore.get, not the new method. The method's signature hasn't changed (it could return an empty array before as well and the above test could fail as well). What do I do in such a case?

I see. I think you'll have to look at the context of the test, and understand why does it get to that state.

Check this out:

    this.fingerprint = Assert.urlParamRequire.optionalString(uncheckedUrlParams, 'fingerprint') || 'primary';

Maybe it's getting primary as a string from somewhere again?

[tomholub]

Or rather, primary is used as a default value, so you'd want to change that to a required string, and make sure it's provided by the page that renders it.

[yantakus]

@tomholub All tests pass. Please, check the code.

[tomholub]

Almost there, some fixes left, and some stuff will be to create issues for later.

[tomholub]

I'll go do some manual testing.

Code looks good. There seems to still be one test failure left.

[tomholub]

Manual testing looks good

[tomholub]

I'll fix it