Feat/restrict metadata by alvinsw · Pull Request #23 · Language-Research-Technology/arocapi

alvinsw · 2026-08-24T01:27:52Z

Added access restriction functionality based on the:

For endpoints /crate, /entity, /file: responds with 403 when access.metadata returned by accessTransformer function is false.
For endpoints /search, /entities, /files: Use the resolveValidLicenses helper function (if it is specified in the options) to retrieve a list of valid licenses granted to the logged-in user and use the list to filter the search query.

github-actions · 2026-08-24T01:30:03Z

Coverage Report

Status	Category	Percentage	Covered / Total
🟢	Lines	100% (🎯 100%)	334 / 334
🟢	Statements	100% (🎯 100%)	345 / 345
🟢	Functions	100% (🎯 100%)	50 / 50
🟢	Branches	100% (🎯 100%)	209 / 209

File Coverage

File	Stmts	Branches	Functions	Lines
Changed Files
src/app.ts	100%	100%	100%	100%
src/routes/crate.ts	100%	100%	100%	100%
src/routes/entities.ts	100%	100%	100%	100%
src/routes/entity.ts	100%	100%	100%	100%
src/routes/file.ts	100%	100%	100%	100%
src/routes/files.ts	100%	100%	100%	100%
src/routes/search.ts	100%	100%	100%	100%
src/transformers/default.ts	100%	100%	100%	100%
src/utils/errors.ts	100%	100%	100%	100%

Generated in workflow #81 for commit 726c8c9 by the Vitest Coverage Report Action

alvinsw · 2026-08-24T01:42:27Z

@johnf How do we fix this error?

ERROR: Coverage for lines (98.81%) does not meet global threshold (100%)
ERROR: Coverage for statements (98.86%) does not meet global threshold (100%)
ERROR: Coverage for branches (88.51%) does not meet global threshold (100%)
 ELIFECYCLE  Test failed. See above for more details.

johnf · 2026-08-24T01:47:56Z

@johnf How do we fix this error?

ERROR: Coverage for lines (98.81%) does not meet global threshold (100%)
ERROR: Coverage for statements (98.86%) does not meet global threshold (100%)
ERROR: Coverage for branches (88.51%) does not meet global threshold (100%)
 ELIFECYCLE  Test failed. See above for more details.

You need to add more tests to cover any extra code you added.

You can check the coverage report in the web version pnpm test:ui from memory to see what needs to be covered.

Also, AI is excellent at doing this for you.

johnf · 2026-08-25T23:56:29Z

@alvinsw Can you let me know when you have fixed the coverage issues, and then I'll review

moisbo · 2026-08-26T23:48:06Z

We fixed with Copilot, pushed to a new branch to see what was was wrong

alvinsw · 2026-08-31T00:43:28Z

@johnf coverage has been fix, can you review please.

johnf

README.md needs updates to explain the new functionality

I'm not sure that this approach is general enough.

resolveValidLicenses doesn't know if this is a meta or content license lookup
What if I wanted to add other restrictions e.g. maybe I have special metadata in the entity I want to match against.

Let's discuss on Thursday I think we may need an approach where you can pass in a more complex Prisma element rather than just a list of licenses.

johnf · 2026-09-02T00:09:20Z

          return reply.code(404).send(createNotFoundError('The requested entity was not found', id));
        }

