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

Refactor/sites #341

Merged
merged 16 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions fixtures/repoInfo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
const repoInfo = {
name: "repo",
private: false,
description:
"Staging: https://repo-staging.netlify.app | Production: https://repo-prod.netlify.app",
pushed_at: "2021-09-09T02:41:37Z",
permissions: {
admin: true,
maintain: true,
push: true,
triage: true,
pull: true,
},
}

const repoInfo2 = {
name: "repo2",
private: false,
description:
"Staging: https://repo2-staging.netlify.app | Production: https://repo2-prod.netlify.app",
pushed_at: "2021-09-09T02:41:37Z",
permissions: {
admin: true,
maintain: true,
push: true,
triage: true,
pull: true,
},
}

const adminRepo = {
name: "isomercms-backend",
private: false,
description:
"Staging: https://isomercms-backend-staging.netlify.app | Production: https://isomercms-backend-prod.netlify.app",
pushed_at: "2021-09-09T02:41:37Z",
permissions: {
admin: true,
maintain: true,
push: true,
triage: true,
pull: true,
},
}

const noAccessRepo = {
name: "noaccess",
private: false,
description:
"Staging: https://noaccess-staging.netlify.app | Production: https://noaccess-prod.netlify.app",
pushed_at: "2021-09-09T02:41:37Z",
permissions: {
admin: false,
maintain: false,
push: false,
triage: false,
pull: true,
},
}

module.exports = {
repoInfo,
repoInfo2,
adminRepo,
noAccessRepo,
}
113 changes: 113 additions & 0 deletions newroutes/__tests__/Sites.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
const cookieParser = require("cookie-parser")
const express = require("express")
const request = require("supertest")

const { NotFoundError } = require("@errors/NotFoundError")

const { errorHandler } = require("@middleware/errorHandler")
const { attachReadRouteHandlerWrapper } = require("@middleware/routeHandler")

const { SitesRouter } = require("../sites")

// Can't set request fields - will always be undefined
const userId = undefined
const accessToken = undefined

const siteName = "siteName"

const reqDetails = { siteName, accessToken }

describe("Sites Router", () => {
const mockSitesService = {
getSites: jest.fn(),
checkHasAccess: jest.fn(),
getLastUpdated: jest.fn(),
getStagingUrl: jest.fn(),
}

const router = new SitesRouter({
sitesService: mockSitesService,
})

const app = express()
app.use(express.json({ limit: "7mb" }))
app.use(express.urlencoded({ extended: false }))
app.use(cookieParser())

// We can use read route handler here because we don't need to lock the repo
app.get("/", attachReadRouteHandlerWrapper(router.getSites))
app.get("/:siteName", attachReadRouteHandlerWrapper(router.checkHasAccess))
app.get(
"/:siteName/lastUpdated",
attachReadRouteHandlerWrapper(router.getLastUpdated)
)
app.get(
"/:siteName/stagingUrl",
attachReadRouteHandlerWrapper(router.getStagingUrl)
)
app.use(errorHandler)

beforeEach(() => {
jest.clearAllMocks()
})

describe("getSites", () => {
it("returns the list of sites accessible to the user", async () => {
const sitesResp = ["site1", "site2"]
mockSitesService.getSites.mockResolvedValueOnce(sitesResp)

const resp = await request(app).get(`/`).expect(200)

expect(resp.body).toStrictEqual({ siteNames: sitesResp })
expect(mockSitesService.getSites).toHaveBeenCalledWith({ accessToken })
})
})

describe("checkHasAccess", () => {
it("rejects if user has no access to a site", async () => {
mockSitesService.checkHasAccess.mockRejectedValueOnce(
new NotFoundError("")
)

await request(app).get(`/${siteName}`).expect(404)

expect(mockSitesService.checkHasAccess).toHaveBeenCalledWith(reqDetails, {
userId,
})
})

it("allows if user has access to a site", async () => {
await request(app).get(`/${siteName}`).expect(200)

expect(mockSitesService.checkHasAccess).toHaveBeenCalledWith(reqDetails, {
userId,
})
})
})

describe("getLastUpdated", () => {
it("returns the last updated time", async () => {
const lastUpdated = "last-updated"
mockSitesService.getLastUpdated.mockResolvedValueOnce(lastUpdated)

const resp = await request(app)
.get(`/${siteName}/lastUpdated`)
.expect(200)

expect(resp.body).toStrictEqual({ lastUpdated })
expect(mockSitesService.getLastUpdated).toHaveBeenCalledWith(reqDetails)
})
})

describe("getStagingUrl", () => {
it("returns the last updated time", async () => {
const stagingUrl = "staging-url"
mockSitesService.getStagingUrl.mockResolvedValueOnce(stagingUrl)

const resp = await request(app).get(`/${siteName}/stagingUrl`).expect(200)

expect(resp.body).toStrictEqual({ stagingUrl })
expect(mockSitesService.getStagingUrl).toHaveBeenCalledWith(reqDetails)
})
})
})
84 changes: 84 additions & 0 deletions newroutes/sites.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const autoBind = require("auto-bind")
const express = require("express")

// Import middleware
const { attachReadRouteHandlerWrapper } = require("@middleware/routeHandler")

