Ответ на пост 1083110 про зашифрованный мессенджер

1083110.md

>>1083110

Про код

Я тут мельком посмотрел код, и конечно, очень не хватает комментариев. Ну хотя бы банально перед классом написать 3 предложения про то, чем занимается этот класс, какие в нем основные методы. Также, не хватает описания принципа работа чата в README, хотя бы в общих чертах. У тебя-то это все есть в голове, но когда я открываю код, я вижу какие-то ключи, пароли, бекенды и приходится гадать, откуда они берутся и для чего нужны. Краткое описание архитектуры, API, схемы шифрования, очень бы пригодилось.

В SQL файле очень бы пригодились внешние ключи и комментарии.

https://github.com/someApprentice/chat/blob/master/public/js/crypter.js#L5

this.privateKey;

Что за странная конструкция? Должно быть this.privateKey = null;

var decrypted = openpgp.decrypt(options).then(function(decrypted) {
    return decrypted;
});

Здесь лучше использовать разные названия переменных для промиса и для самого значения, иначе возникает путаница и как следствие, легко допустить ошибку.

https://github.com/someApprentice/chat/blob/master/public/js/conversation.js#L53

+offset + +1

Немного странное место. Не многовато ли знаков "плюс"?

Ну и конечно класс Conversation тоже не очень понятный: в нем есть методы с непонятными названиями (runMessages), и сами методы большого размера и большой вложенности.

Также, мне кажется, что у тебя в контроллере находится код, относящийся ко view: вызовы jQuery вроде if ($(that.view.moremessages).length) стоило бы перенести во view и писать вместо этого if (that.view.hasMoreMessagesButton()), а еще лучше - if (this.canShowMoreMessages()), так как это довольно коряво, проверять наличие новых сообщений по наличию кнопки на экране. Должно ведь быть наоборот - модель/viewModel сама знает, есть ли новые сообщения.

Из-за того, что ты в контроллере смешиваешь код view, код установки обработчиков, асинхронные вызовы, все это выглядит сложно и запутанно.

Также, твой Crypter является по сути оберткой над openpgp, ничего от себя не добавляя. Непонятно, в чем смысл его существования. Смысл можно добавить, если Crypter начнет например работать не с абстрактными "сообщениями", а с объектами сообщений. Ты ведь скорее всего в будущем захочешь шифровать не только текст, но и метаданные сообщения, и тут удобно сделать 2 сущности - зашифрованное и расшифрованное сообщение.

По вопросам

нужно обработать ошибку дешифровки. Например, может быть передан неправильный passphrase.

Ну тут passphrase - это ведь пароль пользователя? Это должно отлавливаться по идее на этапе логина. Хотя конечно, ошибка все равно возможна, в том числе из-за какого-то дефекта в коде.

Как я понимаю, у тебя контроллер чата вызывает показ формы ввода пароля, чтобы расшифровать сообщения. Но я не уверен, что это правильно. Ввод пароля можно было бы сделать на отдельном экране, и пока пользователь его не введет (и пока мы не расшифруем приватный ключ), не пускать его на экран с чатом. Соответственно, тогда Conversation не пришлось бы заниматься запросом пароля - он бы получал через DI тот же Cryptor с уже расшифрованным приватным ключом.

А вот в сам метод дешифровки завязан на Обещании, и мне до сих пор не понятно как оно работает

Он завязан на промис из-за того, что расшифровка вынесена в web worker. Web worker - это отдельный поток выполнения (тред), в котором JS код выполняется в фоновом режиме параллельно с обычным JS кодом, не мешает ему работать, не тормозит интерфейс браузера. Веб воркер лишем доступа к Window и document (то есть к DOM/BOM), разумеется, он не видит и переменные из основного скрипта, и единственный способ обмениваться данными с ним - отправка сообщений.

Мы не можем вызвать метод в воркере напрямую. Вместо этого, когда мы хотим расшифровать данные, мы из основного треда посылаем сообщение воркеру, он его получает, расшифровывает данные, и присылает нам обратно сообщение с ними. Это все происходит асинхронно, потому функция расшифровки возвращает промис - так как она сама не знает, когда придет ответ.

Такой подход добавляет неудобств, но зато тяжелая операция расшифровки не мешает работе браузера, а на многоядерных процессорах позволяет задействовать отдельное ядро (а в идеале даже несколько ядер, если сделать несколько воркеров). Изоляция окружения воркера от основного потока сделана не просто так, она позволяет нам избежать проблем, которые возникают, когда 2 потока имеют доступ к одним и тем же данным.

Документация:

• https://developer.mozilla.org/ru/docs/DOM/Using_web_workers
• https://www.html5rocks.com/en/tutorials/workers/basics/

Типичный веб воркер выглядит так:

onmessage = function (event) {
    if (event.data.type == DECODE) {
        var result = decodeData(event.data.encoded);
        postMessage({ type: 'decoded', result: result });
    }

    if (event.data.type == ENCODE) {
        ....
    }

    ...
};

