diff --git a/docs/my-resources.md b/docs/my-resources.md index c0be160f..ddd5c66d 100644 --- a/docs/my-resources.md +++ b/docs/my-resources.md @@ -452,13 +452,17 @@ The previous version is replaced, not versioned. ## Sharing -A resource is its author's. `POST /api/my-resources/:id/shares` (by email, -exact — accounts are never listed) and `PUT …/shares/all` extend *reading* — -open, preview, download — to named people or to everyone signed in; the rows -live in `user_resource_shares` and `user_resources.shared_with_all`. Every -write route (modify, theme, delete, the share list itself) still filters on -`user_id`. The list route returns a person's own resources first, then what is -shared with them, each row saying `owned` and `shared_by_name`. +A resource is its author's, and it reaches another person one way only: a +link. `POST /api/my-resources/:id/share-link` mints a token (stored hashed, +valid 30 days by default); `GET …/share-link/:token` says what the link is +for; `POST …/share-link/:token/accept` adds the resource to the signed-in +recipient's list as a reader — open, preview, download — with a row in +`user_resource_shares`. The author sees who accepted and can withdraw any of +them. There is no share-with-everyone and no sharing by name: nobody's +resources appear in anyone else's list unless that person followed a link. +Every write route (modify, theme, delete, the share list itself) still filters +on `user_id`. The list route returns a person's own resources first, then what +is shared with them, each row saying `owned` and `shared_by_name`. ## Preview diff --git a/public/components/faq.html b/public/components/faq.html index 334d5834..a00a22ed 100644 --- a/public/components/faq.html +++ b/public/components/faq.html @@ -272,7 +272,7 @@

Preview shows every page in place, on a phone too. Download as PowerPoint, Word or PDF, or send the rendered file straight to your Nextcloud; articles download as Word or PDF.

-

Share makes a link you can send however you like; whoever follows it, signed in, is asked whether to add the resource to their own library, and can then open, preview and download it — not change it. There is also a switch to open a resource to everyone signed in. What others share with you appears marked "Shared by …".

+

Share makes a link you can send however you like; whoever follows it, signed in, is asked whether to add the resource to their own library, and can then open, preview and download it — not change it. Sharing is by link only: nothing of yours appears in anyone else's library unless they followed your link. What others share with you appears marked "Shared by …".

