Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obsługa sesji użytkownika i refactoring #10

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
10 changes: 8 additions & 2 deletions .babelrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
{
"presets": ["env"]
}
"presets": [
["env", {
"targets": {
"node": "11.9"
}
}]
]
}
1 change: 1 addition & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ PROTOCOL=http
HOST=localhost
HTTP_PORT=3000
WS_PORT=3000
Q2A_PORT=80
SSL_KEY_PATH=
SSL_CERT_PATH=
MAILER_HOST=
Expand Down
3 changes: 3 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": ["node"]
}
2,632 changes: 2,527 additions & 105 deletions package-lock.json

Large diffs are not rendered by default.

10 changes: 7 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
"main": "dist/index.js",
"scripts": {
"prettier": "node_modules/.bin/prettier --write src/ test/",
"dev": "nodemon src/index.js --exec babel-node",
"eslint": "eslint src/",
"dev": "nodemon --exec \"npm run prettier && npm run eslint && babel-node src/index.js\"",
"debug": "node --inspect-brk dist/index.js",
"prebuild": "npm run prettier && rimraf dist",
"prebuild": "npm run prettier && npm run eslint && rimraf dist",
"build": "babel src -d dist",
"serve": "node dist/index.js",
"pretest": "npm run build",
Expand All @@ -25,12 +26,15 @@
"body-parser": "^1.19.0",
"dotenv": "^8.2.0",
"escape-html": "^1.0.3",
"eslint": "^7.31.0",
"eslint-config-node": "^4.1.0",
"express": "^4.17.1",
"html-minifier": "^3.5.3",
"jsdom": "^16.3.0",
"node-fetch": "^2.6.1",
"nodemailer": "^4.0.1",
"rimraf": "^3.0.2",
"ws": "^7.3.1"
"ws": "^7.5.3"
},
"devDependencies": {
"babel-cli": "^6.24.1",
Expand Down
7 changes: 5 additions & 2 deletions src/config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import dotenv from 'dotenv';
import { readFileSync } from 'fs';
import { resolve } from 'path';

dotenv.config();

export const sslConfig = getSslConfig();
Expand All @@ -11,6 +13,7 @@ export default {
port: {
http: Number(process.env.HTTP_PORT) || 3000,
ws: Number(process.env.WS_PORT) || 3000,
q2a: Number(process.env.Q2A_PORT) || 80,
},
mailer: {
host: process.env.MAILER_HOST || '',
Expand Down Expand Up @@ -43,8 +46,8 @@ function getSslConfig() {

if (!isSslConfigSpecified) {
console.warn('SSL config not provided. Self signed certificate is used');
sslKeyPath = path.resolve(__dirname, '../ssl/develop.key');
sslCertPath = path.resolve(__dirname, '../ssl/develop.cert');
sslKeyPath = resolve(__dirname, '../ssl/develop.key');
sslCertPath = resolve(__dirname, '../ssl/develop.cert');
}
key = readFileSync(sslKeyPath, 'utf8');
cert = readFileSync(sslCertPath, 'utf8');
Expand Down
168 changes: 5 additions & 163 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,164 +1,6 @@
import http from 'http';
import https from 'https';
import express from 'express';
import bodyParser from 'body-parser';
import WebSocket from 'ws';
import config, { sslConfig } from './config';
import mailer from './mailer';
import HttpServer from './server/http';
import WebSocketServer from './server/webSocket';

const app = express();
app.use(bodyParser.json());
const server = createServer();
server.listen(config.port.http, () =>
console.log(`Server is listening on port ${config.port.http}, over ${config.protocol.toUpperCase()} protocol.`)
);

const HTTP_ERROR_CODES = Object.freeze({
FORBIDDEN: 403,
METHOD_NOT_ALLOWED: 405,
WS_CLOSE_ERROR_CODE: 4000,
});
const GROUP_REGEXPS = Object.freeze({
main: /^$|\/$/,
activity: /^(\/?)activity/,
});
const GROUP_NAMES = Object.freeze({
MAIN: 'main',
ACTIVITY: 'activity',
});
const ACTIONS = Object.freeze({
ADD_POST: 'add-post',
});

const wss = new WebSocket.Server({ server });
wss.on('error', onWSSError);
wss.on('connection', onWSSConnection);
wss.on('close', onWSSClose);

// check request
app.all('*', onAll);

// send new list html to users
app.post('/', onPost);

function createServer() {
if (config.protocol === 'https') {
return https.createServer(
{
key: sslConfig.key,
cert: sslConfig.cert,
},
app
);
}

return http.createServer(app);
}

let mailSent = false;

function onWSSError(error) {
console.error('WebSocket server error: ', error);

if (!mailSent) {
mailer.sendMail(`<p>Wystąpił błąd na serwerze WebSocket!<br><output>${JSON.stringify(error)}</output></p>`);
mailSent = true;
}
}

function onWSSConnection(ws) {
ws.on('error', onWSError);
ws.on('message', onWSMessage);
}

function onWSSClose(event) {
console.log('WebSocket server closed: ', event);
}

function onWSMessage(event) {
const parsedEvent = parseWSMessage(this, event);

if (!parsedEvent) {
return;
}

assignWSClientToGroup(this, parsedEvent.pathname);
}

function onWSError(event) {
console.error('ws error event: ', event);

if (!event.errno) {
throw event;
}
}

function onAll(req, res, next) {
// check authorization
if (req.headers.token !== config.token) {
res.sendStatus(HTTP_ERROR_CODES.FORBIDDEN);
return;
}

if (req.method !== 'POST') {
res.sendStatus(HTTP_ERROR_CODES.METHOD_NOT_ALLOWED);
return;
}

next();
}

function parseWSMessage(ws, event) {
let parsedEvent = {};

try {
if (!event) {
throw Error('ws empty message event');
}

parsedEvent = JSON.parse(event);
} catch (exception) {
console.error('ws invalid JSON message: ', event, ' threw exception: ', exception);
ws.close(HTTP_ERROR_CODES.WS_CLOSE_ERROR_CODE, 'Message is not valid JSON!');

return null;
}

if (!parsedEvent.pathname) {
console.error('ws empty pathname: ', parsedEvent.pathname);
ws.close(HTTP_ERROR_CODES.WS_CLOSE_ERROR_CODE, 'Empty pathname!');

return null;
}

return parsedEvent;
}

function assignWSClientToGroup(ws, pathname) {
const pathName = pathname.replace(/\//g, '');
const groupName = Object.keys(GROUP_REGEXPS).find((groupName) => GROUP_REGEXPS[groupName].test(pathName));

if (!groupName) {
console.error('ws unexpected groupName: ', groupName, ' from pathName: ', pathName);
ws.close(HTTP_ERROR_CODES.WS_CLOSE_ERROR_CODE, 'Unexpected pathname!');
}

ws._GROUP_NAME = groupName;
}

function onPost(req, res) {
res.sendStatus(200);

const { action } = req.body;
const groupNames = [GROUP_NAMES.ACTIVITY];

if (action === ACTIONS.ADD_POST) {
groupNames.push(GROUP_NAMES.MAIN);
}

for (const wsClient of wss.clients) {
if (groupNames.includes(wsClient._GROUP_NAME)) {
wsClient.send(JSON.stringify({ action }));
}
}
}
const httpServer = new HttpServer();
// eslint-disable-next-line no-new
new WebSocketServer(httpServer);
2 changes: 1 addition & 1 deletion src/mailer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import nodemailer from 'nodemailer';
import config from './config';

export default {
sendMail: function (message) {
sendMail(message) {
const transporter = nodemailer.createTransport(config.mailer);

const mailOptions = {
Expand Down
101 changes: 101 additions & 0 deletions src/server/http.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import http from 'http';
import https from 'https';
import express from 'express';
import bodyParser from 'body-parser';
import config, { sslConfig } from '../config';
import { HTTP_STATUS_CODES } from '../vars';

class HttpServerCore {
constructor() {
this.server = null;
this.app = null;
this.socketClientsNotifier = () => console.error('Method not attached!');
}

initConnectionWithQ2A(middlewareList) {
this.app = express();
this.app.use(bodyParser.json());

if (!Array.isArray(middlewareList) || middlewareList.length === 0) {
throw TypeError('middlewareList argument must be non empty array!');
}

middlewareList.forEach(({ method, path, listener }) => {
if (!method || !path || !listener) {
throw ReferenceError(`
middleware must contain: method, path and listener params!
Received method: "${method}", path: "${path}", listener: "${listener}"
`);
}

this.app[method](path, listener);
});
}
Comment on lines +23 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moim zdaniem to jest przekombinowane, w expressie kolejność middleware/handler-ów ma kluczowe znaczenie. Taki sposób tworzenia serwera wprowadza większe zamieszanie niż z tego jest pożytek.


initServerForWebSocket() {
this.server = this.createHttpServerWithOptionalCert();
this.server.listen(config.port.http, () =>
console.log(`Server is listening on port ${config.port.http}, over ${config.protocol.toUpperCase()} protocol.`)
);
}

createHttpServerWithOptionalCert() {
if (config.protocol === 'https') {
return https.createServer(
{
key: sslConfig.key,
cert: sslConfig.cert,
},
this.app
);
}

return http.createServer(this.app);
}

attachSocketClientsNotifier(fn) {
this.socketClientsNotifier = fn;
}
}

class HttpServer extends HttpServerCore {
constructor() {
super();
this.initConnectionWithQ2A([
{
method: 'all',
path: '*',
listener: HttpServer.onAll,
},
Comment on lines +65 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handler dla nieobsługiwanych route'ów powinien być zarejestrowany jako ostatni żeby uniknąć konfliktów przy routingu.

{
method: 'post',
path: '/',
listener: this.onPost.bind(this),
},
]);
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teoretycznie brakuje handlera dla 404 oraz handlera błędów.

this.initServerForWebSocket();
}

static onAll(req, res, next) {
if (req.headers.token !== config.token) {
res.sendStatus(HTTP_STATUS_CODES.FORBIDDEN);
return;
}

if (req.method !== 'POST') {
res.sendStatus(HTTP_STATUS_CODES.METHOD_NOT_ALLOWED);
return;
}

next();
}
Comment on lines +79 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Łamie SRP. Pierwszy IF jest middleware'em uwierzytelniającym, powinien być zarejestrowany jako app.use i być zarejestrowanym przed pozostałymi handlerami
  2. Drugi If powinien być osobnym handlerem, w expressie się obsługuje taką sytuacje w inny sposob, np app.get('/products', ....);
    app.post('/products', ...);
    // ważna jest kolejność - jest jako ostatni
    app.all('/products', (req, res, next) => { res.status(405).send('Method not allowed'); });


onPost(req, res) {
console.log('(onPost) rq.body:', req.body);

res.sendStatus(HTTP_STATUS_CODES.OK);
this.socketClientsNotifier(req.body.action);
}
}

export default HttpServer;
Loading