diff --git a/acl/drumate.json b/acl/drumate.json index b9d47d76..b99f9053 100644 --- a/acl/drumate.json +++ b/acl/drumate.json @@ -63,6 +63,16 @@ } }, "errors": [ + { + "code": "USERNAME_INVALID", + "message": "Username must be 2-80 letters, digits, dot, dash or underscore, starting with a letter or digit", + "http_status": 200 + }, + { + "code": "USERNAME_TAKEN", + "message": "Username is already used in this domain", + "http_status": 200 + }, { "code": "WRONG_PASSWORD", "message": "OTP code is incorrect", @@ -696,8 +706,8 @@ }, "id": { "type": "string", - "required": true, - "doc": "User ID to update" + "required": false, + "doc": "Ignored. The caller is always the account renamed" } }, "returns": { diff --git a/service/lib/password-policy.js b/service/lib/password-policy.js new file mode 100644 index 00000000..6bcad28f --- /dev/null +++ b/service/lib/password-policy.js @@ -0,0 +1,30 @@ +// service/lib/password-policy.js +// The one password policy for every endpoint that sets a password: signup, +// drumate.change_password and drumate.set_initial_password. +// +// KEEP IN STEP with PW_RULES in the signup UI (signup/src/widgets/form/index.js) +// and the settings change-password modal +// (ui-team/src/drumee/builtins/widget/settings/password-policy.js). The UIs +// check first for the inline message; this is the authoritative check, since +// the endpoints can be called directly. Keys double as the UIs' LOCALE keys +// for the "Your password still needs: …" list. + +const PW_SPECIALS = /[\[\]\{\}\'\"\ \-\_\+\=\|\!\:\;\,\?\.\/\*\%\$\&\#\(\)\@]/; + +const PW_RULES = [ + { key: "PW_NEEDS_MIN", test: (v) => v.length >= 8 }, + { key: "PW_NEEDS_UPPERCASE", test: (v) => /[A-Z]/.test(v) }, + { key: "PW_NEEDS_NUMBER", test: (v) => /[0-9]/.test(v) }, + { key: "PW_NEEDS_SYMBOL", test: (v) => PW_SPECIALS.test(v) }, +]; + +/** + * @param {String} password already trimmed by the caller (login trims too) + * @returns {String[]} keys of the unmet rules, empty when compliant + */ +function missingPasswordRules(password) { + const v = String(password == null ? "" : password); + return PW_RULES.filter((r) => !r.test(v)).map((r) => r.key); +} + +module.exports = { PW_SPECIALS, PW_RULES, missingPasswordRules }; diff --git a/service/private/drumate.js b/service/private/drumate.js index a7c9c263..d63d0695 100644 --- a/service/private/drumate.js +++ b/service/private/drumate.js @@ -32,6 +32,7 @@ const { const { Entity, Generator, MfsTools } = require("@drumee/server-core"); const { get_node_content } = MfsTools; const { purge_account } = require("../lib/account-purge"); +const { missingPasswordRules } = require("../lib/password-policy"); // Contextual tutorial tour ids. See tutorial_seen() below for why this list is // duplicated in acl/drumate.json and in ui-team's tours.js, and what a @@ -184,7 +185,17 @@ class __private_drumate extends Entity { * @returns */ async change_password() { - const new_password = this.input.need(Attr.new_password); + // Trimmed like signup: that is the value login compares against. + const new_password = String(this.input.need(Attr.new_password)).trim(); + // Same policy as signup, checked BEFORE the credential so a weak password + // never burns a single-use email OTP. `missing` lists the unmet rule keys + // (the UI's PW_NEEDS_* LOCALE keys); the error code stays + // uncompliant_password so older clients keep their existing message. + const missing = missingPasswordRules(new_password); + if (missing.length) { + this.output.data({ error: 'uncompliant_password', missing }); + return + } // Accept EITHER credential, strictly verifying whichever was sent — // same contract as unlink_oauth. The FE picks by the ACCOUNT's state: // password-backed accounts send old_password, accounts that never set @@ -209,25 +220,21 @@ class __private_drumate extends Entity { } await this.yp.await_proc('secret_clear', this.uid, 'all'); } - if (!new_password.match(/(.+){8,}/)) { //(/(.+){2,} +(.+){4,}/) - this.output.data({ error: 'uncompliant_password' }); - } else { - r = await this.yp.await_proc('set_password', this.uid, new_password); - // Flag the account as password-backed so step-up flows - // (delete_account, change_email) gate on password rather than OTP. - await this.yp.call_proc('drumate_update_profile', this.uid, { password_set: 1 }); - // "Log out of other devices": drop every other session's cookie and - // socket; the calling session (input.sid) survives. Best-effort — a - // cleanup failure must not report the password change as failed. - if (parseInt(this.input.use('logout_others', 0))) { - try { - await this.yp.await_proc('session_logout_others', this.uid, this.input.sid()); - } catch (e) { - this.warn('change_password: session_logout_others failed:', e && e.message); - } + r = await this.yp.await_proc('set_password', this.uid, new_password); + // Flag the account as password-backed so step-up flows + // (delete_account, change_email) gate on password rather than OTP. + await this.yp.call_proc('drumate_update_profile', this.uid, { password_set: 1 }); + // "Log out of other devices": drop every other session's cookie and + // socket; the calling session (input.sid) survives. Best-effort — a + // cleanup failure must not report the password change as failed. + if (parseInt(this.input.use('logout_others', 0))) { + try { + await this.yp.await_proc('session_logout_others', this.uid, this.input.sid()); + } catch (e) { + this.warn('change_password: session_logout_others failed:', e && e.message); } - this.output.data(r) } + this.output.data(r) } /** @@ -400,8 +407,7 @@ class __private_drumate extends Entity { /** do_update_profile * */ - async do_update_profile() { - let profile = this.input.need(Attr.profile); + async do_update_profile(profile = this.input.need(Attr.profile)) { const profile_str = JSON.stringify(profile); let data = await this.yp.await_proc( 'drumate_update_profile', @@ -457,6 +463,14 @@ class __private_drumate extends Entity { cur_profile = {}; } if (await this.check_otp_and_change()) return; + // A username is not just a profile field: login, the directory and + // avatars read the drumate.username COLUMN (plus the -u vhost built from + // it), which drumate_update_profile never touches. Change it first so a + // taken/invalid name rejects the whole save instead of half-applying it. + if (profile && profile.username !== undefined) { + const failed = await this._changeUsername(profile.username); + if (failed) return this.output.data(failed); + } for (let key in profile) { if (['otp'].includes(key)) { if (cur_profile.otp != null) { @@ -468,6 +482,44 @@ class __private_drumate extends Entity { await this.do_update_profile(); } + /** + * Rename the caller's account: validate, check it is free in the caller's + * domain, then update the username column and vhost (drumate_change_username). + * No-op when the name is unchanged (compared case-insensitively, like the + * column's collation). + * @param {String} username + * @returns {Object|null} { error } to send back, or null on success/no-op + */ + async _changeUsername(username) { + username = String(username == null ? '' : username).trim(); + const rows = toArray(await this.yp.await_query( + 'SELECT username, domain_id FROM drumate WHERE id = ?', this.uid + )); + const cur = rows[0] || {}; + if (username.toLowerCase() === String(cur.username || '').toLowerCase()) { + return null; + } + // 2-80 chars: letters, digits, dot, dash, underscore; must start with a + // letter or digit. It ends up in a hostname label (-u.). + if (!/^[A-Za-z0-9][A-Za-z0-9._-]{1,79}$/.test(username)) { + return { error: 'USERNAME_INVALID' }; + } + // get_user_in_domain also matches by id/email; any hit that is not the + // caller means the name is not available. + const chk = await this.yp.await_proc('get_user_in_domain', username, cur.domain_id || 1); + if (chk && chk.exists == 1 && chk.id !== this.uid) { + return { error: 'USERNAME_TAKEN' }; + } + try { + await this.yp.await_proc('drumate_change_username', this.uid, username); + } catch (e) { + // UNIQUE (username, domain_id) lost a race with another rename. + this.warn('_changeUsername failed', e && e.message); + return { error: 'USERNAME_TAKEN' }; + } + return null; + } + /** * Get or create the signed-in user's referral code + link. * Reads the live reward DB from reward_hub_conf, calls the reward-hub @@ -1023,9 +1075,10 @@ class __private_drumate extends Entity { return; } - const password = this.input.need(Attr.password); - if (!password.match(/(.+){8,}/)) { - this.output.data({ error: "uncompliant_password" }); + const password = String(this.input.need(Attr.password)).trim(); + const missing = missingPasswordRules(password); + if (missing.length) { + this.output.data({ error: "uncompliant_password", missing }); return; } @@ -1236,7 +1289,9 @@ class __private_drumate extends Entity { */ async update_ident() { const ident = this.input.need(Attr.ident); - const id = this.input.need(Attr.id); + // Always the caller: this used to rename whatever `id` the client sent, + // letting any signed-in user rename any other account. + const id = this.uid; let chk; let my_org = await this.yp.await_proc('my_organisation', id) if (isEmpty(my_org)) { diff --git a/service/signup.js b/service/signup.js index 7f93d0ea..289914cb 100644 --- a/service/signup.js +++ b/service/signup.js @@ -23,17 +23,9 @@ const { isEmpty } = require("lodash"); const { notifyMemberJoined } = require("./lib/notify-member-joined"); const { Mfs } = require("@drumee/server-core"); -// Password policy — KEEP IN STEP with PW_RULES in the signup UI -// (signup/src/widgets/form/index.js). The UI checks first for the inline -// message; this is the authoritative check, since the endpoint can be called -// directly. Keys double as the UI's LOCALE keys for the "still needs" list. -const PW_SPECIALS = /[\[\]\{\}\'\"\ \-\_\+\=\|\!\:\;\,\?\.\/\*\%\$\&\#\(\)\@]/; -const PW_RULES = [ - { key: "PW_NEEDS_MIN", test: (v) => v.length >= 8 }, - { key: "PW_NEEDS_UPPERCASE", test: (v) => /[A-Z]/.test(v) }, - { key: "PW_NEEDS_NUMBER", test: (v) => /[0-9]/.test(v) }, - { key: "PW_NEEDS_SYMBOL", test: (v) => PW_SPECIALS.test(v) }, -]; +// Password policy — shared with drumate.change_password; see the lib for the +// UI copies it must stay in step with. +const { missingPasswordRules } = require("./lib/password-policy"); class __signup extends Mfs { @@ -48,7 +40,7 @@ class __signup extends Mfs { // Checked on the TRIMMED value — the one that is hashed and that login // (yp.login, session.signin) compares against. - const missing = PW_RULES.filter((r) => !r.test(password)).map((r) => r.key); + const missing = missingPasswordRules(password); if (missing.length) { return this.output.data({ status: "weak_password", missing }); } diff --git a/test/change-username.test.js b/test/change-username.test.js new file mode 100644 index 00000000..08ba70a8 --- /dev/null +++ b/test/change-username.test.js @@ -0,0 +1,68 @@ +const assert = require('node:assert/strict'); +const test = require('node:test'); + +global.myDrumee = {arch: 'pod', useEmail: 0}; +global.verbosity = 0; +global.debug = {}; + +const DrumatePrivate = require('../service/private/drumate'); +const {_changeUsername} = DrumatePrivate.prototype; + +const UID = 'aaaaaaaaaaaaaaaa'; + +// Stub `this`: the caller is `john` in domain 1. `taken` is the answer +// get_user_in_domain gives; `renameFails` makes drumate_change_username throw. +function worker({taken = null, renameFails = false} = {}) { + const calls = []; + return { + calls, + uid: UID, + warn() {}, + yp: { + async await_query() { + return {username: 'john', domain_id: 1}; + }, + async await_proc(name, ...args) { + calls.push([name, ...args]); + if (name === 'get_user_in_domain') { + return taken ? {id: taken, exists: 1} : {id: 'ffffffffffffffff', exists: 0}; + } + if (name === 'drumate_change_username' && renameFails) { + throw new Error('Duplicate entry'); + } + return {}; + }, + }, + }; +} + +test('an unchanged username (any case) touches nothing', async () => { + const w = worker(); + assert.equal(await _changeUsername.call(w, ' John '), null); + assert.deepEqual(w.calls, []); +}); + +test('a free, valid username renames the column and vhost', async () => { + const w = worker(); + assert.equal(await _changeUsername.call(w, 'john.doe'), null); + assert.deepEqual(w.calls.at(-1), ['drumate_change_username', UID, 'john.doe']); +}); + +test('a username used by someone else is rejected before any write', async () => { + const w = worker({taken: 'bbbbbbbbbbbbbbbb'}); + assert.deepEqual(await _changeUsername.call(w, 'jane'), {error: 'USERNAME_TAKEN'}); + assert.ok(!w.calls.some(([n]) => n === 'drumate_change_username')); +}); + +test('invalid usernames never reach the database', async () => { + for (const bad of ['', 'a', 'john doe', '-john', 'john@x.com', 'x'.repeat(81)]) { + const w = worker(); + assert.deepEqual(await _changeUsername.call(w, bad), {error: 'USERNAME_INVALID'}, bad); + assert.deepEqual(w.calls, [], bad); + } +}); + +test('losing a rename race reports the name as taken', async () => { + const w = worker({renameFails: true}); + assert.deepEqual(await _changeUsername.call(w, 'jane'), {error: 'USERNAME_TAKEN'}); +}); diff --git a/test/password-policy.test.js b/test/password-policy.test.js new file mode 100644 index 00000000..8a22550c --- /dev/null +++ b/test/password-policy.test.js @@ -0,0 +1,26 @@ +const assert = require('node:assert/strict'); +const test = require('node:test'); + +const { missingPasswordRules } = require('../service/lib/password-policy'); + +test('a password meeting every signup rule has nothing missing', () => { + assert.deepEqual(missingPasswordRules('Abcdefg1!'), []); +}); + +test('the old 8-character-only check no longer passes', () => { + assert.deepEqual(missingPasswordRules('abcdefgh'), [ + 'PW_NEEDS_UPPERCASE', 'PW_NEEDS_NUMBER', 'PW_NEEDS_SYMBOL', + ]); +}); + +test('each rule is reported on its own', () => { + assert.deepEqual(missingPasswordRules('Abc1!'), ['PW_NEEDS_MIN']); + assert.deepEqual(missingPasswordRules('abcdefg1!'), ['PW_NEEDS_UPPERCASE']); + assert.deepEqual(missingPasswordRules('Abcdefgh!'), ['PW_NEEDS_NUMBER']); + assert.deepEqual(missingPasswordRules('Abcdefgh1'), ['PW_NEEDS_SYMBOL']); +}); + +test('empty and missing input fail every rule instead of throwing', () => { + assert.equal(missingPasswordRules('').length, 4); + assert.equal(missingPasswordRules(undefined).length, 4); +});