Feat/move whitelist into database #422
Conversation
| unique: true, | ||
| type: Sequelize.TEXT, | ||
| validate: { | ||
| isEmail: true, |
There was a problem hiding this comment.
not sure this is going to work - we only validate a suffix, not an email. this change means that organizational emails have to be added individually (eg: for .edu.sg, we have to add all indiv emails rather than just the suffix)
There was a problem hiding this comment.
You're right, thanks for catching! Fixed in a8805db
| expiry: { | ||
| type: Sequelize.DATE, | ||
| allowNull: true, | ||
| defaultValue: null, | ||
| }, |
There was a problem hiding this comment.
could we do expiry of 6 months from now and let the null be the exception? don't foresee us adding many indefinite whitelists as they would be vendors
There was a problem hiding this comment.
That's a good point, made the change in a8805db
database/models/Whitelist.ts
Outdated
| type: DataType.DATE, | ||
| defaultValue: null, | ||
| }) | ||
| expiry!: Date |
There was a problem hiding this comment.
wrong typing - the exclamation mark means that it can never be null, which is not what's declared
There was a problem hiding this comment.
Good catch, fa8737f
There was a problem hiding this comment.
the type should be Date | null i think
There was a problem hiding this comment.
Right, fixed in df92bcb
| @@ -15,7 +15,7 @@ | |||
| "format-fix": "npx prettier --write .", | |||
| "prepare": "husky install", | |||
| "version": "auto-changelog -p && git add CHANGELOG.md", | |||
| "npm run db:migrate": "source .env && npx sequelize-cli db:migrate", | |||
| "db:migrate": "source .env && npx sequelize-cli db:migrate", | |||
There was a problem hiding this comment.
good fix, i can't believe i didn't catch this when i wrote it @___@
| const whitelistEntries = await this.whitelist.findAll({ | ||
| attributes: ["email"], | ||
| where: { | ||
| expiry: { | ||
| [Op.or]: [{ [Op.is]: null }, { [Op.gt]: new Date() }], | ||
| }, | ||
| }, | ||
| }) | ||
| const whitelistDomains = whitelistEntries.map((entry) => entry.email) | ||
| const hasMatchDomain = | ||
| this.whitelistDomains.filter((domain) => email.endsWith(domain)).length > | ||
| 0 | ||
| whitelistDomains.filter((domain) => email.endsWith(domain)).length > 0 |
There was a problem hiding this comment.
this is fine as-is and it might be over-eager from my end, but wdyt about having a WhitelistService that
- returns valid whitelists
- checks if a given email is valid
There was a problem hiding this comment.
As discussed offline, we currently don't foresee the whitelist being used anywhere other than login - as such, we'll leave it as is for now
database/models/Whitelist.ts
Outdated
| type: DataType.DATE, | ||
| defaultValue: null, | ||
| }) | ||
| expiry!: Date |
There was a problem hiding this comment.
the type should be Date | null i think
| @@ -0,0 +1,36 @@ | |||
| module.exports = { | |||
| up: async (queryInterface, Sequelize) => { | |||
| await queryInterface.createTable("whitelists", { | |||
There was a problem hiding this comment.
nit: could we change the table name to be singular as this is a singular whitelist of emails
There was a problem hiding this comment.
Makes sense, changed in 61220d8
alexanderleegs
force-pushed
the
feat/move-whitelist-into-database
branch
from
50033fc to
f19045c
Compare
alexanderleegs
force-pushed
the
feat/move-whitelist-into-database
branch
from
61220d8 to
d9b00da
Compare
* Feat: add whitelist model * Feat: add migration * Feat: add whitelist table to initialisation * Feat: update canSendEmailOtp to use database * Chore: update tests * Fix: use db:migrate for local migrations * Fix: migration * Fix: whitelist model * fix: database model types * Fix: allow expiry type to be null * Chore: rename table to whitelist
Problem
This PR modifies our existing method for verifying the domains were whitelisted - we were previously relying on env vars for the list of whitelisted domains, but as the number of sites may increase with the CWP migration, we have opted to move this into a database, which removes the character restriction we previously had on the list of names.
Closes #381
Solution
Adds a new table to our database for whitelisted domains.
Breaking Changes
Fixes
This PR also fixes the User table types - we were previously not correctly specifying that the
emailandcontact_numbercould be a null value.New scripts:
db:migrate: modifies this script to usedb:migrateinstead ofnpm run db:migrate