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

feat: collaborators #485

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4592234
Chore: migrate authmiddlewareservice to typescript
alexanderleegs Jul 28, 2022
705d2d8
Feat: add site retrieval for email and admin users
alexanderleegs Jul 28, 2022
d94183a
build(devdeps): add lodash types
prestonlimlianjie Aug 11, 2022
00ec691
fix(model): rectify db model definitions
prestonlimlianjie Aug 11, 2022
7528b75
refactor: add message param to ForbiddenError
prestonlimlianjie Aug 11, 2022
0bdb767
fix(SitesService): fix type change from database model
prestonlimlianjie Aug 11, 2022
3262749
feat: add CollaboratorsService
prestonlimlianjie Aug 11, 2022
f2a1ae2
test: add tests for CollaboratorsService
prestonlimlianjie Aug 11, 2022
542d01f
feat: use CollaboratorService in authorization middleware
prestonlimlianjie Aug 11, 2022
a8e385c
test: add tests for authorization middleware
prestonlimlianjie Aug 11, 2022
e1b4889
feat: add CollaboratorsRouter
prestonlimlianjie Aug 11, 2022
eea564b
test: add tests for CollaboratorsRouter
prestonlimlianjie Aug 11, 2022
739f0bb
feat(db-migration): change site_members role enum in the database
prestonlimlianjie Aug 11, 2022
4f68c34
fix(models): undo erroneous changes
prestonlimlianjie Aug 11, 2022
4b700f0
fix: model field and failing tests
prestonlimlianjie Aug 15, 2022
bd377a0
fix: clean up painful rebase
prestonlimlianjie Aug 24, 2022
a41412d
chore: rename comment for clarity
prestonlimlianjie Sep 22, 2022
ea4e726
chore: remove unnecessary imports
prestonlimlianjie Sep 22, 2022
92575fc
style: rename var to be more boolean-like
prestonlimlianjie Sep 22, 2022
b1b6a00
style: define let vars as const
prestonlimlianjie Sep 22, 2022
cef758b
fix: use correct authZ method
prestonlimlianjie Sep 22, 2022
63bdf5d
refactor: authZ middleware methods
prestonlimlianjie Sep 22, 2022
22b28a7
chore: add types for collaborator mocks
prestonlimlianjie Sep 23, 2022
bd1649e
docs: add comment for deletion check
prestonlimlianjie Sep 28, 2022
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
5 changes: 5 additions & 0 deletions src/constants/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ export enum SiteStatus {
}

export const E2E_ISOMER_ID = "e2e-id"

export enum CollaboratorRoles {
Admin = "ADMIN",
Contributor = "USER",
}
239 changes: 239 additions & 0 deletions src/services/identity/CollaboratorsService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,239 @@
import _ from "lodash"
import { ModelStatic, Op } from "sequelize"
import validator from "validator"

import { ForbiddenError } from "@errors/ForbiddenError"
import { NotFoundError } from "@errors/NotFoundError"
import { UnprocessableError } from "@errors/UnprocessableError"

import { CollaboratorRoles } from "@constants/constants"

import { Whitelist, User, Site, SiteMember } from "@database/models"
import { BadRequestError } from "@root/errors/BadRequestError"
import { ConflictError } from "@root/errors/ConflictError"
import logger from "@root/logger/logger"

import SitesService from "./SitesService"
import UsersService from "./UsersService"

interface CollaboratorsServiceProps {
siteRepository: ModelStatic<Site>
siteMemberRepository: ModelStatic<SiteMember>
sitesService: SitesService
usersService: UsersService
whitelist: ModelStatic<Whitelist>
}

