Безопасно ли не выполнять или отклонять обещание

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

В каждом маршруте у меня может быть:

authorizeOwnership(req, res)
.then(function() {
    // do stuff
    res.send(200, "Yay");
});

Я хочу, чтобы функция authorizeOwnership() обрабатывала ответы 403 (отказано в доступе) и 500 (например, ошибка запроса к базе данных), чтобы каждому маршруту не нужно было делать это явно.

У меня есть функция, которая может запросить базу данных, чтобы проверить право собственности.

function confirmOwnership(resourceId, userId) {
    // SequelizeJS... returns a bluebird promise
    return Resource.find({
        where: {id: resourceId, userId: userId}
    })
    .then(function(resource) {
        if(!resource) {
            return null; // no match for this resource id + user id
        } else {
            return resource;
        }
    });
}

Затем это используется в authorizeOwnership:

function authorizeOwnership(req, res) {
    var rid      = parseInt(req.params.rid, 10),
        userId   = parseInt(req.authInfo.userid, 10);

    return new Promise(function(resolve, reject) {
        confirmOwnership(rid, userId)
        .then(function(resource) {
            if(resource === null) {
                res.send(403, "Forbidden");
                // Note: we don't resolve; outer handler will not be called
            } else {
                resolve(resource);
            }
        })
        .catch(function(err) {
            console.log(err);
            res.send(500, "Server error");
            // Note: we don't resolve; outer handler will not be called
        });
    });
}

В этом сценарии я намеренно не вызываю reject() или resolve() в некоторых путях кода. Если я это сделаю, то моя «внешняя» логика маршрута (код, вызывающий authorizeOwnership()) усложнится, потому что она должна обрабатывать ошибки (либо с проверкой .catch(), либо с проверкой null в .then()).

Но две вещи меня немного нервируют:

  • Можно ли, чтобы обещание, возвращаемое authorizeOwnership(), никогда не разрешалось в сценариях ошибок? Это вызовет задержку или утечку памяти?

  • Логично ли разрешить confirmOwnership() с нулевым значением, чтобы сказать «подходящий ресурс не найден», а затем рассматривать это как ошибку в authorizeOwnership()? Моя первая попытка отклонила обещание в confirmOwnership(), когда не было подходящего ресурса, но это усложнило ситуацию, потому что трудно отличить это (случай 403) от фактической ошибки (случай 500).


person optilude    schedule 12.04.2014    source источник
comment
В общем: пожалуйста, не создавайте «божественные» подпрограммы, подобные этой: функция «authorizeOwnership()» для обработки 403 (отказано в доступе) и 500. Им должно потребоваться разрешение от вызывающей стороны, чтобы иметь возможность обрабатывать ошибки или нет (параметр для возвращать ошибки или нет). Менеджер (вызывающий) всегда знает о контексте больше, чем «вызываемая» процедура.   -  person Ryan Vincent    schedule 13.04.2014
comment
Добавляю свое «Да» к тому, что сказал @RyanVincent. Подпрограмма, которая запрашивает базу данных, не должна принимать решения на уровне приложения. Каждый компонент в вашей программе должен делать ровно одну вещь. Эти вещи могут выйти из-под контроля, если вы не будете очень тщательно структурировать, кто получает доступ к состоянию запроса.   -  person Benjamin Gruenbaum    schedule 13.04.2014
comment
Я согласен с тем, что вы говорите, хотя приведенный выше пример представляет собой упрощение фактического кода с целью прояснить вопрос, и фактический код не страдает от проблем, которые вы предлагаете.   -  person optilude    schedule 13.04.2014


Ответы (1)


Можно ли, чтобы обещание, возвращаемое authorizeOwnership(), никогда не разрешалось в сценариях ошибок? Это вызовет задержку или утечку памяти?

Да, безопасно не разрешать обещание Bluebird (и, честно говоря, любую другую реализацию, которую я проверял - красивые картинки здесь). Сам по себе он не имеет глобального состояния.