+        const standardEntity = {


johnf · 2026-09-02T00:15:28Z

+            const entity = {
+              ...dbFile.entity,
+              ...baseFileTransformer(dbFile),
+              memberOf: dbFile.entity.memberOf ? (refMap.get(dbFile.entity.memberOf) ?? null) : null,


Why did this code change? Is this fixing a bug?

Because originally it only passes standardFile which does not have the metadataLicenseId and contentLicenseId required to determine access in the fileAccessTransformer function.

johnf · 2026-09-02T00:18:35Z

+      let filters = request.body.filters;
+      if (resolveValidLicenses) {
+        filters = filters || {};
+        filters.metadataLicenseId = (await resolveValidLicenses({ request, fastify })) || [];


I think this is right but we should have a quick discussion on Thursday.
Is it right that even for files we filter here by the metadataLicense (I think it is) but I think we need to document the rationale for this if we havne't already in the README.md

What do you mean by "the rationale for this"? We can just discuss them here.
If the metadata access for files is restricted then such files should should be shown in the search result.

johnf · 2026-09-02T00:19:11Z

  size: number;
 };

+export type FileEntity = StandardFile & StandardEntity;


This doesn't logically make sense. Why is it needed?

As mentioned elsewhere, StandardFile type doesn't have all the required fields.

johnf · 2026-09-02T00:20:12Z

 * ```
 */
-export const AllPublicFileAccessTransformer = (file: StandardFile): AuthorisedFile => ({
+export const AllPublicFileAccessTransformer = (file: FileEntity): AuthorisedFile => ({


This feels like a breaking change in the API. Why is this necessary?

The file access transformer needs to check the the metadataLicenseId too, which is not defined in the StandardFile.

johnf · 2026-09-02T00:26:46Z

@alvinsw I also asked Claude to review the PR. It picked up somethings I've mentioned but also some other items worth looking at

Standards

Hard violations (documented standards)

README.md — public API surface undocumented. resolveValidLicenses (src/app.ts:87) is a new consumer-facing Options field. README documents every other option (accessTransformer, queryBuilderClass, fileHandler, …); this one gets nothing. CLAUDE.md: "published npm library … be mindful of the public API surface".

README.md stale — "Applied Routes" (README:489-495). "The /file/:id endpoint … uses the fileHandler system, not file transformers." src/routes/file.ts now runs fileAccessTransformer on both GET and HEAD. Same section's "File Transformation Pipeline" (README:481-487) no longer describes stage 1: files.ts feeds the access transformer {...dbFile.entity,
...baseFileTransformer(dbFile)}, not the base transformer's output.

CLAUDE.md pipeline — src/routes/crate.ts:42-46, 88-93 skips the base transformer. It hand-builds {...entity, memberOf: {id: entity.memberOf || '', name: ''}} from the raw Prisma row, so consumer accessTransformers receive DB fields (meta, createdAt) and a fabricated name: '' instead of resolved references. entity.ts/search.ts use baseEntityTransformer + resolveEntityReferences.

Semver. FileAccessTransformer and AllPublicFileAccessTransformer change input from StandardFile to FileEntity, and AuthorisedFile gains all StandardEntity fields — consumer implementations that construct an AuthorisedFile break. Commits are feat:, no !/BREAKING CHANGE footer (cf. aa5c6b7 feat!:).

Baseline smells (judgement calls)

Duplicated Code — the FileEntity assembly (const entity = {...file.entity, ...baseFileTransformer(file), memberOf: …refMap.get…}) appears three times (file.ts GET + HEAD, files.ts). Extract into transformers/default.ts. Likewise the standardEntity block twice in crate.ts, and createForbiddenError('Access to this resource is restricted') five times.
Data Clumps / missing type — resolveValidLicenses?: (opt: TransformerContext) => Promise<string[]> is re-declared inline in app.ts, entities.ts, files.ts, search.ts. Peer types (AccessTransformer, FileTransformer) live named in src/types/transformers.ts.
Mysterious Name — FileEntity = StandardFile & StandardEntity where id is the file's but name/licences are the parent's; entity used as the variable name for it in file.ts; opt for a TransformerContext.
Speculative Generality — (await resolveValidLicenses(...)) || [] (three sites); the declared return type is Promise<string[]>. The test only compiles via // @ts-expect-error (entities.test.ts:329), which hides the mismatch rather than fixing the signature.

Convention drift worth a look

search.ts:66-70 places the await resolveValidLicenses call outside the try, so a throwing resolver bypasses the route's createInternalError() 500 — entities.ts/files.ts put it inside. It also mutates request.body.filters and silently overwrites a caller-supplied metadataLicenseId.
files.ts:25 drops the destructuring default (fileTransformers = []) for inline fileTransformers || [], against the pattern in entities.ts/search.ts.
Brace-less if returns in entity.ts:52-53 and crate.ts:94-95 versus braced everywhere else (Biome recommended doesn't enforce this).
TESTING.md isolation — entities.test.ts uses a module-level mutable let hasLicense flipped inside tests, making the suite order-sensitive.

Spec

(a) Missing / partial

/file doesn't implement the stated condition. Spec: "For endpoints /crate, /entity, /file: responds with 403 when access.metadata returned by accessTransformer function is false." src/routes/file.ts (both HEAD and GET) gates on !authorisedFile.access.content, not access.metadata — and FileAccessInfo has no metadata field at all (src/transformers/default.ts: "File metadata is
always accessible - only content access is controlled"). Either the spec is wrong about /file, or the type needs a metadata flag. As written, the requirement is unmet and the endpoint gained a different restriction (content-gating) instead. Needs an author decision.

No docs for the new public option. resolveValidLicenses is a new consumer-facing option in src/app.ts but appears nowhere in README.md, unlike every other option (accessTransformer, queryBuilderClass, etc.).

(b) Scope creep

Breaking public API change to FileAccessTransformer. Not asked for anywhere in the spec. src/types/transformers.ts changes the parameter from StandardFile to FileEntity (StandardFile & StandardEntity), and AuthorisedFile gains all entity fields. Every consumer's fileAccessTransformer and the documented example in README.md:414 now receive a different shape. This is a
major-semver change in a published library, driven only by the /file content check.

/files response shape changed. src/routes/files.ts now spreads the raw Prisma entity (...dbFile.entity) into each returned file, so /files responses leak meta, createdAt, updatedAt, name, description, entityType, and both licence IDs. Spec asked only to "filter the search query".

(c) Implemented but looks wrong

/crate builds a malformed entity for the access transformer. src/routes/crate.ts (both routes) uses memberOf: { id: entity.memberOf || '', name: '' } — never null and never a resolved name — instead of baseEntityTransformer + resolveEntityReferences as /entity does. A consumer transformer keying on memberOf/rootCollection (the common case) will see {id:'',name:''} for
top-level entities and make wrong access decisions.

Falsy vs false. Spec says "when access.metadata ... is false"; all three sites use !access.metadata, so undefined also 403s. Probably fine, but it is stricter than specified.

/search licence filter likely matches nothing. filters.metadataLicenseId is pushed through queryBuilder.buildQuery as terms: { metadataLicenseId: [...] } on a raw field; every other exact-match field in the codebase uses .keyword (queryBuilder.ts:31) and metadataLicenseId is absent from the index mapping in src/test/integration.setup.ts. Under dynamic mapping it becomes
analysed text, so URL licence values won't match — filtering the user out of all results. Only unit tests (mocked OpenSearch) cover this.

/search clobbers caller filters and mutates request.body.filters in place; a caller-supplied metadataLicenseId filter is silently replaced rather than intersected.

alvinsw added 4 commits June 18, 2026 00:38

feat: add access restriction

23e44a0

feat: add valid licenses checker

1f581e7

feat: add test cases for access-restriction feature

2f3a22d

fix: replace calls to reply.forbidden with createForbiddenError helper

628773d

alvinsw requested review from johnf and moisbo August 24, 2026 01:27

alvinsw added 3 commits August 27, 2026 05:51

fix: add more test to complete coverage

9b1657b

fix: add more test coverage

0b9cfe8

fix: add more coverage test for file anad files

726c8c9

johnf requested changes Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

alvinsw commented Aug 24, 2026

Uh oh!

github-actions Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

alvinsw commented Aug 24, 2026

Uh oh!

johnf commented Aug 24, 2026

Uh oh!

johnf commented Aug 25, 2026

Uh oh!

moisbo commented Aug 26, 2026

Uh oh!

alvinsw commented Aug 31, 2026

Uh oh!

johnf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

alvinsw Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

johnf commented Sep 2, 2026

Standards

Hard violations (documented standards)

Baseline smells (judgement calls)

Convention drift worth a look

Spec

(a) Missing / partial

(b) Scope creep

(c) Implemented but looks wrong

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 24, 2026 •

edited

Loading

alvinsw Sep 3, 2026 •

edited

Loading