Недавно разработчиками Chrome придумана альтернатива для выполнения фоновых операций: tasklets - https://github.com/GoogleChromeLabs/tasklets

Зачем делать пустое обещание? И зачем дожидаться его выполнения, если мы уже вернули выполненное обещание, которое должно произойти мгновенно?

Мы создаем разрезолвленное в null обещание, но мы его не возвращаем. Ты невнимательно прочел код. Оно используется только, чтобы вызвать then. Вот как он работает, если упростить:

var nullPromise = Promise();
var decryptPromise = nullPromise.then(function () {
    var promise = decryptMessageAsync();
    return promise;
});
return decryptPromise;

Метод nullPromise.then создает новое обещание, которое будет разрезолвлено после того, как будет выполнен метод then. Тут вызов decryptMessageAsync() скорее всего возвращает тоже промис, и decryptPromise будет разрезолвлен, когда разрезолвится этот промис.

Вопрос, почему мы пишем Promise.resolve().then() вместо того, чтобы сразу вызвать decryptMessageAsync? Я думаю, это для того, чтобы отлавливать исключения и превратить их в rejected promise. Там ведь есть код:

throw new Error('Invalid session key for decryption.');

Без обертки в then() это исключение вылетало бы вверх, а авторы хотят, чтобы оно приводило к отклонению (как перевести reject?) обещания.

Затем отфилтрует какие-то пакеты

