Skip to content

Fix downloads for legacy digital URLs and FRBR cataloging lookups - #484

Merged
fabiodalez-dev merged 2 commits into
mainfrom
fix/digital-url-frbr-review
Oct 21, 2026
Merged

fabiodalez-dev merged 2 commits into
mainfrom
fix/digital-url-frbr-review

Conversation

@fabiodalez-dev

Copy link
Copy Markdown
Owner

Valid Digital Library attachments could appear on the book page and in API responses but return 403 when downloaded. The record search now includes legacy relative paths, percent-encoded names, JSON-escaped slashes and JSON Unicode escapes, then checks the exact decoded local URL. The same matching is used when refreshing a populated file index; unrelated encoded URLs do not invalidate it.

Cataloger and Archivist accounts can now use the book editor's FRBR work autocomplete and expression lookup. Only those two lookup routes gain the cataloging capability; plugin settings and unrelated roles remain restricted.

Validation:

  • bash scripts/ci-quality-local.sh passes: PHPStan level 5, dependency audits, frontend lint, locale/route parity, all 267 standalone PHP suites without skips, all CI integration suites, schema/migration behavior and setup-permissions.
  • Regression tests reproduced both findings before the fix.
  • Digital file matrix: 220 checks pass, including each visibility, readers/borrowers, individual grants, cold and populated indexes, and unrelated local/off-site URLs.
  • Role boundaries: 89 checks pass, including the real authentication middleware for both cataloging presets and refusal of unrelated roles/routes.

Follow-up to the review of #481. Refs #480.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: fabiodalez-dev/Pinakes/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 77414bd2-be6b-494c-91bb-f9ee86b0bb53
📥 Commits

Reviewing files that changed from the base of the PR and between afca7a5 and fcbe8aa.

📒 Files selected for processing: 4
  • app/Support/DigitalAccess.php
  • app/Support/Permissions.php
  • tests/digital-access-matrix-480.unit.php
  • tests/role-boundaries-480.unit.php
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@fabiodalez-dev
fabiodalez-dev marked this pull request as ready for review October 21, 2026 06:13
@fabiodalez-dev fabiodalez-dev mentioned this pull request Oct 21, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one prefilter tightening suggested inline.

Reviewed changes

This review covers the single commit that widens Digital Library record matching to legacy URL spellings and opens the two FRBR lookup routes to cataloguing roles.

  • Broader digital-file prefilter — FILE_CANDIDATE_FILTER and the slash-agnostic likePattern() let relative, percent-encoded and JSON-escaped URLs reach lists(), which still makes the exact decoded, local-host comparison.
  • Exact index invalidation — indexOutdated() now outdates the index only when a changed record really lists the file, so unrelated encoded URLs (e.g. a loan on another book) keep the cache warm.
  • FRBR lookups for cataloguers — /api/opere/search and /admin/frbr-lrm/opere/{id}/espressioni map to catalog.manage by exact match. Both are GET-only routes in FrbrLrmPlugin, so no write surface is opened. Plugin settings stay operator-only.
  • Regression coverage — a stored-form × visibility matrix (cold and warm index), a negative case for similar names and off-site URLs, and role-boundary checks through the real AdminAuthMiddleware.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread app/Support/DigitalAccess.php
…rary folder in it

fileNameOf() looks for /uploads/digital/ in the undecoded path, so a percent-encoded value can only name a local file when that prefix is there (with \/ slashes in JSON); only a Unicode escape can hide the prefix, and that arm is unchanged. Off-site links such as archive.org addresses with %20 no longer join every cold scan. The matrix test checks that those rows are not candidates while encoded local ones still are.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. The prefilter tightening from the prior review landed cleanly.

Reviewed changes

This run covers fcbe8aa, which narrows the percent-encoding arms of the Digital Library candidate prefilter.

  • Prefix-gated % arms — each LOCATE('%', …) arm in FILE_CANDIDATE_FILTER now also requires the literal folder prefix (uploads/digital/ in file_url/audio_url, uploads%digital in digital_attachments so JSON \/ still matches). Off-site percent-encoded links no longer reach the JSON parser, and no real match is lost: fileNameOf() needs the prefix unencoded anyway, and the \u arm still stands alone.
  • Direct prefilter regression check — the matrix test runs the filter against an encoded off-site row, an encoded local file_url and a JSON-escaped attachment, and asserts that only the two local rows come back as candidates.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@fabiodalez-dev
fabiodalez-dev merged commit 71945cb into main Oct 21, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant