From 0c549ed497d216cd29a536dddcfbc4e0f9361568 Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:11:16 +0700 Subject: [PATCH 01/20] fix(oauth): register Google/Apple sign-ins in the connection log (#31) server-core logs a connection inside session.signin()/login() and nowhere else, so a session opened by a PROCEDURE is invisible to everything that reads services_log -- yp.show_login_log, and the analytics "Last login" column, which takes MAX(ctime) over rows carrying args.success='1'. Both OAuth paths open sessions that way and neither registered: handleOAuthCallback CASE A -- session_login_with_oauth signs the user in and writes no log row. oauth.verify_otp -- the 2FA leg. The callback returns at CASE D without logging, correctly, since the cookie is only otp_pending and nobody is signed in yet; session_login_otp then finalizes it and writes nothing. An OAuth account with 2FA signed in perfectly and never registered at all. CASE C (new user) needs no equivalent: it signs up through create_account, which finishes on session.signin() and is logged there. This restores behaviour rather than inventing it. Stage still holds google.callback and apple.callback rows, but none newer than 2025-11-18, while yp.signin rows run to today -- the logging was lost when these paths moved into this module. The helper lives on the Account base both services extend, and cannot break a login: the provider has already authenticated the user by the time it runs, so a failure is warned and swallowed. Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- service/lib/loby.js | 35 +++++++++++++++++++++++++++++++++++ service/oauth.js | 8 ++++++++ 2 files changed, 43 insertions(+) diff --git a/service/lib/loby.js b/service/lib/loby.js index ee5c342..ac145fc 100644 --- a/service/lib/loby.js +++ b/service/lib/loby.js @@ -343,6 +343,34 @@ class Account extends Entity { /** * Handle OAuth callback for both Google and Apple */ + /** + * Record an accepted sign-in that was opened by a PROCEDURE rather than by + * session.signin()/session.login(). + * + * server-core logs a connection in those two methods and nowhere else, so a + * session opened by session_login_with_oauth or session_login_otp is invisible + * to everything reading services_log -- yp.show_login_log, and the analytics + * "Last login" column, which takes MAX(ctime) over rows carrying + * args.success='1'. + * + * This is not a new behaviour, it is a restored one: stage still holds + * google.callback and apple.callback rows, but none newer than 2025-11-18, + * while yp.signin rows continue to today. The logging was lost when these + * paths moved into this module. + * + * NEVER LET THIS BREAK A LOGIN. The provider has already authenticated the + * user by the time we run; a logging failure must cost an analytics row, not + * their session. Hence the swallow. + * @param {String} uid + */ + async _logConnection(uid) { + try { + await this.session._log_connection({ uid }); + } catch (e) { + this.warn('[Auth] failed to record login for', uid, e && e.message); + } + } + async handleOAuthCallback(profile) { try { @@ -402,6 +430,13 @@ class Account extends Entity { WHERE user_id = ? AND provider = ?`, access_token, refresh_token, sessionData.id, provider ); + // A completed sign-in, and the only one on this path: the session was + // opened by session_login_with_oauth, which writes no services_log row. + // CASE C below needs no equivalent -- it signs up through + // create_account, which finishes on session.signin() and is logged + // there (stage's signup.create_account rows). CASE D is finalized in + // oauth.verify_otp and logged there. + await this._logConnection(sessionData.id); sessionData.method = 'signin'; return sessionData; } diff --git a/service/oauth.js b/service/oauth.js index 8baa9aa..3ed310d 100644 --- a/service/oauth.js +++ b/service/oauth.js @@ -50,6 +50,14 @@ class OAuth extends Loby { const r = toArray( await this.yp.await_proc('session_login_otp', pending.uid, code, secret, sid) )[0]; + // This COMPLETES the OAuth sign-in that the provider callback started. The + // callback returned at CASE D without logging, correctly -- the cookie was + // only otp_pending, nobody was signed in yet -- and session_login_otp is a + // plain proc that writes no services_log row. So unless we log here, an + // OAuth account with 2FA signs in perfectly and never registers at all. + if (r && r.status === 'success') { + await this._logConnection(pending.uid); + } this.output.data(r || { status: 'error' }); } From 66cd797f9fc8773b188541bcdef07d6f3d14d401 Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:32:02 +0700 Subject: [PATCH 02/20] Fix/mail sender identity (#32) * fix(mail): send from contact@drumee.org and stop the "Drumee>" display name Two changes to the same header, because the first exposed the second. The From used to come from credential/email.json's auth.user -- the transport's SMTP login, an unmonitored butler@ mailbox -- and verify-email and signup-completed passed no `from` at all, so they fell through to the package's module-level default and arrived as a bare address with no display name while the OTP mail beside them showed "Drumee". Pin the address in service/lib/mail-sender.js and give all three a real From. Passing that From through Messenger.send() is what produced "Drumee>". This service pins @drumee/server-essentials ^1.2.29, and 1.2.29's send() does: let from = args.from || `butler@${domain}`; try { from = configs.auth.user; } catch (e) {} // `configs` is not in scope // here, so this always // throws and ours stands ... from: `Drumee <${from}>` // wraps it a SECOND time Given a full mailbox that yields `Drumee <"Drumee" >`, which parsers read as { name: "Drumee>", address: "contact@drumee.org" }. The address still resolves, so the mail delivers and the log says "Message sent" -- nothing anywhere reports a malformed header. Worth recording why this hid for so long: the plugin resolves its OWN bundled copy of the package, not the 1.3.1 under runtime/server/main, and 1.3.1's send() passes `from` through untouched. Reading the wrong copy cleared the code twice. sendAs() drives the transport directly instead. Passing a bare address would paper over it only while 1.2.x is installed -- the caret range also admits 1.3.x, where the bare address would arrive with no display name -- so this is correct under both. It is the same approach analytics-server's _deliver() takes. The transport is module-cached inside the package and is deliberately not closed. Verified end-to-end against the real 1.2.29 through the real relay, reading the delivered message back over IMAP: before From: "Drumee>" after From: Drumee Co-Authored-By: Claude Opus 5 (1M context) * style(mail): cap the loby email templates at 600px The frame table carried width="100%" with max-width:100%, so the mail stretched to whatever width the inbox pane happened to be and the body copy ran the full width of a desktop screen. 600px is the standard email frame width and the width the sibling templates in server-team already use. Fluid below the cap, capped above. The width ATTRIBUTE has to stay a number for Outlook, which ignores the style and treats width="100%" as a broken value; centring comes from the align="center" already on the enclosing td. Verified by rendering each template headless at 1200px and measuring the frame: 600px, with no horizontal page overflow. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- service/lib/loby.js | 27 ++--- service/lib/mail-sender.js | 127 ++++++++++++++++++++++++ service/signup.js | 15 +-- service/templates/otp.html | 8 +- service/templates/signup-completed.html | 8 +- service/templates/verify-email.html | 8 +- 6 files changed, 163 insertions(+), 30 deletions(-) create mode 100644 service/lib/mail-sender.js diff --git a/service/lib/loby.js b/service/lib/loby.js index ac145fc..4712813 100644 --- a/service/lib/loby.js +++ b/service/lib/loby.js @@ -26,20 +26,7 @@ const { resolve } = require("path"); // left it throwing ReferenceError at its first guard. const { template, isEmpty, isArray } = require("lodash"); -// Configured envelope sender (email.json -> auth.user), resolved once. Used to -// build a display-name From ("Drumee" ) for outbound mail, matching the -// password-login OTP path in server-team. -let _butlerSender; -function butlerSender() { - if (_butlerSender !== undefined) return _butlerSender; - try { - const f = resolve(sysEnv().credential_dir, "email.json"); - _butlerSender = (JSON.parse(readFileSync(f, "utf8")).auth || {}).user || null; - } catch (e) { - _butlerSender = null; - } - return _butlerSender; -} +const { sendAs } = require("./mail-sender"); class Account extends Entity { @@ -513,20 +500,18 @@ class Account extends Entity { code: otp.code, why_this_otp: lex._why_this_otp, }; + const subject = lex._your_otp; const msg = new Messenger({ - subject: lex._your_otp, + subject, recipient: _email, handler: this.exception && this.exception.email, }); try { const tpl = resolve(__dirname, "../templates/otp.html"); const html = msg.renderFrom(tpl, data); - // Display-name From ("Drumee" ) so the inbox shows "Drumee" - // instead of the raw sender address. Falls back to the default sender. - const sender = butlerSender(); - const from = sender ? `"Drumee" <${sender}>` : undefined; - await msg.send(from ? { html, from } : { html }); - return 1; + // sendAs, not msg.send: the pinned Messenger re-wraps the From and turns + // a full mailbox into a "Drumee>" display name. See ./mail-sender. + return await sendAs(msg, { to: _email, subject, html }); } catch (e) { this.warn("[Auth] 2FA OTP email send failed", e); return 0; diff --git a/service/lib/mail-sender.js b/service/lib/mail-sender.js new file mode 100644 index 0000000..a3f29f0 --- /dev/null +++ b/service/lib/mail-sender.js @@ -0,0 +1,127 @@ +// service/lib/mail-sender.js +// The address user-facing Drumee mail is sent FROM, and the RFC 5322 mailbox +// built from it. Mirrors server-team's service/lib/mail-sender.js — the two +// repos send from the same brand address and must not drift. +// +// Pinned here rather than read from credential/email.json — which is what the +// butlerSender() copy in lib/loby.js used to do. That file holds the transport's +// SMTP *login*: an unmonitored `butler@` mailbox. Deriving the public +// From from it meant the two could never differ, so a reprovisioned credential +// silently rewrote who user-facing mail appeared to come from. The login is +// unaffected — the transport still authenticates as whatever email.json says. +// +// It also gives verify-email and signup-completed a From at all: both called +// Messenger.send({ html }) with no `from`, falling through to the package's +// module-level FROM (also email.json's auth.user), so they arrived as a bare +// address with no display name while the OTP mail beside them showed "Drumee". +// +// DEPLOYMENT REQUIREMENT, not satisfied by this file. drumee.org is not the +// domain the relay's DKIM key signs (d=drumee.com), and its SPF record +// ("v=spf1 a mx ~all") lists only Firebase hosting and Google's MX — not the +// relay. Its DMARC is published twice (p=none and p=quarantine), which per +// RFC 7489 makes receivers discard the set entirely. Until drumee.org publishes +// an SPF entry for the relay and its own DKIM selector, everything sent from +// this address is unauthenticated mail — and these templates are 2FA codes and +// email verification, where landing in spam locks a user out of signing in. +const MAIL_SENDER_NAME = "Drumee"; +const MAIL_SENDER_ADDRESS = "contact@drumee.org"; + +/** + * Build an RFC 5322 mailbox, accepting EITHER a bare address or a mailbox that + * already carries its own angle brackets. + * + * This exists because of a real bug. Sending modules used to format their From + * by hand as `"Drumee" <${sender}>`, which is correct only if `sender` is a bare + * address. Hand it a full mailbox and the brackets nest: + * + * `"Drumee" <"Drumee" >` + * -> parsers report { name: "Drumee>", address: "contact@drumee.org" } + * + * The address still resolves, so the mail is delivered and nothing is logged — + * the closing bracket is silently absorbed into the display name and every + * recipient sees "Drumee>". Since butlerFrom() returns a full mailbox while + * credential-derived senders are bare addresses, the two shapes are one + * keystroke apart at any call site, so the wrapping is centralised here where + * it can only happen once. + * + * Kept identical to server-team's service/lib/mail-sender.js. + * + * @param {String} name display name; quotes are escaped, never passed raw + * @param {String} address bare address, or a mailbox like `X ` + * @returns {String} e.g. `"Drumee" ` + * @throws {Error} if no address can be recovered — a broken From is worse than + * a loud failure, because it delivers and looks fine in logs + */ +function mailbox(name, address) { + const raw = String(address == null ? "" : address).trim(); + // The innermost <...> pair is the address; a value with no pair is already + // bare, and any loose bracket on it is stripped rather than re-emitted. + const angled = raw.match(/<([^<>]*)>/); + const addr = (angled ? angled[1] : raw.replace(/[<>]/g, "")).trim(); + if (!addr) { + throw new Error(`mail-sender: cannot build a From, no address in ${JSON.stringify(address)}`); + } + // A bare quote in the phrase would terminate it early and turn the rest of + // the header into stray tokens — the same class of break as the nested `>`. + const phrase = String(name == null ? "" : name).replace(/[\\"]/g, "\\$&"); + return `"${phrase}" <${addr}>`; +} + +/** + * The From header for user-facing mail, display name included. + * + * The name is what an inbox actually shows; without it the mail arrives as a + * bare address no recipient recognises. + * + * @returns {String} RFC 5322 mailbox, e.g. `"Drumee" ` + */ +function butlerFrom() { + return mailbox(MAIL_SENDER_NAME, MAIL_SENDER_ADDRESS); +} + +/** + * Send an already-rendered message with OUR From header, bypassing + * Messenger.send(). + * + * Messenger.send() cannot be trusted with a From. This package is pinned at + * `^1.2.29`, and 1.2.29's send() does: + * + * let from = args.from || `butler@${domain}`; + * try { from = configs.auth.user; } catch (e) {} // `configs` is not in + * // scope here, so this + * // always throws and the + * // caller's value stands + * ... + * from: `Drumee <${from}>` // wraps it a SECOND time + * + * Given a full mailbox that yields `Drumee <"Drumee" >`, + * which parsers read as { name: "Drumee>", address: "contact@drumee.org" } and + * every inbox renders as "Drumee>". The address still resolves, so the mail is + * delivered and nothing is logged. That is the 2026-08-04 bug. + * + * Passing a bare address instead would paper over it only while 1.2.x is + * installed: the caret range also admits 1.3.x, whose send() passes `from` + * through untouched, and there the bare address would arrive with no display + * name at all. Driving the transport directly is correct under both, and is the + * same approach analytics-server's _deliver() already takes. + * + * The transport is module-cached inside the package, so it is deliberately NOT + * closed here — closing it would break every later send in the process. + * + * @param {Messenger} msg configured Messenger (used only for its transport) + * @param {{to:String, subject:String, html:String}} parts + * @returns {Promise} 1 sent, 0 not sent (no MTA configured) + */ +async function sendAs(msg, { to, subject, html }) { + const mta = await msg.getMTA(); // sync in 1.2.29, async in 1.3.x; await covers both + if (!mta) return 0; + try { + await mta.sendMail({ from: butlerFrom(), to, subject, html }); + return 1; + } finally { + // What send() does at the end of its run; we replace send(), so we owe it. + if (typeof msg.stop === "function") msg.stop(); + } +} + +module.exports = { MAIL_SENDER_NAME, MAIL_SENDER_ADDRESS, mailbox, butlerFrom, sendAs }; diff --git a/service/signup.js b/service/signup.js index a7861c5..ce298cc 100644 --- a/service/signup.js +++ b/service/signup.js @@ -5,6 +5,7 @@ const { toArray } = require('@drumee/server-essentials').utils; const { resolve } = require('path'); const { isEmpty, isArray } = require('lodash'); const Loby = require("./lib/loby") +const { sendAs } = require("./lib/mail-sender"); const { uniqueNamesGenerator, colors, animals, adjectives } = require('unique-names-generator'); const { randomBytes } = require('crypto'); @@ -145,15 +146,17 @@ class Signup extends Loby { security_title: "Security Note", security_note: "This verification link will expire in 24 hours. For your security, please do not share this email with anyone.", }; + const subject = "Verify your Drumee email address"; const msg = new Messenger({ - subject: "Verify your Drumee email address", + subject, recipient: _email, handler: this.exception.email, }); const tpl = resolve(__dirname, "./templates/verify-email.html"); const html = msg.renderFrom(tpl, data); - await msg.send({ html }); - return 1; + // sendAs, not msg.send: the pinned Messenger re-wraps the From and turns + // a full mailbox into a "Drumee>" display name. See lib/mail-sender. + return await sendAs(msg, { to: _email, subject, html }); } catch (e) { this.warn("[_send_verification_email] failed", e); return 0; @@ -204,15 +207,15 @@ class Signup extends Loby { try { const homepath = this.input.homepath(); const home = `${homepath}#/desk`; + const subject = "Your Drumee account is all set"; const msg = new Messenger({ - subject: "Your Drumee account is all set", + subject, recipient: _email, handler: this.exception.email, }); const tpl = resolve(__dirname, "./templates/signup-completed.html"); const html = msg.renderFrom(tpl, { home, email: _email }); - await msg.send({ html }); - return 1; + return await sendAs(msg, { to: _email, subject, html }); } catch (e) { this.warn("[_send_signup_completed_email] failed", e); return 0; diff --git a/service/templates/otp.html b/service/templates/otp.html index ae6d336..40cd826 100644 --- a/service/templates/otp.html +++ b/service/templates/otp.html @@ -10,7 +10,13 @@
- + +
diff --git a/service/templates/signup-completed.html b/service/templates/signup-completed.html index 634a4ee..15230c5 100644 --- a/service/templates/signup-completed.html +++ b/service/templates/signup-completed.html @@ -10,7 +10,13 @@
- + +
diff --git a/service/templates/verify-email.html b/service/templates/verify-email.html index 039a132..a1551d0 100644 --- a/service/templates/verify-email.html +++ b/service/templates/verify-email.html @@ -10,7 +10,13 @@
- + +
From 2df9472e21c65c1691ba02819d508fd0ec82903a Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Wed, 5 Aug 2026 07:48:13 +0700 Subject: [PATCH 03/20] Fix/onboarding flow reliability (#33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(onboarding): key answers on uid, let any step create the row, true reset Backend and schema half of the onboarding reliability fix. Pairs with onboarding-ui fix/onboarding-flow-reliability — the save_* procedures gain a _uid parameter, so both must ship together. session_id was the only write key. A session is transient: it rotates on re-login, token refresh and expiry, and keying durable survey answers on it meant the answers became unreachable the moment it changed. Worse, only save_onboarding_user_info could INSERT — every other step was a bare UPDATE that raised "Onboarding session not found. Start at step 1." So a single failed or late step-1 wedged the whole wizard permanently, and a mid-flow re-login did the same. Adds a nullable uid column (additive, indexed, non-unique) and routes every procedure through onboarding_resolve_row, which resolves in a fixed order: the row for this session (unchanged behaviour for every existing record, and it stamps uid as it goes, so rows migrate themselves on first touch), else the user's most recent row re-pointed at the new session, else a fresh row. Steps can now create the row they write to, so one failure no longer poisons the rest of the flow. mark_onboarding_complete still validates firstname / industry / role / team_size, so a stub row can never pass as complete. Empty tool and challenge selections now overwrite instead of being rejected: save_onboarding_tools no longer throws on an empty array, and check_onboarding_completion NULL-tests rather than length-tests, so "none of these" is a real answer rather than an unanswered step. The tools "Other" free text moves into its own tools_other column, matching industry_other / role_other, instead of being spliced into the current_tools JSON array where nothing could distinguish a canonical key from user input. Normalisation runs inside the procedure, so a client posting the old inline shape still produces a clean array plus a populated tools_other; an idempotent backfill converts existing rows without touching clean ones or their mtime. reset() cleared authorization and nothing else: it discarded the session but kept the data, orphaning a row per reset and restarting the wizard against a dead session. It now deletes the user's answers (including orphans left by the old behaviour) and leaves the login alone. Security: fast_check "public-api" short-circuits the ACL to GRANTED before src is ever evaluated, so "src": "anonymous" on these services was unreachable rather than merely permissive. User-data endpoints move to src "owner" with no fast_check; get_env, get_countries and save_signup_info stay public. Every handler also gained a service-level identity guard, which does not depend on the ACL being configured correctly. update_profile validated identity AFTER destructuring this.user's profile, throwing for anonymous callers instead of returning the "no-user" answer two lines below; it now checks first and resolves its row by uid before falling back to email. Several handlers tested `!this.uid`, which is truthy for ID_NOBODY and let anonymous callers through. Verified against a scratch MariaDB: all 14 files apply to both a simulated pre-migration v2 table and a fresh install; session rotation, step-out-of-order creation, empty-selection clearing, legacy tools normalisation, uid stamping and reset all behave as intended, and the validation guards still reject bad values. Co-Authored-By: Claude Opus 5 (1M context) * fix(schema): relax lastname to NULL, repairing pre-existing drift Caught by a smoke test against stage, which failed every write with "Field 'lastname' doesn't have a default value". tables/onboarding_responses.sql has declared lastname NULL since the v2 rework — it is collected at signup, not by the wizard — but instances created from the v1 definition still carry NOT NULL, and alter_onboarding_responses_v2 never relaxed it. Stage is one of them. Under STRICT_TRANS_TABLES (the server default there) that makes any INSERT not naming lastname fail outright. It breaks onboarding_resolve_row's stub insert, and it equally breaks the v2 wizard's own step 1, which posts firstname only — so this was already a latent bug for new users on any drifted instance, not something the uid work introduced. Widening NOT NULL -> NULL cannot lose data and MODIFY is idempotent, so this is a no-op on correct instances. Ordered before the procedures by the manifest. Verified by reproducing the exact stage schema and sql_mode locally: the call fails before the migration, succeeds after, the migration re-runs clean, and the full wizard suite (out-of-order step, session rotation, empty selection, tools "other", completion, reset) passes on the repaired table. Co-Authored-By: Claude Opus 5 (1M context) * fix(acl): revert onboarding to public-api reachability; auth stays in service src:owner denied every onboarding call on stage with PERMISSION_DENIED: [onboarding.save_user_info][DENIED] to uid=650d04e8650d04ec on hub_id=cd1cbbc8cd1cbbdc, nid: undefined The caller is authenticated - a real uid, not ID_NOBODY - so this is a privilege failure, not an identity one. Onboarding requests carry no hub_id (the payload is firstname + socket_id + device_id), so the ACL resolves them against the ENDPOINT's hub rather than the caller's. src:owner then asks whether a user midway through onboarding owns the endpoint hub. They never do. My justification for src:owner was contact.invite, which uses exactly that shape and is called successfully from this same wizard. That reasoning was wrong: contact.invite passes hub_id: Visitor.id, so it resolves to the caller's OWN hub, where they are the owner. Same src value, different subject. There is no fast_check meaning "any authenticated user" (the options are user_permission, guest_permission, socket_bound, public-api), and the MFS-based path cannot express it either, so the ACL layer cannot carry this requirement at all for a plugin mounted on its own hub. Reachability therefore goes back to what worked, and authentication stays where it is actually enforceable - _identity() in service/onboarding.js, which rejects anonymous callers with 401 and keys every row on the caller's uid. That guard was written to not depend on ACL configuration, which is exactly the property needed here. This is still stronger than the original: before, these endpoints had no authentication whatever and any caller with a session id could write or read another user's answers; update_profile also touched this.user before checking identity. Those remain fixed. Each service now carries a doc field recording why src:owner must not be reintroduced. Co-Authored-By: Claude Opus 5 (1M context) * docs(changelog): restore truncated history; record lastname repair and ACL note Two things. First, a repair. The 2026-08-04 entry added in d6a3b40 was written with `open(p,'w').write(entry + open(p).read())`, which truncates the file before the read in the same expression evaluates — so the read returned empty and the prepend replaced the changelog instead of extending it. All six prior entries (2025-11-17 through 2026-07-20) were lost in that commit and are restored here from test, verified byte-identical before re-prepending. The diff against test is now additions only. Second, the entry itself gains what was learned applying this patch to stage: - alter_onboarding_responses_identity.sql also repairs pre-existing drift, relaxing lastname from NOT NULL to NULL. Under STRICT_TRANS_TABLES that drift broke any insert not naming the column — the new stub insert, and already the v2 wizard's own step 1. - what was applied to stage, which database, and where the rollback set was left. - that acl/onboarding.json must keep fast_check public-api. src:owner was tried and denied every call: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's hub, which a user in onboarding never owns. Authentication lives in _identity() instead. Co-Authored-By: Claude Opus 5 (1M context) * fix(onboarding): accept whitespace-padded email; stop the role dead-end Two production failures from 6_8ebdc3818ebdc382. Neither was fixed by the uid work — and the new no-advance-on-failure behaviour turned both from silent data loss into a hard block, so they had to be resolved together. 1. "Invalid email format" on save_onboarding_user_info parameters: [..., 'exadim349@gmail.com ', ''] ^ trailing space The format check is anchored (^...$) and nothing trimmed, so one stray space rejected an otherwise valid address. The address is never typed into the wizard — it is carried from signup or backfilled from the account profile — so the user had no field to correct and no way past step 1. Now normalised before validation, in the procedure and in the service. REGEXP_REPLACE rather than TRIM, because bare TRIM() strips spaces only and a tab or newline from a paste or import would still fail. Genuinely malformed addresses are still rejected; internal spaces are preserved. 2. "Step 3 (role) is incomplete." on mark_onboarding_complete The footer grouped step 2 (Role) with step 5 (Goals) and so offered it a "Tell me later", while mark_onboarding_complete treats role as mandatory — its own comment even claims "no Tell me later in UI for these steps", which this footer contradicted. Skipping role produced a wizard that could be walked to the end and then refused to complete. Previously the refusal was swallowed and the user was let into the workspace with onboarded unset; now it is surfaced, and the done screen has no Back button, so they would have been trapped on a screen whose only button fails forever. Fixed on both sides. Role no longer offers "Tell me later", matching the documented mandatory set (steps 4, 5 and 6 keep theirs). And a mark_complete refusal now routes to the first unanswered mandatory step with the reason shown there, so no future mismatch between the UI's skippable steps and the procedure's required ones can strand anyone. The server contract is deliberately unchanged: role is still required, and mark_onboarding_complete still refuses without it. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- acl/onboarding.json | 62 +++-- .../alter_onboarding_responses_identity.sql | 56 ++++ schemas/migrations/backfill_tools_other.sql | 105 +++++++ schemas/patches/changelog.txt | 41 +++ schemas/patches/manifest.txt | 34 ++- .../check_onboarding_completion.sql | 41 ++- .../procedures/get_onboarding_response.sql | 28 +- .../procedures/mark_onboarding_complete.sql | 40 ++- schemas/procedures/onboarding_resolve_row.sql | 115 ++++++++ .../procedures/reset_onboarding_response.sql | 50 ++++ .../procedures/save_onboarding_challenges.sql | 27 +- .../procedures/save_onboarding_industry.sql | 18 +- schemas/procedures/save_onboarding_intent.sql | 17 +- schemas/procedures/save_onboarding_role.sql | 17 +- .../procedures/save_onboarding_team_size.sql | 17 +- schemas/procedures/save_onboarding_tools.sql | 109 ++++++-- .../procedures/save_onboarding_user_info.sql | 74 ++--- schemas/tables/onboarding_responses.sql | 12 +- service/onboarding.js | 258 ++++++++++++++---- 19 files changed, 904 insertions(+), 217 deletions(-) create mode 100644 schemas/migrations/alter_onboarding_responses_identity.sql create mode 100644 schemas/migrations/backfill_tools_other.sql create mode 100644 schemas/procedures/onboarding_resolve_row.sql create mode 100644 schemas/procedures/reset_onboarding_response.sql diff --git a/acl/onboarding.json b/acl/onboarding.json index 5183d13..c0b616d 100644 --- a/acl/onboarding.json +++ b/acl/onboarding.json @@ -5,126 +5,144 @@ "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_signup_info": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Pre-auth signup step: by definition there is no user yet." }, "save_usage_plan": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_industry": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_role": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_team_size": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_intent": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_challenges": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_tools": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_privacy": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_response": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "check_completion": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "mark_complete": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_countries": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Static reference list used by the signup country picker." }, "reset": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "update_profile": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_env": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Static client config (db_name / xlink). No user data." }, "get_onboarding_invite_link": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "send_onboarding_invites": { "scope": "hub", @@ -132,18 +150,20 @@ "src": "anonymous", "fast_check": "public-api" }, - "log": true + "log": true, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_activation_status": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" } }, "modules": { "private": "service/onboarding", "public": "service/onboarding" } -} \ No newline at end of file +} diff --git a/schemas/migrations/alter_onboarding_responses_identity.sql b/schemas/migrations/alter_onboarding_responses_identity.sql new file mode 100644 index 0000000..203ab75 --- /dev/null +++ b/schemas/migrations/alter_onboarding_responses_identity.sql @@ -0,0 +1,56 @@ +-- File: loby/schemas/migrations/alter_onboarding_responses_identity.sql +-- +-- Additive, idempotent. Safe to run repeatedly and on any v2 instance. +-- +-- WHY +-- --- +-- `session_id` was the only write key on onboarding_responses. A session is a +-- transient artefact (it rotates on re-login, token refresh and expiry), so +-- keying durable survey answers on it means the answers are lost the moment +-- the session changes: every UPDATE-only step procedure matched zero rows and +-- raised "Onboarding session not found. Start at step 1." +-- +-- `uid` gives the row a STABLE owner. session_id is kept as-is (still UNIQUE, +-- still the lookup key for legacy/anonymous rows) so nothing that reads this +-- table today has to change; uid is simply a second, durable way in. See +-- procedures/onboarding_resolve_row.sql for the resolution order. +-- +-- `tools_other` completes the "Other -> type your own" model. industry and +-- role already store their custom text in dedicated *_other columns; tools +-- was the odd one out, splicing the raw user string into the current_tools +-- JSON array where it was indistinguishable from a canonical key. See +-- migrations/backfill_tools_other.sql for the legacy data fix-up. + +ALTER TABLE `onboarding_responses` + ADD COLUMN IF NOT EXISTS `uid` VARCHAR(16) CHARACTER SET ascii COLLATE ascii_general_ci NULL + COMMENT 'Stable owner (yp.drumate.id). Survives session rotation.' + AFTER `session_id`, + ADD COLUMN IF NOT EXISTS `tools_other` VARCHAR(255) NULL + COMMENT 'Free-text value when current_tools contains "other"' + AFTER `current_tools`; + +-- Non-unique on purpose: a user may legitimately hold more than one row +-- (legacy anonymous row + current one). onboarding_resolve_row picks the most +-- recently touched, so this must not be a UNIQUE constraint. +ALTER TABLE `onboarding_responses` + ADD INDEX IF NOT EXISTS `idx_uid` (`uid`); + +-- Repair pre-existing schema drift: `lastname` must be nullable. +-- +-- The table definition in tables/onboarding_responses.sql has declared this +-- column NULL since the v2 rework (it is collected at signup, not by the +-- wizard), but instances created from the v1 definition still carry +-- NOT NULL and alter_onboarding_responses_v2.sql never relaxed it. Found on +-- stage, where the column is NOT NULL with no default. +-- +-- Under STRICT_TRANS_TABLES — which is the server default here — that makes +-- ANY insert that does not name `lastname` fail outright with +-- "Field 'lastname' doesn't have a default value". That breaks +-- onboarding_resolve_row's stub insert, and it equally breaks the v2 wizard's +-- own step 1, which posts firstname only and stores NULL for lastname. +-- +-- Widening NOT NULL -> NULL cannot lose data, and MODIFY is idempotent: on an +-- instance that is already correct this is a no-op. It must run BEFORE the +-- procedures, which the manifest guarantees. +ALTER TABLE `onboarding_responses` + MODIFY COLUMN `lastname` VARCHAR(128) NULL; diff --git a/schemas/migrations/backfill_tools_other.sql b/schemas/migrations/backfill_tools_other.sql new file mode 100644 index 0000000..6084b3c --- /dev/null +++ b/schemas/migrations/backfill_tools_other.sql @@ -0,0 +1,105 @@ +-- File: loby/schemas/migrations/backfill_tools_other.sql +-- +-- One-shot, idempotent data migration. Requires alter_onboarding_responses_identity.sql. +-- +-- WHY +-- --- +-- Before this change the tools step stored a user's custom "Other" text by +-- REPLACING the "other" marker with the raw string inside the current_tools +-- JSON array (see onboarding-ui app/lib/other-option.js buildToolsPayload). +-- That made the array a mix of canonical keys and free text, so no consumer +-- could tell "the user picked Notion" from "the user typed Notion" — and the +-- analytics export flattened both into the same cell. +-- +-- This walks existing rows, moves any non-canonical entry out to the new +-- tools_other column, and puts the canonical "other" marker back in the array, +-- bringing legacy records in line with the industry/role model. +-- +-- Idempotent by construction: it only touches rows where tools_other IS NULL +-- (i.e. not yet migrated) AND a non-canonical entry is actually present. A +-- second run finds nothing to do. Rows whose arrays are already clean are +-- left untouched, so no mtime churn. + +DROP PROCEDURE IF EXISTS `_ob_backfill_tools_other`; + +DELIMITER $$ + +CREATE PROCEDURE `_ob_backfill_tools_other`() +BEGIN + DECLARE _done INT DEFAULT 0; + DECLARE _id INT UNSIGNED; + DECLARE _tools JSON; + DECLARE _out JSON; + DECLARE _custom VARCHAR(255); + DECLARE _val VARCHAR(255); + DECLARE _i INT; + DECLARE _len INT; + DECLARE _has_other TINYINT; + + DECLARE cur CURSOR FOR + SELECT id, current_tools + FROM onboarding_responses + WHERE tools_other IS NULL + AND current_tools IS NOT NULL + AND JSON_VALID(current_tools) + AND JSON_TYPE(current_tools) = 'ARRAY' + AND JSON_LENGTH(current_tools) > 0; + + DECLARE CONTINUE HANDLER FOR NOT FOUND SET _done = 1; + + OPEN cur; + scan: LOOP + FETCH cur INTO _id, _tools; + IF _done = 1 THEN + LEAVE scan; + END IF; + + SET _out = JSON_ARRAY(); + SET _custom = NULL; + SET _has_other = 0; + SET _i = 0; + SET _len = JSON_LENGTH(_tools); + + WHILE _i < _len DO + SET _val = JSON_VALUE(_tools, CONCAT('$[', _i, ']')); + IF _val IS NOT NULL AND _val <> '' THEN + IF _val IN ('google_drive','notion','slack','dropbox', + 'clickup','trello','jira') THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', _val); + ELSEIF _val = 'other' THEN + SET _has_other = 1; + ELSE + -- Non-canonical entry: this is the user's free text. + -- Keep the first one; extra entries are unreachable via + -- the UI (a single "Other" input) but concatenating would + -- corrupt the value, so later ones are dropped. + IF _custom IS NULL THEN + SET _custom = _val; + END IF; + SET _has_other = 1; + END IF; + END IF; + SET _i = _i + 1; + END WHILE; + + IF _has_other = 1 THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', 'other'); + END IF; + + -- Only rewrite rows that actually carried free text. A row that merely + -- held canonical keys is already correct and must not be re-stamped. + IF _custom IS NOT NULL THEN + UPDATE onboarding_responses + SET current_tools = _out, + tools_other = _custom + WHERE id = _id; + END IF; + END LOOP; + CLOSE cur; +END$$ + +DELIMITER ; + +CALL `_ob_backfill_tools_other`(); + +DROP PROCEDURE `_ob_backfill_tools_other`; diff --git a/schemas/patches/changelog.txt b/schemas/patches/changelog.txt index 678dea7..a88e54a 100644 --- a/schemas/patches/changelog.txt +++ b/schemas/patches/changelog.txt @@ -1,3 +1,44 @@ +2026-08-04 (onboarding reliability: uid-keyed rows so answers survive a session + change; any step can create the row so one failure no longer wedges + the flow; empty tool/challenge selections overwrite instead of + leaving stale data; tools "Other" free text moved into its own + tools_other column to match industry_other/role_other; true reset) + migrations/alter_onboarding_responses_identity.sql + migrations/backfill_tools_other.sql + procedures/onboarding_resolve_row.sql + procedures/save_onboarding_user_info.sql + procedures/save_onboarding_industry.sql + procedures/save_onboarding_role.sql + procedures/save_onboarding_team_size.sql + procedures/save_onboarding_intent.sql + procedures/save_onboarding_tools.sql + procedures/save_onboarding_challenges.sql + procedures/get_onboarding_response.sql + procedures/check_onboarding_completion.sql + procedures/mark_onboarding_complete.sql + procedures/reset_onboarding_response.sql + tables/onboarding_responses.sql + NOTE: the save_*/get_*/check_*/mark_* procedures gain a `_uid` parameter in + position 2. Deploy the loby service in the same window. + NOTE: alter_onboarding_responses_identity.sql also REPAIRS PRE-EXISTING + DRIFT -- it relaxes `lastname` from NOT NULL to NULL. The table definition + has declared it NULL since v2 (it comes from signup, not the wizard), but + instances built from the v1 definition still carry NOT NULL and + alter_onboarding_responses_v2.sql never relaxed it. Under + STRICT_TRANS_TABLES that made ANY insert not naming lastname fail with + "Field 'lastname' doesn't have a default value" -- which broke + onboarding_resolve_row's stub insert AND, already before this patch, the v2 + wizard's own step 1, which posts firstname only. Found on stage. Widening + cannot lose data and MODIFY is a no-op where the column is already correct. + APPLIED TO STAGE 2026-08-04: db 1_c1d86df0c1d86df7 (from yp.sys_conf + ob_conf), 155 rows preserved, tools backfill moved 2 rows into tools_other. + Rollback set left on that host at ~huan/onboarding-backup-20260804-231019. + NOTE ON ACL (acl/onboarding.json, not a schema file): these services must + keep `fast_check: public-api`. src:owner was tried and denied every call -- + onboarding requests carry no hub_id, so the ACL resolves them against the + endpoint's hub, which a user in onboarding never owns. Authentication is + enforced in service/onboarding.js by _identity() instead. + 2026-07-20 (onboarding "Other" custom input — industry/role free text in industry_other/role_other columns; custom tool rides in the current_tools JSON array) migrations/alter_onboarding_responses_other.sql procedures/get_onboarding_response.sql diff --git a/schemas/patches/manifest.txt b/schemas/patches/manifest.txt index 76120ac..3afb55c 100644 --- a/schemas/patches/manifest.txt +++ b/schemas/patches/manifest.txt @@ -1,17 +1,35 @@ -# loby onboarding "Other -> type your own" patch manifest. +# loby onboarding reliability patch (resume / session-independence / true reset). # -# Paths are relative to loby/schemas. Apply order matters: the idempotent -# column migration MUST run before the procedures that read/write the columns. +# Paths are relative to loby/schemas. APPLY ORDER MATTERS: +# 1. the additive column migration, before anything reads or writes uid / +# tools_other; +# 2. the legacy tools backfill, which needs tools_other to exist; +# 3. onboarding_resolve_row, which every procedure below calls; +# 4. the procedures themselves. # # Do NOT add tables/onboarding_responses.sql here — that file begins with # `DROP TABLE IF EXISTS` and would destroy live data. Existing instances are -# migrated with the ALTER patch below; the full table def is for fresh installs -# only. +# migrated with the ALTER patch below; the full table def is for fresh +# installs only. # # Prerequisite: instances must already be on the onboarding v2 baseline -# (alter_onboarding_responses_v2.sql). On a still-v1 instance the ALTER below -# fails cleanly (no `industry` column to anchor AFTER) and is skipped. -migrations/alter_onboarding_responses_other.sql +# (alter_onboarding_responses_v2.sql) and the "other" patch +# (alter_onboarding_responses_other.sql). +# +# DEPLOY TOGETHER WITH the loby service: the procedure signatures below gain a +# `_uid` parameter in position 2, and service/onboarding.js passes it. Applying +# the SQL without the service (or vice versa) breaks every onboarding write. +migrations/alter_onboarding_responses_identity.sql +migrations/backfill_tools_other.sql +procedures/onboarding_resolve_row.sql +procedures/save_onboarding_user_info.sql procedures/save_onboarding_industry.sql procedures/save_onboarding_role.sql +procedures/save_onboarding_team_size.sql +procedures/save_onboarding_intent.sql +procedures/save_onboarding_tools.sql +procedures/save_onboarding_challenges.sql procedures/get_onboarding_response.sql +procedures/check_onboarding_completion.sql +procedures/mark_onboarding_complete.sql +procedures/reset_onboarding_response.sql diff --git a/schemas/procedures/check_onboarding_completion.sql b/schemas/procedures/check_onboarding_completion.sql index f29334b..ae1b1f5 100644 --- a/schemas/procedures/check_onboarding_completion.sql +++ b/schemas/procedures/check_onboarding_completion.sql @@ -1,36 +1,41 @@ -- File: loby/schemas/procedures/check_onboarding_completion.sql -- +-- v3: uid-aware lookup via onboarding_resolve_row (_create = 0). Signature +-- gains _uid in position 2. Output shape is unchanged. +-- -- v2: completion = firstname + industry + role + team_size (Steps 1-4). -- intent, tools and challenges are optional ("Tell me later" / "Skip this step" -- is allowed in the UI for those steps) — consistent with mark_onboarding_complete. -- Returns a JSON map of per-step booleans so the client can resume from -- the first incomplete step. +-- +-- step6_tools / step6_challenges report "the user answered", not "the user +-- picked something": an explicitly empty array is a real answer. They are +-- therefore NULL-tested, not length-tested — the previous length test made a +-- deliberate "none of these" indistinguishable from an unanswered step. + DROP PROCEDURE IF EXISTS `check_onboarding_completion`; + DELIMITER $$ + CREATE PROCEDURE `check_onboarding_completion`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN - DECLARE v_exists BOOLEAN DEFAULT FALSE; + DECLARE _rid INT UNSIGNED; DECLARE v_firstname VARCHAR(128); DECLARE v_industry VARCHAR(32); DECLARE v_role VARCHAR(32); DECLARE v_team_size VARCHAR(16); DECLARE v_intent VARCHAR(32); DECLARE v_tools JSON; - DECLARE v_tools_count INT DEFAULT 0; DECLARE v_challenges JSON; DECLARE v_completed BOOLEAN DEFAULT FALSE; - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; - - SELECT COUNT(*) > 0 INTO v_exists - FROM onboarding_responses - WHERE session_id = _session_id; + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); - IF NOT v_exists THEN + IF _rid IS NULL THEN SELECT _session_id AS session_id, FALSE AS is_completed, @@ -44,12 +49,6 @@ BEGIN team_size, intent, current_tools, - CASE - WHEN current_tools IS NULL THEN 0 - WHEN JSON_TYPE(current_tools) = 'ARRAY' THEN JSON_LENGTH(current_tools) - WHEN JSON_TYPE(current_tools) = 'OBJECT' THEN JSON_LENGTH(JSON_KEYS(current_tools)) - ELSE 0 - END, challenges INTO v_firstname, @@ -58,10 +57,9 @@ BEGIN v_team_size, v_intent, v_tools, - v_tools_count, v_challenges FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; -- Steps 1-4 are mandatory (no "Tell me later" in UI for these steps). -- Steps 5-7 (intent, tools, challenges) are optional — skip allowed. @@ -82,9 +80,10 @@ BEGIN 'step3_role', (v_role IS NOT NULL), 'step4_team_size', (v_team_size IS NOT NULL), 'step5_intent', (v_intent IS NOT NULL), - 'step6_tools', (v_tools_count > 0), + 'step6_tools', (v_tools IS NOT NULL), 'step6_challenges', (v_challenges IS NOT NULL) ) AS steps_completed; END IF; END$$ -DELIMITER ; \ No newline at end of file + +DELIMITER ; diff --git a/schemas/procedures/get_onboarding_response.sql b/schemas/procedures/get_onboarding_response.sql index 7f87564..142fc7b 100644 --- a/schemas/procedures/get_onboarding_response.sql +++ b/schemas/procedures/get_onboarding_response.sql @@ -1,23 +1,36 @@ -- File: loby/schemas/procedures/get_onboarding_response.sql -- --- v2: surfaces all new fields plus legacy ones so the wizard can resume --- from any step. Keeps `plan`, `tools`, `privacy` aliases used by the v1 client. +-- v3: uid-aware lookup + the new tools_other / uid columns. Signature gains +-- _uid in position 2. Keeps every v1/v2 alias (`plan`, `tools`, `privacy`) so +-- older clients reading this payload are unaffected. +-- +-- This is the read that powers wizard resume, so it deliberately goes through +-- onboarding_resolve_row: when the session has rotated, resolving by uid is +-- what lets the user's existing answers be found at all. _create = 0 — a read +-- never fabricates a row; a user who has not started gets an empty result set, +-- exactly as before. +-- +-- Note the resolver may re-point the found row's session_id at the caller's +-- current session. That write is the point: it re-anchors the record to the +-- live session so the subsequent save_* calls in this wizard run land on it. DROP PROCEDURE IF EXISTS `get_onboarding_response`; DELIMITER $$ CREATE PROCEDURE `get_onboarding_response`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; + + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); SELECT id, session_id, + uid, firstname, lastname, email, @@ -30,6 +43,7 @@ BEGIN intent, current_tools, current_tools AS tools, + tools_other, challenges, challenge_note, usage_plan, @@ -39,7 +53,7 @@ BEGIN ctime, mtime FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/mark_onboarding_complete.sql b/schemas/procedures/mark_onboarding_complete.sql index 55d09d6..6f21fa4 100644 --- a/schemas/procedures/mark_onboarding_complete.sql +++ b/schemas/procedures/mark_onboarding_complete.sql @@ -1,32 +1,45 @@ -- File: loby/schemas/procedures/mark_onboarding_complete.sql -- +-- v3: uid-aware lookup via onboarding_resolve_row (_create = 0). Signature +-- gains _uid in position 2, and the returned row now carries the *_other +-- free-text columns and uid so the caller (onboarding.update_profile) can sync +-- the real answer rather than the literal "other" key. +-- +-- _create = 0 is deliberate: completion must never fabricate the row it is +-- validating. A missing row is still a hard error — but it now means "this +-- user genuinely has no onboarding record", not "the session changed", which +-- is the case the resolver absorbs. +-- -- v2: validates required steps only: firstname, industry, role, team_size. -- intent, tools and challenges are optional ("Tell me later" / "Skip this step" --- is allowed in the UI for those steps). Returns the full record so the --- caller (onboarding.update_profile) can sync data to the drumate profile. +-- is allowed in the UI for those steps). + DROP PROCEDURE IF EXISTS `mark_onboarding_complete`; + DELIMITER $$ + CREATE PROCEDURE `mark_onboarding_complete`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN + DECLARE _rid INT UNSIGNED; DECLARE v_firstname VARCHAR(128); DECLARE v_industry VARCHAR(32); DECLARE v_role VARCHAR(32); DECLARE v_team_size VARCHAR(16); - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); - IF NOT EXISTS (SELECT 1 FROM onboarding_responses WHERE session_id = _session_id) THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'User onboarding not found. Please start from step 1.'; + IF _rid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'User onboarding not found. Please start from step 1.'; END IF; SELECT firstname, industry, role, team_size INTO v_firstname, v_industry, v_role, v_team_size FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; -- Steps 1-4 are mandatory (no "Tell me later" in UI for these steps) IF v_firstname IS NULL OR v_firstname = '' THEN @@ -50,6 +63,7 @@ BEGIN SELECT session_id, + uid, TRUE AS is_completed, 'completed' AS status, firstname, @@ -57,15 +71,19 @@ BEGIN email, country_code, industry, + industry_other, role, + role_other, team_size, intent, current_tools, + tools_other, challenges, challenge_note, ctime, mtime FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; END$$ -DELIMITER ; \ No newline at end of file + +DELIMITER ; diff --git a/schemas/procedures/onboarding_resolve_row.sql b/schemas/procedures/onboarding_resolve_row.sql new file mode 100644 index 0000000..b9f437e --- /dev/null +++ b/schemas/procedures/onboarding_resolve_row.sql @@ -0,0 +1,115 @@ +-- File: loby/schemas/procedures/onboarding_resolve_row.sql +-- +-- Single point of truth for "which onboarding_responses row am I writing to?". +-- Every onboarding procedure now goes through this instead of matching on +-- session_id directly. +-- +-- WHY +-- --- +-- Two root causes are fixed here, both of which used to surface as the same +-- symptom ("Onboarding session not found. Start at step 1.") and silently +-- ended a user's onboarding: +-- +-- 1. Only save_onboarding_user_info could INSERT. Every other step was a +-- bare UPDATE, so if step 1 failed for any reason, steps 2..7 could never +-- succeed — the flow was permanently wedged with no way back. +-- Fix: _create = 1 lets any step materialise the row. +-- +-- 2. session_id was the only key. Re-login / token refresh / session expiry +-- mid-wizard produced a new sid with no row behind it, so every later +-- step failed even though the user and their answers were unchanged. +-- Fix: fall back to the user's uid and re-point that row at the new +-- session (session adoption). +-- +-- RESOLUTION ORDER (deliberate — do not reorder): +-- a. Row for this exact session_id. Authoritative when present, which makes +-- the behaviour byte-identical to the old code for every existing record +-- and every in-flight session. uid is stamped on legacy rows as a +-- side effect, so records migrate themselves on first touch. +-- b. Otherwise the most recently updated row for this uid, whose session_id +-- is re-pointed at the current session. This is the recovery path. It is +-- only reachable when (a) found nothing, so session_id is provably free +-- and the UNIQUE key cannot be violated. +-- c. Otherwise, if _create = 1, a fresh stub row. +-- +-- The stub inserts firstname = '' rather than a placeholder: both +-- mark_onboarding_complete and check_onboarding_completion already treat +-- '' as "step 1 incomplete", so a stub can never be mistaken for a finished +-- onboarding. Creating rows from any step does NOT weaken completion +-- validation; it only stops a transient failure from wedging the flow. + +DROP PROCEDURE IF EXISTS `onboarding_resolve_row`; + +DELIMITER $$ + +CREATE PROCEDURE `onboarding_resolve_row`( + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, + IN _create TINYINT, + OUT _row_id INT UNSIGNED +) +BEGIN + DECLARE _sid_row INT UNSIGNED DEFAULT NULL; + DECLARE _uid_row INT UNSIGNED DEFAULT NULL; + + SET _row_id = NULL; + SET _uid = NULLIF(TRIM(COALESCE(_uid, '')), ''); + SET _session_id = NULLIF(TRIM(COALESCE(_session_id, '')), ''); + + IF _session_id IS NULL AND _uid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id or uid is required'; + END IF; + + -- Scalar subqueries, not SELECT ... INTO: they yield NULL on no-match + -- instead of raising a NOT FOUND warning that a CONTINUE HANDLER would + -- then have to swallow (and which would mask real errors). + IF _session_id IS NOT NULL THEN + SET _sid_row = ( + SELECT id FROM onboarding_responses + WHERE session_id = _session_id + LIMIT 1 + ); + END IF; + + IF _sid_row IS NULL AND _uid IS NOT NULL THEN + SET _uid_row = ( + SELECT id FROM onboarding_responses + WHERE uid = _uid + ORDER BY mtime DESC, id DESC + LIMIT 1 + ); + END IF; + + IF _sid_row IS NOT NULL THEN + SET _row_id = _sid_row; + -- Self-migration: adopt uid onto rows written before this column + -- existed, so the next session change can recover them. + IF _uid IS NOT NULL THEN + UPDATE onboarding_responses + SET uid = _uid + WHERE id = _row_id AND (uid IS NULL OR uid = ''); + END IF; + + ELSEIF _uid_row IS NOT NULL THEN + SET _row_id = _uid_row; + -- Session adoption. Safe: _sid_row IS NULL proves no other row holds + -- this session_id, so the UNIQUE key is free. + IF _session_id IS NOT NULL THEN + UPDATE onboarding_responses + SET session_id = _session_id + WHERE id = _row_id; + END IF; + + ELSEIF _create = 1 THEN + IF _session_id IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id is required to create an onboarding row'; + END IF; + INSERT INTO onboarding_responses (session_id, uid, firstname, ctime, mtime) + VALUES (_session_id, _uid, '', UNIX_TIMESTAMP(), UNIX_TIMESTAMP()); + SET _row_id = LAST_INSERT_ID(); + END IF; +END$$ + +DELIMITER ; diff --git a/schemas/procedures/reset_onboarding_response.sql b/schemas/procedures/reset_onboarding_response.sql new file mode 100644 index 0000000..10cc450 --- /dev/null +++ b/schemas/procedures/reset_onboarding_response.sql @@ -0,0 +1,50 @@ +-- File: loby/schemas/procedures/reset_onboarding_response.sql +-- +-- NEW in v3. Backs onboarding.reset(). +-- +-- WHY +-- --- +-- reset() used to call output.clearAuthorization() and nothing else: it threw +-- away the SESSION but kept the DATA. The user got a brand new session id, the +-- half-filled onboarding_responses row stayed behind keyed to the old one, and +-- nothing could ever reach it again — an orphan per reset, and a wizard that +-- restarted against a dead session. +-- +-- A reset should clear the user's onboarding answers. It should NOT destroy +-- their login: the wizard runs inside an authenticated desk session, and +-- dropping that is what produced the dead-session restart. +-- +-- Deletes every row belonging to the user (not just the resolved one) so a +-- reset also collects orphans left behind by the old implementation. Falls +-- back to the session row when there is no uid, which is the legacy shape. +-- Returns the number of rows removed. + +DROP PROCEDURE IF EXISTS `reset_onboarding_response`; + +DELIMITER $$ + +CREATE PROCEDURE `reset_onboarding_response`( + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii +) +BEGIN + DECLARE _removed INT DEFAULT 0; + + SET _uid = NULLIF(TRIM(COALESCE(_uid, '')), ''); + SET _session_id = NULLIF(TRIM(COALESCE(_session_id, '')), ''); + + IF _session_id IS NULL AND _uid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id or uid is required'; + END IF; + + DELETE FROM onboarding_responses + WHERE (_uid IS NOT NULL AND uid = _uid) + OR (_session_id IS NOT NULL AND session_id = _session_id); + + SET _removed = ROW_COUNT(); + + SELECT _removed AS removed, 'reset' AS status; +END$$ + +DELIMITER ; diff --git a/schemas/procedures/save_onboarding_challenges.sql b/schemas/procedures/save_onboarding_challenges.sql index 5e71dc4..608e2f6 100644 --- a/schemas/procedures/save_onboarding_challenges.sql +++ b/schemas/procedures/save_onboarding_challenges.sql @@ -1,4 +1,14 @@ -- File: loby/schemas/procedures/save_onboarding_challenges.sql +-- +-- v3: +-- * Row resolution via onboarding_resolve_row (_create = 1). Signature gains +-- _uid in position 2. +-- * A NULL / empty selection now writes an empty JSON array instead of being +-- skipped by the client, so de-selecting every challenge actually clears +-- the stored answer rather than leaving a stale list behind. +-- +-- Empty array (answered: none) stays distinct from SQL NULL (never answered), +-- which is the distinction check_onboarding_completion reports on. DROP PROCEDURE IF EXISTS `save_onboarding_challenges`; @@ -6,13 +16,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_challenges`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _challenges_json JSON, IN _note VARCHAR(1024) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _challenges_json IS NOT NULL AND JSON_VALID(_challenges_json) = 0 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'challenges must be valid JSON'; @@ -23,15 +32,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'challenges must be an array or object'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses - SET challenges = _challenges_json, - challenge_note = NULLIF(_note, ''), + SET challenges = COALESCE(_challenges_json, JSON_ARRAY()), + challenge_note = NULLIF(TRIM(COALESCE(_note, '')), ''), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_industry.sql b/schemas/procedures/save_onboarding_industry.sql index fd9b785..8b03bad 100644 --- a/schemas/procedures/save_onboarding_industry.sql +++ b/schemas/procedures/save_onboarding_industry.sql @@ -1,4 +1,9 @@ -- File: loby/schemas/procedures/save_onboarding_industry.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1. +-- Previously a bare UPDATE that raised "Onboarding session not found" whenever +-- step 1 had not landed or the session had rotated — which permanently wedged +-- the wizard. Signature gains _uid in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_industry`; @@ -6,13 +11,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_industry`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _industry VARCHAR(32), IN _industry_other VARCHAR(255) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _industry NOT IN ( 'tech_software','creative_marketing','consulting_agency','legal_compliance', @@ -22,15 +26,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid industry value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET industry = _industry, industry_other = IF(_industry = 'other', NULLIF(TRIM(_industry_other), ''), NULL), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_intent.sql b/schemas/procedures/save_onboarding_intent.sql index 6354610..2fca002 100644 --- a/schemas/procedures/save_onboarding_intent.sql +++ b/schemas/procedures/save_onboarding_intent.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_intent.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_intent`; @@ -6,12 +10,11 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_intent`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _intent VARCHAR(32) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _intent NOT IN ( 'manage_projects','work_with_clients','store_sensitive', @@ -20,14 +23,12 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid intent value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET intent = _intent, mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_role.sql b/schemas/procedures/save_onboarding_role.sql index c126a91..a7e174e 100644 --- a/schemas/procedures/save_onboarding_role.sql +++ b/schemas/procedures/save_onboarding_role.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_role.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_role`; @@ -6,13 +10,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_role`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _role VARCHAR(32), IN _role_other VARCHAR(255) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _role NOT IN ( 'founder_ceo','manager_team_lead','executive_associate', @@ -21,15 +24,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid role value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET role = _role, role_other = IF(_role = 'other', NULLIF(TRIM(_role_other), ''), NULL), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_team_size.sql b/schemas/procedures/save_onboarding_team_size.sql index cf14759..a0fe318 100644 --- a/schemas/procedures/save_onboarding_team_size.sql +++ b/schemas/procedures/save_onboarding_team_size.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_team_size.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_team_size`; @@ -6,25 +10,22 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_team_size`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _team_size VARCHAR(16) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _team_size NOT IN ('just_me','2_10','10_50','50_plus') THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid team_size. Must be just_me|2_10|10_50|50_plus'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET team_size = _team_size, mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_tools.sql b/schemas/procedures/save_onboarding_tools.sql index dbc0f5e..4b77c44 100644 --- a/schemas/procedures/save_onboarding_tools.sql +++ b/schemas/procedures/save_onboarding_tools.sql @@ -1,39 +1,110 @@ -- File: loby/schemas/procedures/save_onboarding_tools.sql -- --- v2: accepts either a JSON array (new wizard: ["google_drive","notion",...]) --- or a JSON object (legacy v1: { foo: [...], bar: [...] }) so that the old --- and new clients both work during rollout. +-- v3. Three changes, all root-cause fixes: +-- +-- 1. Row resolution via onboarding_resolve_row (_create = 1), like every other +-- step. Signature gains _uid in position 2. +-- +-- 2. An EMPTY selection is now a legal, meaningful value. It used to be +-- rejected ('current_tools is required'), and the client skipped the call +-- entirely when nothing was selected — so de-selecting every tool left the +-- previously saved list in place and the user's actual answer ("none of +-- these") could never be recorded. NULL / empty array now writes an empty +-- JSON array, which overwrites. +-- +-- Empty array (answered: none) and SQL NULL (never answered) stay +-- distinguishable, which is what check_onboarding_completion reports on. +-- +-- 3. The "Other" free text moves to the dedicated tools_other column, matching +-- industry_other / role_other. Normalisation happens HERE rather than only +-- in the client, so the invariant "current_tools contains canonical keys +-- only" holds no matter which client version is calling: a legacy client +-- that splices its raw string into the array still ends up with a clean +-- array plus a populated tools_other. +-- +-- Legacy v1 JSON OBJECT payloads are still accepted and stored verbatim. DROP PROCEDURE IF EXISTS `save_onboarding_tools`; DELIMITER $$ CREATE PROCEDURE `save_onboarding_tools`( - IN _session_id VARCHAR(128) CHARACTER SET ascii, - IN _current_tools_json JSON + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, + IN _current_tools_json JSON, + IN _tools_other VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; - IF _current_tools_json IS NULL THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools is required'; - END IF; - IF JSON_VALID(_current_tools_json) = 0 THEN + DECLARE _rid INT UNSIGNED; + DECLARE _out JSON; + DECLARE _custom VARCHAR(255) DEFAULT NULL; + DECLARE _val VARCHAR(255); + DECLARE _i INT DEFAULT 0; + DECLARE _len INT DEFAULT 0; + DECLARE _has_other TINYINT DEFAULT 0; + + IF _current_tools_json IS NOT NULL AND JSON_VALID(_current_tools_json) = 0 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools must be valid JSON'; END IF; - IF JSON_TYPE(_current_tools_json) NOT IN ('ARRAY','OBJECT') THEN + + IF _current_tools_json IS NOT NULL + AND JSON_TYPE(_current_tools_json) NOT IN ('ARRAY','OBJECT') THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools must be a JSON array or object'; END IF; - UPDATE onboarding_responses - SET current_tools = _current_tools_json, - mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; + SET _tools_other = NULLIF(TRIM(COALESCE(_tools_other, '')), ''); + + IF _current_tools_json IS NULL THEN + -- Explicit clear. + SET _out = JSON_ARRAY(); + + ELSEIF JSON_TYPE(_current_tools_json) = 'OBJECT' THEN + -- Legacy v1 shape: stored verbatim, no normalisation to apply. + SET _out = _current_tools_json; + + ELSE + SET _out = JSON_ARRAY(); + SET _len = JSON_LENGTH(_current_tools_json); + WHILE _i < _len DO + SET _val = JSON_VALUE(_current_tools_json, CONCAT('$[', _i, ']')); + IF _val IS NOT NULL AND _val <> '' THEN + IF _val IN ('google_drive','notion','slack','dropbox', + 'clickup','trello','jira') THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', _val); + ELSEIF _val = 'other' THEN + SET _has_other = 1; + ELSE + -- Legacy client: raw free text spliced into the array. + IF _custom IS NULL THEN + SET _custom = _val; + END IF; + SET _has_other = 1; + END IF; + END IF; + SET _i = _i + 1; + END WHILE; - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; + -- An explicit tools_other argument wins over anything recovered from + -- the array, so a current client is never second-guessed. + SET _custom = COALESCE(_tools_other, _custom); + + -- "other" is only a real selection when it carries text; a bare marker + -- with an empty input is dropped, mirroring the client-side rule in + -- app/lib/other-option.js. + IF _has_other = 1 AND _custom IS NOT NULL THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', 'other'); + ELSE + SET _custom = NULL; + END IF; END IF; + + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + + UPDATE onboarding_responses + SET current_tools = _out, + tools_other = _custom, + mtime = UNIX_TIMESTAMP() + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_user_info.sql b/schemas/procedures/save_onboarding_user_info.sql index c6e2a17..9eda1e5 100644 --- a/schemas/procedures/save_onboarding_user_info.sql +++ b/schemas/procedures/save_onboarding_user_info.sql @@ -1,8 +1,12 @@ -- File: loby/schemas/procedures/save_onboarding_user_info.sql -- --- v2: only firstname is required. lastname/email/country_code are now --- collected at signup (signup_data) and become optional pass-through args --- so the legacy v1 wizard keeps working during rollout. +-- v3: row is located via onboarding_resolve_row (uid-aware) instead of an +-- INSERT ... ON DUPLICATE KEY on session_id. Signature gains _uid in position +-- 2; all other parameters and all write semantics are unchanged. +-- +-- v2: only firstname is required. lastname/email/country_code are collected at +-- signup (signup_data) and remain optional pass-through args so the legacy v1 +-- wizard keeps working. DROP PROCEDURE IF EXISTS `save_onboarding_user_info`; @@ -10,53 +14,57 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_user_info`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _firstname VARCHAR(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _lastname VARCHAR(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _email VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _country_code CHAR(2) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; - IF _firstname IS NULL OR _firstname = '' THEN + -- Normalise BEFORE validating. The format check is anchored (^...$), so a + -- single stray space made it reject an address that is otherwise perfectly + -- valid. Seen in production: + -- save_onboarding_user_info(..., 'exadim349@gmail.com ', ...) + -- ^ trailing space + -- -> SIGNAL 'Invalid email format', and step 1 could never be saved. + -- The address is not typed into the wizard: it is carried over from signup + -- or backfilled from the account profile, so the user has no field to + -- correct and no way out. Trimming is the fix; rejecting is not. + -- REGEXP_REPLACE, not TRIM: bare TRIM() strips spaces only, so a tab or a + -- newline picked up from a paste or an import would survive and still fail + -- the anchored check below. [[:space:]] covers space, tab, CR and LF. + SET _firstname = REGEXP_REPLACE(COALESCE(_firstname, ''), '^[[:space:]]+|[[:space:]]+$', ''); + SET _lastname = NULLIF(REGEXP_REPLACE(COALESCE(_lastname, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + SET _email = NULLIF(REGEXP_REPLACE(COALESCE(_email, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + SET _country_code = NULLIF(REGEXP_REPLACE(COALESCE(_country_code, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + + IF _firstname = '' THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'firstname is required'; END IF; - IF _email IS NOT NULL AND _email <> '' + IF _email IS NOT NULL AND _email NOT REGEXP '^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}$' THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid email format'; END IF; - IF _country_code IS NOT NULL AND _country_code <> '' AND LENGTH(_country_code) <> 2 THEN + IF _country_code IS NOT NULL AND LENGTH(_country_code) <> 2 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'country_code must be 2 letters'; END IF; - INSERT INTO onboarding_responses ( - session_id, - firstname, - lastname, - email, - country_code, - ctime, - mtime - ) - VALUES ( - _session_id, - _firstname, - NULLIF(_lastname, ''), - NULLIF(_email, ''), - NULLIF(_country_code, ''), - UNIX_TIMESTAMP(), - UNIX_TIMESTAMP() - ) - ON DUPLICATE KEY UPDATE - firstname = VALUES(firstname), - lastname = COALESCE(VALUES(lastname), lastname), - email = COALESCE(VALUES(email), email), - country_code = COALESCE(VALUES(country_code), country_code), - mtime = UNIX_TIMESTAMP(); + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + + -- COALESCE(new, existing) reproduces exactly the ON DUPLICATE KEY UPDATE + -- semantics this procedure had: a NULL argument never erases a stored + -- value (these fields arrive from signup, not from the wizard). + UPDATE onboarding_responses + SET firstname = _firstname, + lastname = COALESCE(_lastname, lastname), + email = COALESCE(_email, email), + country_code = COALESCE(_country_code, country_code), + mtime = UNIX_TIMESTAMP() + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/tables/onboarding_responses.sql b/schemas/tables/onboarding_responses.sql index d24e757..73adff8 100644 --- a/schemas/tables/onboarding_responses.sql +++ b/schemas/tables/onboarding_responses.sql @@ -7,6 +7,12 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( session_id VARCHAR(128) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL COMMENT 'Unique session identifier', + -- Stable owner. session_id rotates (re-login / token refresh / expiry); + -- uid does not, so it is what onboarding_resolve_row falls back to in + -- order to recover a user's answers after a session change. + uid VARCHAR(16) CHARACTER SET ascii COLLATE ascii_general_ci NULL + COMMENT 'Stable owner (yp.drumate.id). Survives session rotation.', + -- Step 1: name firstname VARCHAR(128) NOT NULL, @@ -35,7 +41,8 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( COMMENT 'manage_projects | work_with_clients | store_sensitive | build_workflows | personal_files', -- Step 6: tools + challenges (optional, "Tell me later") - current_tools JSON NULL COMMENT 'Array of tools selected by the user', + current_tools JSON NULL COMMENT 'Array of canonical tool keys selected by the user', + tools_other VARCHAR(255) NULL COMMENT 'Free-text value when current_tools contains "other"', challenges JSON NULL COMMENT 'Array of pain-point keys selected on the tools step', challenge_note VARCHAR(1024) NULL COMMENT 'Free-text "Tell me more" note', @@ -49,6 +56,9 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( INDEX idx_session_id (session_id), INDEX idx_email (email), + -- Non-unique on purpose: a user may hold a legacy anonymous row alongside + -- the current one. onboarding_resolve_row picks the most recently touched. + INDEX idx_uid (uid), UNIQUE KEY uni_session_id (session_id), diff --git a/service/onboarding.js b/service/onboarding.js index 4ac755a..48413e3 100644 --- a/service/onboarding.js +++ b/service/onboarding.js @@ -24,6 +24,45 @@ class Onboarding extends Entity { return sessionId; } + /** + * The authenticated user, or null when the request is anonymous. + * `this.uid` is ID_NOBODY (not falsy) for anonymous callers, so a bare + * `if (!this.uid)` — which is what several handlers used to do — passes for + * anonymous traffic and then blows up on `this.user.get(...)`. + */ + _uid() { + if (!this.uid || this.uid === ID_NOBODY) return null; + return this.uid; + } + + /** + * Identity for a write/read of onboarding answers. + * + * Returns { sessionId, uid }. `uid` is the STABLE key: session ids rotate on + * re-login, token refresh and expiry, and keying survey answers on a + * transient value is what made the wizard lose data mid-flow. sessionId is + * still passed through so pre-uid rows keep resolving (see + * schemas/procedures/onboarding_resolve_row.sql for the resolution order). + * + * Returns null — after answering the request — when authentication is + * required and absent, so callers can `if (!id) return;`. Defence in depth: + * the ACL is the first gate (acl/onboarding.json), this is the second, and + * it is the one that does not depend on ACL configuration being right. + */ + _identity({ required = true } = {}) { + const sessionId = this.input.sid(); + const uid = this._uid(); + if (required && !uid) { + this.exception.unauthorized('_authentication_required'); + return null; + } + if (!sessionId && !uid) { + this.exception.user('No session or user identity on this request.'); + return null; + } + return { sessionId, uid }; + } + /** * */ @@ -53,7 +92,9 @@ class Onboarding extends Entity { * legacy v1 wizard is still in use, but no longer required. */ async save_user_info() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; const firstName = this.input.need(Attr.firstname); const lastName = this.input.get(Attr.lastname) || null; // Backfill the account email onto the onboarding row when the client @@ -63,11 +104,17 @@ class Onboarding extends Entity { // without it, onboarding_responses.email stays NULL and the onboarding // export's User ID / Username / Email / Joined columns come out empty. let email = this.input.get(Attr.email) || null; - if (!email && this.uid !== ID_NOBODY) { + if (!email && uid) { const profile = this.user.get(Attr.profile) || {}; email = profile.email || null; } - const countryCode = this.input.get('country_code') || null; + // Trim here as well as in the procedure. Neither source of this address is + // typed into the wizard - it comes from signup or from the stored profile - + // and a stray space in either made the anchored format check reject it, + // which blocked step 1 with no field for the user to correct. + const trim = (v) => (typeof v === 'string' ? v.trim() : v); + email = trim(email) || null; + const countryCode = trim(this.input.get('country_code')) || null; if (!firstName) { return this.exception.user("firstname is required."); @@ -75,7 +122,7 @@ class Onboarding extends Entity { await this.db.await_proc( `${this.app_db}.save_onboarding_user_info`, - sessionId, firstName, lastName, email, countryCode + sessionId, uid, firstName, lastName, email, countryCode ); this.output.data({ success: true, message: 'User info saved.', data: {} }); } @@ -84,12 +131,13 @@ class Onboarding extends Entity { * v2 Step 2: industry / kind of work. */ async save_industry() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const industry = this.input.need('industry'); const industryOther = this.input.get('industry_other') || null; await this.db.await_proc( `${this.app_db}.save_onboarding_industry`, - sessionId, industry, industryOther + id.sessionId, id.uid, industry, industryOther ); this.output.data({ success: true, message: 'Industry saved.', data: {} }); } @@ -98,12 +146,13 @@ class Onboarding extends Entity { * v2 Step 3: role. */ async save_role() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const role = this.input.need('role'); const roleOther = this.input.get('role_other') || null; await this.db.await_proc( `${this.app_db}.save_onboarding_role`, - sessionId, role, roleOther + id.sessionId, id.uid, role, roleOther ); this.output.data({ success: true, message: 'Role saved.', data: {} }); } @@ -112,11 +161,12 @@ class Onboarding extends Entity { * v2 Step 4: team size. Replaces save_usage_plan in the new wizard. */ async save_team_size() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const teamSize = this.input.need('team_size'); await this.db.await_proc( `${this.app_db}.save_onboarding_team_size`, - sessionId, teamSize + id.sessionId, id.uid, teamSize ); this.output.data({ success: true, message: 'Team size saved.', data: {} }); } @@ -125,11 +175,12 @@ class Onboarding extends Entity { * v2 Step 5: workspace intent ("What do you want to start with?"). Optional. */ async save_intent() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const intent = this.input.need('intent'); await this.db.await_proc( `${this.app_db}.save_onboarding_intent`, - sessionId, intent + id.sessionId, id.uid, intent ); this.output.data({ success: true, message: 'Intent saved.', data: {} }); } @@ -140,24 +191,50 @@ class Onboarding extends Entity { * the challenges array is required. */ async save_challenges() { - const sessionId = this.input.sid(); - const challenges = toArray(this.input.need('challenges')); + const id = this._identity(); + if (!id) return; + // `get`, not `need`: an EMPTY selection is a legal answer ("none of + // these"), and it must be able to overwrite a previously saved list. + // `need` would still admit [], but defaulting here also keeps a client + // that omits the key entirely from erroring out. + const challenges = toArray(this.input.get('challenges') || []); const note = this.input.get('note') || null; // Pass array directly — Drumee db driver handles JSON serialization. // Do NOT JSON.stringify here (causes double-encoding at the driver layer). await this.db.await_proc( `${this.app_db}.save_onboarding_challenges`, - sessionId, challenges, note + id.sessionId, id.uid, challenges, note ); this.output.data({ success: true, message: 'Challenges saved.', data: {} }); } /** - * + * True reset: clear the user's stored onboarding answers. + * + * The old implementation called clearAuthorization() and nothing else — it + * discarded the SESSION but kept the DATA. That is backwards on both counts: + * + * - the half-filled onboarding_responses row survived, keyed to a session + * id that no longer existed, so it was unreachable forever. Every reset + * leaked one orphan row. + * - the wizard restarted against a dead session, so the first save of the + * "fresh" run wrote under a different identity than the reads. + * + * The wizard runs inside an authenticated desk session; resetting a + * questionnaire is not a reason to destroy the user's login. So this drops + * the answers and leaves the session intact. The stored procedure also + * collects any orphan rows the previous implementation left behind. */ async reset() { - this.output.clearAuthorization(this.input.authorization()); - this.output.data({}); + const id = this._identity(); + if (!id) return; + const res = toArray( + await this.db.await_proc( + `${this.app_db}.reset_onboarding_response`, + id.sessionId, id.uid + ) + )[0] || {}; + this.output.data({ success: true, removed: res.removed || 0, status: 'reset' }); } /** @@ -190,7 +267,9 @@ class Onboarding extends Entity { * Valid values: personal | startup | enterprise */ async save_usage_plan() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const sessionId = id.sessionId; const usagePlan = this.input.need(Attr.args); const VALID_PLANS = ['personal', 'startup', 'enterprise']; @@ -211,18 +290,30 @@ class Onboarding extends Entity { * v2 Step 5A: tools currently used by the team (multi-select). * Valid values: google_drive | notion | slack | dropbox | * clickup | trello | jira | other - * FE sends: { tools: ["notion", "slack"] } + * FE sends: { tools: ["notion", "other"], tools_other: "Obsidian" } + * + * Two deliberate changes from v2: + * + * - An empty array is accepted instead of rejected. It used to throw + * ('tools array is required and must not be empty'), so the client + * skipped the call when nothing was selected — which meant de-selecting + * every tool silently left the previously saved list in the database. + * An empty selection is a real answer and must overwrite. + * + * - `tools_other` carries the "Other" free text in its own field, matching + * industry_other / role_other, instead of being spliced into the array + * where it was indistinguishable from a canonical key. The stored + * procedure normalises either shape, so older clients keep working. */ async save_tools() { - const sessionId = this.input.sid(); - const tools = toArray(this.input.need('tools')); - if (!tools.length) { - return this.exception.user('tools array is required and must not be empty.'); - } + const id = this._identity(); + if (!id) return; + const tools = toArray(this.input.get('tools') || []); + const toolsOther = this.input.get('tools_other') || null; // Pass array directly — Drumee db driver handles JSON serialization. await this.db.await_proc( `${this.app_db}.save_onboarding_tools`, - sessionId, tools + id.sessionId, id.uid, tools, toolsOther ); this.output.data({ success: true, message: 'Tools saved.', data: {} }); } @@ -231,7 +322,9 @@ class Onboarding extends Entity { * */ async save_privacy() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const sessionId = id.sessionId; const privacyLevel = this.input.need('privacy'); const level = parseInt(privacyLevel); @@ -251,11 +344,15 @@ class Onboarding extends Entity { * */ async check_completion() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; let completionStatusRaw; try { - completionStatusRaw = await this.db.await_proc(`${this.app_db}.check_onboarding_completion`, sessionId); + completionStatusRaw = await this.db.await_proc( + `${this.app_db}.check_onboarding_completion`, sessionId, uid + ); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling check_completion SP for session ${sessionId}: ${spError.message}`); throw spError; @@ -276,35 +373,75 @@ class Onboarding extends Entity { * */ async mark_complete() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; try { - await this.db.await_proc(`${this.app_db}.mark_onboarding_complete`, sessionId); + await this.db.await_proc(`${this.app_db}.mark_onboarding_complete`, sessionId, uid); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling mark_complete SP for session ${sessionId}: ${spError.message}`); - throw spError; + // Surface it as a client error rather than a 500: every SIGNAL this + // procedure raises is actionable by the user ("Step 2 is incomplete"), + // and the wizard now shows it and keeps them on the page instead of + // dropping them into a workspace with unsaved answers. + return this.exception.user( + (spError && spError.message) || 'Onboarding could not be completed.' + ); } this.output.data({ success: true, message: 'Onboarding marked as complete (validated).', data: {} }); } /** - * + * Mirror the onboarding answers onto the drumate profile and set + * onboarded = 1 (which is what stops desk from re-launching the wizard). + * + * Identity is now validated BEFORE any user data is touched. The previous + * order destructured `this.user.get(Attr.profile)` first and only then + * checked for ID_NOBODY, so an anonymous request threw on the destructure + * (profile is undefined) instead of returning the intended "no-user" — + * a 500 where a clean answer was already written two lines below. + * + * The row is looked up by uid first and only falls back to email. Email is + * not an identity: it is nullable on this table, it is only backfilled from + * step 1, and "latest row with this address" can belong to a different + * session than the one that just completed. uid is exact. */ async update_profile() { - const { email } = this.user.get(Attr.profile); - if (this.uid === ID_NOBODY) { + const uid = this._uid(); + if (!uid) { return this.output.data({ status: "no-user" }); } - const sql = `SELECT * FROM ${this.app_db}.onboarding_responses WHERE email=? ORDER BY mtime DESC LIMIT 1`; - const row = await this.yp.await_query(sql, email) || {}; - const { firstname, lastname, country_code, industry, role, team_size, intent } = row; + const { email } = this.user.get(Attr.profile) || {}; + + let row = await this.yp.await_query( + `SELECT * FROM ${this.app_db}.onboarding_responses WHERE uid=? ORDER BY mtime DESC LIMIT 1`, + uid + ); + if (!row && email) { + // Pre-migration rows carry no uid. Resolving them by email here is what + // keeps an in-flight onboarding (started before this deploy) completing + // normally instead of syncing an empty profile. + row = await this.yp.await_query( + `SELECT * FROM ${this.app_db}.onboarding_responses WHERE email=? ORDER BY mtime DESC LIMIT 1`, + email + ); + } + row = row || {}; + + const { + firstname, lastname, country_code, + industry, industry_other, role, role_other, team_size, intent + } = row; const profile = { onboarded: 1 }; if (firstname) profile.firstname = firstname; if (lastname) profile.lastname = lastname; if (country_code) profile.country_code = country_code; - if (role) profile.role = role; - if (industry) profile.industry = industry; + // Store what the user actually said, not the literal "other" marker — + // consistent with how the analytics export renders these columns. + if (role) profile.role = (role === 'other' && role_other) ? role_other : role; + if (industry) profile.industry = (industry === 'other' && industry_other) ? industry_other : industry; if (team_size) profile.team_size = team_size; if (intent) profile.intent = intent; await this.yp.await_proc('drumate_update_profile', this.uid, profile); @@ -316,11 +453,15 @@ class Onboarding extends Entity { * @returns */ async get_response() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; let responseDataRaw; let { xlink } = JSON.parse(this.conf); try { - responseDataRaw = await this.db.await_proc(`${this.app_db}.get_onboarding_response`, sessionId); + responseDataRaw = await this.db.await_proc( + `${this.app_db}.get_onboarding_response`, sessionId, uid + ); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling get_response SP for session ${sessionId}: ${spError.message}`); throw spError; @@ -333,13 +474,18 @@ class Onboarding extends Entity { return; } - // Parse JSON tools - if (responseData.current_tools && typeof responseData.current_tools === 'string') { - try { - responseData.current_tools = JSON.parse(responseData.current_tools); - } catch (e) { - this.warn("Failed to parse current_tools JSON for session:", sessionId); - responseData.current_tools = []; + // Parse the JSON columns. `challenges` is parsed too now: this payload + // drives wizard resume, and an unparsed string there meant the challenge + // chips came back unselected on every reload. + for (const key of ['current_tools', 'tools', 'challenges']) { + const v = responseData[key]; + if (v && typeof v === 'string') { + try { + responseData[key] = JSON.parse(v); + } catch (e) { + this.warn(`Failed to parse ${key} JSON for session:`, sessionId); + responseData[key] = []; + } } } this.conf = Cache.getSysConf('ob_conf'); @@ -356,8 +502,10 @@ class Onboarding extends Entity { * If loby's DB user lacks EXECUTE on C_reward, switch to this.yp.await_proc(...). */ async get_onboarding_invite_link() { - if (!this.uid) { - return this.exception.user('User not authenticated.'); + if (!this._uid()) { + // ID_NOBODY is a truthy string, so the old `!this.uid` test let + // anonymous callers straight through to the reward/referral lookups. + return this.exception.unauthorized('User not authenticated.'); } const rewardConf = JSON.parse(Cache.getSysConf('reward_hub_conf') || '{}'); @@ -404,8 +552,10 @@ class Onboarding extends Entity { * If loby DB user lacks EXECUTE on C_reward, switch to this.yp.await_proc(...). */ async send_onboarding_invites() { - if (!this.uid) { - return this.exception.user('User not authenticated.'); + if (!this._uid()) { + // ID_NOBODY is a truthy string, so the old `!this.uid` test let + // anonymous callers straight through to the reward/referral lookups. + return this.exception.unauthorized('User not authenticated.'); } const raw = toArray(this.input.need('emails')); @@ -492,7 +642,7 @@ class Onboarding extends Entity { * - folder_chat_started → channel table (high-frequency, not suitable for services_log) */ async get_activation_status() { - if (!this.uid) { + if (!this._uid()) { return this.output.data({ workspace_created: false, first_file_uploaded: false, From 030ab95872d75a4fb792bbaf2321c7f45e468e55 Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:38:40 +0700 Subject: [PATCH 04/20] feat(onboarding): push the referral row when onboarding completes (#34) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting onboarded = 1 is the New -> Onboarding transition on the analytics Referral users board. That board polls every two minutes, so the row read stale for up to that long after the user pressed the last button in the wizard. It now turns over about a second later. Published AFTER the profile write, never awaited, and it swallows its own errors. Onboarding completion is the user's flow; an analytics push is a bystander. A slow Redis or a dashboard nobody has open must not add latency to update_profile, and must certainly not fail it — the profile is already committed by the time the push runs, so a rejection would report failure for work that succeeded. Publishing before the write would race its own commit and send the status the user just left. The row comes from referral_members, which owns the status CASE; nothing here re-derives a status, so the live badge and the polled badge cannot disagree. Asking that procedure for the row also serves as the cohort gate — it answers nothing for a user who was never referred, and those are the majority. Recipients come from referral_live_sockets (analytics-server schemas): every active socket of every user permitted to read the analytics hub. That is the same rule get_env gates on, and per-socket delivery means multiple open tabs need no extra handling. The mirror of this is server-team desk.track_workspace, which reports the Onboarding -> Activated half of the same transition. Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- service/onboarding.js | 62 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/service/onboarding.js b/service/onboarding.js index 48413e3..de53793 100644 --- a/service/onboarding.js +++ b/service/onboarding.js @@ -1,7 +1,7 @@ // service/onboarding.js const { Entity } = require('@drumee/server-core'); -const { toArray, Cache, Constants, Attr, Messenger } = require('@drumee/server-essentials'); +const { toArray, Cache, Constants, Attr, Messenger, RedisStore } = require('@drumee/server-essentials'); const { resolve } = require('path'); const { ID_NOBODY } = Constants; class Onboarding extends Entity { @@ -445,9 +445,69 @@ class Onboarding extends Entity { if (team_size) profile.team_size = team_size; if (intent) profile.intent = intent; await this.yp.await_proc('drumate_update_profile', this.uid, profile); + // AFTER the write, never before: the dashboard re-reads the row from the + // database, so publishing first would race its own commit and push the old + // status. Not awaited — see _pushReferralLive. + this._pushReferralLive(this.uid); this.output.data(profile); } + /** + * Tell any open analytics dashboard that this user's referral row changed. + * + * WHY IT IS HERE. Setting onboarded = 1 is the New -> Onboarding transition + * on the Referral users board. The board polls every two minutes, so without + * a push the row reads stale for up to that long; with one it turns over + * within a second of the user pressing the last button in the wizard. + * + * NOT AWAITED AND NEVER THROWS. Onboarding completion is the user's flow; + * an analytics push is a bystander. A slow Redis or a dashboard nobody has + * open must not add latency to update_profile, and must certainly not fail + * it — the caller has already committed the profile write by the time we + * run, so a rejection here would report failure for work that succeeded. + * + * THE ROW COMES FROM referral_members. Not from anything assembled here: + * that procedure owns the status CASE, and re-deriving it in a publisher is + * how a live badge and a polled badge start disagreeing. Asking it for the + * row doubles as the cohort gate — it answers nothing for a user who was + * never referred, and those are the majority, so the push is skipped without + * a second query. + * + * Recipients are resolved by referral_live_sockets (analytics-server + * schemas): every active socket of every user permitted to read the + * analytics hub. That covers each open tab, so multi-tab needs nothing + * extra, and it is the same access rule get_env gates on. + * + * The mirror of this method is server-team service/private/desk.js + * track_workspace, which reports the Onboarding -> Activated half of the + * same transition. Keep the payload shape identical. + * + * @param {String} uid the referred user whose row moved + */ + async _pushReferralLive(uid) { + try { + if (!uid) return; + const rows = toArray(await this.yp.await_proc('referral_members', { uid })); + const model = rows && rows[0]; + if (!model) return; // not a referred user — nothing on that board to move + const sockets = toArray(await this.yp.await_proc('referral_live_sockets')); + if (!sockets || !sockets.length) return; // no dashboard open anywhere + await RedisStore.sendData( + { + model, + // Read by the dashboard's onWsMessage. The envelope carries no + // top-level `service`, so router/push stamps it "live.update" and + // the client routes it to the `live` event; this name is what tells + // the widget which live message it is holding. + options: { service: 'live.referral_member', keys: '*' }, + }, + sockets + ); + } catch (e) { + this.warn('[onboarding] referral live push failed', e && e.message); + } + } + /** * * @returns From 1b97c9b4d6096b94b2ff9895805b5e3c605497b5 Mon Sep 17 00:00:00 2001 From: "phamtobao@gmail.com" Date: Sun, 9 Aug 2026 16:30:35 +0400 Subject: [PATCH 05/20] ci: deploy PROD from main branch [skip ci] --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 15711d5..4369df4 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -43,7 +43,7 @@ jobs: case "$TARGET" in TEST) EP=test; PORT=24002; BRANCH=test; SRC=/usr/src/drumee.test/$PLUGIN; MODE=git ;; UAT) EP=preview; PORT=24001; BRANCH=preview; SRC=/usr/src/drumee/$PLUGIN; MODE=git ;; - PROD) EP=main; PORT=24000; BRANCH=preview; SRC=/usr/src/drumee/$PLUGIN; MODE=runtime ;; + PROD) EP=main; PORT=24000; BRANCH=main; SRC=/usr/src/drumee/$PLUGIN; MODE=runtime ;; *) echo "Unknown target: $TARGET"; exit 1 ;; esac # loc=stage -> deploy locally on the runner host (drumee.in) `main` endpoint From 5fc435a1a451eed659771cde5c4f1f6e01ca9d14 Mon Sep 17 00:00:00 2001 From: Drumee Dev Date: Tue, 11 Aug 2026 13:49:49 -0700 Subject: [PATCH 06/20] fix(email): send transactional mail as multipart/alternative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three user-facing mails went out as a lone text/html body. Nodemailer does not synthesise a text part, so `html` with no `text` produces a single-part message — a long-standing spam heuristic, and these are 2FA codes and verification links, where landing in spam locks a user out of signing in. sendAs() now takes an optional `text`. The key is set only when truthy: omitted, the message is byte-for-byte what it was before, so the contract stays backward-compatible; an empty string would build an alternative whose text/plain part is blank, which is worse than having none. Plain-text bodies are hand-written at each call site from the same data the template receives, not stripped from the rendered HTML — these are nested layout tables whose text nodes are spacer   and icon alt text as often as they are prose. Alongside that, three fixes to what the mails actually say: - verify-email printed the full verification URL, token and all, as visible body copy beside the CTA — the shape a phishing template has. The fallback is now an anchor; the copyable URL moved to the text part, where a reader with no anchor to follow actually needs it. - signup-completed emitted the recipient's own address on a line of its own above the copy. It now greets, matching verify-email. - the OTP mail's subject and headline came from lexicon keys absent from the default lexicon. Cache.lex() returns the lexicon MAP, so a missing key reads as undefined (echoing the key name is Cache.message(), a different function) — on any box without a loaded lexicon both went out as the literal string "undefined". Guarded in the data, not the template, so the two MIME parts cannot fall back differently. The OTP expiry line is derived from the row otp_create returns rather than restated in the copy: the procedures disagree (authenticate.sql and session_login_otp.sql expire at 10 minutes, check.sql at 30), and a mail naming the wrong number is worse than one naming none. Social badges now match analytics-server's claim-reward.html, whose footer is the Figma "Email marketing" node and the canonical set. Three of the five channels here were placeholders never corrected: discord.gg/drumee is a dead invite (the API answers "Unknown Invite", code 10006) and x.com/drumee is the wrong handle. Icons are sized per asset — linkedin.png is 13x9, so the uniform 16x16 stretched it — and the badge circle's rgba() became the flattened hex it needs to paint under Outlook's Word engine. OTP generation, validation and expiration, verification-token handling, the signup flow, session behaviour and the sender address are untouched. No List-Unsubscribe: these are transactional. Deliverability is NOT fixed by this commit. drumee.org still softfails SPF (the relay is in neither `a` nor `mx`) and publishes two DMARC records, which RFC 7489 6.6.3 makes receivers discard entirely; whether the relay signs d=drumee.org s=mail is unverified. Those are DNS and relay-side changes — see the header of service/lib/mail-sender.js. Tests: 23 assertions in offline/test, driving the real service methods against real nodemailer MIME with only the DB, request input and transport stubbed. Co-Authored-By: Claude Opus 5 (1M context) --- .../test/transactional-mail-multipart.test.js | 408 ++++++++++++++++++ service/lib/loby.js | 45 +- service/lib/mail-sender.js | 128 +++++- service/signup.js | 54 ++- service/templates/signup-completed.html | 67 +-- service/templates/verify-email.html | 71 +-- 6 files changed, 695 insertions(+), 78 deletions(-) create mode 100644 offline/test/transactional-mail-multipart.test.js diff --git a/offline/test/transactional-mail-multipart.test.js b/offline/test/transactional-mail-multipart.test.js new file mode 100644 index 0000000..941d36d --- /dev/null +++ b/offline/test/transactional-mail-multipart.test.js @@ -0,0 +1,408 @@ +#!/usr/bin/env node + +/** + * Regression tests for the plain-text alternative on transactional mail. + * + * Nodemailer does not synthesise a text part: given only `html` it emits a + * lone text/html body, which is a long-standing spam heuristic. All three + * user-facing mails on this path — email verification, signup-completed and + * the 2FA OTP — now hand sendAs() a hand-written `text` body so the message + * goes out as multipart/alternative. These are 2FA codes and verification + * links, where landing in spam locks a user out of signing in, so the shape of + * the message is worth pinning down. + * + * What is asserted here: + * - sendAs() forwards `text`, and omits the key entirely when absent, so the + * contract stays backward-compatible for any future caller without copy. + * - each mail is multipart/alternative with text/plain BEFORE text/html. + * - the OTP code is identical in both parts, and the server-side secret is + * in neither. + * - the verification URL survives intact, and its token is not rendered as + * visible HTML text. + * - OTP generation, its arguments and its failure path are unchanged. + * + * The real service methods are driven; only the DB (`yp`), the request input + * and the transport are stubbed. Messenger.getMTA is replaced with a capturing + * stream transport, so nothing is submitted to an MTA. + * + * Standalone runner (no test framework in this repo): `node `. + */ + +const assert = require("assert"); +const { readFileSync } = require("fs"); +const { resolve } = require("path"); +const { template } = require("lodash"); +const Nodemailer = require("nodemailer"); +const { Messenger } = require("@drumee/server-essentials"); + +const { + sendAs, + butlerFrom, + supportText, + legalFooterText, +} = require("../../service/lib/mail-sender"); +const Signup = require("../../service/signup"); +const Loby = require("../../service/lib/loby"); + +const tests = []; +const test = (name, fn) => tests.push([name, fn]); + +// -------------------------------------------------------------------------- +// Harness +// -------------------------------------------------------------------------- + +const FROM = '"Drumee" '; +const EMAIL = "user@example.com"; +const HOME = "https://my.drumee.com/"; +const TOKEN = "b3f1".repeat(16); // 64 hex chars, shaped like randomBytes(32) +const CODE = "418302"; +const CTIME = 1770000000; + +/** Messages captured from the last drive() call. */ +let sent = []; + +// sendAs asks the Messenger for its transport. Hand it one that buffers the +// built MIME instead of talking to an MTA. +Messenger.prototype.getMTA = function () { + const t = Nodemailer.createTransport({ streamTransport: true, buffer: true }); + const send = t.sendMail.bind(t); + t.sendMail = async (m) => { + const info = await send(m); + sent.push({ m, mime: info.message.toString() }); + return info; + }; + return t; +}; +Messenger.prototype.stop = function () { }; + +/** + * Build a service instance without running the constructor chain — these + * methods only reach for the members assigned here. + */ +function service(Klass, { row, calls } = {}) { + const s = Object.create(Klass.prototype); + s.warn = () => { }; + s.exception = { email: null }; + s.input = { homepath: () => HOME, ua_language: () => "en" }; + s.yp = { + await_proc: async (...args) => { + if (calls) calls.push(args); + return row; + }, + }; + return s; +} + +/** Run a send and return the single captured message. */ +async function drive(fn) { + sent = []; + const rc = await fn(); + return { rc, ...(sent[0] || {}) }; +} + +/** Assert the message is multipart/alternative, text/plain first. */ +function assertAlternative(mime, label) { + assert.ok(/Content-Type: multipart\/alternative/.test(mime), + `${label}: expected multipart/alternative`); + const p = mime.indexOf("Content-Type: text/plain"); + const h = mime.indexOf("Content-Type: text/html"); + assert.ok(p > -1, `${label}: missing text/plain part`); + assert.ok(h > -1, `${label}: missing text/html part`); + assert.ok(p < h, `${label}: text/plain must precede text/html`); +} + +/** Visible text of an HTML body: tags and comments removed. */ +const visible = (html) => + html.replace(//g, " ").replace(/<[^>]*>/g, " "); + +/** Render a template the way Messenger.renderFrom does (lodash, not EJS). */ +const render = (name, data) => + template(String(readFileSync(resolve(__dirname, "../../service/templates", name))).trim())(data); + +const otpRow = { sys_id: 7, uid: "u1", secret: "s3cr3t", code: CODE, ctime: CTIME, expiry: CTIME + 600 }; + +// -------------------------------------------------------------------------- +// sendAs() contract +// -------------------------------------------------------------------------- + +const fakeMsg = (mta) => { + let stopped = 0; + return { getMTA: async () => mta, stop: () => { stopped++; }, stops: () => stopped }; +}; + +function capturing() { + const seen = []; + const t = Nodemailer.createTransport({ streamTransport: true, buffer: true }); + const send = t.sendMail.bind(t); + t.sendMail = async (m) => { seen.push(m); return send(m); }; + return { t, seen }; +} + +test("sendAs forwards `text` to the transport", async () => { + const { t, seen } = capturing(); + const rc = await sendAs(fakeMsg(t), { to: EMAIL, subject: "s", html: "

h

", text: "plain" }); + assert.strictEqual(rc, 1); + assert.strictEqual(seen[0].text, "plain"); + assert.strictEqual(seen[0].from, FROM); +}); + +test("sendAs omits the `text` key entirely when not supplied", async () => { + const { t, seen } = capturing(); + assert.strictEqual(await sendAs(fakeMsg(t), { to: EMAIL, subject: "s", html: "

h

" }), 1); + assert.ok(!("text" in seen[0]), "text key must be absent, not undefined"); +}); + +test("sendAs ignores an empty-string `text`", async () => { + // An empty string would build a multipart/alternative with a blank + // text/plain part, which is worse than having no text part at all. + const { t, seen } = capturing(); + await sendAs(fakeMsg(t), { to: EMAIL, subject: "s", html: "

h

", text: "" }); + assert.ok(!("text" in seen[0])); +}); + +test("sendAs with no text still produces single-part text/html", async () => { + const info = await Nodemailer.createTransport({ streamTransport: true, buffer: true }) + .sendMail({ from: butlerFrom(), to: EMAIL, subject: "s", html: "

h

" }); + const mime = info.message.toString(); + assert.ok(!/multipart\/alternative/.test(mime)); + assert.ok(/Content-Type: text\/html/.test(mime)); +}); + +test("sendAs returns 0 and does not stop a Messenger it never used", async () => { + const msg = fakeMsg(null); + assert.strictEqual(await sendAs(msg, { to: EMAIL, subject: "s", html: "

h

" }), 0); + assert.strictEqual(msg.stops(), 0); +}); + +test("sendAs stops the Messenger exactly once on the success path", async () => { + const { t } = capturing(); + const msg = fakeMsg(t); + await sendAs(msg, { to: EMAIL, subject: "s", html: "

h

", text: "p" }); + assert.strictEqual(msg.stops(), 1); +}); + +// -------------------------------------------------------------------------- +// Verification email +// -------------------------------------------------------------------------- + +test("verification email is multipart/alternative and keeps its URL intact", async () => { + const svc = service(Signup, { row: { token: TOKEN } }); + const { rc, m, mime } = await drive(() => svc._send_verification_email(42, EMAIL)); + const url = `${HOME}#/welcome/verify?token=${TOKEN}`; + + assert.strictEqual(rc, 1); + assertAlternative(mime, "verification"); + assert.strictEqual(m.from, FROM); + assert.strictEqual(m.subject, "Verify your Drumee email address"); + assert.ok(m.html.includes(`href="${url}"`), "CTA href must be the exact verification URL"); + assert.ok(m.text.includes(url), "text part must carry the full URL — it has no anchor to follow"); + assert.ok(m.text.includes("Hello user@example.com,")); + assert.ok(m.text.includes("Security Note")); + assert.ok(m.text.includes(supportText())); +}); + +test("verification email does not print the token as visible HTML text", async () => { + // A long opaque string beside a call to action is the shape of a phishing + // template; the copy-and-paste fallback belongs in the text part. + const svc = service(Signup, { row: { token: TOKEN } }); + const { m } = await drive(() => svc._send_verification_email(42, EMAIL)); + assert.ok(!visible(m.html).includes(TOKEN)); + assert.ok(m.html.includes('')); +}); + +test("verification email tells text readers to open a link, not click a button", async () => { + const svc = service(Signup, { row: { token: TOKEN } }); + const { m } = await drive(() => svc._send_verification_email(42, EMAIL)); + assert.ok(m.text.includes("by opening the link below")); + assert.ok(m.html.includes("by clicking the button below")); +}); + +test("verification email sends nothing when no token is minted", async () => { + const svc = service(Signup, { row: {} }); + const { rc } = await drive(() => svc._send_verification_email(42, EMAIL)); + assert.strictEqual(rc, 0); + assert.strictEqual(sent.length, 0); +}); + +// -------------------------------------------------------------------------- +// Signup-completed email +// -------------------------------------------------------------------------- + +test("signup-completed is multipart/alternative and greets in both parts", async () => { + const svc = service(Signup); + const { rc, m, mime } = await drive(() => svc._send_signup_completed_email(EMAIL)); + assert.strictEqual(rc, 1); + assertAlternative(mime, "signup-completed"); + assert.ok(visible(m.html).includes(`Hello ${EMAIL},`), "HTML must greet, not float the address alone"); + assert.ok(m.text.includes(`Hello ${EMAIL},`)); + assert.ok(m.html.includes(`href="${HOME}#/desk"`)); + assert.ok(m.text.includes(`${HOME}#/desk`)); +}); + +test("signup-completed drops the greeting when no address resolved", async () => { + // send_welcome resolves the address from the verification row and can + // legitimately come up empty; a dangling "Hello ," reads worse than none. + const svc = service(Signup); + const { m } = await drive(() => svc._send_signup_completed_email("")); + assert.ok(!/Hello/.test(visible(m.html))); + assert.ok(!/Hello/.test(m.text)); + assert.ok(m.text.includes("successfully created"), "body copy must survive"); +}); + +// -------------------------------------------------------------------------- +// 2FA OTP email +// -------------------------------------------------------------------------- + +test("OTP email is multipart/alternative", async () => { + const svc = service(Loby, { row: otpRow }); + const { rc, mime } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.strictEqual(rc, 1); + assertAlternative(mime, "otp"); +}); + +test("OTP code is identical in the text and html parts", async () => { + const svc = service(Loby, { row: otpRow }); + const { m } = await drive(() => svc._send2faOtp("u1", EMAIL)); + const inText = m.text.match(/\b\d{6}\b/g) || []; + const inHtml = visible(m.html).match(/\b\d{6}\b/g) || []; + assert.deepStrictEqual(inText, [CODE], "exactly one code in the text part"); + assert.deepStrictEqual(inHtml, [CODE], "exactly one code in the html part"); +}); + +test("OTP server-side secret appears in neither part", async () => { + const svc = service(Loby, { row: otpRow }); + const { m } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.ok(!m.text.includes(otpRow.secret)); + assert.ok(!m.html.includes(otpRow.secret)); +}); + +test("OTP generation is unchanged", async () => { + const calls = []; + const svc = service(Loby, { row: otpRow, calls }); + await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.strictEqual(calls.length, 1); + assert.strictEqual(calls[0][0], "otp_create"); + assert.strictEqual(calls[0][1], "u1"); + assert.strictEqual(typeof calls[0][2], "string"); + assert.ok(calls[0][2].length > 0, "secret still generated and passed through"); +}); + +test("OTP expiry is derived from the row otp_create returned", async () => { + // The OTP procedures disagree — authenticate.sql and session_login_otp.sql + // expire at 10 minutes, check.sql at 30 — so the window is read back from + // what was actually minted rather than restated in the copy. + const svc = service(Loby, { row: otpRow }); + const { m } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.ok(m.text.includes("expires in 10 minutes")); +}); + +test("OTP omits the expiry claim when the row cannot support one", async () => { + const svc = service(Loby, { row: { code: CODE } }); + const { m, mime } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.ok(!/expires in/.test(m.text), "must not invent an expiry"); + assert.ok(m.text.includes(CODE)); + assertAlternative(mime, "otp without expiry"); +}); + +test("OTP falls back to real copy when the lexicon is empty", async () => { + // Cache.lex() returns the lexicon MAP, so a missing key reads as undefined + // (echoing the key name is Cache.message()). Unguarded, subject and headline + // both went out as the literal string "undefined". + const svc = service(Loby, { row: otpRow }); + const { m } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.ok(!/undefined/.test(m.text)); + assert.ok(!/undefined/.test(m.html)); + assert.ok(m.subject && !/undefined/.test(m.subject)); +}); + +test("OTP text footer mirrors otp.html, not the signup support block", async () => { + // The two alternatives of one message must say the same thing: otp.html has + // Privacy/Terms/Support links and no "Need Help?" block. + const svc = service(Loby, { row: otpRow }); + const { m } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.ok(m.text.includes(legalFooterText())); + assert.ok(!/Need help\?/.test(m.text)); +}); + +test("OTP sends nothing when otp_create yields no code", async () => { + const svc = service(Loby, { row: null }); + const { rc } = await drive(() => svc._send2faOtp("u1", EMAIL)); + assert.strictEqual(rc, 0); + assert.strictEqual(sent.length, 0); +}); + +// -------------------------------------------------------------------------- +// Templates +// -------------------------------------------------------------------------- + +test("social badges match analytics-server's canonical claim-reward.html set", async () => { + // claim-reward.html's footer is the Figma "Email marketing" node and is the + // source of truth for which channels Drumee actually has. These two used to + // carry five, three of them placeholders — discord.gg/drumee is a dead + // invite (API: "Unknown Invite", code 10006) and x.com/drumee is the wrong + // handle. Pinned so a future edit cannot quietly reintroduce them. + const expected = [ + ["https://x.com/DrumeeOS", "x.png", 14, 14], + ["https://t.me/DrumeeAnnChat", "telegram.png", 18, 17], + ["https://www.linkedin.com/company/drumee/posts/?feedView=all", "linkedin.png", 13, 9], + ]; + const rendered = {}; + for (const name of ["verify-email.html", "signup-completed.html"]) { + // Comments are not delivered markup, and this block's comment names the + // retired channels — strip before asserting on what actually ships. + const html = render(name, { + heading: "h", subheading: "s", hello: "x", intro: "i", button_label: "b", + verify_url: "https://e/", fallback_label: "f", security_title: "t", + security_note: "n", home: "https://e/", email: EMAIL, + }).replace(//g, ""); + rendered[name] = html; + + for (const [url, icon, w, h] of expected) { + assert.ok(html.includes(`href="${url}"`), `${name}: missing ${url}`); + assert.ok( + html.includes(`icons/${icon}" width="${w}" height="${h}"`), + `${name}: ${icon} must render at its own ${w}x${h} proportions, not a uniform square`); + } + assert.ok(!/discord|tiktok|instagram/i.test(html), `${name}: retired channel still linked`); + // rgba() is unsupported by Outlook's Word engine; the badge circle must be + // the flattened hex or it does not paint there at all. + assert.ok(!/rgba\(/.test(html), `${name}: rgba() left in delivered markup`); + assert.ok(html.includes('bgcolor="#dcdbf5"'), `${name}: badge circle needs a bgcolor attribute`); + } + + // The two blocks have always matched; a diff between them is a mistake. + const badges = (h) => h.slice(h.indexOf("
")); + assert.strictEqual( + badges(rendered["verify-email.html"]).slice(0, 2000), + badges(rendered["signup-completed.html"]).slice(0, 2000), + "the two social blocks have drifted apart"); +}); + +test("templates keep their email-safe structure", async () => { + for (const name of ["verify-email.html", "signup-completed.html", "otp.html"]) { + const html = render(name, { + heading: "h", subheading: "s", hello: "x", intro: "i", button_label: "b", + verify_url: "https://e/", fallback_label: "f", security_title: "t", + security_note: "n", home: "https://e/", email: EMAIL, code: CODE, why_this_otp: "w", + }); + assert.ok(html.includes('role="presentation"'), `${name}: presentational tables`); + assert.ok(html.includes('width="600"'), `${name}: Outlook width attribute`); + } +}); + +// -------------------------------------------------------------------------- + +(async () => { + let failed = 0; + for (const [name, fn] of tests) { + try { + await fn(); + console.log(`ok ${name}`); + } catch (e) { + failed++; + console.log(`FAIL ${name}\n ${e.message}`); + } + } + console.log(`\n${tests.length - failed}/${tests.length} passed`); + process.exit(failed ? 1 : 0); +})(); diff --git a/service/lib/loby.js b/service/lib/loby.js index 4712813..bf7fdd7 100644 --- a/service/lib/loby.js +++ b/service/lib/loby.js @@ -26,7 +26,7 @@ const { resolve } = require("path"); // left it throwing ReferenceError at its first guard. const { template, isEmpty, isArray } = require("lodash"); -const { sendAs } = require("./mail-sender"); +const { sendAs, legalFooterText } = require("./mail-sender"); class Account extends Entity { @@ -495,12 +495,21 @@ class Account extends Entity { } const lang = this.input.ua_language() || "en"; const lex = Cache.lex(lang); + // Cache.lex() hands back the lexicon MAP, so a key it does not carry reads + // as undefined — echoing the key name is Cache.message(), not this. Both + // keys are absent from the default lexicon, so on any box whose lexicon has + // not been loaded these went out with the literal string "undefined" as the + // subject AND the headline. Guarded here rather than in the template so the + // HTML part and the text part cannot fall back differently. + const heading = lex._your_otp || "Your one-time code"; + const why_this_otp = lex._why_this_otp || + "You are receiving this code because a sign-in to your Drumee account needs to be verified."; const data = { - heading: lex._your_otp, + heading, code: otp.code, - why_this_otp: lex._why_this_otp, + why_this_otp, }; - const subject = lex._your_otp; + const subject = heading; const msg = new Messenger({ subject, recipient: _email, @@ -509,9 +518,35 @@ class Account extends Entity { try { const tpl = resolve(__dirname, "../templates/otp.html"); const html = msg.renderFrom(tpl, data); + // The window is read back from the row otp_create actually minted + // (it returns `expiry` = ctime + 600 beside the code) instead of being + // restated here, because the OTP procedures disagree about it: + // authenticate.sql and session_login_otp.sql expire at 10 minutes, + // check.sql at 30, misc.sql sweeps at 5. A mail naming the wrong number + // is worse than one naming none, so the line is dropped whenever the + // two fields are not both present and sane. + const ttl = Number(otp.expiry) - Number(otp.ctime); + const expiry_line = Number.isFinite(ttl) && ttl > 0 + ? [`This code expires in ${Math.round(ttl / 60)} minutes.`, ""] + : []; + // Built from the same `data` the template gets, so the code and the copy + // cannot diverge between the two alternatives. otp.html carries no + // greeting, so none is invented here. + const text = [ + heading, + "", + String(otp.code), + "", + why_this_otp, + "", + ...expiry_line, + "Never share this code with anyone. Drumee will never ask you for it.", + "", + legalFooterText(), + ].join("\n"); // sendAs, not msg.send: the pinned Messenger re-wraps the From and turns // a full mailbox into a "Drumee>" display name. See ./mail-sender. - return await sendAs(msg, { to: _email, subject, html }); + return await sendAs(msg, { to: _email, subject, html, text }); } catch (e) { this.warn("[Auth] 2FA OTP email send failed", e); return 0; diff --git a/service/lib/mail-sender.js b/service/lib/mail-sender.js index a3f29f0..efbcb4c 100644 --- a/service/lib/mail-sender.js +++ b/service/lib/mail-sender.js @@ -1,7 +1,16 @@ // service/lib/mail-sender.js // The address user-facing Drumee mail is sent FROM, and the RFC 5322 mailbox -// built from it. Mirrors server-team's service/lib/mail-sender.js — the two -// repos send from the same brand address and must not drift. +// built from it. +// +// RELATIONSHIP TO server-team's service/lib/mail-sender.js. The two files are +// NO LONGER identical, and an earlier version of this header said they were. +// What must not drift is the sender identity — MAIL_SENDER_NAME, +// MAIL_SENDER_ADDRESS and mailbox() are the shared part, and a change to any +// of them belongs in both repos. Everything below that is local to this one: +// sendAs(), supportText() and legalFooterText() have no counterpart there, +// because server-team has no sendAs() caller (it uses butlerFrom()/mailbox() +// with its own senders). Do not copy them across to "resynchronise" the files; +// they would be dead code there. // // Pinned here rather than read from credential/email.json — which is what the // butlerSender() copy in lib/loby.js used to do. That file holds the transport's @@ -15,14 +24,30 @@ // module-level FROM (also email.json's auth.user), so they arrived as a bare // address with no display name while the OTP mail beside them showed "Drumee". // -// DEPLOYMENT REQUIREMENT, not satisfied by this file. drumee.org is not the -// domain the relay's DKIM key signs (d=drumee.com), and its SPF record -// ("v=spf1 a mx ~all") lists only Firebase hosting and Google's MX — not the -// relay. Its DMARC is published twice (p=none and p=quarantine), which per -// RFC 7489 makes receivers discard the set entirely. Until drumee.org publishes -// an SPF entry for the relay and its own DKIM selector, everything sent from -// this address is unauthenticated mail — and these templates are 2FA codes and -// email verification, where landing in spam locks a user out of signing in. +// DEPLOYMENT REQUIREMENT, not satisfied by this file. Verified against live +// DNS on 2026-08-11: +// +// SPF "v=spf1 a mx ~all" — resolves to Firebase hosting (199.36.158.100) +// and Google's MX. The relay that actually connects to the receiving +// MX is mail.drumee.com (135.125.104.154) and is in neither, so SPF +// softfails. Needs `a:mail.drumee.com`. +// DMARC published TWICE at _dmarc.drumee.org (p=none and p=quarantine). +// Per RFC 7489 6.6.3 a multi-record set makes receivers apply no +// DMARC processing at all, so the policy is not merely weak, it is +// absent. Exactly one record must remain. +// DKIM mail._domainkey.drumee.org now publishes a valid 2048-bit RSA key +// (modulus SHA256 fdb42636c2c92cd9…), distinct from drumee.com's. +// Whether the relay HOLDS the matching private key and signs +// d=drumee.org s=mail is NOT verifiable from the application side — +// mail.drumee.com refuses ssh here. Do not read the presence of the +// DNS record as proof that signing happens. +// +// Signing is the relay's job in this architecture: the app hands a finished +// message to mail.drumee.com over SMTP submission and the milter signs it. +// Nothing in this file can fix the above. Until it is fixed, everything sent +// from this address is unauthenticated mail — and these templates are 2FA +// codes and email verification, where landing in spam locks a user out of +// signing in. const MAIL_SENDER_NAME = "Drumee"; const MAIL_SENDER_ADDRESS = "contact@drumee.org"; @@ -108,15 +133,37 @@ function butlerFrom() { * The transport is module-cached inside the package, so it is deliberately NOT * closed here — closing it would break every later send in the process. * + * `text` is OPTIONAL and additive. Supplied, nodemailer emits + * `multipart/alternative` with a `text/plain` part ahead of the `text/html` + * one; omitted, the message is byte-for-byte what it was before, so the + * callers that have not been given plain-text copy yet are unaffected. + * + * It is worth supplying. Nodemailer does NOT synthesise a text part — with + * `html` alone the message goes out as a lone `text/html` body, which is a + * long-standing spam heuristic. The key is `text`, not `html`, because + * receivers score the message, not the markup. + * + * The plain-text body is deliberately NOT derived by stripping tags here. + * These templates are nested layout tables whose text nodes are spacer + * ` ` and icon alt text as often as they are prose; a mechanical strip + * yields something no recipient would read. Call sites hand-write the text + * from the same data they hand the template. + * * @param {Messenger} msg configured Messenger (used only for its transport) - * @param {{to:String, subject:String, html:String}} parts + * @param {{to:String, subject:String, html:String, text:String=}} parts * @returns {Promise} 1 sent, 0 not sent (no MTA configured) */ -async function sendAs(msg, { to, subject, html }) { +async function sendAs(msg, { to, subject, html, text }) { const mta = await msg.getMTA(); // sync in 1.2.29, async in 1.3.x; await covers both if (!mta) return 0; try { - await mta.sendMail({ from: butlerFrom(), to, subject, html }); + const message = { from: butlerFrom(), to, subject, html }; + // Only set the key when there is copy to put in it. `text: undefined` is + // harmless in current nodemailer, but an empty string is not — it would + // build a multipart/alternative whose text/plain part is blank, which is + // worse than having no text part at all. + if (text) message.text = text; + await mta.sendMail(message); return 1; } finally { // What send() does at the end of its run; we replace send(), so we owe it. @@ -124,4 +171,57 @@ async function sendAs(msg, { to, subject, html }) { } } -module.exports = { MAIL_SENDER_NAME, MAIL_SENDER_ADDRESS, mailbox, butlerFrom, sendAs }; +/** + * The "Need Help?" block and footer every user-facing template ends with, + * rendered for the `text/plain` part. + * + * Lives here so the two halves of one message cannot drift: the support + * address in the HTML footer is the same brand mailbox this module already + * pins as the From, so it is built from that constant rather than retyped. + * + * @returns {String} trailing block, no leading blank line + */ +function supportText() { + return [ + "Need help?", + "Our customer support team is available to assist you:", + ` Email: ${MAIL_SENDER_ADDRESS}`, + " Hours: Monday - Friday, 9:00 AM - 6:00 PM EST", + "", + `(c) ${new Date().getFullYear()} Drumee. All rights reserved.`, + "https://drumee.org | Privacy Policy: https://drumee.com/privacy/", + ].join("\n"); +} + +/** + * The footer otp.html ends with, rendered for the `text/plain` part. + * + * Separate from supportText() because the OTP template's footer genuinely is + * a different one — Privacy / Terms / Support links under a "DRUMEE WORKSPACE" + * rule, with no "Need Help?" block. Reusing supportText() there would put a + * support address and opening hours in the text part of a message whose HTML + * part shows neither, and the two alternatives of one message should say the + * same thing. + * + * @returns {String} trailing block, no leading blank line + */ +function legalFooterText() { + return [ + "Privacy Policy: https://drumee.com/privacy/", + "Terms of Service: https://drumee.com/terms/", + "Support: https://drumee.com/about", + "", + "DRUMEE WORKSPACE", + `(c) ${new Date().getFullYear()} Drumee. All rights reserved.`, + ].join("\n"); +} + +module.exports = { + MAIL_SENDER_NAME, + MAIL_SENDER_ADDRESS, + mailbox, + butlerFrom, + sendAs, + supportText, + legalFooterText, +}; diff --git a/service/signup.js b/service/signup.js index ce298cc..9c15010 100644 --- a/service/signup.js +++ b/service/signup.js @@ -5,7 +5,7 @@ const { toArray } = require('@drumee/server-essentials').utils; const { resolve } = require('path'); const { isEmpty, isArray } = require('lodash'); const Loby = require("./lib/loby") -const { sendAs } = require("./lib/mail-sender"); +const { sendAs, supportText } = require("./lib/mail-sender"); const { uniqueNamesGenerator, colors, animals, adjectives } = require('unique-names-generator'); const { randomBytes } = require('crypto'); @@ -135,14 +135,21 @@ class Signup extends Loby { // NOTE: Cache.lex() returns the key name itself for keys missing from the // lexicon, so `lex._x || "fallback"` keeps the raw key. These verification // strings aren't in the lexicon, so use literal copy here. + // + // One sentence with two endings: the HTML part has a button to click, the + // plain-text part has a URL to open, and telling a text reader to click a + // button that isn't there is how a mechanically-derived text body reads. + const intro_lead = "Welcome to Drumee! We're excited to have you onboard. To complete your registration and access our services, please verify your email address"; const data = { heading: "Verify Your Email Address", subheading: "Thank you for registering with Drumee", hello: `Hello ${_email},`, - intro: "Welcome to Drumee! We're excited to have you onboard. To complete your registration and access our services, please verify your email address by clicking the button below.", + intro: `${intro_lead} by clicking the button below.`, button_label: "Verify Email Address", verify_url, - fallback_label: "Or copy and paste this link into your browser:", + // Anchor text now, not a label above a printed URL — the template + // stopped rendering the tokenised link as visible body copy. + fallback_label: "Trouble with the button? Use this verification link instead.", security_title: "Security Note", security_note: "This verification link will expire in 24 hours. For your security, please do not share this email with anyone.", }; @@ -154,9 +161,27 @@ class Signup extends Loby { }); const tpl = resolve(__dirname, "./templates/verify-email.html"); const html = msg.renderFrom(tpl, data); + // Written from `data`, not stripped out of the rendered HTML, so the two + // parts say the same thing without the layout tables' spacer cells and + // icon alt text landing in the plain-text body. This is also the ONE + // place the full tokenised URL is shown as text: a plain-text reader has + // no anchor to follow, so the link has to be copyable here. + const text = [ + data.heading, + "", + data.hello, + "", + `${intro_lead} by opening the link below.`, + "", + `${data.button_label}: ${verify_url}`, + "", + `${data.security_title}: ${data.security_note}`, + "", + supportText(), + ].join("\n"); // sendAs, not msg.send: the pinned Messenger re-wraps the From and turns // a full mailbox into a "Drumee>" display name. See lib/mail-sender. - return await sendAs(msg, { to: _email, subject, html }); + return await sendAs(msg, { to: _email, subject, html, text }); } catch (e) { this.warn("[_send_verification_email] failed", e); return 0; @@ -215,7 +240,26 @@ class Signup extends Loby { }); const tpl = resolve(__dirname, "./templates/signup-completed.html"); const html = msg.renderFrom(tpl, { home, email: _email }); - return await sendAs(msg, { to: _email, subject, html }); + // Mirrors the template's own order, greeting included, and drops the + // greeting line on the same condition the template does — send_welcome + // can reach here with no address resolved. + const text = [ + "Your Drumee account is all set! Thanks for joining us.", + "", + ...(_email ? [`Hello ${_email},`, ""] : []), + "A quick note to confirm your account has been successfully created.", + "", + "Thank you so much for your interest in Drumee - we're excited to let you discover it.", + "", + "We're glad to have you!", + "", + `Discover your Drumee desk here: ${home}`, + "", + "The Drumee team", + "", + supportText(), + ].join("\n"); + return await sendAs(msg, { to: _email, subject, html, text }); } catch (e) { this.warn("[_send_signup_completed_email] failed", e); return 0; diff --git a/service/templates/signup-completed.html b/service/templates/signup-completed.html index 15230c5..cfcf065 100644 --- a/service/templates/signup-completed.html +++ b/service/templates/signup-completed.html @@ -39,10 +39,16 @@

 
- + <% if (typeof email !== 'undefined' && email) { %>

- <%= email %> + Hello <%= email %>,

<% } %>

@@ -100,41 +106,50 @@

- + - - diff --git a/service/templates/verify-email.html b/service/templates/verify-email.html index a1551d0..f74ff66 100644 --- a/service/templates/verify-email.html +++ b/service/templates/verify-email.html @@ -1,5 +1,5 @@ - + @@ -65,9 +65,15 @@

- - - + +
X
 +
X
 - - - + +
Discord
 +
Telegram
 - - - -
LinkedIn
- - 		- - - -
TikTok
- - 		- - - + +
Instagram
 +
LinkedIn
 
- -

<%= fallback_label %>

-

<%= verify_url %>

+ +

<%= fallback_label %>

 
@@ -112,41 +118,50 @@

- + - - From b0d5ac7c4494c881017972c5cadf583066770046 Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:42:10 +0700 Subject: [PATCH 07/20] Fix/apple callback error handling (#36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(oauth): answer the browser on every apple.callback failure path apple.callback had no error handling at all: no try/catch, and the success branch guarded by `if (!res.error)` with no else. Apple posts here (response_mode=form_post), so this is a top-level navigation, not an XHR — every exit has to put something in front of the browser. It did not. oauth_not_linked, invalid_state, a rejected token exchange, a JWKS fetch failure, an unverified email, a malformed id_token: all of them either fell out of the method having written nothing, or threw out of an async handler. What the user saw was a hung request or a raw error page, with no way back to sign-in. Now mirrors google.callback, which already had all of this: - missing/invalid code -> sendOauthError('access_denied') (the usual cause is the user cancelling on Apple's consent screen) - res.error -> sendOauthError(res.error) - anything thrown -> sendOauthError('oauth_failed') getOAuthCode now gets its provider argument too, so the warning names Apple instead of logging "undefined", and its quiet flag is set because we send our own response — without it the helper writes a JSON error body into what is a page navigation. This is also what makes client-side handling of ?oauth_error= reachable on the Apple path; until now there was no redirect to carry a reason. Co-Authored-By: Claude Opus 5 (1M context) * chore(oauth): drop the debug leftovers from the Google/Apple services Removed: - apple.js _getAppleProfile: this.debug("AAAA:151", userParam, payload, this.input.toJSON(), this.input.data()). This one mattered — `payload` is the VERIFIED Apple id_token, so every Apple sign-in wrote the user's email and provider sub into the server log, alongside the full request dump. - apple.js initiate: this.debug("AAAA:210", redirect_uri, authUrl), replaced with the same one-line "URL generated with state" trace google.initiate already emits, which is the part worth keeping. - apple.js handleAppleResponse: dead. Nothing calls it, it duplicates what _getAppleProfile does properly from the id_token, and its body was a console.log. - google.js: console.log("AAA:30Attr.state ", Attr.state) at module load. Attr was imported for that line alone, so the import goes too. The "[Auth] Google/Apple Credentials loaded" lines stay — those are real startup diagnostics. No behaviour change beyond what is logged. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- service/apple.js | 76 +++++++++++++++++++++++++---------------------- service/google.js | 3 +- 2 files changed, 41 insertions(+), 38 deletions(-) diff --git a/service/apple.js b/service/apple.js index b24d238..272e28b 100644 --- a/service/apple.js +++ b/service/apple.js @@ -149,7 +149,6 @@ class Register extends Loby { let firstname = ''; let lastname = ''; const userParam = this.input.get('user'); - this.debug("AAAA:151", userParam, payload, this.input.toJSON(), this.input.data()) if (userParam) { try { const userData = JSON.parse(userParam); @@ -182,17 +181,6 @@ class Register extends Loby { }; } - // Handle response - handleAppleResponse(response) { - if (response.authorization) { - const authorization = response.authorization; - const user = authorization.user; - - // Name might be in the id_token or user object - console.log("User:", user); - } - } - /** * Start Apple OAuth flow */ @@ -232,7 +220,7 @@ class Register extends Loby { `&response_mode=form_post` + `&scope=${encodeURIComponent("name email")}` + `&state=${state}`; - this.debug("AAAA:210", redirect_uri, authUrl) + this.debug('[Auth] Apple OAuth URL generated with state:', this.input.sid(), state); this.output.data({ success: true, authUrl, state: state, status: 'prompt' }); } catch (error) { this.warn('[Auth] Error initiating Apple OAuth:', error); @@ -241,36 +229,52 @@ class Register extends Loby { } /** - * - * @returns + * Apple posts here (response_mode=form_post), so EVERY exit from this method + * has to put something in front of the browser — it is a top-level navigation, + * not an XHR. Mirrors google.callback: every failure ends at sendOauthError, + * which bounces back to the signin screen carrying the reason. + * @returns */ - async callback(response) { - console.log("User:", response); - const code = this.getOAuthCode(); - if (!code) return; - const profile = await this._getAppleProfile(code); - profile.provider = 'apple'; - let res = await this.handleOAuthCallback(profile); - this.debug("AAAA:221", res) - // 2FA required: the session is pending, not finalized. Keep the pending - // session cookie (sendHtml/setAuthorization) and bounce the browser to the - // signin app's OTP screen, which finalizes via oauth.verify_otp. - if (res.status === 'otp_required') { - const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}`; - const tpl = resolve(__dirname, './templates/otp-challenge.html'); - // res.session_id is the pending cookie's id (original signin session) — - // sendHtml binds the browser's authorization to it. - this.sendHtml({ ...res, redirect }, tpl); - return; - } - const home = `https://${res.domain}${endpoint_path}/`; - if (!res.error) { + async callback() { + try { + const code = this.getOAuthCode('apple', true); + if (!code) { + // No/invalid code — typically the user cancelled on Apple's consent + // screen, which comes back as error=user_cancelled_authorize. + return this.sendOauthError('access_denied'); + } + const profile = await this._getAppleProfile(code); + profile.provider = 'apple'; + let res = await this.handleOAuthCallback(profile); + // 2FA required: the session is pending, not finalized. Keep the pending + // session cookie (sendHtml/setAuthorization) and bounce the browser to the + // signin app's OTP screen, which finalizes via oauth.verify_otp. + if (res.status === 'otp_required') { + const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}`; + const tpl = resolve(__dirname, './templates/otp-challenge.html'); + // res.session_id is the pending cookie's id (original signin session) — + // sendHtml binds the browser's authorization to it. + this.sendHtml({ ...res, redirect }, tpl); + return; + } + if (res.error) { + // invalid_state, oauth_not_linked, account creation failures... — these + // previously fell through and answered the browser with nothing at all. + return this.sendOauthError(res.error); + } + const home = `https://${res.domain}${endpoint_path}/`; const tpl = resolve(__dirname, './templates/account-created.html'); // New OAuth account: show the welcome card (auto_redirect off) so the // user lands on it; the CTA continues to the desk, where the onboarding // gate kicks in. Existing sign-ins skip the card and go straight home. const is_new = res.method === 'signup'; this.sendHtml({ ...res, home, auto_redirect: is_new ? 0 : 1 }, tpl) + } catch (e) { + // Token exchange rejected, JWKS fetch failed, unverified email, malformed + // id_token — the user gets the signin screen back instead of a hung + // request or a raw 500 page. + this.warn('[Auth] Apple OAuth callback failed:', e.message || e); + this.sendOauthError('oauth_failed'); } } diff --git a/service/google.js b/service/google.js index 9330bec..caa5e05 100644 --- a/service/google.js +++ b/service/google.js @@ -1,6 +1,6 @@ // service/google.js -const { sysEnv, Attr } = require('@drumee/server-essentials'); +const { sysEnv } = require('@drumee/server-essentials'); const { resolve } = require('path'); const { readFileSync: readJson } = require('jsonfile'); @@ -25,7 +25,6 @@ try { } catch (e) { console.error("[Auth] CRITICAL: Failed to load OAuth credentials!", e.message); } -console.log("AAA:30Attr.state ", Attr.state) /** Prevent accidentla changes */ Object.freeze(CREDENTIALS) From eb3869142962e50a706f5acd954dbafa185e0c88 Mon Sep 17 00:00:00 2001 From: "phamtobao@gmail.com" Date: Wed, 12 Aug 2026 22:32:26 +0400 Subject: [PATCH 08/20] feat(oauth): report the Google Ads sign-up conversion for new SSO accounts --- service/apple.js | 2 +- service/google.js | 2 +- service/templates/account-created.html | 28 ++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/service/apple.js b/service/apple.js index 272e28b..97e871e 100644 --- a/service/apple.js +++ b/service/apple.js @@ -268,7 +268,7 @@ class Register extends Loby { // user lands on it; the CTA continues to the desk, where the onboarding // gate kicks in. Existing sign-ins skip the card and go straight home. const is_new = res.method === 'signup'; - this.sendHtml({ ...res, home, auto_redirect: is_new ? 0 : 1 }, tpl) + this.sendHtml({ ...res, home, auto_redirect: is_new ? 0 : 1, is_new: is_new ? 1 : 0 }, tpl) } catch (e) { // Token exchange rejected, JWKS fetch failed, unverified email, malformed // id_token — the user gets the signin screen back instead of a hung diff --git a/service/google.js b/service/google.js index caa5e05..0558bcc 100644 --- a/service/google.js +++ b/service/google.js @@ -176,7 +176,7 @@ class Goggle extends Loby { // user lands on it; the CTA continues to the desk, where the onboarding // gate kicks in. Existing sign-ins skip the card and go straight home. const is_new = res.method === 'signup'; - this.sendHtml({ ...res, home, auto_redirect: is_new ? 0 : 1 }, tpl) + this.sendHtml({ ...res, home, auto_redirect: is_new ? 0 : 1, is_new: is_new ? 1 : 0 }, tpl) } catch (e) { // getToken/verifyIdToken rejected or timed out (reused code, expired // code, Google egress trouble) — the user gets the signin screen back diff --git a/service/templates/account-created.html b/service/templates/account-created.html index 13c78c5..6683561 100644 --- a/service/templates/account-created.html +++ b/service/templates/account-created.html @@ -9,6 +9,34 @@ the desk without showing the welcome card. --> <% } %> + <% if (typeof is_new !== 'undefined' && is_new) { %> + + + <% } %>
- - - + +
X
 +
X
 - - - + +
Discord
 +
Telegram
 - - - -
LinkedIn
- - 		- - - -
TikTok
- - 		- - - + +
Instagram
 +
LinkedIn