1667054 - After importing secret key OpenGPG Key Manager shows properties of wrong key.

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect, P1)

Description

[Lev Serebryakov]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

1. Open Thunderbird 78.3.0 (candidate).
2. Open Tools|OpenPGP Key Manager.
3. Select File|Import Secret Key(s) From File.
4. Select gpg-exported secret key file.
5. Ensure, that one key is found and marked with checkbox.
6. Click Continue.
7. On dialog with (successful results) click Key Properties.

Actual results:

Properties of some random public key from key storage are shown.

Expected results:

Properties of freshly-imported personal secret key are shows,

Comment 1

[Lev Serebryakov]

Attachment: Results of import

Comment 2

[Lev Serebryakov]

Attachment: Shown properties of some random key

Comment 3

[Kai Engert (:KaiE:)]

Can anyone reproduce this bug?

Comment 4

[Wayne Mery (:wsmwk)]

Andreas, can you reproduce?

Comment 5

[Andreas]

Yes, I can reproduce this as described with Thunderbird 78.4.
The key properties shown in the last import dialog are the ones from the key which is preselected in the key manager list, not from the newly imported key.

This error doesn't occur when importing a key through the account settings tab.

Comment 6

[Alessandro Castellani [:aleca]]

Thanks for confirming this, I'll take care of fixing it.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1667054-key-properties.diff

The problem was in the enigmailKeyDetails() method of the Key Manager that was always and only opening properties of selected keys, ignoring the passed value of the recently imported key.

This patch fixes it, alongside a small UI fix.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9186381 [details] [diff] [review]
1667054-key-properties.diff

Pinging Magnus for a review as well just to see if I can get a quicker review since this is a quick fix but pretty important for error prevention.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 9186381 [details] [diff] [review]
1667054-key-properties.diff

>+function enigmailKeyDetails(keyId = null) {
>+  let keyList = getSelectedKeys();

if you already have a keyId, it it seems better to avoid calling getSelectedKeys()

>+  // Interrupt if we don't have a selected key nor a key was passed.
>+  if (!keyId && !keyList.length) {
>+    return;
>+  }
>+
>+  let selectedKey = keyId || gKeyList[keyList[0]].keyId;
>+  if (EnigmailWindows.openKeyDetails(window, selectedKey, false)) {
>+    refreshKeys();
>   }
> }

It's a matter of taste, but I think the following is cleaner, it avoids the unnecessary call, and it protects you against an null keyId in gKeyList:

  // Interrupt if we don't have a selected key nor a key was passed.
  if (!keyId) {
    let keyList = getSelectedKeys();
    if (!keyList.length) {
      return;
    }
    keyId = gKeyList[keyList[0]].keyId;
  }
  if (!keyId) {
    return;
  }
  if (EnigmailWindows.openKeyDetails(window, keyId, false)) {
    refreshKeys();
  }

Comment 10

[Magnus Melin [:mkmelin]]

The second if (!keyId) check shouldn't be necessary.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1667054-key-properties.diff

Thanks for the review

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/945a6cae42e2
Fix incorrect key reported in Key Properties dialog after importing secret key from the Key manager. r=KaiE

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9187008 [details] [diff] [review]
1667054-key-properties.diff

[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: confusing import experience
Testing completed (on c-c, etc.): c-c + beta
Risk to taking this patch (and alternatives if risky): fairly safe

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9187008 [details] [diff] [review]
1667054-key-properties.diff

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/ca72466c8e7d