Вопрос о том, является ли это хорошей практикой, другой. В каком-то смысле это похоже на синхронный break. Лично я не фанат.

Логично ли разрешать confirmOwnership() с нулевым значением, чтобы сказать «подходящий ресурс не найден», а затем рассматривать это как ошибку в authorizeOwnership()?

Это зависит от вашего API. Это снова мнение. Это работает, но я, вероятно, не вернул бы null, а указал бы на ошибку, если случай исключительный. Вы можете отличить отказ, используя объект ошибки. Например, вы можете отклонить объект AuthorizationError, который вы создаете. Примечание. Bluebird также поддерживает типизированные перехваты.

Что-то вроде:

// probably shouldn't send the response to authorizeOwnership but use it externally
// to be fair, should probably not take req either, but rid and userid
var authorizeOwnership = Promise.method(function(req) {
    var rid      = Number(req.params.rid),
        userId   = Number(req.authInfo.userid;
        return confirmOwnership(rid, userId); // return the promise
    });
});

function ServerError(code,reason){
    this.name = "ServerError";
    this.message = reason;
    this.code = code;
    Error.captureStackTrace(this); // capture stack
}
var confirmOwnership = Promise.method(function(resourceId, userId) {
    // SequelizeJS... returns a bluebird promise
    return Resource.find({
        where: {id: resourceId, userId: userId}
    })
    .then(function(resource) {
        if(!resource) {
            throw new ServerError(403,"User not owner"); // promises are throw safe
        }
        return resource;
    });
});

Затем на вашем сервере вы можете сделать что-то вроде:

app.post("/foo",function(req,res){
     authorizeOwnership(req).then(function(){
          res.send(200, "Owner Yay!");
     }).catch(ServerError,function(e){
            if(e.code === 403) return res.send(403,e.message);
            return res.send(500,"Internal Server Error");
     });
});

