Added authentication on websocket connections#4
Added authentication on websocket connections#4NickMitin wants to merge 9 commits intofeature-redis-persistence-supportfrom
Conversation
| if (!cookie) { | ||
| cookie = '' | ||
| } | ||
| const docName = request.url.substring(1) |
There was a problem hiding this comment.
Если честно, довольно непонятно, что именно тут происходит.
Непонятно, что за request.url.substring(1). Предполагаю, что это накладывает определённые условия на request.url? Какой url ожидается изначально?
Что должно храниться в authCallback и что в итоге получится в результате authCallback + docName? Получается, что authCallback тоже должно быть очень определённого формата, так?
Плюс тут, кажется, есть возможность устроить какой-нибудь хак, типа, негодяй в request.url передаст что-то вроде ../../../api/v1/something-that-always-returns-status-ok/ и получит какой-то несанкционированный доступ.
Короче, напрягает, что тут происходит что-то, что сильно полагается на символы в строчках, и что весь процесс никак не прокомментирован, а из кода понять — сложно.
There was a problem hiding this comment.
request.url тут всегда docName, если умышленно не передать что-то другое. Если какой-то негодяй передаст что угодно,то это что угодно просто передастся get-параметром к авторизационному урлу, то получится, просто левое значение.
There was a problem hiding this comment.
А как бы ты передал значение docName?
There was a problem hiding this comment.
А как бы ты передал значение docName?
Написал вот в этом комменте. По сути на самом деле так же, конечно, но слегка аккуратнее.
const docName = request.url.substring(1)
Тут я бы предложил вынести в функцию, чтобы её же задокументировать, и чтобы в логику не просачивались детали реализации, а если в ней будем обнаруживать баги (например, вдруг в каких-то условиях в request.url передастся полный урл?), то там усложним и тестами покроем. Ну или абстрагируем вовсе. Но в любом случае будущее — это про YAGNI, а в настоящем важны документирование и семантика:
...
/*
*/
function getDocNameFromRequest(request) {
const docName = request.url.substring(1)
return docName
}
...
const docName = getDocNameFromRequest(request)(вот тут бы питоновские доктесты были бы идеальны :–)
А ещё я не нашёл в документации request.url. Куда смотреть? Хочу убедиться, что он там действительно относительный, и мы не окажемся с docName вида "ttps://...:" :–)
Может, уместнее будет использовать request.path?
| #!/usr/bin/env node | ||
|
|
||
| const redisPersistenceBridge = require('./redis-persistence-bridge') | ||
| const mysqlConnection = require('../connections/mysql').mysqlConnection |
There was a problem hiding this comment.
Предполагаю, что это было вытащено для авторизации через django_session? Думаю, сейчас можно втащить обратно. Кажется, ни к чему плодить кучи файлов, не?
There was a problem hiding this comment.
Опять-же, потому что это библиотека, а у кого-то может быть Postgres или Mongo.
There was a problem hiding this comment.
Если у него Postgres или Mongo, то и скрипт будет называться transfer-data-from-postrgres-или-mongo-to-redis. И SQL-запросы в этом скрипте будут другие.
Короче, я к тому, что этот скрипт сам по себе очень специфичен для mysql и не вижу причин делить его на два файла.
Либо надо его тоже абстрагировать: transfer-data-from-X-to-Y и настраивать коннекторы… Но это уже, кажется, совсем перебор.
К слову, раз мы вернули level-db, надо, видимо, и эту механику адаптировать, м?
2 participants
Moved mysql operations to separate js file
Added authenticators support to collaborate editing server
Added http authenticator
Temporary switched to leveldb persistence