diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9cd3a21d..477f3acc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -41,3 +41,15 @@ jobs: run: node offline/test/media-search-names.test.js - name: Run factory projection-readiness regression test run: node --test offline/test/factory-projection-readiness.test.js + # Guards a PERMISSION bar, so it earns a CI step rather than sitting in + # offline/test unrun: it pins that Edit still cannot delete a workspace, + # and that delete_hub stays above `read` so the DMZ read-only ceiling and + # the over-limit clamp keep catching it. + - name: Run workspace-delete permission regression test + run: node --test offline/test/hub-delete-permission.test.js + # The other permission bar with a CI step of its own: it pins that a + # view-only member gets no write on the chat staging folder, and that the + # granted value keeps the write bit whichever server-essentials is + # installed. + - name: Run chat staging grant regression test + run: node --test offline/test/chat-upload-grant.test.js diff --git a/acl/hub.json b/acl/hub.json index 68de051e..d8ef1fa7 100644 --- a/acl/hub.json +++ b/acl/hub.json @@ -580,9 +580,10 @@ } }, "delete_hub": { + "doc": "Destroy a workspace: drops its database and deletes its files on disk. IRREVERSIBLE - there is no trash, no backup and no undo, and the directory removal runs detached. Asks ADMIN, not owner, since 2026-09-17 (Lexis, via Duy). The owner bar it used to carry was never a real boundary: hub.change_owner and hub.set_privilege are themselves admin-gated and neither clamps what it writes, so any workspace admin could already take ownership in a single call and then delete. Requiring owner here only added a step, while leaving the client offering a Delete row that answered 403 - the row is gated on the admin bit, see ui-team window/folder/skeleton/settings-action-panel.js. Still refuses a non-hub entity type, so a personal workspace cannot come through here.", "scope": "hub", "permission": { - "src": "owner" + "src": "admin" } }, "get_action_log": { diff --git a/acl/media.json b/acl/media.json index 480bf33a..4eb5d9ea 100644 --- a/acl/media.json +++ b/acl/media.json @@ -1834,7 +1834,7 @@ ] }, "merge_workspace": { - "doc": "Merge one workspace into another. Everything at the top level of the source workspace is moved into a new folder, named after the source, inside the destination. THE SOURCE WORKSPACE SURVIVES, EMPTIED - nothing is deleted and no database is dropped. Only files and folders cross. Chat threads stay in the database that owns them, the same rule workspace_move applies; tasks stay because task.nid addresses media rows in the source database; meetings, share links, trash and version history stay too. Members do NOT cross - the destination member list is untouched, so anyone who reached these files only through the source workspace loses them. That is the same outcome as moving a single file across workspaces today, and it keeps granting access an explicit admin-gated act rather than a side effect of a move. Asks owner on the source because it empties somebody whole workspace, the bar hub.delete_hub uses; asks only write on the destination, the bar workspace_move uses to receive files, because it adds nobody to it.", + "doc": "Merge one workspace into another. Everything at the top level of the source workspace is moved into a new folder, named after the source, inside the destination. THE SOURCE WORKSPACE SURVIVES, EMPTIED - nothing is deleted and no database is dropped. Only files and folders cross. Chat threads stay in the database that owns them, the same rule workspace_move applies; tasks stay because task.nid addresses media rows in the source database; meetings, share links, trash and version history stay too. Members do NOT cross - the destination member list is untouched, so anyone who reached these files only through the source workspace loses them. That is the same outcome as moving a single file across workspaces today, and it keeps granting access an explicit admin-gated act rather than a side effect of a move. Asks owner on the source because it empties somebody whole workspace - deliberately stricter than hub.delete_hub, which moved to admin on 2026-09-17, and not to be aligned with it without a decision of its own; asks only write on the destination, the bar workspace_move uses to receive files, because it adds nobody to it.", "scope": "hub", "permission": { "src": "owner", diff --git a/client/templates/warmup.html b/client/templates/warmup.html index 412b23e7..e0b1fe9a 100644 --- a/client/templates/warmup.html +++ b/client/templates/warmup.html @@ -14,24 +14,221 @@ .warmup-progress-bar { z-index: 1; } + /* The exported lockup (mark + wordmark as ONE vector), replacing the inline + cloud + a drumee set in Armin Grotesk. Same asset the sign-in + card shows (signin/src/assets/drumee-logo.svg, via .signin-form__logo-image), + so boot and sign-in state the brand identically. + + INLINED, not . This screen is on display precisely BECAUSE the + network is busy fetching the app bundles — a logo needing its own request + could arrive after the screen it belongs to is gone. It also drops this + screen's last dependency on a webfont: the wordmark used to be live text, + so a cold boot could paint it in a fallback face before Armin Grotesk + landed. + + Sized HERE rather than in loader.css: that file is not in this repo + (/home/drumee/static/styles/loader.css, deployed to /srv/drumee/static), + and loader.css:457 pins `.warmup-symbol svg` to width:64px — which would + squash the lockup to 64x12.7. This block is later in document order at the + same specificity, so it wins without touching the other file. + + width + height:auto, and NO preserveAspectRatio="none" (which the export + carries): the viewBox then drives the height and the mark can never be + stretched by an off-ratio box. min() keeps it inside a phone viewport. + + 240px is the FOOTPRINT THIS SCREEN ALREADY HAD: the cloud + typeset + wordmark it replaces measured ~230px wide. Sizing the lockup by width + rather than by the mark's old 64px height is what keeps that — matching + the height instead would have made it 322px wide, since the lockup gives + the wordmark more room beside the mark than the hand-built pairing did. */ + .warmup-symbol svg.warmup-logo { + width: min(240px, 72vw); + height: auto; + } + /* The caption in black, not the #433cc5 it inherits from loader.css:437. + #0B0A21 rather than #000: that is the black the lockup's own wordmark is + drawn in (fill="#0B0A21" above), so the two lines below each other read as + one colour instead of near-miss blacks. Overridden here for the same + reason the sizing above is — loader.css is not in this repo. */ + .warmup-progress-text { + color: #0b0a21; + } + /* Air between the caption and the bar, which sat 5px under the text. + PADDING on the row, not a new `bottom` on the bar: the track (::after + above) and the fill (.warmup-progress-bar, loader.css:424) are both + absolutely positioned at bottom:-5px, and an absolute child resolves + `bottom` against its containing block's PADDING box — so one property + moves the two together and they cannot drift apart. Overriding `bottom` + would have meant keeping two numbers in step, in two files. */ + .warmup-progress { + padding-bottom: 12px; + } + + /* ========================================================================= + RESPONSIVE + Measured before writing any of this, with loader.css + this template in a + headless Chromium at 320/375/390/740x360/768/834/1024/1440. Every number + quoted below is from that run. + ========================================================================= */ + + /* The splash is the viewport, not a 100vw x 100vh box inside a margined body. + At boot still carries the UA's default 8px margin — loader.css ships + no reset and the app's stylesheet has not arrived yet, which is the entire + reason this screen is on display. So loader.css:380's `width:100vw; + height:100vh` overflowed by 8px across and 16px down at EVERY viewport + measured: a 375px phone scrolled 383x683. On a handset that is a splash + screen you can pan sideways, with the card's right edge cut off. + + position:fixed resolves against the initial containing block, so the body + margin cannot reach it and no ancestor can resize it — one property instead + of a calc() that would have to name the 8px margin and track it. + + 100dvh AFTER 100vh, both declared: 100vh on a phone is the LARGEST viewport + (URL bar retracted), so centring against it puts the lockup low, partly + behind the toolbar, for as long as the toolbar is showing — which on a boot + screen is the whole time. dvh tracks the bar. The 100vh line stays as the + fallback for engines that drop the dvh declaration. */ + .warmup-main { + position: fixed; + top: 0; + left: 0; + width: 100%; + height: 100vh; + height: 100dvh; + } + + /* The card must be a ceiling, not a floor. loader.css:405 sets width:455px on + a content-box with 32px padding — a 519px border box — and caps it with + max-width:100vw, which caps the CONTENT box, so the border box stayed 64px + wider than the screen. Measured on a 375px phone: the card was 439px at + x:-24, hanging 24px off BOTH edges, with zero effective padding. The + progress track (::after above) is `left:0; right:0` on that card, so it ran + the full width of the screen and had its 100px rounded ends clipped off at + each side. + + border-box makes the declared width include the padding, so max-width:100% + clamps the whole box. 100% rather than another 100vw: on desktop 100vw + includes the scrollbar gutter, 100% does not. + + The width is restated as calc(455px + 64px) because switching to border-box + would otherwise have RESHAPED the desktop card: loader.css's 455px would + start including the padding, leaving 391px of content and a 391px progress + bar where the design has always had 455px. Written as a sum rather than as + 519px so it stays legible as "loader.css:405's width, plus the padding a + side it was always drawn outside of". Nothing above phone width is resized + by this change. */ + .warmup-01 { + box-sizing: border-box; + width: calc(455px + 64px); + max-width: 100%; + } + + /* --- Phone, portrait ---------------------------------------------------- */ + @media (max-width: 599px) { + /* loader.css:385 rounds .warmup-main by 32px, but that element is the whole + screen — on a handset the radius does not round a card, it punches four + notches of the body's #f6f6f6 into the corners of the display. */ + .warmup-main { + border-radius: 0; + } + + /* 32px a side spends 64px of a 320px screen on air. 20px keeps a visible + gutter while leaving the progress bar a run long enough to read as a + progress bar. */ + .warmup-01 { + padding: 24px 20px; + gap: 12px; + } + + /* 72vw was sized for the desktop card, where it never binds (it is 240px + from 334px up). On a phone it DOES bind, and 72vw of a 320px screen is + 230px of lockup — the mark ends up louder than the app it is booting. + 60vw holds it to ~192px there and still hits the designed 240px by 400px + wide. */ + .warmup-symbol svg.warmup-logo { + width: min(240px, 60vw); + } + + /* 20px/30px is the desktop caption. At phone width it sits directly over a + bar that is now ~335px instead of 455px; 17px keeps "Warming Up..." on + one line at 320px with the shorter measure. */ + .warmup-progress-text { + font-size: 17px; + line-height: 24px; + } + } + + /* --- Tablet ------------------------------------------------------------- */ + /* The 519px card is already right on a 768-1024px screen, so this block does + not resize it — the lockup deliberately keeps its designed 240px footprint + here rather than being inflated to fill a bigger display. What a tablet + needs is the same full-bleed correction as a phone (it is a device screen, + not a window, so the same four corner notches apply) and a guaranteed + gutter so the card never grows flush to the edges on the narrow end of the + range. */ + @media (min-width: 600px) and (max-width: 1024px) { + .warmup-main { + border-radius: 0; + } + + .warmup-01 { + max-width: calc(100% - 64px); + } + } + + /* --- Short viewports: phone and tablet in landscape --------------------- */ + /* Keyed on height, not on a device: a 740x360 phone held sideways and a + tablet with the keyboard up both land here. The screen is centred content + in a box roughly 300px tall once the browser chrome is out, and the + portrait stack (240px lockup + 16px gap + 42px progress row + 64px padding) + is most of that. Compacting the lockup and the padding keeps the caption + and the bar clear of the chrome instead of being clipped by + .warmup-main's overflow:hidden. */ + @media (max-height: 480px) { + .warmup-01 { + padding: 16px 20px; + gap: 8px; + } + + .warmup-symbol svg.warmup-logo { + width: min(180px, 34vw); + } + + .warmup-progress-text { + font-size: 16px; + line-height: 22px; + } + }
- drumee
diff --git a/offline/test/chat-upload-grant.test.js b/offline/test/chat-upload-grant.test.js new file mode 100644 index 00000000..85713104 --- /dev/null +++ b/offline/test/chat-upload-grant.test.js @@ -0,0 +1,139 @@ +#!/usr/bin/env node + +// Who may write to the chat staging folder, and with what value. +// +// THE REPORT: a member whose workspace role is Chat could not attach a file in +// chat. The upload answered 403 and the chat showed nothing at all -- no +// attachment chip, no error -- while the same account attached files normally +// in a workspace where it happened to be an admin. +// +// An attachment stages in the hidden folder '/__chat__/__upload__' before it +// becomes a message. A chat member has no write bit for the workspace, which is +// intended, so the membership paths give write on that one folder through a +// grant written with assign_via 'no_traversal'. Two things were wrong: the +// value granted was 4, which meant write before the permission bits were +// renumbered and means download now, and the grant went out regardless of role, +// so view-only members held it too. +// +// These lock the two properties the fix rests on, both of which can be undone +// by an edit that looks like a simplification: +// +// 1. the gate admits a role that may chat and refuses one that may not; +// 2. the granted value carries the write bit, and does not come from the +// package, whose 1.3.6 release republished the pre-1.3.0 layout. +// +// Deliberately dependency-free: member-capability.js requires nothing, and the +// call sites are read as text, so this runs on a stock runner with no install. + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { readFileSync } = require("node:fs"); +const { join } = require("node:path"); + +const { + CAN_CHAT, + CAN_WRITE, + CAN_READ, + CHAT_UPLOAD_GRANT, + privilegeAllows, +} = require("../../service/lib/member-capability"); + +// The stored privilege for each role selector the UI offers. +const VIEW = 0b000011; +const CHAT = 0b000111; +const EDIT = 0b001111; +const ADMIN = 0b011111; +const OWNER = 0b111111; + +test("the gate refuses a view-only member", () => { + assert.equal(privilegeAllows(VIEW, CAN_CHAT), false, + "a member who may only read must not be given write on the chat staging " + + "folder; the grant used to go out regardless of role, so rows for " + + "view-only members already exist in the wild"); +}); + +test("the gate admits every role at or above chat", () => { + for (const [name, privilege] of [ + ["chat", CHAT], ["edit", EDIT], ["admin", ADMIN], ["owner", OWNER], + ]) { + assert.equal(privilegeAllows(privilege, CAN_CHAT), true, + `${name} may chat and must be able to attach a file`); + } +}); + +// The trap the gate exists to avoid. `Constants.permission.chat` is 0b0000110 -- +// read OR download -- so it OVERLAPS read, and a loose `privilege & chat` is +// truthy for a view-only member (3 & 6 = 2). privilegeAllows asks for every bit +// of the mask, so the overlap is not enough. +test("a multi-bit mask that overlaps read still refuses a view-only member", () => { + const READ_OR_DOWNLOAD = 0b0000110; + assert.notEqual(VIEW & READ_OR_DOWNLOAD, 0, + "the premise: the loose form would admit a view-only member here"); + assert.equal(privilegeAllows(VIEW, READ_OR_DOWNLOAD), false, + "privilegeAllows must require the whole mask, not a partial overlap"); +}); + +test("the granted value carries the write bit", () => { + assert.equal((CHAT_UPLOAD_GRANT & CAN_WRITE) === CAN_WRITE, true, + "without the write bit the upload ACL refuses and the member gets the " + + "same silent 403 this change exists to fix"); + assert.equal((CHAT_UPLOAD_GRANT & CAN_READ) === CAN_READ, true, + "the member has to read the staging folder as well as write to it"); +}); + +// server-essentials 1.3.6 republished the pre-1.3.0 layout: Privilege.WRITE +// resolves to 7 there and to 15 under the pinned 1.3.1. package.json asks for +// ^1.3.1, so reading the value from the package means a dependency bump nobody +// connects to chat silently removes the write bit again. +test("the granted value is not the value the republished package would give", () => { + assert.notEqual(CHAT_UPLOAD_GRANT, 0b0000111, + "0b0000111 is Privilege.WRITE under server-essentials 1.3.6 and carries " + + "no write bit in this schema"); +}); + +// The gate has to sit at the call site. A check one layer up is not the same +// invariant: three separate paths grant this row, and each has its own idea of +// where `privilege` comes from. +const HUB_SOURCE = readFileSync( + join(__dirname, "..", "..", "service", "private", "hub.js"), + "utf8" +); + +test("every chat staging grant in hub.js is gated at the call site", () => { + const lines = HUB_SOURCE.split("\n"); + const sites = []; + lines.forEach((line, i) => { + if (line.includes("chat_upload_id") && !line.trim().startsWith("*")) { + sites.push(i); + } + }); + assert.ok(sites.length >= 3, + `expected the three membership paths to grant this row, found ${sites.length}`); + + for (const i of sites) { + const window = lines.slice(Math.max(0, i - 12), i + 2).join("\n"); + const isGrant = window.includes("permission_grant"); + if (!isGrant) continue; + assert.ok( + window.includes("CAN_CHAT") && window.includes("privilegeAllows"), + `the permission_grant near line ${i + 1} writes the chat staging row ` + + "without a chat-bit check above it" + ); + assert.ok( + !/,\s*4\s*,/.test(window), + `the permission_grant near line ${i + 1} still passes a bare 4, the ` + + "value that means download since the bits were renumbered" + ); + } +}); + +test("a role change gives the row back and takes it away", () => { + const start = HUB_SOURCE.indexOf("async set_privilege()"); + assert.notEqual(start, -1, "set_privilege not found in hub.js"); + const body = HUB_SOURCE.slice(start, start + 2000); + assert.ok(body.includes("permission_grant"), + "promoting a member to a chat role has to give them the staging row"); + assert.ok(body.includes("permission_revoke"), + "demoting a member to view-only has to take the staging row back; " + + "set_privilege only ever granted, so a demoted member kept uploading"); +}); diff --git a/offline/test/hub-delete-permission.test.js b/offline/test/hub-delete-permission.test.js new file mode 100644 index 00000000..cfb43140 --- /dev/null +++ b/offline/test/hub-delete-permission.test.js @@ -0,0 +1,127 @@ +#!/usr/bin/env node + +/** + * @license + * Copyright 2024 Thidima SA. All Rights Reserved. + * Licensed under the GNU AFFERO GENERAL PUBLIC LICENSE, Version 3 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.gnu.org/licenses/agpl-3.0.html + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ============================================================================= + */ + +/** + * `hub.delete_hub` asks for ADMIN, and the blast radius that decision rests on. + * + * Lexis, via Duy, 2026-09-17. The owner bar this service used to carry was never + * a real boundary: `hub.change_owner` and `hub.set_privilege` are themselves + * `src: admin` and neither clamps what it writes, so any workspace admin could + * already take ownership in a single call and then delete. Requiring owner only + * added a step, while leaving the client offering a Delete row that answered + * 403 (ui-team gates that row on the admin bit). + * + * What is pinned here is not the constant — it is the two properties that made + * widening it safe, either of which a later edit could quietly take away: + * + * 1. Edit / Chat / View are still refused. That is the whole point of the + * report this came from. + * 2. `delete_hub` still sits ABOVE `read`, which is what keeps the DMZ / + * secure-share read-only ceiling and the over-limit clamp catching it + * (router/rest/index.js `mightMutate`). Owner and admin both clear that + * bar; anything at or below `read` would not. + * + * DEPENDENCY-FREE BY DESIGN. The CI job that runs this deliberately does not + * `npm install` (see .github/workflows/test.yml), so the private + * @drumee/server-essentials package is usually absent on the runner. The bit + * values are therefore declared locally and every assertion works from them; + * when the package IS present we additionally check the local copy still + * matches the shipped table, so drift is caught rather than assumed away. Same + * arrangement as secure-share-session.test.js. + */ +const test = require("node:test"); +const assert = require("node:assert"); +const { readFileSync } = require("node:fs"); +const { join } = require("node:path"); + +const REPO_ROOT = join(__dirname, "..", ".."); +const ACL = (name) => + JSON.parse(readFileSync(join(REPO_ROOT, "acl", `${name}.json`), "utf8")).services; + +// server-essentials lib/lex/permission.js — the single BITS a service asks for. +const BIT = { read: 0b0000010, write: 0b0001000, admin: 0b0010000, owner: 0b0100000 }; + +// The stored privilege WORDS a member can hold — hub.set_privilege writes these +// and user_permission() hands them back. +const ROLE = { view: 0b0000011, chat: 0b0000111, edit: 0b0001111, admin: 0b0011111, owner: 0b0111111 }; + +// Exactly what lib/acl.js check_source does with each row. +const granted = (privilege, asked) => !!(privilege & asked); + +let permissionValue = null; +try { + ({ permissionValue } = require("@drumee/server-essentials")); +} catch (e) { + console.log(` ~ shipped-table cross-check SKIPPED (server-essentials not installed: ${e.code || e.message})`); +} + +test("the local bit values still match the shipped table", { skip: !permissionValue }, () => { + for (const [name, value] of Object.entries(BIT)) { + assert.equal(permissionValue(name), value, `permission bit \`${name}\` drifted`); + } +}); + +test("hub.delete_hub asks for the admin bit", () => { + const spec = ACL("hub").delete_hub; + assert.equal(spec.permission.src, "admin"); + assert.equal(spec.scope, "hub"); +}); + +test("Admin and Owner may delete; Edit, Chat and View may not", () => { + const asked = BIT[ACL("hub").delete_hub.permission.src]; + assert.ok(asked, "delete_hub asks for a bit this test does not know"); + for (const [role, want] of Object.entries({ + view: false, chat: false, edit: false, admin: true, owner: true, + })) { + assert.equal( + granted(ROLE[role], asked), want, + `${role} (${ROLE[role]}) got the wrong answer against asked=${asked}`, + ); + } +}); + +test("delete_hub still sits above `read`, so the read-only clamps still catch it", () => { + // router/rest/index.js: mightMutate = permission.src > READ_LEVEL. A + // secure-share recipient carries a read-only session ceiling and an + // over-limit domain is clamped to reads; both rely on this comparison, NOT on + // a service name, so widening owner -> admin must not cross that line. + const src = BIT[ACL("hub").delete_hub.permission.src]; + assert.ok(src > BIT.read, "delete_hub would escape the read-only clamps"); +}); + +test("the other workspace-destroying services are deliberately NOT aligned", () => { + // merge_workspace empties a whole workspace and copy_workspace duplicates + // one; both still ask owner. They were left alone on purpose — moving them is + // a decision of its own, not a tidy-up. See their `doc` in acl/media.json. + const media = ACL("media"); + assert.equal(media.merge_workspace.permission.src, "owner"); + assert.equal(media.copy_workspace.permission.src, "owner"); +}); + +test("a personal workspace can never come through delete_hub", () => { + // service/private/hub.js refuses anything whose entity type is not `hub`, so + // a personal workspace (a folder in the caller's own home, whose hub_id is + // the caller's drumate) meets WRONG_ENTITY_TYPE rather than this permission. + const src = readFileSync(join(REPO_ROOT, "service", "private", "hub.js"), "utf8"); + const body = src.slice(src.indexOf("async delete_hub()")); + const guard = body.indexOf("WRONG_ENTITY_TYPE"); + const destroy = body.indexOf("entity_delete"); + assert.ok(guard > 0, "delete_hub lost its entity-type guard"); + assert.ok(destroy > guard, "entity_delete is reachable before the type guard"); +}); diff --git a/service/lib/member-capability.js b/service/lib/member-capability.js index d7231656..087cd925 100644 --- a/service/lib/member-capability.js +++ b/service/lib/member-capability.js @@ -41,6 +41,24 @@ const CAN_CHAT = 0b0000100; const CAN_WRITE = 0b0001000; const CAN_ADMIN = 0b0010000; +/** + * What a member is granted on the hidden chat staging folder + * ('/__chat__/__upload__'), where an attachment is held before it becomes a + * message. read + download + write, plus the low bit every stored role mask + * carries. The grant is written with assign_via 'no_traversal', so it applies + * to that one folder and user_permission will not let it reach anything + * inside it. + * + * 🚨 Pinned here for the same reason every other value in this file is, and + * this one has already moved twice. `Privilege.WRITE` resolves to 15 under + * server-essentials 1.3.1 and to 7 under 1.3.6, which republished the + * pre-1.3.0 layout. package.json asks for ^1.3.1, so a bare `npm install` + * picks up 1.3.6 and a grant written from `Privilege.WRITE` would carry no + * write bit at all -- the exact 403 this value exists to fix, reintroduced + * by a dependency bump nobody connected to chat. + */ +const CHAT_UPLOAD_GRANT = 0b0001111; + /** * Does this stored privilege carry every bit of `bit`? * @@ -145,6 +163,7 @@ async function memberCan(service, bit) { module.exports = { CAN_READ, + CHAT_UPLOAD_GRANT, CAN_DOWNLOAD, CAN_CHAT, CAN_WRITE, diff --git a/service/private/hub.js b/service/private/hub.js index 5f1af3a3..24563ef7 100644 --- a/service/private/hub.js +++ b/service/private/hub.js @@ -33,6 +33,9 @@ const { butlerFrom } = require("../lib/mail-sender"); const { mailFailure } = require("../lib/mail-result"); const { resolveHubInviteName } = require("../lib/hub-invite-name"); const { resolveHubDisplayName } = require("../lib/hub-display-name"); +const { + CAN_CHAT, CHAT_UPLOAD_GRANT, privilegeAllows +} = require("../lib/member-capability"); const { MfsTools } = require("@drumee/server-core"); const { remove_dir } = MfsTools; const { toArray } = utils; @@ -1001,10 +1004,17 @@ class __private_hub extends Hub { await this.db.await_proc( "permission_grant", "*", uid, expiry, privilege, "system", message ); - await this.db.await_proc( - "permission_grant", mfs_home.chat_upload_id, uid, 0, 4, - "no_traversal", "chat upload permission" - ); + // Chat attachments stage in a hidden folder before they become a message. + // A member who may chat has to write there even though the role carries no + // write bit for the workspace at large -- 'no_traversal' keeps that raised + // access on this one folder. A view-only member may not chat, so granting + // it would hand them an upload path they are not entitled to. + if (privilegeAllows(privilege, CAN_CHAT)) { + await this.db.await_proc( + "permission_grant", mfs_home.chat_upload_id, uid, 0, CHAT_UPLOAD_GRANT, + "no_traversal", "chat upload permission" + ); + } await writeAudit(this, { db: this.hub.get(Attr.db_name), uid: this.uid, @@ -2139,14 +2149,16 @@ class __private_hub extends Hub { '*', uid, expiry, privilege, 'system', msg ); - // Grant chat upload permission if chat folder exists - if (mfs_home && mfs_home.chat_upload_id) { + // Write access to the chat staging folder only, and only for a role + // that may chat -- see _grantMembership for why it is scoped this way. + if (mfs_home && mfs_home.chat_upload_id && + privilegeAllows(privilege, CAN_CHAT)) { await this.yp.await_proc( `${hub_db}.permission_grant`, mfs_home.chat_upload_id, uid, 0, // no expiry on chat upload - 4, // read+write for uploads + CHAT_UPLOAD_GRANT, 'no_traversal', 'chat upload permission' ); @@ -2812,15 +2824,25 @@ class __private_hub extends Hub { for (let uid of users) { await this.db.await_proc("permission_set", uid, privilege); - await this.db.await_proc( - "permission_grant", - mfs_home.chat_upload_id, - uid, - 0, - 4, - "no_traversal", - "chat upload permission" - ); + // A role change has to move the chat staging grant in BOTH directions. + // Granting on the way up is what lets a chat member attach a file; + // revoking on the way down is what stops a member demoted to view-only + // from keeping an upload path the new role does not carry. + if (privilegeAllows(privilege, CAN_CHAT)) { + await this.db.await_proc( + "permission_grant", + mfs_home.chat_upload_id, + uid, + 0, + CHAT_UPLOAD_GRANT, + "no_traversal", + "chat upload permission" + ); + } else { + await this.db.await_proc( + "permission_revoke", mfs_home.chat_upload_id, uid + ); + } hub = {}; hub.privilege = privilege;