const { authMiddleware } = require("@root/newmiddleware/index")

class SitesRouter {
constructor({ sitesService }) {
this.sitesService = sitesService
// We need to bind all methods because we don't invoke them from the class directly
autoBind(this)
}

async getSites(req, res) {
const { accessToken } = req
const siteNames = await this.sitesService.getSites({ accessToken })
return res.status(200).json({ siteNames })
}

async checkHasAccess(req, res) {
const {
accessToken,
userId,
params: { siteName },
} = req

await this.sitesService.checkHasAccess(
{
accessToken,
siteName,
},
{ userId }
)
return res.status(200).send("OK")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the .send("OK") if it's unneeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

^this commit got lost, unsure if there are others. i'm fine iwth not doing this as it's relatively minor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit was preserved as 9b05ff7 - rebasing seems to overwrite the original commit hash! I did reintroduce the .send("OK") in a later commit because one of the tests was failing, and elected to use .send("OK") to keep it in line with how we return responses in the rest of our routers

}

async getLastUpdated(req, res) {
const {
accessToken,
params: { siteName },
} = req
const lastUpdated = await this.sitesService.getLastUpdated({
accessToken,
siteName,
})
return res.status(200).json({ lastUpdated })
}

async getStagingUrl(req, res) {
const {
accessToken,
params: { siteName },
} = req

const stagingUrl = await this.sitesService.getStagingUrl({
accessToken,
siteName,
})
return res.status(200).json({ stagingUrl })
}

getRouter() {
const router = express.Router()

router.use(authMiddleware.verifyJwt)

router.get("/", attachReadRouteHandlerWrapper(this.getSites))
router.get("/:siteName", attachReadRouteHandlerWrapper(this.checkHasAccess))
router.get(
"/:siteName/lastUpdated",
attachReadRouteHandlerWrapper(this.getLastUpdated)
)
router.get(
"/:siteName/stagingUrl",
attachReadRouteHandlerWrapper(this.getStagingUrl)
)

return router
}
}

module.exports = { SitesRouter }
5 changes: 5 additions & 0 deletions server.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ const { ResourceCategoriesRouter } = require("./newroutes/resourceCategories")
const { ResourcePagesRouter } = require("./newroutes/resourcePages")
const { ResourceRoomRouter } = require("./newroutes/resourceRoom")
const { SettingsRouter } = require("./newroutes/settings")
const { SitesRouter } = require("./newroutes/sites")
const { UnlinkedPagesRouter } = require("./newroutes/unlinkedPages")
const { SitesService } = require("./services/utilServices/SitesService")

const authService = new AuthService()
const gitHubService = new GitHubService({ axiosInstance })
Expand All @@ -120,6 +122,7 @@ const homepagePageService = new HomepagePageService({ gitHubService })
const configYmlService = new ConfigYmlService({ gitHubService })
const footerYmlService = new FooterYmlService({ gitHubService })
const navYmlService = new NavYmlService({ gitHubService })
const sitesService = new SitesService({ gitHubService, configYmlService })
const collectionPageService = new CollectionPageService({
gitHubService,
collectionYmlService,
Expand Down Expand Up @@ -175,6 +178,7 @@ const settingsService = new SettingsService({
})

const authV2Router = new AuthRouter({ authService })
const sitesV2Router = new SitesRouter({ sitesService })
const unlinkedPagesRouter = new UnlinkedPagesRouter({
unlinkedPageService,
unlinkedPagesDirectoryService,
Expand Down Expand Up @@ -242,6 +246,7 @@ app.use("/v1/sites", navigationRouter)
app.use("/v1/sites", netlifyTomlRouter)

app.use("/v2/auth", authV2Router.getRouter())
app.use("/v2/sites", sitesV2Router.getRouter())
app.use("/v2/sites", collectionPagesV2Router.getRouter())
app.use("/v2/sites", unlinkedPagesRouter.getRouter())
app.use("/v2/sites", collectionsV2Router.getRouter())
Expand Down
18 changes: 18 additions & 0 deletions services/db/GitHubService.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,24 @@ class GitHubService {
{ headers }
)
}

async checkHasAccess({ accessToken, siteName }, { userId }) {
const endpoint = `${siteName}/collaborators/${userId}`

const headers = {
Authorization: `token ${accessToken}`,
"Content-Type": "application/json",
}
try {
await this.axiosInstance.get(endpoint, { headers })
} catch (err) {
const { status } = err.response
// If user is unauthorized or site does not exist, show the same NotFoundError
if (status === 404 || status === 403)
throw new NotFoundError("Site does not exist")
throw err
}
}
}

module.exports = { GitHubService }
15 changes: 15 additions & 0 deletions services/db/__tests__/GitHubService.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -649,4 +649,19 @@ describe("Github Service", () => {
)
})
})

describe("checkHasAccess", () => {
const userId = "userId"
const refEndpoint = `${siteName}/collaborators/${userId}`
const headers = {
Authorization: `token ${accessToken}`,
"Content-Type": "application/json",
}
it("Checks whether user has write access to site", async () => {
await service.checkHasAccess({ accessToken, siteName }, { userId })
expect(mockAxiosInstance.get).toHaveBeenCalledWith(refEndpoint, {
headers,
})
})
})
})
Loading