Пакеты с зашифрованным сообщением, как я понял. Если посмотреть на конструктор класса Message, то видно, что в него передается некий packetlist (его код тут https://openpgpjs.org/openpgpjs/doc/packet_packetlist.js.html ). А что это за пакеты? Придется погуглить и поискать спецификации по OpenPGP.

https://tools.ietf.org/html/rfc4880#section-4

An OpenPGP message is constructed from a number of records that are traditionally called packets.

A packet is a chunk of data that has a tag specifying its meaning.

The defined tags (in decimal) are as follows:

9 -- Symmetrically Encrypted Data Packet

18 -- Sym. Encrypted and Integrity Protected Data Packet

В общем, класс Message содержит в себе Packetlist, который содержит куски двоичных данных - пакеты разных типов. Я подозреваю, что типичное сообщение может содержать, например, пакет зашифрованных данных и пакет с какой-нибудь подписью, и мы расшифровываем только пакет данных (а проверка подписи может делаться отдельной функцией). Или одно сообщение может содержать только подпись, а сами данные могут быть в другом сообщении. Такая схема, когда сообщение составляется из пакетов, скорее всего, придумана для максимальной гибкости, чтобы мы могли использовать только те возможности, которые нам нужны. Там еще, например, сжатие есть.

Все виды пакетов описаны в RFC по ссылке, и довольно понятным текстом.

Кстати, для публичного/приватного ключа предусмотрены свои типы пакетов, и блок вроде BEGIN PGP PRIVATE KEY BLOCK как раз внутри должен содержать сообщение с соответствующим типом пакета.

Если тебе эти пакеты не нужны, то тебе надо вызывать более низкоуровневую функцию, которая сразу расшифровывает данные.

Если проследить цепочку вызовов, то она идет так:

Message.prototype.decrypt - берет из сообщения пакет и вызывает на нем SymmetricallyEncrypted.prototype.decrypt. Она берет из пакета данные и вызывает crypto.cfb.decrypt ( https://openpgpjs.org/openpgpjs/doc/crypto_cfb.js.html#line181 ).

То есть если тебе например нужна только функция расшифровки, и не нужны пакеты и сообщения, ты бы мог напрямую вызывать crypto.cfb.decrypt. Но я не знаю, поддержит ли это библиотека openpgp на сервере - может ли она работать с сырыми данными или они должны быть завернуты в пакеты и сообщения.

Затем отфилтрует какие-то пакеты

И если ни одного из них ни найдено вернет false (null?)

Вернет undefined.

Причём, когда происходит ошибка не возвращается Promise.reject(new Error(...)). Я не понимаю как это работает.

Там выбрасывается исключение, которое ловит then() и превращает в неудачный результат промиса. Пример:

var rejected = Promise.resolve().then(function () {
    throw new Error();
});

rejected превратится в rejected промис.

Ой-ой, изучил ли ты внимательно принцип работы промисов?

Вы поэтому упоминули про unhandledRejection? Его нужно где-то повесить?

Это глобальный обработчик, который ловит не обработанные rejection во всех промисах. Я пока не понимаю, надо ли его применять и как.

Мне не нравится то, что он будет срабатывать при необработанном reject вообще в любом промисе - может, ошибка в HTTP запросе, может ошибка в какой-то другой операции - и непонятно, как на это реагировать. Я бы его оставил только для отлова ошибок в коде (отлов того, что где-то не была обрабатана ошибка в промисе) и для отправки этих ошибок разработчику.

Я вижу такие пути:

1. явно обрабатывать reject, примерно так:

    backend.getData().then(function (data) { 
        displayData();
    }, function (error) {
        displayError();
    });
   

2. обрабатывать reject где-то внутри сервиса. Ввести "статус соединия" (идет передача данных/все успешно передано/есть проблемы со связью) и отображать статус где-то на экране, например, иконкой. То есть, при ошибке связи с бекендом менять статус соединения на "есть проблемы" и делать повторный запрос. Таким образом, бекенд в принципе не возвращает rejected promise:

    backend.statusChangeEvent.subscribe(function (status) {
        displayConnectionStatus(status);
    });
   
    backend.getData().then(function (data) {
        displayData();
    });
   

   Это хорошо работает для ситуаций вроде "пропал интернет" или "на бекенде временно появилась ошибка 503". Но что, если бекенд например отдает 404/403? Это ведь значит, что повторные запросы делать нет смысла. Но в нашу концепцию "ошибок не бывает, бывают задержки с получением данных" это не вписывается.

   Также, есть риск, что при таком подходе число "подвисших" запросов, которые повторно отправляются, может неограниченно расти, что в итоге приведет к проблемам с производительностью и DoS-атаке на бекенд этими запросами.

3. делать локальное хранилище и фоновую синхронизацию с сервером.

Вообще, если взять тот же скайп, то там используется локальное хранилище и синхронизация с сервером. При отправке сообщения оно кладется в локальное хранилище, в интерфейсе оно помечается как неотправленное (после синхронизации с сервером оно помечается как отправленное). Соответственно, вывод сообщений на экран тоже делается из локального хранилища.

Синхронизация локального и серверного хранилища делается где-то отдельно в фоне, и при проблемах связи иконка скайпа меняет свой вид.

То есть скайп заточен под работу с ненадежной сетью.

Если ты не готов делать локальное хранилище, то наверно тебе подойдет первый вариант с ручной обработкой ошибок. Можно его дополнить вторым вариантом с ограничением числа попыток (если с N попыток не удалось отправить/получить сообщение, возвращаем ошибку).

Также, надо разделять "исправимые" ошибки (пропал интернет, проблема решается повторным запросом - автоматически либо по команде пользователя) и "неисправимые" (сообщение не расшифровалось - бесполезно его пытаться расшифровать снова, или например, запись в локальное хранилище не удалась из-за переполнения жесткого диска). Неисправимые ошибки можно разделить далее на глобальные - приложение больше не работает и нужна перезагрузка страницы или даже повторный логин или локальные - не работает только часть приложения. Ну например, если не удалось расшифровать одно сообщение (оно повреждено), это локальная ошибка - можно вместо этого сообщения вывести иконку ошибки, но приложение в остальном работает.

В случае неисправимых ошибок можно сообщить об этом пользователю, при желании еще сообщить на сервер.

Мне кажется, что логично делать так:

• ловить все непойманные исключения (событие onerror) и считать их "неисправимыми" локальными ошибками, о которых можно сообщить примерно так: "в приложении произошла ошибка и оно может работать неверно. Попробуйте перезагрузить страницу или продолжайте работать на свой страх и риск.". При желании дополнительно сообщать о них на сервер.
• аналогично поступать с необработанными reject в промисах

А что касается "исправимых" ошибок со связью - я описал выше варианты действий. Лишь бы ошибки не игнорировались и не оставались незамеченными.

Если абстрогироваться от всего этого запутанного кода, можно разделить ошибки на:

(список ошибок)

Еще есть вариант, что сообщение содержит синтаксические ошибки и не поддается разделению на пакеты, или пакет содержит ошибки и не парсится. То есть кривые данные.

Если в промисе произошло reject и оно не было обработано, то сработает событие unhandledrejection.

Т.е. в случае если не был повешан .then(..., onRejected) или .catch(onRejected)?

Да. Если мне не изменяет память, браузер при этом по умолчанию в таком случае выводит сообщение в консоль.

в коде библиотеки не происходит reject, а просто вбрасываются исключения внутри then(),

Это разрешено спецификацией промисов: https://promisesaplus.com/#point-42

If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason.

Мне нужно сделать что-то вроде

  var promise = openpgp.decrypt(options).then(function(decrypted) {
      if (decrypted instanceof Error) {

Нет, при ошибке промис будет rejected, и это ловится например через catch или через onRejected в then.

Надо спросить у авторов библиотеки почему они не делают reject?

Они его делают выбросом исключения внутри then.

Можно с его помощью централизованно собирать ошибки.

А можно небольшой пример как это делается?

Выше я описал варианты обработки ошибок, которые мне пришли в голову. Если это был вопрос про unhandledrejection, то есть документация.

И в ФФ судя по MDN не работает.

Не совсем понял, что именно не работает?

События unhandlenrejection нету в Firefox, если я ничего не путаю.

Тогда обещания выполняться асинхронно и сообщения могут быть в не правильном порядке

Да, такое возможно.

>>1083114

Но где вызывается сам reject() тогда?

Выбрасывается исключение внутри then().