diff --git a/CHANGELOG.md b/CHANGELOG.md index d7908ec31..4b62583a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ version, see the upgrade guides in `docs/`. - **server:** Dito.js handles logins and sessions itself instead of through koa-passport, which `@ditojs/server` no longer depends on. `ctx.req.user` and custom Passport strategies are gone, `app.passport` has no effect, and `UserMixin.login()` reads the credentials only from the request body. Existing sessions stay valid. - **server:** Changing a user's password ends their other sessions. +- **server:** The `patch` and `put` actions of `UsersController` no longer accept passwords, which only change through `'post password'`: apps that change passwords in a form through `PATCH /users/self` use the admin's Security dialog or `'post password'` instead. Admins can't set the passwords of other users through them any more, only password resets can. +- **server:** The actions of `UsersController` no longer accept the `hash` of the password, and changing the username through `patch` and `put` needs confirmed access. - **server:** `UsersController` only treats users of its own model class as logged in, not users of its sub-classes. - **server:** `Controller.setup()` and `UserMixin.sessionQuery()` are internal: apps override `initialize()` and set `sessionScope` instead. - **admin:** Dialogs validate their data on submit, like forms, and only resolve once it's valid. @@ -20,6 +22,7 @@ version, see the upgrade guides in `docs/`. - **server, admin:** Two-factor authentication with TOTP, with `app.auth.totp` and `login: { totp: true }`, see [Two-factor authentication](docs/guide/server/controllers.md#two-factor-authentication). - **server, admin:** Password resets by email, with `app.auth.passwordReset` and `login: { passwordReset: true }`, see [Password reset](docs/guide/server/controllers.md#password-reset). - **server, admin:** Users change their password through `'post password'` of `UsersController`, and the Security dialog of the account menu. +- **server, admin:** Security changes need confirmed access, from a recent login or `'post confirm-access'` within `app.auth.reconfirmAccessAfter`. Turning off TOTP takes it instead of a code. See [Confirmed access](docs/guide/server/controllers.md#confirmed-access). - **server:** Authorize functions are called with their controller as `this`, in relations the `RelationController`. - **server:** `CredentialMixin` and `CredentialModel` store the credentials of all user models in one table, set in `app.auth.credentials`. - **admin:** `settings.minDuration` keeps dialogs visible for a minimum time. diff --git a/docs/api/admin/admin.md b/docs/api/admin/admin.md index 26bebf557..d9d78dcd4 100644 --- a/docs/api/admin/admin.md +++ b/docs/api/admin/admin.md @@ -45,8 +45,8 @@ new DitoAdmin(element, options) | `options.dito` | [`GlobalConfig`](#globalconfig) | The global configuration. Defaults to `globalThis.dito`, which `AdminController` provides. | | `options.login.additionalComponents` | `Components` | Components added to the login dialog after `username` and `password`. Their values are sent with the credentials. | | `options.login.redirectAfterLogin` | `string` | A URL to load after a successful login, instead of resolving the views in place. | -| `options.login.passkeys` | `boolean` | Adds a Sign In with Passkey button to the login dialog, and a Security item to the account menu, whose dialog changes the user's password through [`api.users.password`](#users), and lists, adds and removes the user's passkeys, named after their providers. Needs passkeys on the server, see [Passkeys](../../guide/server/controllers.md#passkeys), and the peer dependency `@simplewebauthn/browser`. | -| `options.login.totp` | `boolean` | Adds a Security item to the account menu, whose dialog changes the user's password, sets up two-factor authentication with TOTP, with a QR code for the authenticator app and the backup codes, and turns it off again. Needs TOTP on the server, see [Two-factor authentication](../../guide/server/controllers.md#two-factor-authentication), and the peer dependency `lean-qr`. The login dialog asks for the code whenever the server requires one. | +| `options.login.passkeys` | `boolean` | Adds a Sign In with Passkey button to the login dialog, and lists, adds and removes the user's passkeys, named after their providers, in the Security dialog of the account menu, which always changes the user's password through [`api.users.password`](#users). Needs passkeys on the server, see [Passkeys](../../guide/server/controllers.md#passkeys), and the peer dependency `@simplewebauthn/browser`. | +| `options.login.totp` | `boolean` | Sets up two-factor authentication with TOTP in the Security dialog of the account menu, with a QR code for the authenticator app and the backup codes, and turns it off again. Needs TOTP on the server, see [Two-factor authentication](../../guide/server/controllers.md#two-factor-authentication), and the peer dependency `lean-qr`. The login dialog asks for the code whenever the server requires one. | | `options.login.passwordReset` | `boolean` | Adds a Forgot password? button to the login dialog, which asks for the username and posts it to [`api.users.passwordReset`](#users), for the server to email a link. The link opens the admin with `#password-reset=` and the token, which asks for the new password, posts it to `api.users.passwordResetConfirm`, asking again with the server's validation errors, and removes the token from the URL, before asking to log in. Needs password resets on the server, see [Password reset](../../guide/server/controllers.md#password-reset). | | `options.spinner` | `{ size?: string, color?: string } \| null` | Size and color of the loading spinner in the header. `null` hides it. | | `options.account.settings` | `Form \| Promise \| () => …` | The form of the account settings, which the account menu opens at `/account/settings`, to edit the logged-in user through [`api.users.self`](#users). Also a promise or a function returning it, e.g. a dynamic `import()`. Its `label` is the view's label, default `'Settings'`. See [Account settings](../../guide/admin/views.md#account-settings). | @@ -160,23 +160,26 @@ api: { passkeys: { path: 'passkeys' }, loginCode: { path: 'login-code', method: 'post' }, totp: { path: 'totp' }, - totpConfirm: { path: 'totp-confirm', method: 'post' } + totpConfirm: { path: 'totp-confirm', method: 'post' }, + confirmAccess: { path: 'confirm-access', method: 'post' } } } ``` `users.path` is the collection of the user model; `users: 'users'` is short for `users: { path: 'users' }`. `login`, `logout`, `session`, `self`, -`password`, `loginCode`, the password reset resources of `login.passwordReset`, -the passkey resources of `login.passkeys` and the TOTP resources of -`login.totp` are resources nested under it, shown here with +`password`, `loginCode`, `confirmAccess`, the password reset resources of +`login.passwordReset`, the passkey resources of `login.passkeys` and the TOTP +resources of `login.totp` are resources nested under it, shown here with their defaults, so the admin logs in through `POST users/login`. The server's `UsersController` provides them. The admin reads `user` from the responses of `login` and `session`, and `success` from the response of `logout`. The [account settings](#parameters) load the logged-in user from `self` and patch -it there, after which the admin fetches the session's user again. See [AdminContext](#session) for -the session that uses them. +it there, after which the admin fetches the session's user again. Requests +that answer `confirmationRequired` ask for the password, the TOTP code or a +passkey, post them to `confirmAccess` and are sent again. See +[AdminContext](#session) for the session that uses them. ### defaults diff --git a/docs/api/server/application.md b/docs/api/server/application.md index 77f589eed..c6054a2ed 100644 --- a/docs/api/server/application.md +++ b/docs/api/server/application.md @@ -144,6 +144,7 @@ app: { | `passwordReset` | `object` | Enables password resets by email in [`UsersController`](controllers.md#userscontroller), which sends the emails through [`sendPasswordReset()`](controllers.md#sendpasswordreset). Needs `credentials`, `credentials.secret` and a `session` stored on the server, else the setup fails. | | `passwordReset.confirmUrl` | `string` | The URL of the page that the emails link to, with a `{token}` placeholder for the token, best in the fragment, e.g. `'https://example.com/admin/#password-reset={token}'` for the admin's [`login.passwordReset`](../admin/admin.md). One URL for the whole app: all users controllers send links to it. | | `passwordReset.expires` | `number \| string` | How long the links are valid, in milliseconds or as a duration string, e.g. `'30m'`. Default `'1h'`. | +| `reconfirmAccessAfter` | `number \| string` | How long logins and `'post confirm-access'` confirm the access for the security changes of [`UsersController`](controllers.md#userscontroller), in milliseconds or as a duration string. Default `'10m'`. | Passkeys need the optional peer dependency `@simplewebauthn/server`, TOTP needs `otpauth`. diff --git a/docs/api/server/controllers.md b/docs/api/server/controllers.md index 993b8e7af..b64542d6d 100644 --- a/docs/api/server/controllers.md +++ b/docs/api/server/controllers.md @@ -804,7 +804,21 @@ inherited actions, see [`allow`](#allow): a subclass that sets | `'post logout'` | `{ success, authenticated }` | | `'get session'` | `{ authenticated, user }` | | `'get self'` | The authenticated user through `member.get`, or `null`. | -| `'post password'` | Changes the password of the authenticated user to `password` in the body, after verifying `currentPassword`: `{ success }`, or 400 if `currentPassword` is wrong. The new password is validated by the user model, e.g. its `minLength`. | +| `'post password'` | Changes the password of the authenticated user to `password` in the body, after verifying `currentPassword`: `{ success }`, or 400 if `currentPassword` is wrong. The new password is validated by the user model, e.g. its `minLength`. Needs confirmed access. | +| `'post confirm-access'` | Confirms the access of the authenticated user with `{ password }`, plus `code` for users with TOTP, or with `{ passkey }`, the response of `startAuthentication()` for the options of `'post passkey-login-options'`: `{ success }`, `{ success: false, codeRequired: true }` without a code, or 401 with `'Password is incorrect'`, `'Code is incorrect'` or `'Passkey is incorrect'`. | + +Logins and `'post confirm-access'` confirm the access of the session for +[`app.auth.reconfirmAccessAfter`](./application.md#configappauth). Without +it, `'post password'`, `'post passkey-options'`, `'post passkeys'`, +`'delete passkeys/:id'`, `'post totp'`, `'post totp-confirm'` and +`'delete totp'` answer 403 with +`{ message: 'Confirmation required', confirmationRequired: true }`, see +[Confirmed access](../../guide/server/controllers.md#confirmed-access), and so +do `patch` and `put` actions that change the username. +`hasConfirmedAccess(ctx)` does the same for the `authorize` of other actions. +The `patch`, `put` and `post` actions answer 400 for bodies with the `hash`, +and `patch` and `put` also for bodies with the password. With `relate`, `post` +treats users with ids like updates. With [`app.auth.passkeys`](./application.md#configappauth), users log in with passkeys and manage their own ones through these collection actions, which @@ -838,7 +852,7 @@ collection actions, which aren't routed without it: | `'get totp'` | `{ enabled, backupCodes }`, with the number of the remaining backup codes. | | `'post totp'` | A new secret, kept in the session until it is confirmed: `{ uri, secret }`, the `otpauth://` URI for the QR code of authenticator apps, and the secret in base32 to enter by hand. 409 if TOTP is already set up. | | `'post totp-confirm'` | Stores the secret with `{ code }`, a code of the new secret, and creates 10 backup codes: `{ backupCodes }`, only shown in this response. | -| `'delete totp'` | Turns TOTP off with `{ code }`, a TOTP code or a backup code, and deletes the backup codes: `{ success }`. | +| `'delete totp'` | Turns TOTP off and deletes the backup codes: `{ success }`. Needs confirmed access. | For users with TOTP, `'post login'` with the right password doesn't log in, but responds with status 200 and `{ success: false, codeRequired: true, @@ -846,7 +860,7 @@ authenticated: false, user: null }`, and keeps a pending login in the session, see [`login()`](./model.md#static-login), with a new session id. `'post login-code'` completes it within 5 minutes. Wrong codes answer `'Code is incorrect'` with status 401, also in `'post totp-confirm'` and -`'delete totp'`, and missing or expired pending logins and setups status 400. +`'post confirm-access'`, and missing or expired pending logins and setups status 400. Ten wrong codes in a row lock the account for 15 minutes, across pending logins and sessions: the tenth and all codes during the lock answer `'Too many attempts'` with status 400. Ten wrong codes in `'post totp-confirm'` @@ -870,20 +884,18 @@ user exists or not, and doesn't wait for the email, but the time it takes can still differ, as it stores a token for existing users. Users get at most one email a minute. Limit the rate of requests per client in the proxy in front of the app. Each link works once, until it expires, and setting the password, also -through `'post password'` or a member `patch` or `update`, invalidates the -user's other links. Only users of the controller's `modelClass` are found, -through their `sessionScope`. A subclass that sets `collection.allow` has to -list the password reset actions too, else they aren't routed, and only -controllers that route `'post password-reset'` need to override -`sendPasswordReset()`. All users controllers of the app share the one -`confirmUrl` of `app.auth.passwordReset`. +through `'post password'`, invalidates the user's other links. Only users of +the controller's `modelClass` are found, through their `sessionScope`. A +subclass that sets `collection.allow` has to list the password reset actions +too, else they aren't routed, and only controllers that route +`'post password-reset'` need to override `sendPasswordReset()`. All users +controllers of the app share the one `confirmUrl` of `app.auth.passwordReset`. Any change of a user's `hash` ends the sessions of the user. The session of the -request that changes the password stays logged in after `'post password'`, -`'post password-reset-confirm'` and the member `patch` and `update` of -`UsersController`, e.g. `PATCH /users/self`. Elsewhere, e.g. in other -controllers, custom actions or collection patches, it ends too, unless the -action logs the user in again with `ctx.login(user)` afterwards. +request that changes the password stays logged in after `'post password'` and +`'post password-reset-confirm'`. Elsewhere, e.g. in other controllers or custom +actions, it ends too, unless the action logs the user in again with +`ctx.login(user)` afterwards. TOTP is only asked for by [`UserMixin.login()`](./model.md#static-login), and thus by `'post login'`. `ctx.login(user)` logs in without it, by design, e.g. diff --git a/docs/guide/admin/views.md b/docs/guide/admin/views.md index dbac06766..fa62492c5 100644 --- a/docs/guide/admin/views.md +++ b/docs/guide/admin/views.md @@ -172,16 +172,16 @@ export default { type: 'email', label: 'Email', required: true - }, - // Only sent when a new password is entered: - password: { - type: 'password', - label: 'New Password' } } } ``` +Passwords don't change through `PATCH /api/users/self`, but through +`'post password'` of the `UsersController`, which the Security dialog of the +account menu uses, see +[Confirmed access](../server/controllers.md#confirmed-access). + On the server, `PATCH /users/self` goes through the `patch` member action of the `UsersController`, so the users need to be allowed to patch themselves: `member = { allow: ['get', 'patch'] }`, which only allows users to patch diff --git a/docs/guide/server/controllers.md b/docs/guide/server/controllers.md index 3a19362da..582e3de25 100644 --- a/docs/guide/server/controllers.md +++ b/docs/guide/server/controllers.md @@ -581,8 +581,9 @@ export class Users extends UsersController { - `GET /users/self` returns the logged-in user. - In member routes, `self` stands for the logged-in user, e.g. `PATCH /users/self` runs the member's `patch` action on them, if it is - allowed. The admin's [account settings](../admin/views.md#account-settings) - use it. + allowed, but doesn't change passwords, see + [Confirmed access](#confirmed-access). The admin's + [account settings](../admin/views.md#account-settings) use it. `authorize` on the controller doesn't apply to login, logout and session, which authenticate, so users can always log in; `self` follows it. Member @@ -710,7 +711,8 @@ const { user } = response Ten wrong codes in a row lock the account for 15 minutes. `GET /users/totp` tells whether TOTP is set up and how many backup codes are left, and -`DELETE /users/totp` with `{ code }` turns it off. Only password logins through +`DELETE /users/totp` turns it off, with +[confirmed access](#confirmed-access). Only password logins through `login()` ask for the code, while `ctx.login(user)` logs in without it, e.g. to impersonate users. The admin's login dialog asks for the code, and its `login.totp` option sets up and turns off @@ -778,6 +780,86 @@ the request to its login dialog, with `confirmUrl` pointing to the admin, e.g. `'https://example.com/admin/#password-reset={token}'`, see [`DitoAdmin`](../../api/admin/admin.md#parameters). +### Confirmed access + +Security changes need confirmed access, so that a session alone, e.g. a stolen +one, can't change how the account logs in: changing the password through +`'post password'` or the username, adding and removing passkeys, and turning +TOTP on and off. Logging in confirms the access, also with a passkey or with +the TOTP code, for the time of `reconfirmAccessAfter`: + +```js +// config/index.js +export default { + app: { + auth: { + reconfirmAccessAfter: '10m' // The default + } + } +} +``` + +Later, these actions answer status 403 with `confirmationRequired`, and the +user confirms the access again with their password, plus the code of their +authenticator app or a backup code if they use TOTP, or with a passkey: + +```js +let response = await post('confirm-access', { password }) +if (response.codeRequired) { + response = await post('confirm-access', { password, code: '123456' }) +} +// Or with a passkey, through the options of passkey logins: +const optionsJSON = await post('passkey-login-options') +await post('confirm-access', { + passkey: await startAuthentication({ optionsJSON }) +}) +``` + +Wrong codes count towards the lock of the account, as in logins. Passwords +only change through `'post password'`: the `patch` and `put` actions of users +controllers answer 400 for bodies with a password or its `hash`, while `post` +still creates users with a password. Changing the username through them needs +confirmed access, as password resets go to it. The admin asks to confirm the +access whenever an action answers `confirmationRequired`, and then repeats the +action. + +Protect other changes the same way, e.g. the `email` that the app sends the +password resets to, or custom actions with +`authorize: { 'post export': ctx => this.hasConfirmedAccess(ctx) }`: + +```js +export class Users extends UsersController { + modelClass = User + + member = { + allow: ['get', 'patch'], + + async patch(ctx, modify) { + const { email } = ctx.request.body + if (email !== undefined) { + // Compare with the user that is patched, not the logged-in one: + const user = await this.modelClass.query().findById(ctx.memberId) + // `hasConfirmedAccess()` throws 403 with `confirmationRequired` without + // confirmed access, and returns `false` for users of other models: + if (email !== user?.email && !this.hasConfirmedAccess(ctx)) { + throw new AuthorizationError() + } + } + return super.patch(ctx, modify) + } + } +} +``` + +Passwords set through other routes of the app aren't covered: relations and +graphs that include user models, other controllers, and new users created +through an authorized collection `post`. Keep such routes to admins. Deleting +users through the member `delete`, e.g. `DELETE /users/self`, doesn't need +confirmed access either: apps that allow it can protect it with +`authorize: { delete: ctx => this.hasConfirmedAccess(ctx) && '$self' }` in +`member`, keeping `'$self'`, which the setting replaces, so that users only +delete themselves. + ## Admin The `AdminController` serves the admin interface at its path. In development diff --git a/packages/admin/src/DitoAdmin.js b/packages/admin/src/DitoAdmin.js index 247595367..a148944c7 100644 --- a/packages/admin/src/DitoAdmin.js +++ b/packages/admin/src/DitoAdmin.js @@ -66,7 +66,8 @@ export default class DitoAdmin { passkeyLogin: 'post', passkeyOptions: 'post', loginCode: 'post', - totpConfirm: 'post' + totpConfirm: 'post', + confirmAccess: 'post' } for (const [name, method] of Object.entries(methods)) { users[name] = getResource(users[name] || hyphenate(name), { diff --git a/packages/admin/src/components/DitoAccount.test.js b/packages/admin/src/components/DitoAccount.test.js index 48df8986e..490296407 100644 --- a/packages/admin/src/components/DitoAccount.test.js +++ b/packages/admin/src/components/DitoAccount.test.js @@ -26,8 +26,8 @@ describe('DitoAccount', () => { const logout = vi.spyOn(session, 'logout').mockResolvedValue() const { button, items } = await openMenu(admin) expect(button.text()).toBe('tester') - expect(items.map(item => item.text())).toEqual(['Sign Out']) - await items[0].trigger('click') + expect(items.map(item => item.text())).toEqual(['Security', 'Sign Out']) + await items[1].trigger('click') expect(logout).toHaveBeenCalledOnce() expect(button.attributes('aria-expanded')).toBe('false') }) @@ -78,7 +78,11 @@ describe('DitoAccount', () => { it('opens the settings of the user from the menu', async () => { const { admin, request } = await mountWithSettings() const { items } = await openMenu(admin) - expect(items.map(item => item.text())).toEqual(['Settings', 'Sign Out']) + expect(items.map(item => item.text())).toEqual([ + 'Settings', + 'Security', + 'Sign Out' + ]) await items[0].trigger('click') await flushPromises() expect(admin.router.currentRoute.value.path).toBe('/account/settings') @@ -119,8 +123,10 @@ describe('DitoAccount', () => { describe('security', () => { // A server with passkeys, which adds and deletes them, and changes the - // password 'secret': + // password 'secret'. Without `isConfirmed`, the changes need the access + // confirmed with the password first. `login` sets the admin's options: const mountWithPasskeys = async ({ + login = { passkeys: true }, passkeys = [ { id: 1, @@ -128,10 +134,33 @@ describe('DitoAccount', () => { createdAt: '2026-01-02T12:00:00Z', lastUsedAt: null } - ] + ], + isConfirmed = true } = {}) => { const request = vi.fn(({ method = 'get', url, data }) => { - if (url === '/users/session') { + const isChange = method !== 'get' && url !== '/users/confirm-access' + if (isChange && !isConfirmed) { + throw Object.assign(new Error('Request failed'), { + response: { + status: 403, + data: { + message: 'Confirmation required', + confirmationRequired: true + } + } + }) + } else if (url === '/users/confirm-access') { + isConfirmed = data.password === 'secret' + if (!isConfirmed) { + throw Object.assign(new Error('Request failed'), { + response: { + status: 401, + data: { message: 'Password is incorrect' } + } + }) + } + return { data: { success: true } } + } else if (url === '/users/session') { return { data: { user: { id: 1, username: 'tester' } } } } else if (url === '/users/passkeys' && method === 'get') { return { data: passkeys } @@ -168,7 +197,7 @@ describe('DitoAccount', () => { views, api: { users: { path: 'users' } }, request, - options: { login: { passkeys: true } } + options: { login } }) const { items } = await openMenu(admin) expect(items.map(item => item.text())).toEqual(['Security', 'Sign Out']) @@ -266,6 +295,109 @@ describe('DitoAccount', () => { ) }) + const confirmAccess = async (admin, password) => { + const dialog = getLastDialog(admin) + expect(dialog.attributes('aria-label')).toBe('Confirm Access') + await enterValue(dialog.find('input[name="password"]'), password) + await dialog.find('button[type="submit"]').trigger('click') + await flushPromises() + } + + describe('confirmed access', () => { + it('confirms the access, and then adds the passkey', async () => { + const { admin, request, getRows } = await mountWithPasskeys({ + isConfirmed: false + }) + await findButton(admin, 'Add Passkey').trigger('click') + await flushPromises() + await confirmAccess(admin, 'secret') + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'post', + url: '/users/confirm-access', + data: { password: 'secret' } + }) + ) + expect( + request.mock.calls.filter( + ([{ url }]) => url === '/users/passkey-options' + ) + ).toHaveLength(2) + expect(getRows()).toEqual([ + '1PasswordJanuary 2, 2026Never', + 'PasskeyJanuary 3, 2026Never' + ]) + }) + + it('asks again after a wrong password', async () => { + const { admin, getRows } = await mountWithPasskeys({ + isConfirmed: false + }) + await findButton(admin, 'Add Passkey').trigger('click') + await flushPromises() + await confirmAccess(admin, 'wrong') + expect(admin.wrapper.text()).toContain('Password is incorrect') + await confirmAccess(admin, 'secret') + expect(getRows()).toHaveLength(2) + }) + + it('confirms the access, and then deletes the passkey', async () => { + const { admin, request, getRows } = await mountWithPasskeys({ + isConfirmed: false + }) + await admin.wrapper + .find('.dito-dialog .dito-list .dito-button--delete') + .trigger('click') + await flushPromises() + await findButton(admin, 'Delete').trigger('click') + await flushPromises() + await confirmAccess(admin, 'secret') + expect( + request.mock.calls.filter( + ([{ method, url }]) => ( + method === 'delete' && url === '/users/passkeys/1' + ) + ) + ).toHaveLength(2) + expect(getRows()).toEqual(['No passkeys yet.']) + }) + + it('stops quietly when the confirmation is cancelled', async () => { + const { admin, request, getRows } = await mountWithPasskeys({ + isConfirmed: false + }) + const notify = vi.spyOn(admin.context, 'notify') + const cancel = async () => { + await flushPromises() + const dialog = getLastDialog(admin) + expect(dialog.attributes('aria-label')).toBe('Confirm Access') + await findButton(admin, 'Cancel').trigger('click') + await flushPromises() + } + await findButton(admin, 'Add Passkey').trigger('click') + await cancel() + // A button without its own error handling: + await findButton(admin, 'Change').trigger('click') + await flushPromises() + const dialog = getLastDialog(admin) + for (const [name, value] of Object.entries({ + currentPassword: 'secret', + password: 'changed', + confirmation: 'changed' + })) { + await enterValue(dialog.find(`input[name="${name}"]`), value) + } + await dialog.find('button[type="submit"]').trigger('click') + await cancel() + expect(notify).not.toHaveBeenCalled() + expect(startRegistration).not.toHaveBeenCalled() + expect(request).not.toHaveBeenCalledWith( + expect.objectContaining({ url: '/users/confirm-access' }) + ) + expect(getRows()).toHaveLength(1) + }) + }) + describe('password', () => { const changePassword = async (admin, values) => { await findButton(admin, 'Change').trigger('click') @@ -279,6 +411,39 @@ describe('DitoAccount', () => { return dialog } + it('changes the password without passkeys or TOTP', async () => { + const { admin, request } = await mountWithPasskeys({ + login: {}, + isConfirmed: false + }) + const notify = vi.spyOn(admin.context, 'notify') + const security = getLastDialog(admin) + expect(security.findAll('.dito-object')).toHaveLength(1) + expect(security.find('.dito-list').exists()).toBe(false) + await changePassword(admin, { + currentPassword: 'secret', + password: 'changed', + confirmation: 'changed' + }) + await confirmAccess(admin, 'secret') + expect( + request.mock.calls.filter(([{ url }]) => url === '/users/password') + ).toHaveLength(2) + expect(request).toHaveBeenLastCalledWith( + expect.objectContaining({ + method: 'post', + url: '/users/password', + data: { currentPassword: 'secret', password: 'changed' } + }) + ) + expect(notify).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'success', + text: 'Your password was changed.' + }) + ) + }) + it('shows a placeholder instead of the password', async () => { const { admin } = await mountWithPasskeys() const row = getLastDialog(admin).find('.dito-object') @@ -391,7 +556,6 @@ describe('DitoAccount', () => { enabled = true return { data: { backupCodes } } } else if (url === '/users/totp' && method === 'delete') { - checkCode(data.code) enabled = false return { data: { success: true } } } @@ -491,20 +655,22 @@ describe('DitoAccount', () => { expect(getState()).toContain('Not set up') }) - it('turns off TOTP with a code', async () => { + it('turns off TOTP after asking, without a code', async () => { const { admin, request, getState } = await mountWithTotp({ enabled: true }) expect(getState()).toContain('Enabled · 10 backup codes left') await findButton(admin, 'Turn Off').trigger('click') await flushPromises() - await submitCode(admin, '123456') + const dialog = getLastDialog(admin) + expect(dialog.text()).toContain( + 'Do you really want to turn off two-factor authentication?' + ) + expect(dialog.find('input[name="code"]').exists()).toBe(false) + await dialog.find('button[type="submit"]').trigger('click') + await flushPromises() expect(request).toHaveBeenCalledWith( - expect.objectContaining({ - method: 'delete', - url: '/users/totp', - data: { code: '123456' } - }) + expect.objectContaining({ method: 'delete', url: '/users/totp' }) ) expect(getState()).toContain('Not set up') }) diff --git a/packages/admin/src/components/DitoAccount.vue b/packages/admin/src/components/DitoAccount.vue index f43fd0869..c7dcb59b7 100644 --- a/packages/admin/src/components/DitoAccount.vue +++ b/packages/admin/src/components/DitoAccount.vue @@ -35,10 +35,8 @@ export default { items: computed(() => [ settingsRoute.value && { value: 'settings', label: 'Settings' }, - (props.passkeys || props.totp) && { - value: 'security', - label: 'Security' - }, + // Users change their password there, also without passkeys or TOTP: + { value: 'security', label: 'Security' }, { value: 'logout', label: 'Sign Out' } ].filter(Boolean) ), diff --git a/packages/admin/src/components/DitoRoot.vue b/packages/admin/src/components/DitoRoot.vue index 2d543bd18..0c22d73cc 100644 --- a/packages/admin/src/components/DitoRoot.vue +++ b/packages/admin/src/components/DitoRoot.vue @@ -292,6 +292,47 @@ export default { }, settings: { label: 'Set New Password' } }) + }, + + // Return the password to confirm the access with, `'passkey'` to + // confirm it with a passkey, or `null` if the user cancels: + requestAccessConfirmation() { + const hasPasskeys = ( + props.options.login?.passkeys && + !!window.PublicKeyCredential + ) + return showDialog({ + components: { + password: { + type: 'password', + label: 'Password', + info: 'Confirm your access to change your security settings.', + autocomplete: 'current-password', + autofocus: true, + required: true + } + }, + buttons: { + confirm: { + type: 'submit', + text: 'Confirm' + }, + + ...(hasPasskeys && { + passkey: { + type: 'button', + text: 'Use Passkey', + onClick: ({ closeDialog }) => closeDialog('passkey') + } + }), + + cancel: { + type: 'button', + text: 'Cancel' + } + }, + settings: { label: 'Confirm Access' } + }) } } ) diff --git a/packages/admin/src/types/DitoTypeButton.vue b/packages/admin/src/types/DitoTypeButton.vue index 320e126ce..d01f09342 100644 --- a/packages/admin/src/types/DitoTypeButton.vue +++ b/packages/admin/src/types/DitoTypeButton.vue @@ -105,10 +105,13 @@ const DitoTypeButton = { await services.submit?.() } } catch (error) { - const context = field.createContext({ error }) - const res = await emitEvent('error', { context }) - if (res === undefined && !context.wasNotified) { - services.adminContext.notify({ type: 'error', text: error }) + // Aborted requests, e.g. cancelled confirmations, end quietly: + if (error?.name !== 'AbortError') { + const context = field.createContext({ error }) + const res = await emitEvent('error', { context }) + if (res === undefined && !context.wasNotified) { + services.adminContext.notify({ type: 'error', text: error }) + } } } finally { isRunning.value = false diff --git a/packages/admin/src/utils/Session.test.js b/packages/admin/src/utils/Session.test.js index f6e9a08a1..0129a933c 100644 --- a/packages/admin/src/utils/Session.test.js +++ b/packages/admin/src/utils/Session.test.js @@ -21,6 +21,7 @@ function createSession({ loginCodes = [], passwordResets = [], newPasswords = [], + accessConfirmations = [], resolvedViews = [true], redirectAfterLogin = null } = {}) { @@ -33,7 +34,8 @@ function createSession({ passkeyLogin: { method: 'post', path: 'passkey-login' }, loginCode: { method: 'post', path: 'login-code' }, passwordReset: { method: 'post', path: 'password-reset' }, - passwordResetConfirm: { method: 'post', path: 'password-reset-confirm' } + passwordResetConfirm: { method: 'post', path: 'password-reset-confirm' }, + confirmAccess: { method: 'post', path: 'confirm-access' } }, resources: { any: resource => `/${resource.path}` }, getApiUrl: ({ url }) => url, @@ -59,6 +61,9 @@ function createSession({ requestLoginCode: vi.fn(async () => loginCodes.shift() ?? null), requestPasswordReset: vi.fn(async () => passwordResets.shift() ?? null), requestNewPassword: vi.fn(async () => newPasswords.shift() ?? null), + requestAccessConfirmation: vi.fn( + async () => accessConfirmations.shift() ?? null + ), notify: vi.fn() } const detach = session.attachUserInterface(userInterface) @@ -603,4 +608,87 @@ describe('Session', () => { consoleError.mockRestore() }) }) + + describe('confirmAccess()', () => { + const success = { success: true } + + it('confirms the access with the password', async () => { + const { session, api } = createSession({ + responses: { 'post /confirm-access': success }, + accessConfirmations: [{ password: 'secret' }] + }) + expect(await session.confirmAccess()).toBe(true) + expect(api.request).toHaveBeenLastCalledWith({ + method: 'post', + url: '/confirm-access', + data: { password: 'secret' } + }) + }) + + it('asks users with TOTP for the code after the password', async () => { + let attempt = 0 + const { session, api, userInterface } = createSession({ + responses: { + 'post /confirm-access': () => + ++attempt === 1 ? { success: false, codeRequired: true } : success + }, + accessConfirmations: [{ password: 'secret' }], + loginCodes: [{ code: '123456' }] + }) + expect(await session.confirmAccess()).toBe(true) + expect(userInterface.requestLoginCode).toHaveBeenCalledOnce() + expect(api.request).toHaveBeenLastCalledWith({ + method: 'post', + url: '/confirm-access', + data: { password: 'secret', code: '123456' } + }) + }) + + it('ends when the code is cancelled', async () => { + const { session, userInterface } = createSession({ + responses: { + 'post /confirm-access': { success: false, codeRequired: true } + }, + accessConfirmations: [{ password: 'secret' }, { password: 'secret' }] + }) + expect(await session.confirmAccess()).toBe(false) + expect(userInterface.requestLoginCode).toHaveBeenCalledOnce() + expect(userInterface.requestAccessConfirmation).toHaveBeenCalledOnce() + }) + + it('confirms the access with a passkey', async () => { + const { session, api } = createSession({ + responses: { + 'post /passkey-login-options': { challenge: 'challenge' }, + 'post /confirm-access': success + }, + accessConfirmations: ['passkey'] + }) + expect(await session.confirmAccess()).toBe(true) + expect(api.request).toHaveBeenLastCalledWith({ + method: 'post', + url: '/confirm-access', + data: { passkey: { id: 'passkey' } } + }) + }) + + it('asks again after failures, until the user cancels', async () => { + const { session, userInterface } = createSession({ + responses: { + 'post /confirm-access': Object.assign(new Error('Failed'), { + response: { + status: 401, + data: { message: 'Password is incorrect' } + } + }) + }, + accessConfirmations: [{ password: 'wrong' }] + }) + expect(await session.confirmAccess()).toBe(false) + expect(userInterface.requestAccessConfirmation).toHaveBeenCalledTimes(2) + expect(userInterface.notify).toHaveBeenCalledWith( + expect.objectContaining({ text: 'Password is incorrect' }) + ) + }) + }) }) diff --git a/packages/admin/src/utils/Session.ts b/packages/admin/src/utils/Session.ts index 92d760117..b1fdbddf5 100644 --- a/packages/admin/src/utils/Session.ts +++ b/packages/admin/src/utils/Session.ts @@ -14,6 +14,7 @@ interface UserInterface { requestLoginCode(): Promise<{ code: string } | null> requestPasswordReset(): Promise | null> requestNewPassword(): Promise<{ password: string } | null> + requestAccessConfirmation(): Promise<{ password: string } | 'passkey' | null> notify(options: NotifyOptions): void } @@ -50,13 +51,16 @@ export class Session { // `requestLoginData()` returns the credentials, `'passkey'` to log in with a // passkey, `'passwordReset'` to reset the password, or `null` when cancelled. // `requestLoginCode()` returns the TOTP or backup code of users with TOTP, - // `requestPasswordReset()` the username to reset the password of, and - // `requestNewPassword()` the new password, or `null` when cancelled. + // `requestPasswordReset()` the username to reset the password of, + // `requestNewPassword()` the new password, and `requestAccessConfirmation()` + // the password or `'passkey'` to confirm the access with, or `null` when + // cancelled. attachUserInterface({ requestLoginData, requestLoginCode, requestPasswordReset, requestNewPassword, + requestAccessConfirmation, notify }: UserInterface) { const userInterface = { @@ -64,6 +68,7 @@ export class Session { requestLoginCode, requestPasswordReset, requestNewPassword, + requestAccessConfirmation, notify } this.#userInterface = userInterface @@ -105,9 +110,10 @@ export class Session { const { users } = this.#api let response if (loginData === 'passkey') { - const options = await this.#request(users.passkeyLoginOptions) - const passkey = await startAuthentication({ optionsJSON: options.data }) - response = await this.#request(users.passkeyLogin, passkey) + response = await this.#request( + users.passkeyLogin, + await this.#getPasskey() + ) } else { response = await this.#request(users.login, loginData) } @@ -157,6 +163,43 @@ export class Session { } } + // Security changes need a recent login, or a confirmation of the access with + // the password and the code of users with TOTP, or with a passkey. Ask again + // after each failure, and resolve to whether the access was confirmed: + async confirmAccess(): Promise { + const data = await this.#userInterface?.requestAccessConfirmation() + if (data) { + try { + const { users } = this.#api + const body = + data === 'passkey' ? { passkey: await this.#getPasskey() } : data + let response = await this.#request(users.confirmAccess, body) + if (response.data.codeRequired) { + const codeData = await this.#userInterface?.requestLoginCode() + if (codeData) { + response = await this.#request(users.confirmAccess, { + ...body, + ...codeData + }) + } else { + // Cancelling the code ends the confirmation, like the password: + return false + } + } + if (response.data.success) { + return true + } + } catch (error) { + if (!isPasskeyPromptCancelled(error)) { + this.#notifyError(error) + } + } + return this.confirmAccess() + } else { + return false + } + } + async #requestPasswordReset() { const data = await this.#userInterface?.requestPasswordReset() if (data) { @@ -253,6 +296,12 @@ export class Session { } } + // Answer a challenge of the server with a passkey of the browser: + async #getPasskey() { + const options = await this.#request(this.#api.users.passkeyLoginOptions) + return startAuthentication({ optionsJSON: options.data }) + } + #request(resource: any, data: any = null) { const url = this.#api.getApiUrl({ url: this.#api.resources.any(resource) }) return this.#api.request({ method: resource.method, url, data }) diff --git a/packages/admin/src/utils/security.js b/packages/admin/src/utils/security.js index f41b9f705..b8289099e 100644 --- a/packages/admin/src/utils/security.js +++ b/packages/admin/src/utils/security.js @@ -1,6 +1,7 @@ import { startRegistration } from '@simplewebauthn/browser' import DitoQrCode from '../components/DitoQrCode.vue' import { isPasskeyPromptCancelled } from './Session.ts' +import { confirm } from './dialogs.js' // Dialogs are as wide as their content, up to their `width` setting, so // components as wide as the setting fill them: @@ -207,26 +208,20 @@ export function showSecurityDialog(adminContext, { passkeys, totp }) { turnOff: { text: 'Turn Off', if: ({ item }) => item.enabled, - async onClick(context) { - const { request, reload } = context - const result = await submitCode( - context, + // Without confirmed access, requests ask for the code first: + async onClick({ request, reload }) { + const isConfirmed = await confirm( + { adminContext }, { - components: { code: createCodeComponent() }, - buttons: { - cancel: {}, - turnOff: { type: 'submit', text: 'Turn Off' } - }, - settings: { label: 'Turn Off Two-Factor Authentication' } - }, - code => - request({ - resource: users.totp, - method: 'delete', - data: { code } - }) + message: ( + 'Do you really want to turn off two-factor ' + + 'authentication?' + ), + verb: 'turn off' + } ) - if (result) { + if (isConfirmed) { + await request({ resource: users.totp, method: 'delete' }) await reload() } } @@ -244,7 +239,7 @@ export function showSecurityDialog(adminContext, { passkeys, totp }) { }) } -// The code field of the login and the security dialogs: +// The code field of the login dialog, also when confirming the access: export function createCodeComponent() { return { type: 'text', diff --git a/packages/admin/src/utils/services.ts b/packages/admin/src/utils/services.ts index 11888039b..a55906095 100644 --- a/packages/admin/src/utils/services.ts +++ b/packages/admin/src/utils/services.ts @@ -99,7 +99,21 @@ export async function sendRequest( if (checkUser) { await session.ensureUser() } - const response = await api.request({ method, url, query, data, signal }) + const send = () => api.request({ method, url, query, data, signal }) + const response = await send().catch(async error => { + // Security changes answer `confirmationRequired` without a recent login, + // so we confirm the access and send them again. Cancelled confirmations + // end the request quietly, like aborted ones: + if (checkUser && error.response?.data?.confirmationRequired) { + if (await session.confirmAccess()) { + return send() + } else { + throw new DOMException('The confirmation was cancelled.', 'AbortError') + } + } else { + throw error + } + }) // Fetch the own user again when it changed, through its member or `self`: if (checkUser && method === 'patch') { const { users } = api diff --git a/packages/admin/types/index.d.ts b/packages/admin/types/index.d.ts index b0b0f6b87..5751ed653 100644 --- a/packages/admin/types/index.d.ts +++ b/packages/admin/types/index.d.ts @@ -334,6 +334,12 @@ export interface ApiConfig { */ totp?: { path?: string } totpConfirm?: { path?: string; method?: HttpMethod } + /** + * Posted to with the password, the code of users with TOTP or a passkey, + * when security changes answer `confirmationRequired`, with the default + * path `'confirm-access'`. + */ + confirmAccess?: { path?: string; method?: HttpMethod } } /** * Custom resource path handlers, keyed by resource type. Merged diff --git a/packages/server/src/controllers/UsersController.js b/packages/server/src/controllers/UsersController.js index 9026d1e07..880d6ce1f 100644 --- a/packages/server/src/controllers/UsersController.js +++ b/packages/server/src/controllers/UsersController.js @@ -6,7 +6,7 @@ import { randomBytes } from 'node:crypto' import aaguids from 'aaguids' -import { isString } from '@ditojs/utils' +import { asArray, isString } from '@ditojs/utils' import { ModelController } from './ModelController.js' import { getUserIdentifier, @@ -26,7 +26,8 @@ import { AuthorizationError, ControllerError, NotFoundError, - ResponseError + ResponseError, + ValidationError } from '../errors/index.js' export class UsersController extends ModelController { @@ -42,17 +43,19 @@ export class UsersController extends ModelController { 'post passkey-login': true, 'post password-reset': true, 'post password-reset-confirm': true, - // Users manage their own password and passkeys: - 'post password': isAuthenticated, + // Users manage their own password and passkeys, and need confirmed + // access to change them: + 'post confirm-access': isAuthenticated, + 'post password': hasConfirmedAccess, 'get passkeys': isAuthenticated, - 'post passkey-options': isAuthenticated, - 'post passkeys': isAuthenticated, - 'delete passkeys/:id': isAuthenticated, + 'post passkey-options': hasConfirmedAccess, + 'post passkeys': hasConfirmedAccess, + 'delete passkeys/:id': hasConfirmedAccess, // ...and their TOTP: 'get totp': isAuthenticated, - 'post totp': isAuthenticated, - 'post totp-confirm': isAuthenticated, - 'delete totp': isAuthenticated + 'post totp': hasConfirmedAccess, + 'post totp-confirm': hasConfirmedAccess, + 'delete totp': hasConfirmedAccess }, 'post login'(ctx) { @@ -110,6 +113,36 @@ export class UsersController extends ModelController { } }, + // Confirm the access of the logged-in user with their password, and the + // code of users with TOTP, or with a passkey: + 'post confirm-access': { + parameters: { + password: { type: 'string' }, + code: { type: 'string' }, + passkey: { type: 'object' } + }, + + async handler(ctx, { password, code, passkey }) { + const { user } = ctx.state + if (this.passkeys && passkey) { + if (!(await verifyPasskey(this, ctx, passkey))?.$is(user)) { + throw new AuthenticationError('Passkey is incorrect') + } + } else if (!(password && (await user.$verifyPassword(password)))) { + throw new AuthenticationError('Password is incorrect') + } else if (this.totp && (await hasTotp(this.modelClass, ctx))) { + // Like password logins, ask users with TOTP for a code: + if (!code) { + return { success: false, codeRequired: true } + } else if (!(await this.verifyCode(ctx, user, code))) { + throw new AuthenticationError('Code is incorrect') + } + } + storeConfirmedAccess(this, ctx, user) + return { success: true } + } + }, + 'post password': { parameters: { currentPassword: { type: 'string', required: true }, @@ -262,58 +295,16 @@ export class UsersController extends ModelController { rpID: this.passkeys.id, userVerification: 'required' }) - storeChallenge(ctx, options) + storeChallenge(ctx, 'authentication', options) return options }, 'post passkey-login'(ctx) { return this.handleLogin(ctx, async () => { - const { verifyAuthenticationResponse } = this.webAuthn - const response = ctx.request.body - const challenge = consumeChallenge(ctx) - const credential = await this.modelClass - .credentialQuery(ctx.transaction) - .findOne({ type: 'passkey', key: response.id ?? null }) - const user = credential - ? await this.modelClass - .sessionQuery(ctx.transaction) - .findById(credential.userId) - : null + const user = await verifyPasskey(this, ctx, ctx.request.body) if (user) { - const { publicKey, counter, transports } = credential.data - const { verified, authenticationInfo } = - await verifyAuthenticationResponse({ - response, - expectedChallenge: challenge, - expectedOrigin: this.passkeys.origins, - expectedRPID: this.passkeys.id, - credential: { - id: credential.key, - publicKey: Buffer.from(publicKey, 'base64url'), - counter, - transports - } - }).catch(err => { - // Don't tell clients why, but keep the reason for the logs: - throw new AuthenticationError({ - message: 'Passkey is incorrect', - cause: err - }) - }) - if (verified) { - await credential.$patch( - { - data: { - ...credential.data, - counter: authenticationInfo.newCounter - }, - lastUsedAt: new Date() - }, - ctx.transaction - ) - await ctx.login(user) - return user - } + await ctx.login(user) + return user } throw new AuthenticationError('Passkey is incorrect') }) @@ -347,7 +338,7 @@ export class UsersController extends ModelController { userVerification: 'required' } }) - storeChallenge(ctx, options) + storeChallenge(ctx, 'registration', options) return options }, @@ -355,7 +346,7 @@ export class UsersController extends ModelController { const { verifyRegistrationResponse } = this.webAuthn const { verified, registrationInfo } = await verifyRegistrationResponse({ response: ctx.request.body, - expectedChallenge: consumeChallenge(ctx), + expectedChallenge: consumeChallenge(ctx, 'registration'), expectedOrigin: this.passkeys.origins, expectedRPID: this.passkeys.id }).catch(err => { @@ -492,17 +483,12 @@ export class UsersController extends ModelController { }, async 'delete totp'(ctx) { - const { code } = ctx.request.body - if (await this.verifyCode(ctx, ctx.state.user, code)) { - await getTotpQuery( - this.modelClass, - ctx.state.user, - ctx.transaction - ).delete() - return { success: true } - } else { - throw new AuthenticationError('Code is incorrect') - } + await getTotpQuery( + this.modelClass, + ctx.state.user, + ctx.transaction + ).delete() + return { success: true } } } @@ -515,6 +501,7 @@ export class UsersController extends ModelController { const { session, auth } = this.app.config.app this.passkeys = auth?.passkeys ?? null this.totp = auth?.totp ?? null + this.reconfirmAccessAfter = getDuration(auth?.reconfirmAccessAfter ?? '10m') this.passwordReset = auth?.passwordReset ? { ...auth.passwordReset, @@ -645,16 +632,32 @@ export class UsersController extends ModelController { } } + // @override + async executeAndFetch(action, ctx, modify, body = ctx.request.body) { + await validateUserChanges( + this, + ctx, + action, + asArray(body).map(user => [this.getModelId(user), user]) + ) + return super.executeAndFetch(action, ctx, modify, body) + } + + // @override + async relateAndFetch(ctx, modify) { + await validateUserChanges( + this, + ctx, + 'relate', + asArray(ctx.request.body).map(user => [this.getModelId(user), user]) + ) + return super.relateAndFetch(ctx, modify) + } + // @override async executeAndFetchById(action, ctx, modify, body = ctx.request.body) { - const result = await super.executeAndFetchById(action, ctx, modify, body) - const { passwordProperty } = this.modelClass.definition.options - if (body[passwordProperty] !== undefined) { - // Users change their password through patches too, e.g. in - // `PATCH /users/self`: - await handlePasswordChange(this, ctx, result) - } - return result + await validateUserChanges(this, ctx, action, [[ctx.memberId, body]]) + return super.executeAndFetchById(action, ctx, modify, body) } isAuthenticated(ctx) { @@ -666,6 +669,18 @@ export class UsersController extends ModelController { ) } + // Security changes need a login or a 'post confirm-access' of the user within + // `app.auth.reconfirmAccessAfter`. Without one, we answer 403 with + // `confirmationRequired` instead of denying the access: + hasConfirmedAccess(ctx) { + if (this.isAuthenticated(ctx)) { + checkConfirmedAccess(ctx) + return true + } else { + return false + } + } + async handleLogin(ctx, login) { let user let error @@ -674,6 +689,7 @@ export class UsersController extends ModelController { user = await login() if (user) { await user.$patch({ lastLogin: new Date() }, ctx.transaction) + storeConfirmedAccess(this, ctx, user) } else { // `UserMixin.login()` asks users with TOTP for a code first: codeRequired = true @@ -799,6 +815,10 @@ function isAuthenticated(ctx) { return this.isAuthenticated(ctx) } +function hasConfirmedAccess(ctx) { + return this.hasConfirmedAccess(ctx) +} + function getPasskeyQuery(modelClass, ctx) { return modelClass.credentialQuery(ctx.transaction).where({ type: 'passkey', @@ -806,17 +826,20 @@ function getPasskeyQuery(modelClass, ctx) { }) } -function storeChallenge(ctx, { challenge }) { - ctx.session.passkeyChallenge = { +// Registrations and authentications keep their challenges apart, so that +// confirming the access with a passkey doesn't end a pending registration: +function storeChallenge(ctx, type, { challenge }) { + ctx.session[`${type}Challenge`] = { challenge, expires: Date.now() + authTimeout } } // Each challenge can only be answered once, before it expires: -function consumeChallenge(ctx) { - const { challenge, expires } = ctx.session.passkeyChallenge ?? {} - delete ctx.session.passkeyChallenge +function consumeChallenge(ctx, type) { + const key = `${type}Challenge` + const { challenge, expires } = ctx.session[key] ?? {} + delete ctx.session[key] if (challenge && expires > Date.now()) { return challenge } else { @@ -827,6 +850,56 @@ function consumeChallenge(ctx) { } } +// Verify the response of `startAuthentication()` against the challenge in the +// session, and resolve to the user of the passkey, or `null`: +async function verifyPasskey(controller, ctx, response) { + const { modelClass, passkeys, webAuthn } = controller + const challenge = consumeChallenge(ctx, 'authentication') + const credential = await modelClass + .credentialQuery(ctx.transaction) + .findOne({ type: 'passkey', key: response.id ?? null }) + const user = credential + ? await modelClass.sessionQuery(ctx.transaction).findById(credential.userId) + : null + if (user) { + const { publicKey, counter, transports } = credential.data + const { verified, authenticationInfo } = await webAuthn + .verifyAuthenticationResponse({ + response, + expectedChallenge: challenge, + expectedOrigin: passkeys.origins, + expectedRPID: passkeys.id, + credential: { + id: credential.key, + publicKey: Buffer.from(publicKey, 'base64url'), + counter, + transports + } + }) + .catch(err => { + // Don't tell clients why, but keep the reason for the logs: + throw new AuthenticationError({ + message: 'Passkey is incorrect', + cause: err + }) + }) + if (verified) { + await credential.$patch( + { + data: { + ...credential.data, + counter: authenticationInfo.newCounter + }, + lastUsedAt: new Date() + }, + ctx.transaction + ) + return user + } + } + return null +} + // The attempt that uses up the limit answers it too, so that clients don't ask // for another code: function createTooManyAttemptsError() { @@ -913,6 +986,62 @@ async function handlePasswordChange(controller, ctx, user) { updatePasswordFingerprint(ctx, user) } +// Passwords are set when users are created, but only change through +// 'post password', which needs confirmed access and the current password, and +// their hashes are never set directly. Usernames only change with confirmed +// access, as password resets go to them. `users` are `[id, user]` entries: +async function validateUserChanges(controller, ctx, action, users) { + const { modelClass } = controller + const { usernameProperty, passwordProperty } = modelClass.definition.options + for (const [id, user] of users) { + // Relating users with ids updates them: + const isInsert = action === 'insert' || action === 'relate' && id == null + const property = + user?.hash !== undefined + ? 'hash' + : !isInsert && user?.[passwordProperty] !== undefined + ? passwordProperty + : null + if (property) { + throw new ValidationError({ + type: 'BodyValidation', + message: `Unable to change '${property}' outside of 'post password'` + }) + } + const username = user?.[usernameProperty] + if (!isInsert && username !== undefined) { + const stored = await modelClass.query(ctx.transaction).findById(id) + if (stored?.[usernameProperty] !== username) { + checkConfirmedAccess(ctx) + } + } + } +} + +// Answer 403 with `confirmationRequired` to users without a login or a +// 'post confirm-access' within `app.auth.reconfirmAccessAfter`: +function checkConfirmedAccess(ctx) { + const { user, expires } = ctx.session?.confirmedAccess ?? {} + if ( + !ctx.state.user || + user !== getUserIdentifier(ctx.state.user) || + !(expires > Date.now()) + ) { + throw new AuthorizationError({ + message: 'Confirmation required', + status: 403, + confirmationRequired: true + }) + } +} + +function storeConfirmedAccess(controller, ctx, user) { + ctx.session.confirmedAccess = { + user: getUserIdentifier(user), + expires: Date.now() + controller.reconfirmAccessAfter + } +} + function getPasswordResetQuery(modelClass, user, trx) { return modelClass.credentialQuery(trx).where({ type: 'password-reset', diff --git a/packages/server/types/index.d.ts b/packages/server/types/index.d.ts index d98778398..ba6ab24c3 100644 --- a/packages/server/types/index.d.ts +++ b/packages/server/types/index.d.ts @@ -252,6 +252,14 @@ export interface ApplicationConfig { */ expires?: number | string } + /** + * How long logins and `'post confirm-access'` confirm the access of + * sessions for the security changes of `UsersController`, in + * milliseconds or as a duration string. + * + * @defaultValue `'10m'` + */ + reconfirmAccessAfter?: number | string } /** * Set signed cookie keys. @@ -2876,6 +2884,12 @@ export class UsersController< * and the user is an instance of the controller's model. */ isAuthenticated(ctx: KoaContext): boolean + /** + * Like `isAuthenticated()`, but throws a 403 error with + * `confirmationRequired` for users without confirmed access, see + * `app.auth.reconfirmAccessAfter`. For the `authorize` of security changes. + */ + hasConfirmedAccess(ctx: KoaContext): boolean /** * Sends the user the email with the `url` that resets their password, with * `app.auth.passwordReset`. Apps override it. diff --git a/tests/e2e/account/admin/index.spec.ts b/tests/e2e/account/admin/index.spec.ts index 304a882b1..593e7d063 100644 --- a/tests/e2e/account/admin/index.spec.ts +++ b/tests/e2e/account/admin/index.spec.ts @@ -31,7 +31,7 @@ test.describe('account settings', () => { await expect(dialog.getByLabel('Username')).toBeFocused() }) - test('edit the logged-in user', async ({ page, browser, url }) => { + test('edit the logged-in user', async ({ page, url }) => { await login(page, url, ['ada', 'secret']) await expect(getAccountButton(page)).toHaveText('ada') @@ -43,25 +43,17 @@ test.describe('account settings', () => { await expect(page).toHaveURL(`${url}/admin/account/settings`) // The form shows the user's data, without the password or its hash: - const { hash } = (await User.query().findOne('username', 'ada'))! expect(self).toMatchObject({ username: 'ada', email: 'ada@example.com' }) expect(self).not.toHaveProperty('hash') expect(self).not.toHaveProperty('password') const username = page.getByLabel('Username', { exact: true }) const email = page.getByLabel('Email', { exact: true }) - const password = page.getByLabel('New Password', { exact: true }) await expect(username).toHaveValue('ada') await expect(email).toHaveValue('ada@example.com') - // Password fields without a value show a mask until they are focused: - await expect(password).toHaveValue('****************') - await password.focus() - await expect(password).toHaveValue('') - expect(await password.inputValue()).not.toBe(hash) // Save the changes: await username.fill('lovelace') await email.fill('lovelace@example.com') - await password.fill('changed') const selfPatched = page.waitForResponse(isSelfResponse('PATCH')) await page.getByRole('button', { name: 'Save' }).click() expect((await selfPatched).status()).toBe(200) @@ -69,19 +61,7 @@ test.describe('account settings', () => { // The record changed, and the account menu shows it right away: const user = (await User.query().findOne('username', 'lovelace'))! expect(user.email).toBe('lovelace@example.com') - expect(user.hash).not.toBe(hash) - expect(await user.$verifyPassword('changed')).toBe(true) await expect(getAccountButton(page)).toHaveText('lovelace') - - // The new password works for a fresh login: - const context = await browser.newContext() - try { - const freshPage = await context.newPage() - await login(freshPage, url, ['lovelace', 'changed']) - await expect(getAccountButton(freshPage)).toHaveText('lovelace') - } finally { - await context.close() - } }) test('cancel the settings for the root', async ({ page, url }) => { diff --git a/tests/e2e/account/views.ts b/tests/e2e/account/views.ts index 692b81228..f100ecbbe 100644 --- a/tests/e2e/account/views.ts +++ b/tests/e2e/account/views.ts @@ -10,8 +10,7 @@ export const account = { label: 'Account', components: { username: { type: 'text', label: 'Username' }, - email: { type: 'email', label: 'Email' }, - password: { type: 'password', label: 'New Password' } + email: { type: 'email', label: 'Email' } } } } as const diff --git a/tests/e2e/passkeys/admin/__screenshots__/totp-turn-off.png b/tests/e2e/passkeys/admin/__screenshots__/totp-turn-off.png index 9d3689bae..815f2db62 100644 Binary files a/tests/e2e/passkeys/admin/__screenshots__/totp-turn-off.png and b/tests/e2e/passkeys/admin/__screenshots__/totp-turn-off.png differ diff --git a/tests/e2e/passkeys/admin/security.screenshot.ts b/tests/e2e/passkeys/admin/security.screenshot.ts index aec19d629..1ebb9ec9d 100644 --- a/tests/e2e/passkeys/admin/security.screenshot.ts +++ b/tests/e2e/passkeys/admin/security.screenshot.ts @@ -204,9 +204,9 @@ test.describe('screenshots', () => { const security = await openSecurity(page) await security.getByRole('button', { name: 'Turn Off' }).click() const dialog = page.getByRole('dialog', { - name: 'Turn Off Two-Factor Authentication' + name: 'Do you really want to turn off two-factor authentication?' }) - await expect(dialog.getByLabel('Authentication Code')).toBeFocused() + await expect(dialog.getByRole('button', { name: 'Turn Off' })).toBeVisible() await expectScreenshot(page, 'totp-turn-off.png') }) }) diff --git a/tests/e2e/passkeys/admin/totp.spec.ts b/tests/e2e/passkeys/admin/totp.spec.ts index 220084645..4892f444b 100644 --- a/tests/e2e/passkeys/admin/totp.spec.ts +++ b/tests/e2e/passkeys/admin/totp.spec.ts @@ -71,13 +71,12 @@ test.describe('TOTP', () => { const [credential] = await Credential.query().where('type', 'totp') expect(credential.lastUsedAt).toBeInstanceOf(Date) - // Turn off TOTP with a backup code: + // Turn off TOTP, with the access confirmed by the login: const turnOff = await openSecurity(page) await turnOff.getByRole('button', { name: 'Turn Off' }).click() const confirm = page.getByRole('dialog', { - name: 'Turn Off Two-Factor Authentication' + name: 'Do you really want to turn off two-factor authentication?' }) - await confirm.getByLabel('Authentication Code').fill(backupCodes[0]) await confirm.getByRole('button', { name: 'Turn Off' }).click() await expect(turnOff).toContainText('Not set up') expect(await Credential.query()).toEqual([]) diff --git a/tests/e2e/screenshots/admin/__screenshots__/account-menu.png b/tests/e2e/screenshots/admin/__screenshots__/account-menu.png index 87c727780..22903d933 100644 Binary files a/tests/e2e/screenshots/admin/__screenshots__/account-menu.png and b/tests/e2e/screenshots/admin/__screenshots__/account-menu.png differ diff --git a/tests/server/confirmed-access.test.ts b/tests/server/confirmed-access.test.ts new file mode 100644 index 000000000..2d7260f07 --- /dev/null +++ b/tests/server/confirmed-access.test.ts @@ -0,0 +1,466 @@ +import { vi } from 'vitest' +import { + Controller, + CredentialModel, + SessionModel, + UserModel, + UsersController +} from '@ditojs/server' +import { TOTP } from 'otpauth' +import { createTestApp, getAppUrl } from '../utils/app.js' +import { createAuthenticator } from '../utils/authenticator.js' +import { createClient } from '../utils/client.js' +import { createTestDatabase } from '../utils/database.js' + +class Session extends SessionModel {} + +class Credential extends CredentialModel { + declare type: string + declare data: Record +} + +class Member extends UserModel { + declare id: number +} + +class Members extends UsersController { + override modelClass = Member + override member: any = { + allow: ['patch', 'put'] + } +} + +// Log in another user without 'post login', as apps do to impersonate users: +class Impersonations extends Controller { + override actions: any = { + async 'post .'(ctx: any) { + const { username } = ctx.request.body + await ctx.login(await Member.query().findOne('username', username)) + return { success: true } + } + } +} + +type Client = ReturnType + +describe('Confirmed access', () => { + const app = createTestApp({ + config: { + app: { + normalizePaths: true, + helmet: false, + keys: ['test-secret'], + session: { modelClass: 'Session' }, + auth: { + credentials: { modelClass: 'Credential', secret: 'secret' }, + passkeys: { + name: 'Example', + id: 'example.com', + origins: ['https://example.com'] + }, + totp: { issuer: 'Example' }, + reconfirmAccessAfter: '5m' + } + } + }, + models: { Session, Credential, Member }, + controllers: { Members, Impersonations } + }) + + let url: string + + const post = (request: Client, path: string, body?: any) => + request(`/members/${path}`, { method: 'POST', body }) + + const loginWithPassword = async (username = 'ada') => { + const request = createClient(url) + const login = await post(request, 'login', { + username, + password: 'secret' + }) + return { request, login } + } + + const createPasskeyAuthenticator = () => + createAuthenticator({ origin: 'https://example.com', rpId: 'example.com' }) + + const registerPasskey = async (request: Client) => { + const authenticator = createPasskeyAuthenticator() + const options = await post(request, 'passkey-options') + const passkey = await post( + request, + 'passkeys', + authenticator.register(options.data) + ) + expect(passkey.status).toBe(200) + return { authenticator, passkey: passkey.data } + } + + // Set up TOTP for ada, and return the TOTP of her authenticator app: + const setupTotp = async (request: Client) => { + const setup = await post(request, 'totp') + const totp = new TOTP({ secret: setup.data.secret }) + const confirm = await post(request, 'totp-confirm', { + code: totp.generate() + }) + expect(confirm.status).toBe(200) + return { totp, backupCodes: confirm.data.backupCodes } + } + + // The setup used the current code, so we use the next one, which is accepted + // for clocks that drift: + const getNextCode = (totp: TOTP) => + totp.generate({ timestamp: Date.now() + totp.period * 1000 }) + + // Move the clock past the confirmed access, which only fakes `Date`, also + // for the app: + const expireAccess = () => { + vi.setSystemTime(Date.now() + 5 * 60 * 1000 + 1) + } + + // Request each security change, which all answer 200 within the confirmed + // access, except for the empty registration and the wrong code: + const requestChanges = async (request: Client, passkeyId = 0) => [ + await post(request, 'passkey-options'), + await post(request, 'passkeys', {}), + await request(`/members/passkeys/${passkeyId}`, { method: 'DELETE' }), + await post(request, 'totp'), + await post(request, 'totp-confirm', { code: 'wrong' }), + await request('/members/totp', { method: 'DELETE' }), + await post(request, 'password', { + currentPassword: 'secret', + password: 'secret' + }) + ] + + const expectConfirmedAccess = async (request: Client) => { + expect((await post(request, 'passkey-options')).status).toBe(200) + } + + beforeAll(async () => { + await createTestDatabase(app, Session, Member, Credential) + await app.knex.schema.alterTable('Member', table => { + table.dropColumn('password') + }) + await app.start() + url = getAppUrl(app) + await Member.query().insert({ + username: 'ada', + password: 'secret' + } as any) + }) + + afterAll(async () => { + await app.stop() + await app.knex.destroy() + }) + + beforeEach(() => { + vi.useFakeTimers({ toFake: ['Date'] }) + }) + + afterEach(async () => { + vi.useRealTimers() + await app.knex('Session').del() + await app.knex('Credential').del() + }) + + it('allows security changes right after logging in', async () => { + const { request } = await loginWithPassword() + const { passkey } = await registerPasskey(request) + const responses = await requestChanges(request, passkey.id) + expect(responses.map(({ status }) => status)).toEqual([ + 200, 400, 200, 200, 401, 200, 200 + ]) + }) + + it('asks to confirm the access for security changes later', async () => { + const { request } = await loginWithPassword() + const { passkey } = await registerPasskey(request) + expireAccess() + for (const response of await requestChanges(request, passkey.id)) { + expect(response.status).toBe(403) + expect(response.data).toEqual({ + message: 'Confirmation required', + confirmationRequired: true + }) + } + // Reading the passkeys and the TOTP needs no confirmation: + expect((await request('/members/passkeys')).status).toBe(200) + expect((await request('/members/totp')).status).toBe(200) + expect(await Credential.query().where('type', 'passkey')).toHaveLength(1) + }) + + it('denies security changes without a login', async () => { + const request = createClient(url) + for (const response of [ + ...(await requestChanges(request)), + await post(request, 'confirm-access', { password: 'secret' }) + ]) { + expect(response.status).toBe(401) + } + }) + + it('confirms the access with the password', async () => { + const { request } = await loginWithPassword() + expireAccess() + const confirm = await post(request, 'confirm-access', { + password: 'secret' + }) + expect(confirm.status).toBe(200) + expect(confirm.data).toEqual({ success: true }) + await expectConfirmedAccess(request) + // ...until it expires again: + expireAccess() + expect((await post(request, 'passkey-options')).status).toBe(403) + }) + + it('rejects wrong or missing passwords', async () => { + const { request } = await loginWithPassword() + expireAccess() + for (const body of [{ password: 'wrong' }, {}]) { + const confirm = await post(request, 'confirm-access', body) + expect(confirm.status).toBe(401) + expect(confirm.data).toEqual({ message: 'Password is incorrect' }) + } + expect((await post(request, 'passkey-options')).status).toBe(403) + }) + + describe('with TOTP', () => { + const loginWithTotp = async () => { + const { request } = await loginWithPassword() + const { totp, backupCodes } = await setupTotp(request) + expireAccess() + return { request, totp, backupCodes } + } + + const getFailures = async () => ( + (await Credential.query().findOne({ type: 'totp' }))!.data.failures ?? 0 + ) + + it('asks for the code after the password', async () => { + const { request, totp } = await loginWithTotp() + const password = await post(request, 'confirm-access', { + password: 'secret' + }) + expect(password.status).toBe(200) + expect(password.data).toEqual({ success: false, codeRequired: true }) + expect((await post(request, 'passkey-options')).status).toBe(403) + const code = await post(request, 'confirm-access', { + password: 'secret', + code: getNextCode(totp) + }) + expect(code.data).toEqual({ success: true }) + await expectConfirmedAccess(request) + }) + + it('confirms the access with a backup code', async () => { + const { request, backupCodes } = await loginWithTotp() + const confirm = await post(request, 'confirm-access', { + password: 'secret', + code: backupCodes[0] + }) + expect(confirm.data).toEqual({ success: true }) + await expectConfirmedAccess(request) + }) + + it('counts wrong codes, but not wrong passwords', async () => { + const { request, totp } = await loginWithTotp() + const wrongPassword = await post(request, 'confirm-access', { + password: 'wrong', + code: 'wrong' + }) + expect(wrongPassword.data.message).toBe('Password is incorrect') + expect(await getFailures()).toBe(0) + const confirm = (code: string) => + post(request, 'confirm-access', { password: 'secret', code }) + for (let i = 1; i < 10; i++) { + const wrong = await confirm('wrong') + expect(wrong.status).toBe(401) + expect(wrong.data.message).toBe('Code is incorrect') + } + expect(await getFailures()).toBe(9) + const last = await confirm('wrong') + expect(last.status).toBe(400) + expect(last.data.message).toBe('Too many attempts') + const right = await confirm(getNextCode(totp)) + expect(right.data.message).toBe('Too many attempts') + expect((await post(request, 'passkey-options')).status).toBe(403) + }) + + it(`doesn't let sessions guess codes by turning off TOTP`, async () => { + const { request, totp } = await loginWithTotp() + for (let i = 0; i < 10; i++) { + const remove = await request('/members/totp', { + method: 'DELETE', + body: { code: 'wrong' } + }) + expect(remove.status).toBe(403) + } + expect(await getFailures()).toBe(0) + expect(await Credential.query().where('type', 'totp')).toHaveLength(1) + const confirm = await post(request, 'confirm-access', { + password: 'secret', + code: getNextCode(totp) + }) + expect(confirm.data).toEqual({ success: true }) + }) + + it('confirms the access with logins with the code', async () => { + const { totp } = await loginWithTotp() + const { request, login } = await loginWithPassword() + expect(login.data.codeRequired).toBe(true) + const code = await post(request, 'login-code', { + code: getNextCode(totp) + }) + expect(code.data.success).toBe(true) + await expectConfirmedAccess(request) + }) + }) + + describe('with passkeys', () => { + const confirmWithPasskey = async ( + request: Client, + authenticator: ReturnType + ) => { + const options = await post(request, 'passkey-login-options') + return post(request, 'confirm-access', { + passkey: authenticator.authenticate(options.data) + }) + } + + it('confirms the access with a passkey, also of users with TOTP', async () => { + const { request } = await loginWithPassword() + const { authenticator } = await registerPasskey(request) + await setupTotp(request) + expireAccess() + const confirm = await confirmWithPasskey(request, authenticator) + expect(confirm.status).toBe(200) + expect(confirm.data).toEqual({ success: true }) + await expectConfirmedAccess(request) + }) + + it('rejects the passkeys of other users and unknown ones', async () => { + const other = await Member.query().insert({ + username: 'bo', + password: 'secret' + } as any) + try { + const { request: otherRequest } = await loginWithPassword('bo') + const { authenticator } = await registerPasskey(otherRequest) + const { request } = await loginWithPassword() + expireAccess() + for (const passkeyAuthenticator of [ + authenticator, + createPasskeyAuthenticator() + ]) { + const confirm = await confirmWithPasskey( + request, + passkeyAuthenticator + ) + expect(confirm.status).toBe(401) + expect(confirm.data).toEqual({ message: 'Passkey is incorrect' }) + } + expect((await post(request, 'passkey-options')).status).toBe(403) + } finally { + await Member.query().deleteById(other.id) + } + }) + + it('keeps pending registrations when confirming the access', async () => { + const { request } = await loginWithPassword() + const { authenticator } = await registerPasskey(request) + const other = createPasskeyAuthenticator() + const options = await post(request, 'passkey-options') + expect((await confirmWithPasskey(request, authenticator)).status).toBe( + 200 + ) + const passkey = await post( + request, + 'passkeys', + other.register(options.data) + ) + expect(passkey.status).toBe(200) + }) + + it('confirms the access with passkey logins', async () => { + const { request } = await loginWithPassword() + const { authenticator } = await registerPasskey(request) + expireAccess() + const client = createClient(url) + const options = await post(client, 'passkey-login-options') + const login = await post( + client, + 'passkey-login', + authenticator.authenticate(options.data) + ) + expect(login.data.success).toBe(true) + await expectConfirmedAccess(client) + }) + }) + + describe('username changes', () => { + const patchSelf = (request: Client, body: Record) => + request('/members/self', { method: 'PATCH', body }) + + afterEach(async () => { + await Member.query().where('username', 'eve').patch({ username: 'ada' }) + }) + + it('need confirmed access', async () => { + const { request } = await loginWithPassword() + expireAccess() + for (const response of [ + await patchSelf(request, { username: 'eve' }), + await request('/members/self', { + method: 'PUT', + body: { username: 'eve' } + }) + ]) { + expect(response.status).toBe(403) + expect(response.data.confirmationRequired).toBe(true) + } + expect(await Member.query().findOne('username', 'ada')).toBeDefined() + await post(request, 'confirm-access', { password: 'secret' }) + const patch = await patchSelf(request, { username: 'eve' }) + expect(patch.status).toBe(200) + expect(patch.data.username).toBe('eve') + }) + + it(`don't include unchanged usernames`, async () => { + const { request } = await loginWithPassword() + expireAccess() + expect((await patchSelf(request, { username: 'ada' })).status).toBe(200) + }) + }) + + it('confirms the access with logins again', async () => { + const { request } = await loginWithPassword() + expireAccess() + expect((await post(request, 'passkey-options')).status).toBe(403) + const login = await post(request, 'login', { + username: 'ada', + password: 'secret' + }) + expect(login.data.success).toBe(true) + await expectConfirmedAccess(request) + }) + + it('confirms the access only for the user who confirmed it', async () => { + const other = await Member.query().insert({ + username: 'bo', + password: 'secret' + } as any) + try { + const { request } = await loginWithPassword() + await request('/impersonations', { + method: 'POST', + body: { username: 'bo' } + }) + expect((await request('/members/session')).data.user.id).toBe(other.id) + expect((await post(request, 'passkey-options')).status).toBe(403) + } finally { + await Member.query().deleteById(other.id) + } + }) +}) diff --git a/tests/server/passkeys.test.ts b/tests/server/passkeys.test.ts index df68c903b..053f5f4c3 100644 --- a/tests/server/passkeys.test.ts +++ b/tests/server/passkeys.test.ts @@ -299,13 +299,13 @@ describe('Passkeys', () => { }) // Let the challenge of the stored session expire: const session = (await Session.query()).find( - session => session.value.passkeyChallenge + session => session.value.authenticationChallenge )! await session.$query().patch({ value: { ...session.value, - passkeyChallenge: { - ...session.value.passkeyChallenge, + authenticationChallenge: { + ...session.value.authenticationChallenge, expires: Date.now() - 1 } } diff --git a/tests/server/password-reset.test.ts b/tests/server/password-reset.test.ts index 7594c4aff..64a617d4e 100644 --- a/tests/server/password-reset.test.ts +++ b/tests/server/password-reset.test.ts @@ -57,10 +57,6 @@ class Guest extends UserModel { class Members extends UsersController { override modelClass = Member - override member: any = { - allow: ['patch'] - } - // Not async, so that its errors are thrown right away: override sendPasswordReset(ctx: any, user: Member, url: string) { if (user.username === 'broken') { @@ -343,18 +339,15 @@ describe('Password reset', () => { }) it('deletes the tokens of the user with password changes', async () => { - for (const [path, method, body] of [ - ['password', 'POST', { currentPassword: 'secret', password: 'changed' }], - ['self', 'PATCH', { password: 'changed' }] - ] as const) { - await Member.query().patchById(member.id, { password: 'secret' } as any) - const token = await getToken() - const { request } = await login('secret') - const response = await request(`/members/${path}`, { method, body }) - expect(response.status).toBe(200) - expect(await Credential.query()).toEqual([]) - expect((await confirmReset(token, 'another')).status).toBe(400) - } + const token = await getToken() + const { request } = await login('secret') + const response = await request('/members/password', { + method: 'POST', + body: { currentPassword: 'secret', password: 'changed' } + }) + expect(response.status).toBe(200) + expect(await Credential.query()).toEqual([]) + expect((await confirmReset(token, 'another')).status).toBe(400) }) it('rejects expired tokens', async () => { diff --git a/tests/server/totp.test.ts b/tests/server/totp.test.ts index 451b72c5e..c002f03d3 100644 --- a/tests/server/totp.test.ts +++ b/tests/server/totp.test.ts @@ -385,21 +385,6 @@ describe('TOTP', () => { expect(right.data.error).toBe('Too many attempts') }) - it('count when turning off TOTP too', async () => { - const { request: self, totp } = await setupTotp() - const remove = (code: string) => - self('/members/totp', { method: 'DELETE', body: { code } }) - for (let i = 0; i < 9; i++) { - expect((await remove('wrong')).status).toBe(401) - } - const { request } = await loginWithPassword() - const last = await postCode(request, 'wrong') - expect(last.data.error).toBe('Too many attempts') - const right = await remove(getNextCode(totp)) - expect(right.status).toBe(400) - expect(right.data.message).toBe('Too many attempts') - }) - it('start anew after a right code', async () => { const { totp } = await setupTotp() const { request } = await loginWithPassword() @@ -565,33 +550,15 @@ describe('TOTP', () => { expect(login.data).toMatchObject({ success: true, authenticated: true }) }) - it('turns off TOTP with a code', async () => { - const { request, totp } = await setupTotp() - const wrong = await request('/members/totp', { - method: 'DELETE', - body: { code: 'wrong' } - }) - expect(wrong.status).toBe(401) - const remove = await request('/members/totp', { - method: 'DELETE', - body: { code: getNextCode(totp) } - }) + it('turns off TOTP with confirmed access, without a code', async () => { + const { request } = await setupTotp() + const remove = await request('/members/totp', { method: 'DELETE' }) expect(remove.data).toEqual({ success: true }) expect(await Credential.query()).toEqual([]) const { login } = await loginWithPassword() expect(login.data).toMatchObject({ success: true, authenticated: true }) }) - it('turns off TOTP with a backup code', async () => { - const { request, backupCodes } = await setupTotp() - const remove = await request('/members/totp', { - method: 'DELETE', - body: { code: backupCodes[3] } - }) - expect(remove.data).toEqual({ success: true }) - expect(await Credential.query()).toEqual([]) - }) - it('only lets logged-in users manage their TOTP', async () => { const request: Client = createClient(url) for (const [method, path] of [ diff --git a/tests/server/users-controller.test.ts b/tests/server/users-controller.test.ts index e3647b546..6abeb0dfa 100644 --- a/tests/server/users-controller.test.ts +++ b/tests/server/users-controller.test.ts @@ -150,12 +150,38 @@ class Profiles extends UsersController { } } -// Members that patch any member, also their password: +// Members that create, update and patch any member: class Moderators extends UsersController { + override modelClass = Member + override collection: any = { + allow: ['post', 'put', 'patch'], + authorize: { post: true, put: true, patch: true } + } + + override member: any = { + allow: ['put', 'patch'], + authorize: { put: true, patch: true } + } +} + +// Members that relate existing members when posting them with ids: +class Relaters extends UsersController { + override modelClass = Member + override relate = true + override collection: any = { + allow: ['post'], + authorize: { post: true } + } +} + +// Members that delete themselves with confirmed access, as the docs show: +class Deleters extends UsersController { override modelClass = Member override member: any = { - allow: ['patch'], - authorize: { patch: true } + allow: ['delete'], + authorize: { + delete: (ctx: any) => this.hasConfirmedAccess(ctx) && '$self' + } } } @@ -192,7 +218,9 @@ describe('UsersController', () => { SubWriters, Editors, Profiles, - Moderators + Moderators, + Relaters, + Deleters } }) @@ -463,23 +491,6 @@ describe('UsersController', () => { expect((await request('/members/session')).data.authenticated).toBe(true) }) - it(`keeps the session that changes others' passwords`, async () => { - const request = createClient(url) - await request('/members/login', { - method: 'POST', - body: { username: 'ada', password: 'secret' } - }) - const response = await request(`/moderators/${other.id}`, { - method: 'PATCH', - body: { password: 'changed' } - }) - expect(response.status).toBe(200) - expect((await request('/members/session')).data).toMatchObject({ - authenticated: true, - user: { id: member.id } - }) - }) - it('passes on errors of the session query', async context => { context.skip(isSQLiteTest, 'SQLite compares text with integer ids') const request = await loginOther() @@ -566,22 +577,126 @@ describe('UsersController', () => { }) const self = await request('/members/self', { method: 'PATCH', - body: { nickname: 'Bobo', password: 'changed' } + body: { nickname: 'Bobo' } }) expect(self.status).toBe(200) expect(self.data).toMatchObject({ id: other.id, nickname: 'Bobo' }) expect(self.data).not.toHaveProperty('hash') expect((await Member.query().findById(member.id))!.nickname).toBeNull() - // Changing the own password keeps the session: - expect((await request('/members/session')).data.authenticated).toBe(true) - // The password is hashed like in other patches: + } finally { + await Member.query().deleteById(other.id) + } + }) + + it('only changes passwords through `post password`', async () => { + const request = createClient(url) + await request('/members/login', { + method: 'POST', + body: { username: 'ada', password: 'secret' } + }) + for (const [path, method, body] of [ + ['/members/self', 'PATCH', { nickname: 'Countess', password: 'changed' }], + [`/moderators/${member.id}`, 'PATCH', { password: 'changed' }], + [`/moderators/${member.id}`, 'PUT', { username: 'ada', password: 'x' }], + ['/moderators', 'PATCH', [{ id: member.id, password: 'changed' }]], + ['/moderators', 'PUT', [{ id: member.id, password: 'changed' }]] + ] as const) { + const response = await request(path, { method, body }) + expect(response.status).toBe(400) + expect(response.data).toEqual({ + type: 'BodyValidation', + message: `Unable to change 'password' outside of 'post password'` + }) + } + const stored = (await Member.query().findById(member.id))! + expect(stored.nickname).toBeNull() + expect(await stored.$verifyPassword('secret')).toBe(true) + }) + + it('never sets the hash of the password directly', async () => { + const request = createClient(url) + await request('/members/login', { + method: 'POST', + body: { username: 'ada', password: 'secret' } + }) + // A real hash, of the password 'changed': + const { hash } = Member.fromJson({ + username: 'ada', + password: 'changed' + } as any) + for (const [path, method, body] of [ + ['/members/self', 'PATCH', { hash }], + [`/moderators/${member.id}`, 'PUT', { username: 'ada', hash }], + ['/moderators', 'PATCH', [{ id: member.id, hash }]], + ['/moderators', 'PUT', [{ id: member.id, hash }]], + ['/moderators', 'POST', { username: 'cy', hash }] + ] as const) { + const response = await request(path, { method, body }) + expect(response.status).toBe(400) + expect(response.data).toEqual({ + type: 'BodyValidation', + message: `Unable to change 'hash' outside of 'post password'` + }) + } + const stored = (await Member.query().findById(member.id))! + expect(await stored.$verifyPassword('secret')).toBe(true) + expect(await Member.query().findOne('username', 'cy')).toBeUndefined() + }) + + it('validates related users with ids like updates', async () => { + // Without a login, so without confirmed access: + const request = createClient(url) + for (const [body, status] of [ + [{ id: member.id, hash: 'pwned' }, 400], + [{ id: member.id, password: 'changed' }, 400], + [{ id: member.id, username: 'eve' }, 403] + ] as const) { + const response = await request('/relaters', { method: 'POST', body }) + expect(response.status).toBe(status) + } + const stored = (await Member.query().findById(member.id))! + expect(stored.username).toBe('ada') + expect(await stored.$verifyPassword('secret')).toBe(true) + }) + + it('only lets users delete themselves with confirmed access', async () => { + const [cy, dee] = await Promise.all( + ['cy', 'dee'].map(username => + Member.query().insert({ username, password: 'secret' } as any) + ) + ) + try { + const request = createClient(url) + await request('/members/login', { + method: 'POST', + body: { username: 'cy', password: 'secret' } + }) + const other = await request(`/deleters/${dee.id}`, { method: 'DELETE' }) + expect(other.status).toBe(401) + const self = await request('/deleters/self', { method: 'DELETE' }) + expect(self.status).toBe(200) + expect(await Member.query().findById(cy.id)).toBeUndefined() + expect(await Member.query().findById(dee.id)).toBeDefined() + } finally { + await Member.query().whereIn('id', [cy.id, dee.id]).delete() + } + }) + + it('creates users with passwords', async () => { + const request = createClient(url) + const response = await request('/moderators', { + method: 'POST', + body: { username: 'cy', password: 'secret' } + }) + expect(response.status).toBe(201) + try { const login = await createClient(url)('/members/login', { method: 'POST', - body: { username: 'bo', password: 'changed' } + body: { username: 'cy', password: 'secret' } }) expect(login.data.success).toBe(true) } finally { - await Member.query().deleteById(other.id) + await Member.query().deleteById(response.data.id) } }) diff --git a/tests/types/server/application.test-d.ts b/tests/types/server/application.test-d.ts index 8ac6242bb..3f2bf22e4 100644 --- a/tests/types/server/application.test-d.ts +++ b/tests/types/server/application.test-d.ts @@ -87,4 +87,12 @@ describe('Application', () => { } }) }) + + it('accepts the confirmed access in app.auth', () => { + assertType({ + app: { + auth: { reconfirmAccessAfter: '5m' } + } + }) + }) })