Review of attachment 9169818 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. Some general comments below. I think it's not great that the usefulness of the feature require syncing a file into the Thunderbird directory. Perhaps it would be useful to define a scheme by which such configuration is loaded, so that it's applicable also across domains. Or is there something out there already? E.g. an header with a URL to the configuration file? `X-OpenPGP-Multiparty-Config: https://example.com/openpgp-multiparty.json; Last-Modified="Fri, 14 Aug 2020 07:28:00 GMT"` It would only be allowed to host for @example.com lists on example.com Thunderbird could then fetch this as needed. ::: mail/extensions/openpgp/content/modules/encryption.jsm @@ +127,5 @@ > + // Expand groups if necessary > + var toExpanded = []; > + for (let k = 0; k < result.to.length; k++) { > + var group = EnigmailKeyRing.expandGroup(result.to[k]); > + group.forEach(recipient => toExpanded.push(recipient)); This could use spread (...) so toExpanded.push(... EnigmailKeyRing.expandGroup(result.to[k])); ::: mail/extensions/openpgp/content/modules/keyRing.jsm @@ +1269,5 @@ > + // Do group expansion > + var addressesExpanded = []; > + for (let k = 0; k < addresses.length; k++) { > + var group = EnigmailKeyRing.expandGroup(addresses[k]); > + group.forEach(recipient => addressesExpanded.push(recipient)); same here, could use spread @@ +1406,5 @@ > + loadGroupsFromConfig(); > + return gGroups; > + }, > + > + expandGroup(alias) { I think this should check it never overrides an email we do have a normal key for. @@ +1412,5 @@ > + let aliasFmt = "<" + alias + ">"; > + let recipients = []; > + if (!groups[aliasFmt]) { // is not a group > + return [alias]; > + } else { nit: no else after return please @@ +1413,5 @@ > + let recipients = []; > + if (!groups[aliasFmt]) { // is not a group > + return [alias]; > + } else { > + groups[aliasFmt].forEach(recipient => { nit: forEach is not as efficient as for .. of, so for these cases prefer something like for (let recipient of groups[aliasFmt]) { @@ +1556,5 @@ > EnigmailLog.ERROR("keyRing.jsm: loadKeyList: exception: " + ex.toString()); > } > } > > +function loadGroupsFromConfig() { I think it would be better to have a JSON based configuration file, like ``` [ { name: "Group 1", alias: list@example.com, recipients: [ "foo@example.com", "bar@example.com" ] }, { name: "Group 2", alias: list2@example.com, recipients: [ "a@example.com", "b@example.com" ] } ] ```
Bug 1644085 Comment 5 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9169818 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. Some general comments below. I think it's not great that the usefulness of the feature require syncing a file into the Thunderbird directory. Perhaps it would be useful to define a scheme by which such configuration is loaded, so that it's applicable also across domains. Or is there something out there already? E.g. an header with a URL to the configuration file? `X-OpenPGP-Multiparty-Config: https://example.com/openpgp-multiparty.json; Last-Modified="Fri, 14 Aug 2020 07:28:00 GMT"` It would only be allowed to host for @example.com lists on example.com Thunderbird could then fetch this as needed. ::: mail/extensions/openpgp/content/modules/encryption.jsm @@ +127,5 @@ > + // Expand groups if necessary > + var toExpanded = []; > + for (let k = 0; k < result.to.length; k++) { > + var group = EnigmailKeyRing.expandGroup(result.to[k]); > + group.forEach(recipient => toExpanded.push(recipient)); This could use spread (...) so toExpanded.push(... EnigmailKeyRing.expandGroup(result.to[k])); ::: mail/extensions/openpgp/content/modules/keyRing.jsm @@ +1269,5 @@ > + // Do group expansion > + var addressesExpanded = []; > + for (let k = 0; k < addresses.length; k++) { > + var group = EnigmailKeyRing.expandGroup(addresses[k]); > + group.forEach(recipient => addressesExpanded.push(recipient)); same here, could use spread @@ +1406,5 @@ > + loadGroupsFromConfig(); > + return gGroups; > + }, > + > + expandGroup(alias) { I think this should check it never overrides an email we do have a normal key for. @@ +1412,5 @@ > + let aliasFmt = "<" + alias + ">"; > + let recipients = []; > + if (!groups[aliasFmt]) { // is not a group > + return [alias]; > + } else { nit: no else after return please @@ +1413,5 @@ > + let recipients = []; > + if (!groups[aliasFmt]) { // is not a group > + return [alias]; > + } else { > + groups[aliasFmt].forEach(recipient => { nit: forEach is not as efficient as for .. of, so for these cases prefer something like for (let recipient of groups[aliasFmt]) { @@ +1556,5 @@ > EnigmailLog.ERROR("keyRing.jsm: loadKeyList: exception: " + ex.toString()); > } > } > > +function loadGroupsFromConfig() { I think it would be better to have a JSON based configuration file, like ``` [ { name: "Group 1", alias: "list@example.com", recipients: [ "foo@example.com", "bar@example.com" ] }, { name: "Group 2", alias: "list2@example.com", recipients: [ "a@example.com", "b@example.com" ] } ] ```