diff --git a/public/js/myResources.js b/public/js/myResources.js index 63195843..ad600c8c 100644 --- a/public/js/myResources.js +++ b/public/js/myResources.js @@ -816,12 +816,6 @@ from.style.cssText = 'font-size:11px;color:var(--blue);'; from.textContent = 'Shared by ' + row.shared_by_name; body.appendChild(from); - } else if (row.owned !== false && row.shared_with_all) { - var all = document.createElement('div'); - all.className = 'mr-shared-note'; - all.style.cssText = 'font-size:11px;color:var(--g500);'; - all.textContent = 'Shared with everyone'; - body.appendChild(all); } body.appendChild(title); body.appendChild(meta); @@ -997,17 +991,15 @@ '' + 'Send it however you like. Whoever follows it and accepts gets it in their resources; the link works for 30 days.' + '' + - '' + '
'; rowEl.appendChild(panel); var base = '/api/my-resources/' + encodeURIComponent(id) + '/shares'; function paint(state) { - panel.querySelector('.mr-share-all').checked = !!state.sharedWithAll; var list = panel.querySelector('.mr-share-people'); list.innerHTML = ''; if (!state.people.length) { var none = document.createElement('div'); none.style.cssText = 'font-size:12px;color:var(--g500);'; - none.textContent = state.sharedWithAll ? 'Shared with everyone.' : 'Nobody has accepted a link yet.'; + none.textContent = 'Nobody has accepted a link yet.'; list.appendChild(none); return; } state.people.forEach(function (p) { @@ -1032,26 +1024,9 @@ .then(function (d) { if (!d.success) throw new Error(d.error || 'Could not load'); paint(d); - var note = rowEl.querySelector('.mr-shared-note'); - if (d.sharedWithAll && !note) { - note = document.createElement('div'); - note.className = 'mr-shared-note'; - note.style.cssText = 'font-size:11px;color:var(--g500);'; - note.textContent = 'Shared with everyone'; - var body = rowEl.querySelector('.mr-row-body') || rowEl.firstElementChild; - if (body) body.appendChild(note); - } else if (!d.sharedWithAll && note) { - note.remove(); - } }) .catch(function (e) { showToast(e.message, 'error'); }); } - panel.querySelector('.mr-share-all').addEventListener('change', function (e) { - fetch(base + '/all', { method: 'PUT', headers: getAuthHeaders(), body: JSON.stringify({ value: e.target.checked }) }) - .then(function (r) { return r.json(); }) - .then(function (d) { if (!d.success) throw new Error(d.error || 'Could not change sharing'); showToast(d.sharedWithAll ? 'Shared with everyone on the site' : 'No longer shared with everyone', 'success'); return load(); }) - .catch(function (err) { showToast(err.message, 'error'); load(); }); - }); panel.querySelector('.mr-share-link').addEventListener('click', function () { var out = panel.querySelector('.mr-share-link-url'); fetch('/api/my-resources/' + encodeURIComponent(id) + '/share-link', { method: 'POST', headers: getAuthHeaders(), body: JSON.stringify({ days: 30 }) }) diff --git a/src/routes/myResources.js b/src/routes/myResources.js index 67d221a2..c37805a8 100644 --- a/src/routes/myResources.js +++ b/src/routes/myResources.js @@ -597,13 +597,13 @@ async function readableResource(id, userId, columns) { return db.get( 'SELECT ' + columns.split(',').map(function (c) { return 'r.' + c.trim(); }).join(', ') + ' FROM user_resources r LEFT JOIN user_resource_shares s ON s.resource_id = r.id AND s.user_id = ? ' + - 'WHERE r.id = ? AND (r.user_id = ? OR r.shared_with_all OR s.user_id IS NOT NULL)', + 'WHERE r.id = ? AND (r.user_id = ? OR s.user_id IS NOT NULL)', [userId, parseInt(id, 10), userId] ); } async function ownedResource(id, userId) { - return db.get('SELECT id, title, shared_with_all FROM user_resources WHERE id = ? AND user_id = ?', [parseInt(id, 10), userId]); + return db.get('SELECT id, title FROM user_resources WHERE id = ? AND user_id = ?', [parseInt(id, 10), userId]); } router.get('/my-resources/:id/shares', async function (req, res) { @@ -613,23 +613,12 @@ router.get('/my-resources/:id/shares', async function (req, res) { var people = await db.all( 'SELECT u.id, u.name, u.email FROM user_resource_shares s JOIN users u ON u.id = s.user_id ' + 'WHERE s.resource_id = ? ORDER BY s.created_at', [row.id]); - res.json({ success: true, sharedWithAll: !!row.shared_with_all, people: people }); + res.json({ success: true, people: people }); } catch (err) { res.status(500).json({ error: 'Could not load the share list' }); } }); // Everyone signed in, or not. A switch rather than a list, since "the whole // department" has no useful list. -router.put('/my-resources/:id/shares/all', async function (req, res) { - try { - var row = await ownedResource(req.params.id, req.user.id); - if (!row) return res.status(404).json({ error: 'Not found' }); - var on = req.body.value === true || String(req.body.value) === 'true'; - await db.run('UPDATE user_resources SET shared_with_all = ? WHERE id = ? AND user_id = ?', [on, row.id, req.user.id]); - logger.audit(req.user.id, on ? 'resource_share_all' : 'resource_unshare_all', 'Resource ' + row.id + (on ? ' shared with everyone' : ' no longer shared with everyone'), req, { category: 'clinical' }); - res.json({ success: true, sharedWithAll: on }); - } catch (err) { res.status(500).json({ error: 'Could not change sharing' }); } -}); - // By link. The author makes one and sends it however they like; whoever // follows it, signed in, is shown what it is and who from, accepts, and the // resource joins their library. Only the token's hash is stored. A link can @@ -837,11 +826,11 @@ router.get('/my-resources', async function (req, res) { 'SELECT r.id, r.title, r.kind, r.topic, r.grounded_count, r.created_at, r.updated_at, ' + "(r.deck IS NOT NULL AND jsonb_array_length(COALESCE(r.deck->'slides', '[]'::jsonb)) > 0) AS has_deck, " + "COALESCE(r.theme, r.deck->>'theme', '') AS theme, " + - '(r.user_id = ?) AS owned, r.shared_with_all, ' + + '(r.user_id = ?) AS owned, ' + 'CASE WHEN r.user_id = ? THEN NULL ELSE u.name END AS shared_by_name ' + 'FROM user_resources r JOIN users u ON u.id = r.user_id ' + 'LEFT JOIN user_resource_shares s ON s.resource_id = r.id AND s.user_id = ? ' + - 'WHERE r.user_id = ? OR r.shared_with_all OR s.user_id IS NOT NULL ' + + 'WHERE r.user_id = ? OR s.user_id IS NOT NULL ' + 'ORDER BY (r.user_id = ?) DESC, r.created_at DESC LIMIT ?', [req.user.id, req.user.id, req.user.id, req.user.id, req.user.id, MAX_PER_USER * 2] ); diff --git a/test/backend-hardening.test.js b/test/backend-hardening.test.js index 2fa2eeec..ce5d4573 100644 --- a/test/backend-hardening.test.js +++ b/test/backend-hardening.test.js @@ -329,7 +329,6 @@ test('opening the share panel does not redraw the library, and the Nextcloud upl const js = read('public/js/myResources.js'); const panel = js.slice(js.indexOf('function openSharePanel('), js.indexOf('function openPreview(')); assert.doesNotMatch(panel, /loadLibrary\(\)/, 'a redraw rebuilds the rows and takes the panel with it'); - assert.match(panel, /mr-shared-note/); assert.match(js, /createTextNode\(' Nextcloud'\)/); }); diff --git a/test/resource-sharing.test.js b/test/resource-sharing.test.js index c74cc1b2..36cd45dc 100644 --- a/test/resource-sharing.test.js +++ b/test/resource-sharing.test.js @@ -8,7 +8,8 @@ const read = f => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); test('every read route goes through the reader rule; every write route still filters on the owner', () => { const route = read('src/routes/myResources.js'); - assert.match(route, /WHERE r\.id = \? AND \(r\.user_id = \? OR r\.shared_with_all OR s\.user_id IS NOT NULL\)/); + assert.match(route, /WHERE r\.id = \? AND \(r\.user_id = \? OR s\.user_id IS NOT NULL\)/); + assert.doesNotMatch(route, /shares\/all|shared_with_all/, 'sharing is by link only'); for (const marker of ["router.get('/my-resources/:id'", "router.get('/my-resources/:id/export'", "router.get('/my-resources/:id/preview'", "router.get('/my-resources/:id/preview/:page'"]) { const body = route.slice(route.indexOf(marker), route.indexOf('\n});', route.indexOf(marker))); assert.match(body, /await readableResource\(req\.params\.id, req\.user\.id/, marker + ' reads through the rule'); @@ -19,7 +20,7 @@ test('every read route goes through the reader rule; every write route still fil assert.doesNotMatch(body, /readableResource/, marker + ' is not opened to readers'); } // The share list itself is the owner's, and adding by email never lists accounts. - for (const marker of ["router.get('/my-resources/:id/shares'", "router.put('/my-resources/:id/shares/all'", "router.post('/my-resources/:id/share-link'", "router.delete('/my-resources/:id/shares/:userId'"]) { + for (const marker of ["router.get('/my-resources/:id/shares'", "router.post('/my-resources/:id/share-link'", "router.delete('/my-resources/:id/shares/:userId'"]) { const body = route.slice(route.indexOf(marker), route.indexOf('\n});', route.indexOf(marker))); assert.match(body, /await ownedResource\(req\.params\.id, req\.user\.id\)/, marker + ' is owner-only'); } @@ -37,13 +38,13 @@ test('every read route goes through the reader rule; every write route still fil test('the list carries whose each row is, and the page offers changes only on your own', () => { const route = read('src/routes/myResources.js'); - assert.match(route, /\(r\.user_id = \?\) AS owned, r\.shared_with_all/); + assert.match(route, /\(r\.user_id = \?\) AS owned, /); assert.match(route, /CASE WHEN r\.user_id = \? THEN NULL ELSE u\.name END AS shared_by_name/); const js = read('public/js/myResources.js'); assert.match(js, /if \(row\.owned === false\) \{ return wrap; \}/, 'a shared row has Preview and downloads, nothing else'); assert.match(js, /library\.filter\(function \(row\) \{ return row\.owned !== false; \}\)/, 'Modify offers only your own'); assert.match(js, /'Shared by ' \+ row\.shared_by_name/); - assert.match(js, /mr-share-all/); assert.match(js, /mr-share-link/); assert.match(js, /Withdraw the share/); + assert.doesNotMatch(js, /mr-share-all/); assert.match(js, /mr-share-link/); assert.match(js, /Withdraw the share/); assert.match(js, /localStorage\.getItem\('ped_pending_share'\)/, 'a followed link is acted on once signed in'); assert.match(js, /Add it to your resources\?/); assert.match(read('public/js/app.js'), /raw\.indexOf\('share=' \) === 0|raw\.indexOf\('share='\) === 0/);