class CollaboratorsService {
// NOTE: Explicitly specifying using keyed properties to ensure
// that the types are synced.
private readonly siteRepository: CollaboratorsServiceProps["siteRepository"]

private readonly siteMemberRepository: CollaboratorsServiceProps["siteMemberRepository"]

private readonly sitesService: CollaboratorsServiceProps["sitesService"]

private readonly usersService: CollaboratorsServiceProps["usersService"]

private readonly whitelist: CollaboratorsServiceProps["whitelist"]

constructor({
siteRepository,
siteMemberRepository,
sitesService,
usersService,
whitelist,
}: CollaboratorsServiceProps) {
this.siteRepository = siteRepository
this.siteMemberRepository = siteMemberRepository
this.sitesService = sitesService
this.usersService = usersService
this.whitelist = whitelist
}

deriveAllowedRoleFromEmail = async (fullEmail: string) => {
const whitelistEntries = await this.whitelist.findAll({
where: {
expiry: {
[Op.or]: [{ [Op.is]: null }, { [Op.gt]: new Date() }],
},
},
})

const matchedDomains = whitelistEntries.filter((entry) =>
fullEmail.endsWith(entry.email)
)

if (!matchedDomains.length) return null

// TODO: Modify this method because the presence of the expiry field is not
// the best way of differentiating Admin/Contributor roles
return matchedDomains[0].expiry
? CollaboratorRoles.Contributor
: CollaboratorRoles.Admin
dcshzj marked this conversation as resolved.
Show resolved Hide resolved
}

list = async (siteName: string, requesterId?: string) => {
// Note:
// ===============================================
// We need to query from the Sites table instead of the SiteMembers table
// because Sequelize only recognizes that there is a relationship between Sites <-> Users.
// This means that we cannot retrieve joins if we start the query in the SiteMembers table.
//
// However, the converse is possible, i.e. we can query the Sites table and retrieve joined
// records from the Users table, along with the SiteMember records.
const site = await this.siteRepository.findOne({
where: { name: siteName },
include: [
{
model: User,
as: "site_members",
},
],
})
const collaborators = site?.site_members ?? []

// We perform the following sort via application code because:
// - sorting it via the ORM code alone is quite complicated
// - putting the sorting logic into a stored SQL function involves DB migration work
// - we can achieve this easily with lodash, and there is unlikely to be a performance hit
// given the small number of collaborators in each site
return _.orderBy(
collaborators,
[
// Prioritize Admins over Contributors
(
collaborator: User & {
SiteMember: SiteMember
}
) => collaborator.SiteMember.role === CollaboratorRoles.Admin,
// Prioritize elements where the userId matches the requesterId (i.e. "you")
(
collaborator: User & {
SiteMember: SiteMember
}
) => collaborator.id.toString() === requesterId,
// Prioritize the last logged in user
prestonlimlianjie marked this conversation as resolved.
Show resolved Hide resolved
(
collaborator: User & {
SiteMember: SiteMember
}
) => collaborator.lastLoggedIn,
],
["desc", "desc", "asc"]
)
}

create = async (siteName: string, email: string, acknowledged: boolean) => {
let site
let user
dcshzj marked this conversation as resolved.
Show resolved Hide resolved

if (!email || !validator.isEmail(email)) {
return new BadRequestError(
"That doesn't look like a valid email. Try a gov.sg or other whitelisted email."
)
Comment on lines +129 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw the error directly here instead of letting it be handled at the router level?

Copy link
Contributor Author

@prestonlimlianjie prestonlimlianjie Sep 22, 2022

Choose a reason for hiding this comment

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

My thought here was that we should move towards a neverthrow-like pattern (but I didn't want to introduce neverthrow into this specific PR just yet - we should do it in a separate PR to prevent scope creep).

In this pattern, we want to ensure that library methods don't throw errors themselves as far as possible. Instead, we want to get them to return errors of specific types so that their callers can figure out what to do with them.

In this PR, because we're not introducing neverthrow yet:

  • the business-logic/expected errors (e.g. site not found) will be returned as a specific error
  • the unexpected errors (e.g. database-related errors) will be thrown

In a separate PR, when we introduce neverthrow, errors in both categories will be returned as specific errors.

tagging @seaerchin since he is familiar with neverthrow from his work in FormSG - thoughts?

}

// 1. Check if email address is whitelisted, and derive the collaborator role
const derivedRole = await this.deriveAllowedRoleFromEmail(email)
if (!derivedRole) {
// Error - the user email is not whitelisted
logger.error(
`create collaborators error: user email ${email} is not whitelisted`
)
return new ForbiddenError(
`This collaborator couldn't be added. Visit our guide for more assistance.`
)
}

// 2. Check if site exists
site = await this.sitesService.getBySiteName(siteName)
if (!site) {
// Error - site does not exist
logger.error(`create collaborators error: site ${siteName} is not valid`)
return new NotFoundError(`Site does not exist`)
}

// 3. Check if valid user exists
user = await this.usersService.findByEmail(email)
if (!user) {
// Error - user with a valid gov email does not exist
logger.error(`create collaborators error: user ${email} is not valid`)
return new NotFoundError(
`This user does not have an Isomer account. Ask them to log in to Isomer and try adding them again.`
)
}

// 4. Check if user is already a site member
const existingSiteMember = await this.siteMemberRepository.findOne({
where: {
siteId: site.id,
userId: user.id,
},
})
if (existingSiteMember) {
return new ConflictError(`User is already a member of the site`)
}

// 5. Ensure that acknowledgement is true if the email role is contributor
if (derivedRole === CollaboratorRoles.Contributor && !acknowledged) {
return new UnprocessableError("Acknowledgement required")
dcshzj marked this conversation as resolved.
Show resolved Hide resolved
}

// 6. Create the SiteMembers record
return this.siteMemberRepository.create({
siteId: site.id,
userId: user.id,
role: derivedRole,
})
}

delete = async (siteName: string, userId: string) => {
dcshzj marked this conversation as resolved.
Show resolved Hide resolved
const site = await this.siteRepository.findOne({
where: { name: siteName },
include: [
{
model: User,
as: "site_members",
},
],
})

const siteMembers = site?.site_members ?? []
if (
!siteMembers.filter((member) => member.id.toString() === userId).length
) {
return new NotFoundError(`User is not a site member`)
}

const siteAdmins = siteMembers.filter(
(member) => member.SiteMember.role === CollaboratorRoles.Admin
)
if (siteAdmins.length === 1 && siteAdmins[0].id.toString() === userId) {
return new UnprocessableError(`Cannot delete final site admin`)
}
prestonlimlianjie marked this conversation as resolved.
Show resolved Hide resolved

return this.siteMemberRepository.destroy({
where: { siteId: site?.id, userId },
})
}

getRole = async (siteName: string, userId: string) => {
const site = await this.siteRepository.findOne({
where: { name: siteName },
include: [
{
model: User,
as: "site_members",
where: {
id: userId,
},
},
],
})

return (site?.site_members?.[0]?.SiteMember?.role as string | null) ?? null
}
}

export default CollaboratorsService