diff --git a/services/api/openapi.json b/services/api/openapi.json index 7145b96f..f9517ebb 100644 --- a/services/api/openapi.json +++ b/services/api/openapi.json @@ -63,7 +63,6 @@ }, "channel": { "type": "string", - "default": "email", "enum": [ "email", "sms" @@ -878,6 +877,40 @@ "required": [], "additionalProperties": false }, + "createdAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, + "updatedAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, "roles": { "type": "object", "properties": { @@ -963,40 +996,6 @@ "isTester": { "type": "boolean" }, - "createdAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, - "updatedAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, "include": { "$ref": "#/components/schemas/Includes" }, @@ -1463,7 +1462,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } } }, "/1/products/:id": { @@ -1562,7 +1565,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } }, "delete": { "summary": "Delete product", @@ -1581,7 +1588,11 @@ { "bearerAuth": [] } - ] + ], + "x-permissions": { + "endpoint": "products", + "permission": "write" + } } }, "/1/products/search": { @@ -3894,6 +3905,40 @@ "required": [], "additionalProperties": false }, + "createdAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, + "updatedAt": { + "anyOf": [ + { + "$ref": "#/components/schemas/DateTime" + }, + { + "type": "array", + "items": { + "$ref": "#/components/schemas/DateTime" + } + }, + { + "$ref": "#/components/schemas/DateRange" + } + ], + "description": "Allows searching by a date, array of dates, or a range." + }, "roles": { "type": "object", "properties": { @@ -3979,40 +4024,6 @@ "isTester": { "type": "boolean" }, - "createdAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, - "updatedAt": { - "anyOf": [ - { - "$ref": "#/components/schemas/DateTime" - }, - { - "type": "array", - "items": { - "$ref": "#/components/schemas/DateTime" - } - }, - { - "$ref": "#/components/schemas/DateRange" - } - ], - "description": "Allows searching by a date, array of dates, or a range." - }, "include": { "$ref": "#/components/schemas/Includes" }, @@ -4995,6 +5006,12 @@ "additionalProperties": false } }, + "createdAt": { + "$ref": "#/components/schemas/DateTime" + }, + "updatedAt": { + "$ref": "#/components/schemas/DateTime" + }, "roles": { "type": "array", "items": { @@ -5023,12 +5040,6 @@ }, "isTester": { "type": "boolean" - }, - "createdAt": { - "$ref": "#/components/schemas/DateTime" - }, - "updatedAt": { - "$ref": "#/components/schemas/DateTime" } }, "required": [ diff --git a/services/api/src/models/definitions/user.json b/services/api/src/models/definitions/user.json index 648a4369..15f9a441 100644 --- a/services/api/src/models/definitions/user.json +++ b/services/api/src/models/definitions/user.json @@ -16,6 +16,12 @@ "writeAccess": [ "admin", "superAdmin" + ], + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" ] }, "emailVerified": { @@ -30,6 +36,12 @@ "writeAccess": [ "admin", "superAdmin" + ], + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" ] }, "phoneVerified": { @@ -71,29 +83,44 @@ } } ], - "roles": [ - { - "role": { - "type": "String", - "required": true - }, - "scope": { - "type": "String", - "required": true, - "enum": [ - "global", - "organization" - ] - }, - "scopeRef": { - "type": "ObjectId", - "ref": "Organization" + "$staff": { + "type": "Scope", + "readAccess": [ + "self", + "viewer", + "admin", + "superAdmin" + ], + "writeAccess": [ + "admin", + "superAdmin" + ], + "attributes": { + "roles": [ + { + "role": { + "type": "String", + "required": true + }, + "scope": { + "type": "String", + "required": true, + "enum": [ + "global", + "organization" + ] + }, + "scopeRef": { + "type": "ObjectId", + "ref": "Organization" + } + } + ], + "isTester": { + "type": "Boolean", + "default": false } } - ], - "isTester": { - "type": "Boolean", - "default": false }, "$readOnly": { "type": "Scope", @@ -135,6 +162,10 @@ "type": "Boolean", "readAccess": "none" }, + "channel": { + "type": "String", + "readAccess": "none" + }, "info": { "type": "Object", "readAccess": "none" diff --git a/services/api/src/routes/auth/apple.js b/services/api/src/routes/auth/apple.js index da0a4a4d..ef032986 100644 --- a/services/api/src/routes/auth/apple.js +++ b/services/api/src/routes/auth/apple.js @@ -3,7 +3,7 @@ import yd from '@bedrockio/yada'; import { validateBody } from '../../utils/middleware/validate.js'; import { authenticate } from '../../utils/middleware/authenticate.js'; -import { login } from '../../utils/auth/index.js'; +import { login, claimUnverifiedUser } from '../../utils/auth/index.js'; import { createAuthToken } from '../../utils/tokens.js'; import { verifyToken, upsertAppleAuthenticator, removeAppleAuthenticator } from '../../utils/auth/apple.js'; import { User, AuditEntry } from '../../models/index.js'; @@ -36,6 +36,7 @@ router let result; if (user) { + claimUnverifiedUser(user); token = await login(ctx, user, { message: 'Logged in with Apple', }); diff --git a/services/api/src/routes/auth/google.js b/services/api/src/routes/auth/google.js index 0c118a25..fec1a548 100644 --- a/services/api/src/routes/auth/google.js +++ b/services/api/src/routes/auth/google.js @@ -3,7 +3,7 @@ import yd from '@bedrockio/yada'; import { validateBody } from '../../utils/middleware/validate.js'; import { authenticate } from '../../utils/middleware/authenticate.js'; -import { login } from '../../utils/auth/index.js'; +import { login, claimUnverifiedUser } from '../../utils/auth/index.js'; import { createAuthToken } from '../../utils/tokens.js'; import { verifyToken, upsertGoogleAuthenticator, removeGoogleAuthenticator } from '../../utils/auth/google.js'; @@ -35,6 +35,7 @@ router let result; if (user) { + claimUnverifiedUser(user); token = await login(ctx, user, { message: 'Logged in with Google', }); diff --git a/services/api/src/routes/auth/google.test.js b/services/api/src/routes/auth/google.test.js index ae52a243..a8f853ed 100644 --- a/services/api/src/routes/auth/google.test.js +++ b/services/api/src/routes/auth/google.test.js @@ -63,6 +63,27 @@ describe('/1/auth/google', () => { expect(lastUsedAt).toEqual(new Date('2020-01-01T00:00:01.000Z')); }); + it('should remove credentials added before the email was verified', async () => { + let user = await createUser({ + email: 'foo@bar.com', + password: 'attacker password', + }); + const code = createCode({ + email: 'foo@bar.com', + }); + + const response = await request('POST', '/1/auth/google', { + code, + }); + expect(response).toHaveStatus(200); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(true); + expect(hasAuthenticator(user, 'password')).toBe(false); + expect(hasAuthenticator(user, 'google')).toBe(true); + expect(user.authTokens).toHaveLength(1); + }); + it('should not be able to register with an unverified email', async () => { const code = createCode({ givenName: 'Bob', diff --git a/services/api/src/routes/auth/otp.js b/services/api/src/routes/auth/otp.js index 1c752a29..1ef10744 100644 --- a/services/api/src/routes/auth/otp.js +++ b/services/api/src/routes/auth/otp.js @@ -4,28 +4,32 @@ import { validateBody } from '../../utils/middleware/validate.js'; import { sendOtp } from '../../utils/auth/otp.js'; import { verifyOtp } from '../../utils/auth/otp.js'; -import { login, verifyLoginAttempts } from '../../utils/auth/index.js'; +import { login, verifyLoginAttempts, claimUnverifiedUser } from '../../utils/auth/index.js'; import { AuditEntry } from '../../models/index.js'; -import { findUser } from './utils.js'; +import { findUser, validateIdentity } from './utils.js'; const router = new Router(); router .post( '/send', - validateBody({ - type: yd.string().allow('link', 'code').default('code'), - channel: yd.string().allow('email', 'sms').default('email'), - email: yd.string().email(), - phone: yd.string().phone(), - }), + validateBody( + validateIdentity({ + type: yd.string().allow('link', 'code').default('code'), + channel: yd.string().allow('email', 'sms'), + email: yd.string().email(), + phone: yd.string().phone(), + }), + ), async (ctx) => { const { body } = ctx.request; const user = await findUser(ctx); const challenge = await sendOtp(user, { ...body, + // Send to whichever identifier named the account unless told otherwise. + channel: body.channel || (body.phone ? 'sms' : 'email'), phase: 'login', }); @@ -38,13 +42,15 @@ router ) .post( '/login', - validateBody({ - code: yd.string().length(6).required(), - email: yd.string().email(), - phone: yd.string().phone(), - }), + validateBody( + validateIdentity({ + code: yd.string().length(6).required(), + email: yd.string().email(), + phone: yd.string().phone(), + }), + ), async (ctx) => { - const { code, email, phone } = ctx.request.body; + const { code } = ctx.request.body; const user = await findUser(ctx); if (!user) { @@ -58,8 +64,9 @@ router ctx.throw(401, error); } + let authenticator; try { - await verifyOtp(user, code); + authenticator = await verifyOtp(user, code); } catch (error) { await user.save(); await AuditEntry.append('OTP Verification Failure', { @@ -69,9 +76,15 @@ router ctx.throw(401, error); } - if (email) { - user.emailVerified = true; - } else if (phone) { + // Verify the channel the code was delivered to, not the one used to look up the user. + if (authenticator.channel === 'email') { + // An MFA code follows a password login, so this user already owns the account. + if (authenticator.isMfa) { + user.emailVerified = true; + } else { + claimUnverifiedUser(user); + } + } else if (authenticator.channel === 'sms') { user.phoneVerified = true; } diff --git a/services/api/src/routes/auth/otp.test.js b/services/api/src/routes/auth/otp.test.js index 7a683a8b..5846233c 100644 --- a/services/api/src/routes/auth/otp.test.js +++ b/services/api/src/routes/auth/otp.test.js @@ -175,7 +175,7 @@ describe('/1/auth/otp', () => { const user = await createUser({ email: 'foo@bar.com', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'email' }); const response = await request('POST', '/1/auth/otp/login', { email: user.email, code, @@ -188,7 +188,7 @@ describe('/1/auth/otp', () => { const user = await createUser({ phone: '+12223456789', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'sms' }); const response = await request('POST', '/1/auth/otp/login', { phone: user.phone, code, @@ -258,7 +258,7 @@ describe('/1/auth/otp', () => { let user = await createUser({ email: 'foo@bar.com', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'email' }); await request('POST', '/1/auth/otp/login', { email: user.email, code, @@ -272,7 +272,7 @@ describe('/1/auth/otp', () => { let user = await createUser({ phone: '+12223456789', }); - const code = await createOtp(user); + const code = await createOtp(user, { channel: 'sms' }); await request('POST', '/1/auth/otp/login', { phone: user.phone, code, @@ -282,6 +282,59 @@ describe('/1/auth/otp', () => { expect(user.phoneVerified).toBe(true); }); + it('should text the code when identified by phone', async () => { + let user = await createUser({ + phone: '+12223456789', + }); + + await request('POST', '/1/auth/otp/send', { + phone: user.phone, + }); + assertSmsSent({ + phone: user.phone, + }); + + user = await User.findById(user.id); + const { code } = user.authenticators[0]; + await request('POST', '/1/auth/otp/login', { + phone: user.phone, + code, + }); + + user = await User.findById(user.id); + expect(user.phoneVerified).toBe(true); + }); + + it('should not accept both an email and a phone', async () => { + const user = await createUser({ + email: 'foo@bar.com', + phone: '+12223456789', + }); + const code = await createOtp(user, { channel: 'email' }); + const response = await request('POST', '/1/auth/otp/login', { + email: user.email, + phone: user.phone, + code, + }); + expect(response).toHaveStatus(400); + }); + + it('should not verify email when the code was sent by sms', async () => { + let user = await createUser({ + email: 'foo@bar.com', + phone: '+12223456789', + }); + const code = await createOtp(user, { channel: 'sms' }); + await request('POST', '/1/auth/otp/login', { + email: user.email, + code, + }); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(false); + expect(user.phoneVerified).toBe(true); + }); + it('should throttle logins', async () => { mockTime('2020-01-01'); let response; diff --git a/services/api/src/routes/auth/password.js b/services/api/src/routes/auth/password.js index 42ecf750..d289a9ad 100644 --- a/services/api/src/routes/auth/password.js +++ b/services/api/src/routes/auth/password.js @@ -7,7 +7,7 @@ import { validateBody } from '../../utils/middleware/validate.js'; import { authenticate } from '../../utils/middleware/authenticate.js'; import { createAuthToken, createAccessToken } from '../../utils/tokens.js'; -import { login, verifyLoginAttempts } from '../../utils/auth/index.js'; +import { login, verifyLoginAttempts, claimUnverifiedUser } from '../../utils/auth/index.js'; import { verifyPassword } from '../../utils/auth/password.js'; import { sendOtp } from '../../utils/auth/otp.js'; import { sendMail } from '../../utils/messaging/index.js'; @@ -125,8 +125,15 @@ router password: yd.string().password().required(), }), async (ctx) => { - const { authUser } = ctx.state; + const { authUser, jwt } = ctx.state; const { password } = ctx.request.body; + + // Other access tokens (e.g. unsubscribe links) share this token type. + if (jwt.action !== 'reset-password') { + ctx.throw(401, 'Token is not valid for password reset.'); + } + + claimUnverifiedUser(authUser); authUser.password = password; authUser.loginAttempts = 0; diff --git a/services/api/src/routes/auth/password.test.js b/services/api/src/routes/auth/password.test.js index 33ba7212..9a35bf66 100644 --- a/services/api/src/routes/auth/password.test.js +++ b/services/api/src/routes/auth/password.test.js @@ -320,7 +320,7 @@ describe('/1/auth', () => { let user = await createUser(); const password = 'very new password'; const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); @@ -341,6 +341,7 @@ describe('/1/auth', () => { user = await User.findById(user.id); await expect(verifyPassword(user, password)).resolves.not.toThrow(); + expect(user.emailVerified).toBe(true); expect(user.authTokens).toEqual([ expect.objectContaining({ @@ -349,6 +350,38 @@ describe('/1/auth', () => { ]); }); + it('should remove sessions created before the email was verified', async () => { + let user = await createUser(); + createAuthToken(context(), user); + const token = createAccessToken(user, { + action: 'reset-password', + duration: '30m', + }); + await user.save(); + + const response = await request('POST', '/1/auth/password/update', { password: 'new password' }, { token }); + expect(response).toHaveStatus(200); + + user = await User.findById(user.id); + expect(user.emailVerified).toBe(true); + expect(user.authTokens).toEqual([ + expect.objectContaining({ + jti: getJti(response.body.data.token), + }), + ]); + }); + + it('should reject access tokens issued for other actions', async () => { + const user = await createUser(); + const token = createAccessToken(user, { + action: 'unsubscribe', + channel: 'email', + }); + + const response = await request('POST', '/1/auth/password/update', { password: 'new password' }, { token }); + expect(response).toHaveStatus(401); + }); + it('should error without user', async () => { const response = await request('POST', '/1/auth/password/update', { password: 'new password', @@ -360,7 +393,7 @@ describe('/1/auth', () => { mockTime('2020-01-01T00:00:00.000Z'); const user = await createUser(); const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); @@ -419,7 +452,7 @@ describe('/1/auth', () => { const password = 'very new password'; const token = createAccessToken(user, { - action: 'reset', + action: 'reset-password', duration: '30m', }); await user.save(); diff --git a/services/api/src/routes/auth/totp.js b/services/api/src/routes/auth/totp.js index ea81f17d..9dceab58 100644 --- a/services/api/src/routes/auth/totp.js +++ b/services/api/src/routes/auth/totp.js @@ -8,18 +8,20 @@ import { login, verifyLoginAttempts } from '../../utils/auth/index.js'; import { verifyCode, verifyTotp, generateTotp, enableTotp, revokeTotp } from '../../utils/auth/totp.js'; import { AuditEntry } from '../../models/index.js'; -import { findUser } from './utils.js'; +import { findUser, validateIdentity } from './utils.js'; const router = new Router(); router .post( '/login', - validateBody({ - phone: yd.string().phone(), - email: yd.string().email(), - code: yd.string().length(6).required(), - }), + validateBody( + validateIdentity({ + phone: yd.string().phone(), + email: yd.string().email(), + code: yd.string().length(6).required(), + }), + ), async (ctx) => { const { code } = ctx.request.body; diff --git a/services/api/src/routes/auth/totp.test.js b/services/api/src/routes/auth/totp.test.js index e606af49..0de6462c 100644 --- a/services/api/src/routes/auth/totp.test.js +++ b/services/api/src/routes/auth/totp.test.js @@ -10,7 +10,7 @@ describe('/1/auth/totp', () => { it('should verify a code', async () => { mockTime('2020-01-01T00:00:00.000Z'); - let user = await createUser(); + let user = await createUser({ password: 'password' }); const secret = createSecret(); enableTotp(user, secret); @@ -37,12 +37,26 @@ describe('/1/auth/totp', () => { assertAuthToken(user, response.body.data.token); user = await User.findById(user.id); - expect(user.authenticators.toObject()).toMatchObject([ - { - type: 'totp', - lastUsedAt: new Date('2020-01-01T00:00:01.000Z'), - }, - ]); + expect(user.authenticators.find((a) => a.type === 'totp').lastUsedAt).toEqual( + new Date('2020-01-01T00:00:01.000Z'), + ); + }); + + it('should require a recent password login', async () => { + mockTime('2020-01-01T00:00:00.000Z'); + + const user = await createUser({ password: 'password' }); + const secret = createSecret(); + enableTotp(user, secret); + await user.save(); + + advanceTime(10 * 60 * 1000); + + const response = await request('POST', '/1/auth/totp/login', { + email: user.email, + code: speakeasy.totp({ secret }), + }); + expect(response).toHaveStatus(401); }); it('should throttle logins', async () => { @@ -51,6 +65,7 @@ describe('/1/auth/totp', () => { let code; const user = await createUser({ + password: 'password', loginAttempts: 5, lastLoginAttemptAt: new Date(), }); diff --git a/services/api/src/routes/auth/utils.js b/services/api/src/routes/auth/utils.js index 563260f9..f523eebd 100644 --- a/services/api/src/routes/auth/utils.js +++ b/services/api/src/routes/auth/utils.js @@ -1,24 +1,21 @@ +import yd from '@bedrockio/yada'; import { User } from '../../models/index.js'; -async function findUser(ctx) { - const { phone, email } = ctx.request.body; - - let query; - if (phone) { - query = { phone }; - } else if (email) { - query = { email }; - } else { - if (phone === '') { - ctx.throw(400, 'Phone is required.'); - } else if (email === '') { - ctx.throw(400, 'Email is required.'); - } else { - ctx.throw(400, 'Phone or email is required.'); +// Identify the account by exactly one of email or phone, so the channel a code +// is sent to is never ambiguous. +function validateIdentity(body) { + return yd.object(body).custom((val) => { + if (val.email && val.phone) { + throw new Error('Cannot provide both an email and a phone.'); + } else if (!val.email && !val.phone) { + throw new Error('Either email or phone is required.'); } - } + }); +} - return await User.findOne(query); +async function findUser(ctx) { + const { phone, email } = ctx.request.body; + return await User.findOne(phone ? { phone } : { email }); } -export { findUser }; +export { findUser, validateIdentity }; diff --git a/services/api/src/routes/index.js b/services/api/src/routes/index.js index 8d6571ef..08d832a5 100644 --- a/services/api/src/routes/index.js +++ b/services/api/src/routes/index.js @@ -1,4 +1,5 @@ import Router from '@koa/router'; +import config from '@bedrockio/config'; import meta from './meta.js'; import docs from './docs.js'; @@ -21,7 +22,12 @@ const router = new Router({ }); router.use('/meta', meta.routes()); -router.use('/docs', docs.routes()); + +// Docs editing writes to openapi.json on disk and is unauthenticated. +if (config.get('ENV_NAME') === 'development') { + router.use('/docs', docs.routes()); +} + router.use('/auth', auth.routes()); router.use('/users', users.routes()); router.use('/products', products.routes()); diff --git a/services/api/src/routes/invites.js b/services/api/src/routes/invites.js index 22f007c9..d0137cdf 100644 --- a/services/api/src/routes/invites.js +++ b/services/api/src/routes/invites.js @@ -7,6 +7,7 @@ import { authenticate } from '../utils/middleware/authenticate.js'; import { requirePermissions } from '../utils/middleware/permissions.js'; import { createAuthToken } from '../utils/tokens.js'; +import { claimUnverifiedUser } from '../utils/auth/index.js'; import { Invite, User, AuditEntry } from '../models/index.js'; import { sendMessage, sendMail } from '../utils/messaging/index.js'; @@ -62,6 +63,7 @@ router }); if (user) { + claimUnverifiedUser(user); const token = createAuthToken(ctx, user); await user.save(); ctx.body = { @@ -74,6 +76,7 @@ router const user = new User({ ...ctx.request.body, email, + emailVerified: true, ...(role && { roles: [ { diff --git a/services/api/src/routes/products.js b/services/api/src/routes/products.js index d6004606..4805f339 100644 --- a/services/api/src/routes/products.js +++ b/services/api/src/routes/products.js @@ -2,6 +2,7 @@ import Router from '@koa/router'; import { fetchByParam } from '../utils/middleware/params.js'; import { validateBody } from '../utils/middleware/validate.js'; import { authenticate } from '../utils/middleware/authenticate.js'; +import { requirePermissions } from '../utils/middleware/permissions.js'; import { csvExport } from '../utils/csv.js'; import { Product } from '../models/index.js'; @@ -10,7 +11,7 @@ const router = new Router(); router .use(authenticate()) .param('id', fetchByParam(Product)) - .post('/', validateBody(Product.getCreateValidation()), async (ctx) => { + .post('/', requirePermissions('products.write'), validateBody(Product.getCreateValidation()), async (ctx) => { const product = await Product.create(ctx.request.body); ctx.body = { @@ -44,7 +45,7 @@ router }; }, ) - .patch('/:id', validateBody(Product.getUpdateValidation()), async (ctx) => { + .patch('/:id', requirePermissions('products.write'), validateBody(Product.getUpdateValidation()), async (ctx) => { const { product } = ctx.state; product.assign(ctx.request.body); @@ -54,7 +55,7 @@ router data: product, }; }) - .delete('/:id', async (ctx) => { + .delete('/:id', requirePermissions('products.write'), async (ctx) => { const { product } = ctx.state; await product.delete(); ctx.status = 204; diff --git a/services/api/src/routes/products.test.js b/services/api/src/routes/products.test.js index 7296244f..f70be882 100644 --- a/services/api/src/routes/products.test.js +++ b/services/api/src/routes/products.test.js @@ -1,12 +1,12 @@ import mongoose from 'mongoose'; -import { request, createUser } from '../utils/testing/index.js'; +import { request, createUser, createAdmin } from '../utils/testing/index.js'; import { Product } from '../models/index.js'; describe('/1/products', () => { describe('POST /', () => { it('should be able to create product', async () => { - const user = await createUser(); + const user = await createAdmin(); const response = await request( 'POST', '/1/products', @@ -20,6 +20,16 @@ describe('/1/products', () => { expect(response).toHaveStatus(200); expect(data.name).toBe('some other product'); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('POST', '/1/products', { name: 'x', shop: product.shop }, { user }); + expect(response).toHaveStatus(403); + }); }); describe('GET /:product', () => { @@ -74,7 +84,7 @@ describe('/1/products', () => { describe('PATCH /:product', () => { it('admins should be able to update product', async () => { - const user = await createUser(); + const user = await createAdmin(); const product = await Product.create({ name: 'test 1', description: 'Some description', @@ -86,11 +96,21 @@ describe('/1/products', () => { const dbProduct = await Product.findById(product.id); expect(dbProduct.name).toEqual('new name'); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('PATCH', `/1/products/${product.id}`, { name: 'new name' }, { user }); + expect(response).toHaveStatus(403); + }); }); describe('DELETE /:product', () => { it('should be able to delete product', async () => { - const user = await createUser(); + const user = await createAdmin(); const product = await Product.create({ name: 'test 1', description: 'Some description', @@ -101,5 +121,15 @@ describe('/1/products', () => { const dbProduct = await Product.findByIdDeleted(product.id); expect(dbProduct.deletedAt).toBeDefined(); }); + + it('should deny access to non-admins', async () => { + const user = await createUser(); + const product = await Product.create({ + name: 'test 1', + shop: new mongoose.Types.ObjectId(), + }); + const response = await request('DELETE', `/1/products/${product.id}`, {}, { user }); + expect(response).toHaveStatus(403); + }); }); }); diff --git a/services/api/src/routes/uploads-local.test.js b/services/api/src/routes/uploads-local.test.js index cf70e6f4..b6031608 100644 --- a/services/api/src/routes/uploads-local.test.js +++ b/services/api/src/routes/uploads-local.test.js @@ -34,6 +34,18 @@ describe('/1/uploads', () => { expect(response.body.data.filename).toBe('test.png'); }); + it('should not expose owner contact details when included', async () => { + const owner = await createUser({ phone: '+12125551234' }); + const upload = await createUpload({ owner }); + const response = await request('GET', `/1/uploads/${upload.id}`, { include: 'owner' }, {}); + expect(response).toHaveStatus(200); + const { owner: data } = response.body.data; + expect(data.firstName).toBe(owner.firstName); + expect(data.email).toBeUndefined(); + expect(data.phone).toBeUndefined(); + expect(data.roles).toBeUndefined(); + }); + it('should be able to access private upload as admin', async () => { const admin = await createAdmin(); const upload = await createUpload({ diff --git a/services/api/src/routes/users.test.js b/services/api/src/routes/users.test.js index 6e53e0ae..6efbeb59 100644 --- a/services/api/src/routes/users.test.js +++ b/services/api/src/routes/users.test.js @@ -77,6 +77,29 @@ describe('/1/users', () => { user = await User.findById(user._id); expect(user.deviceToken).toBe('new-token'); }); + + it('should not allow changing own roles', async () => { + let user = await createUser(); + const response = await request( + 'PATCH', + '/1/users/me', + { + roles: [{ role: 'superAdmin', scope: 'global' }], + }, + { user }, + ); + expect(response).toHaveStatus(401); + user = await User.findById(user._id); + expect(user.roles).toHaveLength(0); + }); + + it('should not allow setting tester flag', async () => { + let user = await createUser(); + const response = await request('PATCH', '/1/users/me', { isTester: true }, { user }); + expect(response).toHaveStatus(401); + user = await User.findById(user._id); + expect(user.isTester).toBe(false); + }); }); describe('POST /', () => { diff --git a/services/api/src/utils/auth/google.js b/services/api/src/utils/auth/google.js index d6073e78..d339e4e1 100644 --- a/services/api/src/utils/auth/google.js +++ b/services/api/src/utils/auth/google.js @@ -25,6 +25,7 @@ async function verifyToken(code) { } return { email: payload.email, + emailVerified: true, firstName: payload.given_name, lastName: payload.family_name, }; diff --git a/services/api/src/utils/auth/login.js b/services/api/src/utils/auth/login.js index 2489806f..ffef0ae0 100644 --- a/services/api/src/utils/auth/login.js +++ b/services/api/src/utils/auth/login.js @@ -33,6 +33,18 @@ async function login(ctx, user, options = {}) { return token; } +// Call once the user proves they own the email (OAuth, reset link, invite). +// Until then anyone could have signed up with this address, so its existing +// logins (password, passkeys, TOTP, sessions) are removed rather than trusted. +function claimUnverifiedUser(user) { + if (!user.emailVerified) { + user.authenticators = []; + user.authTokens = []; + user.mfaMethod = 'none'; + user.emailVerified = true; + } +} + async function verifyLoginAttempts(user, ctx) { let { loginAttempts = 0, lastLoginAttemptAt } = user; @@ -63,4 +75,4 @@ async function verifyLoginAttempts(user, ctx) { } } -export { login, verifyLoginAttempts }; +export { login, claimUnverifiedUser, verifyLoginAttempts }; diff --git a/services/api/src/utils/auth/otp.js b/services/api/src/utils/auth/otp.js index 699b5663..b0e34452 100644 --- a/services/api/src/utils/auth/otp.js +++ b/services/api/src/utils/auth/otp.js @@ -54,12 +54,13 @@ async function createOtp(user, options = {}) { clearAuthenticators(user, 'otp'); const code = user.isTester ? TESTER_CODE : generateCode(); - const { isMfa = false } = options; + const { isMfa = false, channel } = options; addAuthenticator(user, { type: 'otp', code, isMfa, + channel, expiresAt: new Date(Date.now() + EXPIRE), }); @@ -85,6 +86,7 @@ async function verifyOtp(user, code) { } clearAuthenticators(user, 'otp'); + return authenticator; } export { sendOtp, createOtp, verifyOtp }; diff --git a/services/api/src/utils/auth/totp.js b/services/api/src/utils/auth/totp.js index 5a00741b..6d48a2d3 100644 --- a/services/api/src/utils/auth/totp.js +++ b/services/api/src/utils/auth/totp.js @@ -48,10 +48,8 @@ async function revokeTotp(user) { async function verifyTotp(user, code) { const authenticator = assertAuthenticator(user, 'totp'); verifyCode(authenticator.secret, code); - - if (authenticator.isMfa) { - await verifyRecentPassword(user); - } + // TOTP is only ever a second factor after password login. + verifyRecentPassword(user); authenticator.lastUsedAt = new Date(); }