Skip to content

Commit

Permalink
feature/1309: API version separation in NodeJS backend (#1318)
Browse files Browse the repository at this point in the history
* Total restructuring inside the /api dir:
- v1 and v2 have their own dirs
- I appended a .v2 to all logger definitions for API V2 controllers/services
- I removed the obsolete endpoints from V2
- Added the new endpoints: PUT and DELETE /mapconfig/{name} instead of /mapconfig/create/{name} and /mapconfig/delete/{name}. This was already changed in the API spec so we will now conform to that
- This is by no means complete (at least I think there's room for improvement). See this as a proposal for future structure inside the Backend dir and please let me know what you think in #1309.

* Next step in the transision: clean up the Router a bit

* Another step forward:
- Separation of API requires either duplicating a lot of functionality (common to the API versions) or drawing a line somewhere and saying 'hey, this part of code should be shared between versions'.
- That's what I'm attemptign to do here and opinions are welcome.
- The 'line' is currently drawn at 'stuff that don't expose /api/vN endpoint should be shared'. So we share the error handler, logger and detailed logger middlewares. The proxy middlewares and perhaps even the static files exposer middleware will be placed inside each API's version. That will come in a future commit.

* Prepared two product proxies for versioning.

* Major changes in server.js to make room for versioning:
- Added a key in .env, API_VERSIONS. It can contain a comma-separated list of integers that corespond to API versions. By default any empty value or no value at all means that all API versions are enabled.
- Added a apiVersions constant both in server.js and as an Express app variable, so we can do app.get(apiVersions) anywhere and get the array. Nice.
- Moved two product proxies (FME Server and Sokigo FB) to helper methods. Inside of them, we iterate and import required proxies dynamically.
- Another iteration takes place to set up the OpenAPI Validator middleware and expose the specification, depending on which versions are required. Also nice.

* Disabled http-middleware-proxy's internal logging:
- There were issues with it: seemingly the last middleware to be setup got all logging messages to it, which is unintended.
- The logging we setup ourselves in the middlewares is fine though. Since we take care of both success and error and log
then correctly, there was no use for HPM's internal logging.

* Small cleanups

* Generic proxy versioning added. Also major improvements to API version check.
- The generic proxy is not really versioned as it merely exposes some endpoints using HTTP Proxy Middleware. But we do expose proxies for each of the enabled versions of API to keep it consistent with previous Hajk versions.
- The check added at startup ensure that we only attempt to initialize valid API versions. Those are, for now, kept in const ALLOWED_API_VERSIONS but could be moved further on.

* Version bumped logger

* Discovered that the Router imports all API versions - must be fixed prior release.

* Added coherent initialization messages to Services in V2

* Added version info to V1 middlewares, for consistency.

* The Static Exposer is now versioned too:
- The static routes are still exposed outside the /api/vN, but the code used is dynamically imported.
- Admin can specify a certain version of active API versions to use. Default value is the highest (latest) active API version.

* Routes are created only for the active API versions, takes care of the bug discovered in 59b7915.
  • Loading branch information
jacobwod authored Apr 13, 2023
1 parent 2759c55 commit 0d56eb1
Show file tree
Hide file tree
Showing 56 changed files with 2,849 additions and 158 deletions.
18 changes: 17 additions & 1 deletion new-backend/.env
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,17 @@ NODE_ENV=development
# Port that the HTTP server will listen on
PORT=3002

# *** Backend API versions ***
# Hajk's backend comes with a versioned API.
# The initial, .NET compatible API is version 1.
# The improved version, which includes e.g. consolidated loading of
# config in client is version 2.
#
# The recommended setting is to leave this empty (i.e. outcommented). This means that all
# currently supported versions of the API are enabled.
# If you want to enable only specific version(s), supply a comma-separated list, e.g.
#API_VERSIONS=2,3 # Disables /api/v1, enables /api/v2 and /api/v3

# When an Express app is running behind a proxy, app.set('trust proxy') should be
# set. This setting simply passes its value to Express as the value of 'trust proxy'.
# To sum up: if set to true, the client IP will be extracted from the leftmost portion
Expand All @@ -43,7 +54,12 @@ REQUEST_LIMIT=1000kb
SESSION_SECRET=mySecret

# Control which directories will be statically exposed on the HTTP server.
# / can contain Hajk's client app
# Because the endpoints of the Static Exposer are not versioned, we must
# decide which API version we want to use for those routes. The default value
# is "LATEST" but you can specify any of the allowed API versions. Usually,
# the default value is fine.
STATIC_EXPOSER_VERSION=LATEST
# Expose Hajk's client app directly under /
EXPOSE_CLIENT=true
# If we expose /admin, we want probably to restrict access to it. Make sure
# to enable AD_* settings below in order for this to work.
Expand Down
42 changes: 21 additions & 21 deletions new-backend/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion new-backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"fast-xml-parser": "^4.0.12",
"helmet": "^6.0.1",
"http-proxy-middleware": "^2.0.6",
"log4js": "^6.7.1",
"log4js": "^6.9.1",
"node-windows": "^1.0.0-beta.8"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { createProxyMiddleware } from "http-proxy-middleware";
import log4js from "log4js";
import fs from "fs";

// Value of process.env.LOG_LEVEL will be one of the allowed
// log4js-values. We will customize HPM to use log4js too,
Expand All @@ -21,13 +20,12 @@ const logLevels = {
};

// Grab a logger
const logger = log4js.getLogger("proxy.fmeServer");
const logger = log4js.getLogger("proxy.fmeServer.v1");

export default function fmeServerProxy(err, req, res, next) {
return createProxyMiddleware({
target: process.env.FME_SERVER_BASE_URL,
logLevel: logLevels[process.env.LOG_LEVEL],
logProvider: () => logger,
logLevel: "silent", // We don't care about logLevels[process.env.LOG_LEVEL] in this case as we log success and errors ourselves
changeOrigin: true,
secure: process.env.FME_SERVER_SECURE === "true", // should SSL certs be verified?
onProxyReq: (proxyReq, req, res) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import log4js from "log4js";
import ad from "../services/activedirectory.service";

const logger = log4js.getLogger("router");
const logger = log4js.getLogger("router.v1");

export default async function restrictAdmin(req, res, next) {
logger.trace("Attempt to access admin API methods");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import log4js from "log4js";
import ad from "../services/activedirectory.service";

const logger = log4js.getLogger("hajk.static.restrict");
const logger = log4js.getLogger("hajk.static.restrict.v1");
/**
* @summary Determine if current user is member in any of the required groups in order to access a given path.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,12 @@ const logLevels = {
};

// Grab a logger
const logger = log4js.getLogger("proxy.sokigo");
const logger = log4js.getLogger("proxy.sokigo.v1");

export default function sokigoFBProxy(err, req, res, next) {
return createProxyMiddleware({
target: process.env.FB_SERVICE_BASE_URL,
logLevel: logLevels[process.env.LOG_LEVEL],
logProvider: () => logger,
logLevel: "silent", // We don't care about logLevels[process.env.LOG_LEVEL] in this case as we log success and errors ourselves
pathRewrite: (originalPath, req) => {
// Remove the portion that shouldn't be there when we proxy the request
// and split the remaining string on "?" to separate any query params
Expand Down
15 changes: 15 additions & 0 deletions new-backend/server/apis/v1/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as express from "express";

import configRouter from "./controllers/config/router";
import mapconfigRouter from "./controllers/mapconfig/router";
import settingsRouter from "./controllers/settings/router";
import informativeRouter from "./controllers/informative/router";
import adRouter from "./controllers/ad/router";

export default express
.Router()
.use("/config", configRouter)
.use("/informative", informativeRouter)
.use("/mapconfig", mapconfigRouter)
.use("/settings", settingsRouter)
.use("/ad", adRouter);
File renamed without changes.
43 changes: 43 additions & 0 deletions new-backend/server/apis/v2/controllers/ad/controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import ActiveDirectoryService from "../../services/activedirectory.service";
import handleStandardResponse from "../../utils/handleStandardResponse";
import log4js from "log4js";

// Create a logger for admin events, those will be saved in a separate log file.
const ael = log4js.getLogger("adminEvent.v2");

export class Controller {
availableADGroups(req, res) {
ActiveDirectoryService.getAvailableADGroups().then((data) =>
handleStandardResponse(res, data)
);
}

findCommonADGroupsForUsers(req, res) {
ActiveDirectoryService.findCommonADGroupsForUsers(req.query.users).then(
(data) => handleStandardResponse(res, data)
);
}

getStore(req, res) {
// Extract the store name from request's path
const store = req.route.path.substring(1);
ActiveDirectoryService.getStore(store).then((data) => {
handleStandardResponse(res, data);
// If data doesn't contain the error property, we're good - print event to admin log
!data.error &&
ael.info(
`${res.locals.authUser} viewed contents of AD store "${store}"`
);
});
}

flushStores(req, res) {
ActiveDirectoryService.flushStores().then((data) => {
handleStandardResponse(res, data);
// If data doesn't contain the error property, we're good - print event to admin log
!data.error && ael.info(`${res.locals.authUser} flushed all AD stores`);
});
}
}

export default new Controller();
13 changes: 13 additions & 0 deletions new-backend/server/apis/v2/controllers/ad/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as express from "express";
import controller from "./controller";
import restrictAdmin from "../../middlewares/restrict.admin";

export default express
.Router()
.use(restrictAdmin) // We will not allow any of the following routes unless user is admin
.get("/availableadgroups", controller.availableADGroups)
.get("/findcommonadgroupsforusers", controller.findCommonADGroupsForUsers)
.get("/users", controller.getStore)
.get("/groups", controller.getStore)
.get("/groupsPerUser", controller.getStore)
.put("/flushStores", controller.flushStores);
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ConfigServiceV2 from "../../services/config.service.v2";
import ConfigService from "../../services/config.service";
import ad from "../../services/activedirectory.service";
import handleStandardResponse from "../../utils/handleStandardResponse";

Expand All @@ -12,7 +12,7 @@ export class Controller {
* @memberof Controller
*/
byMap(req, res) {
ConfigServiceV2.getMapWithLayers(
ConfigService.getMapWithLayers(
req.params.map,
ad.getUserFromRequestHeader(req)
).then((data) => handleStandardResponse(res, data));
Expand Down
62 changes: 62 additions & 0 deletions new-backend/server/apis/v2/controllers/informative/controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import InformativeService from "../../services/informative.service";
import log4js from "log4js";

// Create a logger for admin events, those will be saved in a separate log file.
const ael = log4js.getLogger("adminEvent.v2");

export class Controller {
create(req, res) {
const { documentName, mapName } = JSON.parse(req.body);
InformativeService.create(documentName, mapName).then((r) => {
// FIXME: The buggy admin expects 200 and this string on success,
// but I think that we'd do better with a meaningful JSON response.
if (r && !r.error) {
res.status(200).send("Document created");
ael.info(
`${res.locals.authUser} created a new document, ${documentName}.json, and connected it to map ${mapName}.json`
);
} else res.status(500).send(r.error.message);
});
}

getByName(req, res) {
InformativeService.getByName(req.params.name).then((r) => {
if (r && !r.error) res.json(r);
else {
res
.status(404)
.send(`Document "${req.params.name}" could not be found`);
}
});
}

saveByName(req, res) {
InformativeService.saveByName(req.params.name, req.body).then((r) => {
if (r && !r.error) {
res.status(200).send("File saved");
ael.info(
`${res.locals.authUser} saved document ${req.params.name}.json`
);
} else res.status(500).send(r.error.message);
});
}

deleteByName(req, res) {
InformativeService.deleteByName(req.params.name).then((r) => {
if (r && !r.error) {
res.status(200).send("File saved");
ael.info(
`${res.locals.authUser} deleted document ${req.params.name}.json`
);
} else res.status(500).send(r.error.message);
});
}

list(req, res) {
InformativeService.getAvailableDocuments().then((r) => {
if (r && !r.error) res.json(r);
else res.status(500).send(r.error.message);
});
}
}
export default new Controller();
13 changes: 13 additions & 0 deletions new-backend/server/apis/v2/controllers/informative/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as express from "express";
import controller from "./controller";
import restrictAdmin from "../../middlewares/restrict.admin";

export default express
.Router()
.get("/load/:name", controller.getByName)
.use(restrictAdmin) // All routes that follow are admin-only!
.put("/create", controller.create) // PUT is correct here, as this operation is idempotent
.delete("/delete/:name", controller.deleteByName)
.get("/list", controller.list)
.get("/list/:name", controller.list) // FIXME: For now, the name paramter is ignored - should list only documents connected to specified map
.put("/save/:name", controller.saveByName); // PUT is correct here, as this operation is idempotent
Loading

0 comments on commit 0d56eb1

Please sign in to comment.