diff --git a/packages/bsky/tests/indexing.test.ts b/packages/bsky/tests/indexing.test.ts index cee3ed5a768..f5c8083df09 100644 --- a/packages/bsky/tests/indexing.test.ts +++ b/packages/bsky/tests/indexing.test.ts @@ -648,8 +648,9 @@ describe('indexing', () => { headers: sc.getHeaders(alice), }) const { token } = await network.pds.ctx.db.db - .selectFrom('delete_account_token') + .selectFrom('email_token') .selectAll() + .where('purpose', '=', 'delete_account') .where('did', '=', alice) .executeTakeFirstOrThrow() await pdsAgent.api.com.atproto.server.deleteAccount({ diff --git a/packages/pds/src/api/com/atproto/server/deleteAccount.ts b/packages/pds/src/api/com/atproto/server/deleteAccount.ts index 9ebfcfa7fdf..4d12edb1b32 100644 --- a/packages/pds/src/api/com/atproto/server/deleteAccount.ts +++ b/packages/pds/src/api/com/atproto/server/deleteAccount.ts @@ -2,7 +2,6 @@ import { AuthRequiredError } from '@atproto/xrpc-server' import { Server } from '../../../../lexicon' import { TAKEDOWN } from '../../../../lexicon/types/com/atproto/admin/defs' import AppContext from '../../../../context' -import Database from '../../../../db' import { MINUTE } from '@atproto/common' const REASON_ACCT_DELETION = 'ACCOUNT DELETION' @@ -22,40 +21,18 @@ export default function (server: Server, ctx: AppContext) { throw new AuthRequiredError('Invalid did or password') } - const tokenInfo = await ctx.db.db - .selectFrom('did_handle') - .innerJoin( - 'delete_account_token as token', - 'token.did', - 'did_handle.did', - ) - .where('did_handle.did', '=', did) - .where('token.token', '=', token.toUpperCase()) - .select([ - 'token.token as token', - 'token.requestedAt as requestedAt', - 'token.did as did', - ]) - .executeTakeFirst() - - if (!tokenInfo) { - return createInvalidTokenError() - } + await ctx.services + .account(ctx.db) + .assertValidToken(did, 'delete_account', token) const now = new Date() - const requestedAt = new Date(tokenInfo.requestedAt) - const expiresAt = new Date(requestedAt.getTime() + 15 * minsToMs) - if (now > expiresAt) { - await removeDeleteToken(ctx.db, tokenInfo.did) - return createExpiredTokenError() - } - await ctx.db.transaction(async (dbTxn) => { + const accountService = ctx.services.account(dbTxn) const moderationTxn = ctx.services.moderation(dbTxn) const [currentAction] = await moderationTxn.getCurrentActions({ did }) if (currentAction?.action === TAKEDOWN) { // Do not disturb an existing takedown, continue with account deletion - return await removeDeleteToken(dbTxn, did) + return await accountService.deleteEmailToken(did, 'delete_account') } if (currentAction) { // Reverse existing action to replace it with a self-takedown @@ -74,7 +51,7 @@ export default function (server: Server, ctx: AppContext) { createdAt: now, }) await moderationTxn.takedownRepo({ did, takedownId: takedown.id }) - await removeDeleteToken(dbTxn, did) + await accountService.deleteEmailToken(did, 'delete_account') }) ctx.backgroundQueue.add(async (db) => { @@ -90,34 +67,3 @@ export default function (server: Server, ctx: AppContext) { }, }) } - -type ErrorResponse = { - status: number - error: string - message: string -} - -const minsToMs = 60 * 1000 - -const createInvalidTokenError = (): ErrorResponse & { - error: 'InvalidToken' -} => ({ - status: 400, - error: 'InvalidToken', - message: 'Token is invalid', -}) - -const createExpiredTokenError = (): ErrorResponse & { - error: 'ExpiredToken' -} => ({ - status: 400, - error: 'ExpiredToken', - message: 'The password reset token has expired', -}) - -const removeDeleteToken = async (db: Database, did: string) => { - await db.db - .deleteFrom('delete_account_token') - .where('delete_account_token.did', '=', did) - .execute() -} diff --git a/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts b/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts index a448d97c02e..c438c32f69f 100644 --- a/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts +++ b/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts @@ -1,7 +1,6 @@ import { InvalidRequestError } from '@atproto/xrpc-server' import { Server } from '../../../../lexicon' import AppContext from '../../../../context' -import { getRandomToken } from './util' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.requestAccountDelete({ @@ -12,15 +11,9 @@ export default function (server: Server, ctx: AppContext) { if (!user) { throw new InvalidRequestError('user not found') } - const token = getRandomToken().toUpperCase() - const requestedAt = new Date().toISOString() - await ctx.db.db - .insertInto('delete_account_token') - .values({ did, token, requestedAt }) - .onConflict((oc) => - oc.column('did').doUpdateSet({ token, requestedAt }), - ) - .execute() + const token = await ctx.services + .account(ctx.db) + .createEmailToken(did, 'delete_account') await ctx.mailer.sendAccountDelete({ token }, { to: user.email }) }, }) diff --git a/packages/pds/src/api/com/atproto/server/requestPasswordReset.ts b/packages/pds/src/api/com/atproto/server/requestPasswordReset.ts index 5d81e43c68b..61b17ebb9a9 100644 --- a/packages/pds/src/api/com/atproto/server/requestPasswordReset.ts +++ b/packages/pds/src/api/com/atproto/server/requestPasswordReset.ts @@ -1,6 +1,5 @@ import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import { getRandomToken } from './util' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.requestPasswordReset(async ({ input }) => { @@ -9,16 +8,9 @@ export default function (server: Server, ctx: AppContext) { const user = await ctx.services.account(ctx.db).getAccountByEmail(email) if (user) { - const token = getRandomToken().toUpperCase() - const grantedAt = new Date().toISOString() - await ctx.db.db - .updateTable('user_account') - .where('did', '=', user.did) - .set({ - passwordResetToken: token, - passwordResetGrantedAt: grantedAt, - }) - .execute() + const token = await ctx.services + .account(ctx.db) + .createEmailToken(user.did, 'reset_password') await ctx.mailer.sendResetPassword( { handle: user.handle, token }, { to: user.email }, diff --git a/packages/pds/src/api/com/atproto/server/resetPassword.ts b/packages/pds/src/api/com/atproto/server/resetPassword.ts index de8d10382c0..a84b6249a3c 100644 --- a/packages/pds/src/api/com/atproto/server/resetPassword.ts +++ b/packages/pds/src/api/com/atproto/server/resetPassword.ts @@ -1,6 +1,5 @@ import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import Database from '../../../../db' import { MINUTE } from '@atproto/common' export default function (server: Server, ctx: AppContext) { @@ -14,69 +13,16 @@ export default function (server: Server, ctx: AppContext) { handler: async ({ input }) => { const { token, password } = input.body - const tokenInfo = await ctx.db.db - .selectFrom('user_account') - .select(['did', 'passwordResetGrantedAt']) - .where('passwordResetToken', '=', token.toUpperCase()) - .executeTakeFirst() - - if (!tokenInfo?.passwordResetGrantedAt) { - return createInvalidTokenError() - } - - const now = new Date() - const grantedAt = new Date(tokenInfo.passwordResetGrantedAt) - const expiresAt = new Date(grantedAt.getTime() + 15 * minsToMs) - - if (now > expiresAt) { - await unsetResetToken(ctx.db, tokenInfo.did) - return createExpiredTokenError() - } + const did = await ctx.services + .account(ctx.db) + .assertValidTokenAndFindDid('reset_password', token) await ctx.db.transaction(async (dbTxn) => { - await unsetResetToken(dbTxn, tokenInfo.did) - await ctx.services - .account(dbTxn) - .updateUserPassword(tokenInfo.did, password) - await await ctx.services - .auth(dbTxn) - .revokeRefreshTokensByDid(tokenInfo.did) + const accountService = ctx.services.account(ctx.db) + await accountService.updateUserPassword(did, password) + await accountService.deleteEmailToken(did, 'reset_password') + await ctx.services.auth(dbTxn).revokeRefreshTokensByDid(did) }) }, }) } - -type ErrorResponse = { - status: number - error: string - message: string -} - -const minsToMs = 60 * 1000 - -const createInvalidTokenError = (): ErrorResponse & { - error: 'InvalidToken' -} => ({ - status: 400, - error: 'InvalidToken', - message: 'Token is invalid', -}) - -const createExpiredTokenError = (): ErrorResponse & { - error: 'ExpiredToken' -} => ({ - status: 400, - error: 'ExpiredToken', - message: 'The password reset token has expired', -}) - -const unsetResetToken = async (db: Database, did: string) => { - await db.db - .updateTable('user_account') - .where('did', '=', did) - .set({ - passwordResetToken: null, - passwordResetGrantedAt: null, - }) - .execute() -} diff --git a/packages/pds/src/api/com/atproto/server/updateEmail.ts b/packages/pds/src/api/com/atproto/server/updateEmail.ts index e5b013d8eba..1873f5e0157 100644 --- a/packages/pds/src/api/com/atproto/server/updateEmail.ts +++ b/packages/pds/src/api/com/atproto/server/updateEmail.ts @@ -1,6 +1,7 @@ import { Server } from '../../../../lexicon' import AppContext from '../../../../context' import { InvalidRequestError } from '@atproto/xrpc-server' +import disposable from 'disposable-email' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.updateEmail({ @@ -8,22 +9,30 @@ export default function (server: Server, ctx: AppContext) { handler: async ({ auth, input }) => { const did = auth.credentials.did const { token, email } = input.body + if (!disposable.validate(email)) { + throw new InvalidRequestError( + 'This email address is not supported, please use a different email.', + ) + } const user = await ctx.services.account(ctx.db).getAccount(did) if (!user) { throw new InvalidRequestError('user not found') } + if (!user.emailConfirmedAt) { + throw new InvalidRequestError('email must be confirmed (temporary)') + } // require valid token - // @TODO re-enable updating non-verified emails - // if (user.emailConfirmedAt) { - if (!token) { - throw new InvalidRequestError( - 'confirmation token required', - 'TokenRequired', - ) + if (user.emailConfirmedAt) { + if (!token) { + throw new InvalidRequestError( + 'confirmation token required', + 'TokenRequired', + ) + } + await ctx.services + .account(ctx.db) + .assertValidToken(did, 'update_email', token) } - await ctx.services - .account(ctx.db) - .assertValidToken(did, 'update_email', token) await ctx.db.transaction(async (dbTxn) => { const accntSrvce = ctx.services.account(dbTxn) diff --git a/packages/pds/src/db/database-schema.ts b/packages/pds/src/db/database-schema.ts index ee92742edff..a3fe7ad8799 100644 --- a/packages/pds/src/db/database-schema.ts +++ b/packages/pds/src/db/database-schema.ts @@ -14,7 +14,6 @@ import * as inviteCode from './tables/invite-code' import * as notification from './tables/user-notification' import * as blob from './tables/blob' import * as repoBlob from './tables/repo-blob' -import * as deleteAccountToken from './tables/delete-account-token' import * as emailToken from './tables/email-token' import * as moderation from './tables/moderation' import * as mute from './tables/mute' @@ -40,7 +39,6 @@ export type DatabaseSchemaType = runtimeFlag.PartialDB & notification.PartialDB & blob.PartialDB & repoBlob.PartialDB & - deleteAccountToken.PartialDB & emailToken.PartialDB & moderation.PartialDB & mute.PartialDB & diff --git a/packages/pds/src/db/migrations/20230926T195532354Z-email-tokens.ts b/packages/pds/src/db/migrations/20230926T195532354Z-email-tokens.ts index ce8e6574731..4200e64477b 100644 --- a/packages/pds/src/db/migrations/20230926T195532354Z-email-tokens.ts +++ b/packages/pds/src/db/migrations/20230926T195532354Z-email-tokens.ts @@ -10,7 +10,10 @@ export async function up(db: Kysely, dialect: Dialect): Promise { .addColumn('token', 'varchar', (col) => col.notNull()) .addColumn('requestedAt', timestamp, (col) => col.notNull()) .addPrimaryKeyConstraint('email_token_pkey', ['purpose', 'did']) - .addUniqueConstraint('email_token_token_unique', ['purpose', 'token']) + .addUniqueConstraint('email_token_purpose_token_unique', [ + 'purpose', + 'token', + ]) .execute() await db.schema diff --git a/packages/pds/src/db/migrations/20230927T212334019Z-simplify-email-tokens.ts b/packages/pds/src/db/migrations/20230927T212334019Z-simplify-email-tokens.ts new file mode 100644 index 00000000000..3724de1cf56 --- /dev/null +++ b/packages/pds/src/db/migrations/20230927T212334019Z-simplify-email-tokens.ts @@ -0,0 +1,34 @@ +import { Kysely } from 'kysely' + +export async function up(db: Kysely): Promise { + await db.schema + .alterTable('user_account') + .dropColumn('passwordResetToken') + .execute() + + await db.schema + .alterTable('user_account') + .dropColumn('passwordResetGrantedAt') + .execute() + + await db.schema.dropTable('delete_account_token').execute() +} + +export async function down(db: Kysely): Promise { + await db.schema + .alterTable('user_account') + .addColumn('passwordResetToken', 'varchar') + .execute() + + await db.schema + .alterTable('user_account') + .addColumn('passwordResetGrantedAt', 'varchar') + .execute() + + await db.schema + .createTable('delete_account_token') + .addColumn('did', 'varchar', (col) => col.primaryKey()) + .addColumn('token', 'varchar', (col) => col.notNull()) + .addColumn('requestedAt', 'varchar', (col) => col.notNull()) + .execute() +} diff --git a/packages/pds/src/db/migrations/index.ts b/packages/pds/src/db/migrations/index.ts index 3636d304e46..e177084ff86 100644 --- a/packages/pds/src/db/migrations/index.ts +++ b/packages/pds/src/db/migrations/index.ts @@ -67,3 +67,4 @@ export * as _20230825T142507884Z from './20230825T142507884Z-blob-tempkey-idx' export * as _20230828T153013575Z from './20230828T153013575Z-repo-history-rewrite' export * as _20230922T033938477Z from './20230922T033938477Z-remove-appview' export * as _20230926T195532354Z from './20230926T195532354Z-email-tokens' +export * as _20230927T212334019Z from './20230927T212334019Z-simplify-email-tokens' diff --git a/packages/pds/src/db/tables/delete-account-token.ts b/packages/pds/src/db/tables/delete-account-token.ts deleted file mode 100644 index da748c639a7..00000000000 --- a/packages/pds/src/db/tables/delete-account-token.ts +++ /dev/null @@ -1,9 +0,0 @@ -export interface DeleteAccountToken { - did: string - token: string - requestedAt: string -} - -export const tableName = 'delete_account_token' - -export type PartialDB = { [tableName]: DeleteAccountToken } diff --git a/packages/pds/src/db/tables/user-account.ts b/packages/pds/src/db/tables/user-account.ts index ef9fdbecb3c..808663ca468 100644 --- a/packages/pds/src/db/tables/user-account.ts +++ b/packages/pds/src/db/tables/user-account.ts @@ -6,8 +6,6 @@ export interface UserAccount { passwordScrypt: string createdAt: string emailConfirmedAt: string | null - passwordResetToken: string | null - passwordResetGrantedAt: string | null invitesDisabled: Generated<0 | 1> inviteNote: string | null } diff --git a/packages/pds/src/services/account/index.ts b/packages/pds/src/services/account/index.ts index 33978be9b8b..04561737144 100644 --- a/packages/pds/src/services/account/index.ts +++ b/packages/pds/src/services/account/index.ts @@ -564,7 +564,7 @@ export class AccountService { .selectAll() .where('purpose', '=', purpose) .where('did', '=', did) - .where('token', '=', token) + .where('token', '=', token.toUpperCase()) .executeTakeFirst() if (!res) { throw new InvalidRequestError('Token is invalid', 'InvalidToken') @@ -575,6 +575,27 @@ export class AccountService { } } + async assertValidTokenAndFindDid( + purpose: EmailTokenPurpose, + token: string, + expirationLen = 15 * MINUTE, + ): Promise { + const res = await this.db.db + .selectFrom('email_token') + .selectAll() + .where('purpose', '=', purpose) + .where('token', '=', token.toUpperCase()) + .executeTakeFirst() + if (!res) { + throw new InvalidRequestError('Token is invalid', 'InvalidToken') + } + const expired = !lessThanAgoMs(res.requestedAt, expirationLen) + if (expired) { + throw new InvalidRequestError('Token is expired', 'ExpiredToken') + } + return res.did + } + async getLastSeenNotifs(did: string): Promise { const res = await this.db.db .selectFrom('user_state') diff --git a/packages/pds/tests/account.test.ts b/packages/pds/tests/account.test.ts index 26328966312..d2ca47e7a83 100644 --- a/packages/pds/tests/account.test.ts +++ b/packages/pds/tests/account.test.ts @@ -528,24 +528,23 @@ describe('account', () => { it('allows only unexpired password reset tokens', async () => { await agent.api.com.atproto.server.requestPasswordReset({ email }) - const user = await db.db - .updateTable('user_account') - .where('email', '=', email) + const res = await db.db + .updateTable('email_token') + .where('purpose', '=', 'reset_password') + .where('did', '=', did) .set({ - passwordResetGrantedAt: new Date( - Date.now() - 16 * minsToMs, - ).toISOString(), + requestedAt: new Date(Date.now() - 16 * minsToMs), }) - .returning(['passwordResetToken']) + .returning(['token']) .executeTakeFirst() - if (!user?.passwordResetToken) { + if (!res?.token) { throw new Error('Missing reset token') } // Use of expired token fails await expect( agent.api.com.atproto.server.resetPassword({ - token: user.passwordResetToken, + token: res.token, password: passwordAlt, }), ).rejects.toThrow(ComAtprotoServerResetPassword.ExpiredTokenError) diff --git a/packages/pds/tests/email-confirmation.test.ts b/packages/pds/tests/email-confirmation.test.ts index 48a0c375510..fc3c4caadcd 100644 --- a/packages/pds/tests/email-confirmation.test.ts +++ b/packages/pds/tests/email-confirmation.test.ts @@ -61,7 +61,7 @@ describe('email confirmation', () => { expect(session.data.emailConfirmed).toEqual(false) }) - it('disallows email update without token when unverified', async () => { + it('disallows email update when unverified', async () => { const res = await agent.api.com.atproto.server.requestEmailUpdate( undefined, { headers: sc.getHeaders(alice.did) }, @@ -75,22 +75,36 @@ describe('email confirmation', () => { { headers: sc.getHeaders(alice.did), encoding: 'application/json' }, ) await expect(attempt).rejects.toThrow() - - // await agent.api.com.atproto.server.updateEmail( - // { - // email: 'new-alice@example.com', - // }, - // { headers: sc.getHeaders(alice.did), encoding: 'application/json' }, - // ) - // const session = await agent.api.com.atproto.server.getSession( - // {}, - // { headers: sc.getHeaders(alice.did) }, - // ) - // expect(session.data.email).toEqual('new-alice@example.com') - // expect(session.data.emailConfirmed).toEqual(false) - // alice.email = session.data.email + const session = await agent.api.com.atproto.server.getSession( + {}, + { headers: sc.getHeaders(alice.did) }, + ) + expect(session.data.email).toEqual(alice.email) + expect(session.data.emailConfirmed).toEqual(false) }) + // it('allows email update without token when unverified', async () => { + // const res = await agent.api.com.atproto.server.requestEmailUpdate( + // undefined, + // { headers: sc.getHeaders(alice.did) }, + // ) + // expect(res.data.tokenRequired).toBe(false) + + // await agent.api.com.atproto.server.updateEmail( + // { + // email: 'new-alice@example.com', + // }, + // { headers: sc.getHeaders(alice.did), encoding: 'application/json' }, + // ) + // const session = await agent.api.com.atproto.server.getSession( + // {}, + // { headers: sc.getHeaders(alice.did) }, + // ) + // expect(session.data.email).toEqual('new-alice@example.com') + // expect(session.data.emailConfirmed).toEqual(false) + // alice.email = session.data.email + // }) + let confirmToken it('requests email confirmation', async () => { @@ -190,6 +204,19 @@ describe('email confirmation', () => { ) }) + it('fails email update with a badly formatted email', async () => { + const attempt = agent.api.com.atproto.server.updateEmail( + { + email: 'bad-email@disposeamail.com', + token: updateToken, + }, + { headers: sc.getHeaders(alice.did), encoding: 'application/json' }, + ) + await expect(attempt).rejects.toThrow( + 'This email address is not supported, please use a different email.', + ) + }) + it('updates email', async () => { await agent.api.com.atproto.server.updateEmail( { diff --git a/packages/pds/tests/moderation.test.ts b/packages/pds/tests/moderation.test.ts index edbb23c6578..80536a6933f 100644 --- a/packages/pds/tests/moderation.test.ts +++ b/packages/pds/tests/moderation.test.ts @@ -286,7 +286,8 @@ describe('moderation', () => { headers: sc.getHeaders(deleteme.did), }) const { token: deletionToken } = await server.ctx.db.db - .selectFrom('delete_account_token') + .selectFrom('email_token') + .where('purpose', '=', 'delete_account') .where('did', '=', deleteme.did) .selectAll() .executeTakeFirstOrThrow()