Skip to content

Commit

Permalink
fix(server): shift api logger to be initialised within each subrouter (
Browse files Browse the repository at this point in the history
…#621)

* fix(server): shift api logger to be initialised within each subrouter

* refactor(auth): add logging back into `/auth` endpoint

this will log the ip with a username of `undefined` but this is expected so that we can monitor and
potentially block malicious actors who are trying to ddos us
  • Loading branch information
seaerchin authored Feb 23, 2023
1 parent 4eae950 commit df3182e
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 11 deletions.
10 changes: 8 additions & 2 deletions src/routes/v1/authenticated/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,20 @@ const express = require("express")
const sitesRouter = require("@routes/v1/authenticated/sites")
const { UsersRouter } = require("@routes/v2/authenticated/users")

const getAuthenticatedSubrouter = ({ authMiddleware, usersService }) => {
const getAuthenticatedSubrouter = ({
authMiddleware,
usersService,
apiLogger,
}) => {
// Workaround - no v1 users router exists
const usersRouter = new UsersRouter({ usersService })

const authenticatedSubrouter = express.Router({ mergeParams: true })

authenticatedSubrouter.use(authMiddleware.verifyJwt)

// NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username
// which is only available after verifying that the jwt is valid
authenticatedSubrouter.use(apiLogger)
authenticatedSubrouter.use("/sites", sitesRouter)
authenticatedSubrouter.use("/user", usersRouter.getRouter())

Expand Down
5 changes: 4 additions & 1 deletion src/routes/v1/authenticatedSites/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ const resourceRoomRouter = require("@routes/v1/authenticatedSites/resourceRoom")
const resourcesRouter = require("@routes/v1/authenticatedSites/resources")
const settingsRouter = require("@routes/v1/authenticatedSites/settings")

const getAuthenticatedSitesSubrouter = ({ authMiddleware }) => {
const getAuthenticatedSitesSubrouter = ({ authMiddleware, apiLogger }) => {
const authenticatedSitesSubrouter = express.Router({ mergeParams: true })

authenticatedSitesSubrouter.use(authMiddleware.verifyJwt)
authenticatedSitesSubrouter.use(authMiddleware.useSiteAccessTokenIfAvailable)
// NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username
// which is only available after verifying that the jwt is valid
authenticatedSitesSubrouter.use(apiLogger)

authenticatedSitesSubrouter.use("/pages", pagesRouter)
authenticatedSitesSubrouter.use("/collections", collectionsRouter)
Expand Down
4 changes: 3 additions & 1 deletion src/routes/v2/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ const CSRF_COOKIE_NAME = "isomer-csrf"
const COOKIE_NAME = "isomercms"

class AuthRouter {
constructor({ authService, authMiddleware }) {
constructor({ authService, authMiddleware, apiLogger }) {
this.authService = authService
this.authMiddleware = authMiddleware
this.apiLogger = apiLogger
// We need to bind all methods because we don't invoke them from the class directly
autoBind(this)
}
Expand Down Expand Up @@ -90,6 +91,7 @@ class AuthRouter {

getRouter() {
const router = express.Router()
router.use(this.apiLogger)

router.get(
"/github-redirect",
Expand Down
6 changes: 4 additions & 2 deletions src/routes/v2/authenticated/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import InfraService from "@services/infra/InfraService"

const express = require("express")

const {
Expand All @@ -16,6 +14,7 @@ const getAuthenticatedSubrouter = ({
gitHubService,
configYmlService,
usersService,
apiLogger,
}) => {
const sitesService = new SitesService({ gitHubService, configYmlService })
const netlifyTomlService = new NetlifyTomlService()
Expand All @@ -27,6 +26,9 @@ const getAuthenticatedSubrouter = ({
const authenticatedSubrouter = express.Router({ mergeParams: true })

authenticatedSubrouter.use(authMiddleware.verifyJwt)
// NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username
// which is only available after verifying that the jwt is valid
authenticatedSubrouter.use(apiLogger)

authenticatedSubrouter.use("/sites", sitesV2Router.getRouter())
authenticatedSubrouter.use("/user", usersRouter.getRouter())
Expand Down
4 changes: 4 additions & 0 deletions src/routes/v2/authenticatedSites/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const getAuthenticatedSitesSubrouter = ({
authMiddleware,
gitHubService,
configYmlService,
apiLogger,
}) => {
const collectionYmlService = new CollectionYmlService({ gitHubService })
const homepagePageService = new HomepagePageService({ gitHubService })
Expand Down Expand Up @@ -186,6 +187,9 @@ const getAuthenticatedSitesSubrouter = ({

authenticatedSitesSubrouter.use(authMiddleware.verifyJwt)
authenticatedSitesSubrouter.use(authMiddleware.useSiteAccessTokenIfAvailable)
// NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username
// which is only available after verifying that the jwt is valid
authenticatedSitesSubrouter.use(apiLogger)

authenticatedSitesSubrouter.use(
"/collections/:collectionName",
Expand Down
11 changes: 6 additions & 5 deletions src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import QueueService from "@services/identity/QueueService"
import ReposService from "@services/identity/ReposService"
import InfraService from "@services/infra/InfraService"

import { apiLogger } from "./middleware/apiLogger"
import getAuthenticatedSubrouterV1 from "./routes/v1/authenticated"
import getAuthenticatedSitesSubrouterV1 from "./routes/v1/authenticatedSites"
import getAuthenticatedSubrouter from "./routes/v2/authenticated"
Expand Down Expand Up @@ -62,7 +63,6 @@ const { FRONTEND_URL } = process.env
// Import middleware

// Import routes
const { apiLogger } = require("@middleware/apiLogger")
const { errorHandler } = require("@middleware/errorHandler")

const { FormsgRouter } = require("@routes/formsgSiteCreation")
Expand Down Expand Up @@ -109,9 +109,11 @@ const authMiddleware = getAuthMiddleware({ identityAuthService })
const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({
authMiddleware,
usersService,
apiLogger,
})
const authenticatedSitesSubrouterV1 = getAuthenticatedSitesSubrouterV1({
authMiddleware,
apiLogger,
})

const authenticatedSubrouterV2 = getAuthenticatedSubrouter({
Expand All @@ -121,13 +123,15 @@ const authenticatedSubrouterV2 = getAuthenticatedSubrouter({
usersService,
reposService,
deploymentsService,
apiLogger,
})
const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({
authMiddleware,
gitHubService,
configYmlService,
apiLogger,
})
const authV2Router = new AuthRouter({ authMiddleware, authService })
const authV2Router = new AuthRouter({ authMiddleware, authService, apiLogger })
const formsgRouter = new FormsgRouter({ usersService, infraService })
const formsgSiteLaunchRouter = new FormsgSiteLaunchRouter({
usersService,
Expand All @@ -148,9 +152,6 @@ app.use(express.urlencoded({ extended: false }))
app.use(cookieParser())
app.use(express.static(path.join(__dirname, "public")))

// Log api requests
app.use(apiLogger)

// Health endpoint
app.use("/v2/ping", (req, res, next) => res.status(200).send("Ok"))

Expand Down

0 comments on commit df3182e

Please sign in to comment.