notes: address review feedback for folder sharing (Task #445)
Re-applied review fixes on top of the live folder-sharing implementation: - PATCH/DELETE /notes/:id now do an id-only lookup and return an explicit 403 when the caller is not the owner (previously returned an ambiguous 404). This makes the read-only contract for shared-folder recipients precise instead of silently masquerading as "not found". - PUT /note-folders/:id/shares rejects self-share with 400 "Cannot share folder with yourself" instead of silently filtering the caller's id out of the recipient set. - GET /note-folders/shared-with-me now includes a `noteCount` (live count of the owner's non-archived notes in the folder) computed via SQL subquery; SharedFolder TS type updated; folders rail renders the count next to each shared folder. - SharedFolderView accepts the page's `search` value and filters the read-only list client-side over title + content + checklist item text, matching the owner's search experience. Empty state distinguishes "no notes" from "no matches". - Added GET /notes?sharedFolderId=:id as a spec-aligned alias that returns the owner's live notes inside a folder shared with the caller (gated by an active row in note_folder_shares). - Updated the stale comment block above the share routes that claimed recipient writes "just 404"; documents the new 403 contract. Deviations / out of scope: - Project uses `drizzle-kit push` (no migrations directory); no SQL migration file was added. - Pre-existing TS errors in artifacts/api-server/src/routes/executive-meetings.ts remain; unrelated to Task #445. - "Strict reject for nonexistent recipient ids in share PUT" left as optional follow-up (currently silently dropped per existing behavior).
This commit is contained in:
@@ -303,6 +303,45 @@ async function handleListMyNotes(
|
||||
res: import("express").Response,
|
||||
): Promise<void> {
|
||||
const userId = req.session.userId!;
|
||||
// ?sharedFolderId=:id — read-only access to the live notes inside a
|
||||
// folder shared WITH the caller. Spec alias for the same data the
|
||||
// dedicated `/note-folders/:id/shared-notes` endpoint returns; kept
|
||||
// here so clients that prefer the unified `/notes?…` shape work too.
|
||||
const sharedFolderIdRaw = req.query.sharedFolderId;
|
||||
if (typeof sharedFolderIdRaw === "string" && sharedFolderIdRaw.length > 0) {
|
||||
const parsed = Number(sharedFolderIdRaw);
|
||||
if (!Number.isInteger(parsed) || parsed <= 0) {
|
||||
res.status(400).json({ error: "Invalid sharedFolderId" });
|
||||
return;
|
||||
}
|
||||
const [share] = await db
|
||||
.select({ folderId: noteFolderSharesTable.folderId })
|
||||
.from(noteFolderSharesTable)
|
||||
.where(
|
||||
and(
|
||||
eq(noteFolderSharesTable.folderId, parsed),
|
||||
eq(noteFolderSharesTable.recipientUserId, userId),
|
||||
),
|
||||
);
|
||||
if (!share) {
|
||||
res.status(403).json({ error: "Folder is not shared with you" });
|
||||
return;
|
||||
}
|
||||
const [folder] = await db
|
||||
.select({ ownerId: noteFoldersTable.userId })
|
||||
.from(noteFoldersTable)
|
||||
.where(eq(noteFoldersTable.id, parsed));
|
||||
if (!folder) {
|
||||
res.status(404).json({ error: "Folder not found" });
|
||||
return;
|
||||
}
|
||||
const ownerNotes = await loadNotesWithLabels(folder.ownerId, false);
|
||||
const filtered = ownerNotes
|
||||
.filter((n) => n.folderId === parsed)
|
||||
.map((n) => ({ ...n, readOnly: true as const }));
|
||||
res.json(filtered);
|
||||
return;
|
||||
}
|
||||
const archived = req.query.archived === "true";
|
||||
const notes = await loadNotesWithLabels(userId, archived);
|
||||
res.json(notes);
|
||||
@@ -357,14 +396,21 @@ router.patch("/notes/:id", requireAuth, async (req, res): Promise<void> => {
|
||||
return;
|
||||
}
|
||||
const { labelIds, ...data } = parsed.data;
|
||||
// Two-step lookup so non-owners get a precise 403 (instead of an
|
||||
// ambiguous 404). Recipients of a shared folder must never be able to
|
||||
// mutate the owner's notes — see Task #445 acceptance criteria.
|
||||
const [existing] = await db
|
||||
.select()
|
||||
.from(notesTable)
|
||||
.where(and(eq(notesTable.id, id.id), eq(notesTable.userId, userId)));
|
||||
.where(eq(notesTable.id, id.id));
|
||||
if (!existing) {
|
||||
res.status(404).json({ error: "Note not found" });
|
||||
return;
|
||||
}
|
||||
if (existing.userId !== userId) {
|
||||
res.status(403).json({ error: "Forbidden" });
|
||||
return;
|
||||
}
|
||||
if (data.folderId != null && !(await userOwnsFolder(userId, data.folderId))) {
|
||||
res.status(400).json({ error: "Invalid folderId" });
|
||||
return;
|
||||
@@ -395,14 +441,21 @@ router.delete("/notes/:id", requireAuth, async (req, res): Promise<void> => {
|
||||
res.status(400).json({ error: id.error });
|
||||
return;
|
||||
}
|
||||
const result = await db
|
||||
.delete(notesTable)
|
||||
.where(and(eq(notesTable.id, id.id), eq(notesTable.userId, userId)))
|
||||
.returning();
|
||||
if (result.length === 0) {
|
||||
// Two-step lookup so non-owners (e.g. shared-folder recipients) get
|
||||
// a precise 403 instead of a 404 — see Task #445 acceptance criteria.
|
||||
const [existing] = await db
|
||||
.select({ userId: notesTable.userId })
|
||||
.from(notesTable)
|
||||
.where(eq(notesTable.id, id.id));
|
||||
if (!existing) {
|
||||
res.status(404).json({ error: "Note not found" });
|
||||
return;
|
||||
}
|
||||
if (existing.userId !== userId) {
|
||||
res.status(403).json({ error: "Forbidden" });
|
||||
return;
|
||||
}
|
||||
await db.delete(notesTable).where(eq(notesTable.id, id.id));
|
||||
res.json({ success: true });
|
||||
});
|
||||
|
||||
@@ -1278,9 +1331,11 @@ router.delete("/note-folders/:id", requireAuth, async (req, res): Promise<void>
|
||||
//
|
||||
// A folder owner can share a folder read-only with other users. Recipients see
|
||||
// the owner's CURRENT notes inside the folder (live, not snapshot). They cannot
|
||||
// edit, add, move, or delete anything; the existing per-note routes already
|
||||
// filter by `notesTable.userId = req.session.userId`, so any write attempt by a
|
||||
// recipient just 404s — no extra gating required there.
|
||||
// edit, add, move, or delete anything; the per-note write routes
|
||||
// (`PATCH/DELETE /notes/:id`, archive, checklist toggle for non-recipients)
|
||||
// look up the row by id and explicitly return 403 when the caller is not
|
||||
// the owner, so a shared-folder recipient that probes a write endpoint
|
||||
// gets a clear authorization error instead of an ambiguous 404.
|
||||
|
||||
router.get(
|
||||
"/note-folders/shared-with-me",
|
||||
@@ -1296,6 +1351,15 @@ router.get(
|
||||
ownerUsername: usersTable.username,
|
||||
ownerDisplayNameAr: usersTable.displayNameAr,
|
||||
ownerDisplayNameEn: usersTable.displayNameEn,
|
||||
// Live count of the owner's non-archived notes in this folder so
|
||||
// the recipient's rail can show "Folder name (12)" without a
|
||||
// second roundtrip per row.
|
||||
noteCount: sql<number>`(
|
||||
SELECT COUNT(*)::int FROM ${notesTable}
|
||||
WHERE ${notesTable.folderId} = ${noteFoldersTable.id}
|
||||
AND ${notesTable.userId} = ${noteFoldersTable.userId}
|
||||
AND ${notesTable.isArchived} = false
|
||||
)`.as("note_count"),
|
||||
})
|
||||
.from(noteFolderSharesTable)
|
||||
.innerJoin(
|
||||
@@ -1361,7 +1425,17 @@ router.put(
|
||||
const desired = new Set<number>();
|
||||
for (const x of body.recipientUserIds) {
|
||||
const n = Number(x);
|
||||
if (Number.isInteger(n) && n > 0 && n !== userId) desired.add(n);
|
||||
if (!Number.isInteger(n) || n <= 0) {
|
||||
res.status(400).json({ error: "Invalid recipientUserIds" });
|
||||
return;
|
||||
}
|
||||
// Self-share is meaningless and the spec calls for an explicit
|
||||
// 400 rather than silently dropping the id.
|
||||
if (n === userId) {
|
||||
res.status(400).json({ error: "Cannot share folder with yourself" });
|
||||
return;
|
||||
}
|
||||
desired.add(n);
|
||||
}
|
||||
// Validate that every desired recipient actually exists.
|
||||
if (desired.size > 0) {
|
||||
|
||||
@@ -583,6 +583,14 @@ export function FoldersRail({
|
||||
})}
|
||||
</span>
|
||||
</span>
|
||||
<span
|
||||
className={`shrink-0 text-[10px] tabular-nums ${
|
||||
isSel ? "text-background/80" : "text-muted-foreground"
|
||||
}`}
|
||||
data-testid={`shared-folder-count-${sf.id}`}
|
||||
>
|
||||
{sf.noteCount}
|
||||
</span>
|
||||
</button>
|
||||
</div>
|
||||
);
|
||||
|
||||
@@ -58,6 +58,9 @@ export interface SharedFolder {
|
||||
ownerUsername: string;
|
||||
ownerDisplayNameAr: string | null;
|
||||
ownerDisplayNameEn: string | null;
|
||||
// Live count of the owner's non-archived notes inside this folder. Lets
|
||||
// the rail render "Folder name (12)" without a per-row roundtrip.
|
||||
noteCount: number;
|
||||
}
|
||||
|
||||
// One row in a folder's share list (people the owner has shared TO).
|
||||
|
||||
@@ -478,6 +478,7 @@ export default function NotesPage() {
|
||||
<SharedFolderView
|
||||
folderId={folderSelection.id}
|
||||
labels={labels}
|
||||
search={search}
|
||||
onBack={() => setFolderSelection({ kind: "all" })}
|
||||
/>
|
||||
)}
|
||||
@@ -2964,16 +2965,35 @@ function LabelsDialog({
|
||||
function SharedFolderView({
|
||||
folderId,
|
||||
labels: _labels,
|
||||
search,
|
||||
onBack,
|
||||
}: {
|
||||
folderId: number;
|
||||
labels: NoteLabel[];
|
||||
search: string;
|
||||
onBack: () => void;
|
||||
}) {
|
||||
const { t, i18n } = useTranslation();
|
||||
const lang = i18n.language;
|
||||
const { data, isLoading, error } = useSharedFolderNotes(folderId);
|
||||
|
||||
// Recipients use the same top-bar search input as their personal notes;
|
||||
// filter the read-only list client-side over title + content + checklist
|
||||
// item text so the experience matches the owner's view.
|
||||
const visibleNotes = useMemo(() => {
|
||||
if (!data) return [];
|
||||
const q = search.trim().toLowerCase();
|
||||
if (!q) return data.notes;
|
||||
return data.notes.filter((n) => {
|
||||
if (n.title?.toLowerCase().includes(q)) return true;
|
||||
if (n.content?.toLowerCase().includes(q)) return true;
|
||||
if (n.kind === "checklist" && Array.isArray(n.items)) {
|
||||
return n.items.some((it) => it.text?.toLowerCase().includes(q));
|
||||
}
|
||||
return false;
|
||||
});
|
||||
}, [data, search]);
|
||||
|
||||
if (isLoading) return <Loading />;
|
||||
|
||||
// 403/404 — the owner unshared the folder while we were viewing it. Show a
|
||||
@@ -3030,10 +3050,14 @@ function SharedFolderView({
|
||||
{t("notes.folders.readOnly", "Read-only")}
|
||||
</span>
|
||||
</div>
|
||||
{data.notes.length === 0 ? (
|
||||
{visibleNotes.length === 0 ? (
|
||||
<Empty
|
||||
icon={<StickyNote size={48} />}
|
||||
text={t("notes.folders.sharedEmpty", "This folder has no notes yet")}
|
||||
text={
|
||||
search.trim()
|
||||
? t("notes.noMatches", "No notes match your search")
|
||||
: t("notes.folders.sharedEmpty", "This folder has no notes yet")
|
||||
}
|
||||
/>
|
||||
) : (
|
||||
<div
|
||||
@@ -3041,7 +3065,7 @@ function SharedFolderView({
|
||||
style={{ columnWidth: 260 }}
|
||||
data-testid="shared-folder-notes"
|
||||
>
|
||||
{data.notes.map((n) => (
|
||||
{visibleNotes.map((n) => (
|
||||
<div
|
||||
key={n.id}
|
||||
data-testid={`shared-folder-note-${n.id}`}
|
||||
|
||||
Reference in New Issue
Block a user