Примечание: вы также используете отложенный анти-шаблон в своем коде. Нет необходимости делать new Promise(function(...){ в вашем коде, вы можете просто вернуть промис.

person Benjamin Gruenbaum    schedule 12.04.2014
comment
Бенджамин, это выглядит очень хорошо, но есть одна вещь, которую я не совсем понимаю. Если строка throw new ServerError(403,"User not owner"); - единственное место, где вызывается ServerError, то наверняка 500 ошибок не пробьют улов, как написано? Я, вероятно, что-то упустил, но документация Bluebird предполагает, что общий catch должен быть цепочкой, а именно .catch(ServerError, handler1).catch(handler2). - person Roamer-1888; 13.04.2014
comment
@ Roamer-1888 да, верны, неявное предположение, не показанное здесь, заключается в том, что код, который должен вызывать внутреннюю ошибку сервера, должен выдавать ServerError(500,..., как правило, если у меня есть синтаксическая ошибка программиста или ошибка ссылки - я не хочу возвращать 500 ответ, я хочу знать немедленно. Bluebird уловит это и зарегистрирует. - person Benjamin Gruenbaum; 13.04.2014
comment
Хм, чтобы уточнить мой последний комментарий - я бы все равно вернул 500 в реальном коде, но не из обработчика маршрута, а через глобальное промежуточное ПО. Общие ловушки действительно рискованны и создают много проблем, таких как обнаружение ошибок программиста, о которых я не думал :) - person Benjamin Gruenbaum; 13.04.2014
comment
Спасибо за объяснение Бенджамин. На первый взгляд это не кажется неуместным для чего-то вроде .catch(ServerError, function(e) { res.send(e.code, e.message); }).catch(function(e) { res.send(500, e.messsage || "Internal Server Error"); });. Меня беспокоит необходимость явного отлова любых ошибок, отличных от ServerError, на этом этапе кода. Но, возможно, я бы изменил свое мнение, если бы понял «глобальную промежуточную одежду». - person Roamer-1888; 13.04.2014
comment
@ Roamer-1888 допустим, у вас есть myArray.forEch(function(...) (обратите внимание на опечатку) в вашем коде, и этот код находится в шаблоне EJS или каком-то пути, который выполняется редко, или что-то в этом роде. У вас есть ошибка программиста, которая теперь будет молча игнорироваться (поскольку вы поймаете TypeErrors) - если вы не добавите этот улов. Bluebird обнаружит, что он не пойман, и даст вам хороший журнал. Вы можете обнаружить необработанные отклонения с помощью обработчика Promise.onPossiblyUnhandledRejection или зарегистрировать их. - person Benjamin Gruenbaum; 13.04.2014
comment
@ Roamer-1888 У нас есть лучшее решение в работе через Promise.using, дающее цепочке обещаний «право собственности» на ответ, что позволит ему отправить 500 через диспоузер и отправить необработанный отказ, который показать ошибку явно. Однако на данный момент, по крайней мере, вам нужно сделать: catch(function(e) { res.send(500, e.messsage || "Internal Server Error"); throw e; }); чтобы ошибка продолжала распространяться и регистрировалась (например, исключения в синхронном коде) или явно регистрировать ее (не так хорошо, как imo). - person Benjamin Gruenbaum; 13.04.2014
comment
Вениамин, спасибо за это. Интересно, есть ли место для .butterFingersCatch(), то есть немаскирующего улова, который автоматически повторно выдает ошибку. (Термин «Масляные пальцы» часто можно услышать на поле для крикета, когда мяч пойман только наполовину). - person Roamer-1888; 13.04.2014
comment
@ Roamer-1888 вы можете предложить это в трекере Bluebird github. Мы можем обсудить это на irccloud.com/#. !/ircs://irc.freenode.net:6697/%23обещания . Stackoverflow, как правило, очень не любит обсуждения в комментариях :) - person Benjamin Gruenbaum; 13.04.2014
comment
Бенджамин, да, я знаю, но это отличная почва для идей. Спасибо за ссылку. - person Roamer-1888; 13.04.2014
comment
@ Roamer-1888 ну, если быть честным - здесь есть я, а еще есть (простой) я, Петька (автор Bluebird), Доменик (соавтор спецификации и евангалист обещаний), Крис (автор Q), Стеф (RSVP автор), spion (множество библиотек промисов и вкладчик Bluebird) и куча других балеров на #promises :) - person Benjamin Gruenbaum; 13.04.2014
comment
Спасибо, @BenjaminGruenbaum! Тем не менее, пара комментариев/вопросов: (a) причина, по которой у меня есть authorizeOwnership() в первую очередь, заключается в том, что это утилита/шаблон, чтобы гарантировать, что обработка несанкционированных запросов (по этой конкретной причине) выполняется последовательно на дюжине или около того маршрутов . По этой причине я не хочу иметь бит res.send() в маршруте верхнего уровня, как вы показали. (b) Когда я пытался сделать что-то подобное, я получаю необработанные исключения типа ошибки, если на верхнем уровне нет перехвата. Вот как я закончил с нерешенной вещью. - person optilude; 13.04.2014
comment
(Возможно, лучше выполнить проверку в промежуточном программном обеспечении и полностью отказаться от помощника authorizeOwnership(), но это сложнее сделать, не требуя двух запросов к базе данных, когда мне также нужно будет использовать объект ресурса позже.) - person optilude; 13.04.2014
comment
@optilude вы можете использовать для этого onPossiblyUnhandledRejection, но на вашем месте я бы написал (тонкую) оболочку поверх app.get/post/whatever, которая выполняет ожидаемое поведение - принимает (возвращенное) обещание, ждет его, проверяет ошибку и отвечает соответственно. Спион однажды написал такой образец маршрутизатора для развлечения, вы можете вдохновиться его реализацией. В Bluebird 2 есть средства, которые сделают это еще проще. - person Benjamin Gruenbaum; 13.04.2014