Add accessRole field to records and folders by liam-lloyd · Pull Request #835 · PermanentOrg/stela

liam-lloyd · 2026-08-06T16:10:58Z

When clients access records and folders, they need to know what permissions level the caller has with respect to that record or folder. To that end, this commit adds an accessRole field to record and folder objects returned by the API.

codecov · 2026-08-07T00:25:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.52%. Comparing base (586edd7) to head (c41110b).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #835      +/-   ##
==========================================
+ Coverage   98.48%   98.52%   +0.04%     
==========================================
  Files          92       92              
  Lines        2566     2583      +17     
  Branches      482      487       +5     
==========================================
+ Hits         2527     2545      +18     
+ Misses         39       38       -1

Flag	Coverage Δ
api	`98.52% <ø> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR adds an accessRole field to record and folder objects returned by the API so clients can understand the caller’s effective permissions (across archive membership, archive-to-archive shares, share tokens, and public access).

Changes:

Add accessRole to record and folder response models and OpenAPI schemas.
Extend record/folder listing SQL to return caller-specific access inputs (membership role, applicable share roles, share-token access).
Add/extend tests to assert accessRole behavior across membership/share/public scenarios.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
packages/api/src/record/service.ts	Maps new SQL access inputs into a computed `accessRole` for returned records.
packages/api/src/record/queries/get_records.sql	Adds per-record access-role inputs (`archiveAccessRole`, `shareAccessRoles`, `shareTokenGrantsAccess`).
packages/api/src/record/models.ts	Updates record row/DTO types to include `accessRole` and supporting access inputs.
packages/api/src/record/controller/get_single_record.test.ts	Asserts `accessRole` is present for single-record fetch.
packages/api/src/record/controller/get_records_page.test.ts	Adds coverage for `accessRole` on paged record listings across scenarios.
packages/api/src/folder/service.ts	Maps new SQL access inputs into a computed `accessRole` for returned folders.
packages/api/src/folder/queries/get_folders.sql	Adds per-folder access-role inputs (`archiveAccessRole`, `shareAccessRoles`, `shareTokenGrantsAccess`).
packages/api/src/folder/models.ts	Updates folder row/DTO types to include `accessRole` and supporting access inputs.
packages/api/src/folder/controller/get_folders_page.test.ts	Adds coverage for `accessRole` on paged folder listings across scenarios.
packages/api/src/access/permission.ts	Introduces helpers to resolve an effective access role across multiple access paths.
packages/api/src/access/permission.test.ts	Adds unit tests for the new access-role resolution helpers.
packages/api/docs/src/models/record.yaml	Documents `accessRole` on record objects.
packages/api/docs/src/models/folder.yaml	Documents `accessRole` on folder objects.

Suppressed comments (2)

packages/api/src/folder/queries/get_folders.sql:367

shareAccessRoles is computed via a per-row correlated subquery; when :email is NULL (unauthenticated), the join on account.primaryemail = :email can never match but the subquery still executes. Add an explicit :email IS NOT NULL predicate so the DB can short-circuit this subquery for unauthenticated requests.

      WHERE
        access.status = 'status.generic.ok'
    ) AS "shareAccessRoles",

packages/api/src/record/queries/get_records.sql:331

shareAccessRoles is computed via a per-row correlated subquery; when :accountEmail is NULL (unauthenticated), the join on account.primaryemail = :accountEmail can never match, but the subquery still executes. Add an explicit :accountEmail IS NOT NULL predicate so the DB can short-circuit this subquery for unauthenticated requests.

      WHERE
        access.status = 'status.generic.ok'
    ) AS "shareAccessRoles",

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+		accessRole: resolveAccessRole({
+			archiveAccessRole,
+			shareAccessRoles,
+			shareTokenGrantsAccess,
+			isPublic: row.publicAt !== null && new Date(row.publicAt) <= new Date(),


+		accessRole: resolveAccessRole({
+			archiveAccessRole,
+			shareAccessRoles,
+			shareTokenGrantsAccess,
+			isPublic: row.publicAt !== null && new Date(row.publicAt) <= new Date(),


+      WHERE
+        account_archive.archiveid = record.archiveid
+        AND account.primaryemail = :accountEmail
+        AND account_archive.status = 'status.generic.ok'
+        AND account.status = 'status.auth.ok'
+    ) AS "archiveAccessRole",


+      WHERE
+        account_archive.archiveid = folder.archiveid
+        AND account.primaryemail = :email
+        AND account_archive.status = 'status.generic.ok'
+        AND account.status = 'status.auth.ok'
+    ) AS "archiveAccessRole",


 	return accessRoleRank.get(roleOne) < accessRoleRank.get(roleTwo);
 };


slifty

Some items from a claude-guided code analysis

slifty · 2026-08-12T19:09:02Z

+	const roles: Array<AccessRole | null> = [
+		input.archiveAccessRole,
+		...(input.shareAccessRoles ?? []).map((share) =>
+			leastPermissiveAccessRole(share.archiveAccessRole, share.shareAccessRole),


Claude flagged this line: A null accessrole on a share removes the cap instead of denying

It's saying that this line "caps" a share at the caller's own archive role via leastPermissiveAccessRole. But apparently this helper treats nullish as "use the other" instead of returning null.

It's saying this means a share row where accessrole IS NULL pointing at an archive the caller owns resolves to accessRole: "owner" on a folder in someone else's archive.

Basically, missing / null should mean "no access" in this case.

Claude's catching an extreme edge case here, because access doesn't require that accessrole is non-nullable. I'll fix that rather than change the logic here (leastPermissiveAccessRole has uses elsewhere where its null handling is desired, and if we do things correctly nulls will be impossible here)

slifty · 2026-08-12T19:10:46Z

+			"Unable to resolve caller's access role for item",
+		);
+	}
+	return accessRoleToArchiveMembershipRole(accessRole);


We actually have a little bug in our implementation of accessRoleToArchiveMembershipRole which claude flagged -- the way it's defined can return undefined if the accessRole doesn't map to an enum value.

TS treats array access as never returning undefined -- that's not accurate here, and could mean we return undefined at runtime even though TS doesn't account for that.

We should harden that implementation / cover the undefined case.

We should be protected here by Typescript's type checking; that shouldn't allow a non-AccessRole argument to this function

slifty · 2026-08-12T19:11:46Z

+
+	const accessRole = mostPermissiveAccessRole(roles);
+	if (accessRole === null) {
+		// Should be unreachable: the SQL WHERE clause that selects a row already


Claude flagged that this comment isn't true... I have to admit I don't fully grok why... but it sounded credible enough so I'm going to be rude and post claude's comment verbatim:

1. Visibility and role resolution are two independent copies of the access rules, and they don't agree resolveAccessRole throws a 500 when no path applies, justified by packages/api/src/access/permission.ts:84: ▎ // Should be unreachable: the SQL WHERE clause that selects a row already guarantees at least one access path applies. That invariant isn't actually enforced — the new subqueries re-derive the access rules with different predicates than the WHERE clause that admits the row. Where they disagree, the "unreachable" branch is reachable and the whole page 500s. get_folders.sql — visibility via account_by_share (packages/api/src/folder/queries/get_folders.sql:448) is gated by access.status != 'status.generic.deleted' (:210) and folder_link.status != 'status.generic.deleted' (:393). The new shareAccessRoles subquery requires access.status = 'status.generic.ok' and share_folder_link.status = 'status.generic.ok'. status.generic.pending / invited / processing are all live values in this codebase, and access.status is nullable text with no constraint (database/base.sql:27). Any such row → visible folder, NULL share roles, 500. Same query, membership path: account_by_archive (:438) never checks account.status; the new archiveAccessRole subquery adds AND account.status = 'status.auth.ok'. Your own fixture has an account in exactly that state — account 5 / test+3@permanent.org is status.generic.invited (packages/api/src/folder/fixtures/create_test_accounts.sql) and is an ok-status owner of archive 6 (create_test_account_archives.sql, row id 36). A request from that account for a folder in archive 6 is visible-but-unresolvable. get_records.sql — the same two divergences: record_account / share_account in the WHERE (packages/api/src/record/queries/get_records.sql:407-408) have no account.status filter, and the outer folder_link join uses != 'status.generic.deleted' (:374) against the subquery's = 'status.generic.ok'. The public and share-token paths do match exactly, so those are fine. The fix I'd push for: compute the roles in a CTE and drive the WHERE clause off that same CTE, so visibility and role are one expression rather than two that must be kept in sync by hand. Then the throw is genuinely unreachable. Short of that, the predicates need to be made character-for-character identical — and one of the divergences is a real behavior question you should decide deliberately rather than inherit: should an account whose status isn't auth.ok see these items at all? Right now the two halves of the code disagree about the answer.

The problem Claude is seeing here isn't real because both folder_link and access have only the two statuses. But it does seem like a good idea to make these checks consistent.

slifty · 2026-08-12T19:16:14Z

-    ) AS "thumbnailUrls"
+    ) AS "thumbnailUrls",
+    (
+      SELECT account_archive.accessrole


Claude flagged that we don't have a unique constraint on account_archive(accountid, archiveid) and so this subquery could return more than one result => this would then throw a Two ok-status rows for the same pair and Postgres raises 21000 more than one row returned by a subquery used as an expression

It suggests adding an ORDER BY ... LIMIT 1 clause to resolve the potential edge case.

slifty · 2026-08-12T19:26:48Z

+		// Should be unreachable: the SQL WHERE clause that selects a row already
+		// guarantees at least one access path applies.
+		throw createError.InternalServerError(
+			"Unable to resolve caller's access role for item",


Claude suggested including the item ID in the error her so if it does somehow happen we'd be able to more easily diagnose. Sounds reasonable to me!

There's not another reason to pass itemId to this function though, and this is supposed to be unreachable

When clients access records and folders, they need to know what permissions level the caller has with respect to that record or folder. To that end, this commit adds an accessRole field to record and folder objects returned by the API.

liam-lloyd force-pushed the per-10716_add_access_role_to_records_and_folders branch from 18c6626 to 63955d3 Compare August 7, 2026 00:20

slifty requested a lite review from Copilot August 12, 2026 18:20

Copilot started reviewing on behalf of slifty August 12, 2026 18:20 View session

Copilot AI reviewed Aug 12, 2026

View reviewed changes

slifty reviewed Aug 12, 2026

View reviewed changes

Add accessRole field to records and folders

c41110b

When clients access records and folders, they need to know what permissions level the caller has with respect to that record or folder. To that end, this commit adds an accessRole field to record and folder objects returned by the API.

liam-lloyd force-pushed the per-10716_add_access_role_to_records_and_folders branch from 63955d3 to c41110b Compare August 12, 2026 22:31

liam-lloyd requested a review from slifty August 12, 2026 22:39

Navigation Menu

Uh oh!

Add accessRole field to records and folders - #835

Add accessRole field to records and folders#835
liam-lloyd wants to merge 1 commit into
mainfrom
per-10716_add_access_role_to_records_and_folders

liam-lloyd commented Aug 6, 2026

Uh oh!

codecov Bot commented Aug 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

slifty left a comment

Uh oh!

slifty Aug 12, 2026

Uh oh!

liam-lloyd Aug 12, 2026

Uh oh!

slifty Aug 12, 2026

Uh oh!

liam-lloyd Aug 12, 2026

Uh oh!

slifty Aug 12, 2026

Uh oh!

liam-lloyd Aug 12, 2026

Uh oh!

slifty Aug 12, 2026

Uh oh!

slifty Aug 12, 2026

Uh oh!

liam-lloyd Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		return accessRoleRank.get(roleOne) < accessRoleRank.get(roleTwo);
		};

Uh oh!

Conversation

liam-lloyd commented Aug 6, 2026

Uh oh!

codecov Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

slifty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 7, 2026 •

edited

Loading