Controller/routing cleanup: trailing-slash middleware, admin_only! helper, test settings isolation

[alexskr]

Summary

Controller/routing cleanup plus test-isolation hardening. No client-visible behavior change beyond trailing-slash redirects now using correct per-verb status codes and Cache-Control: no-store.

1. Move trailing-slash redirect to Rack middleware

Trailing-slash canonicalization was done in init.rb by registering a companion "#{pattern}/" 301-redirect route for every route. That approach:

• ran in the route-dispatch phase, so it sat behind every before filter and doubled the route table;
• hardcoded 301 for all verbs, which can downgrade the method and drop POST/PUT/PATCH bodies.

Replaced with Rack::TrailingSlashRedirect (lib/rack/trailing_slash_redirect.rb), wired as the innermost middleware so it runs before Sinatra routing. It strips the trailing slash and redirects — 301 for GET/HEAD, 308 for other verbs (preserves method + body) — preserving forwarded scheme (X-Forwarded-Proto), query string, and percent-encoded path segments. HEAD redirects carry no body, and the redirect is marked Cache-Control: no-store (its scheme-dependent Location must not be stored by the outer Rack::Cache and replayed to a different-scheme client).

2. Extract admin_only! helper

The guard error 403, "Access denied" unless current_user && current_user.admin? was copy-pasted across slices (4x) and annotator (2x), with a nil-guard-less variant in users. Extracted a single admin_only! helper next to current_user, called from all seven sites.

3. Isolate process-wide settings in tests

Settings like enable_security live on global singletons, so a suite that flips one (or errors after flipping it) leaked the value into later tests. Added a with_settings block helper (restores prior values even on raise) plus per-test and per-suite snapshot/restore nets in the test base class (test/test_case.rb), and converted the suites. Drops the ad-hoc enable_security/disable_security/reset_security helpers.

Tests

• Added test/middleware/test_trailing_slash_redirect.rb — backend-free Rack::MockRequest unit tests: verb split (301 GET/HEAD, 308 others), HEAD → 301 with empty body, root + no-trailing-slash pass-through, Location building (query string, %2F-encoded segments, X-Forwarded-Proto), and Cache-Control: no-store. Runs in ~3ms with no backend.
• Added test/controllers/test_admin_only_endpoints.rb — data-driven gate test asserting every reachable admin_only! endpoint returns 403 for an authenticated non-admin.
• Remaining suites need the backend stack — CI validates.

Notes

• An earlier attempt to dedupe the repeated get_ontology_and_submission + check_last_modified_segment in classes/instances via a namespace before filter was dropped: sinatra-contrib scopes namespace filters by URL prefix, so it fired for sibling routes in other controllers under /classes/:cls/.
• The Host-header reflection and Rack::Cache scheme concerns in the middleware predate this PR (the old init.rb redirect had the same host_with_port / X-Forwarded-Proto logic); no-store closes the cache angle.
• Follow-ups found during this work: HTTP-caching gaps Inconsistent HTTP caching: properties and SKOS endpoints missing check_last_modified #237, and an unreachable admin route GET /slices/synchronize_groups is unreachable (shadowed by /:slice_id route) #239 (/slices/synchronize_groups shadowed by /:slice_id).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.30%. Comparing base (50595c0) to head (001b03c).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
controllers/slices_controller.rb	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #238      +/-   ##
===========================================
+ Coverage    78.10%   78.30%   +0.20%     
===========================================
  Files           66       67       +1     
  Lines         3727     3748      +21     
===========================================
+ Hits          2911     2935      +24     
+ Misses         816      813       -3     

Flag	Coverage Δ
ag	78.30% <96.42%> (+0.20%)	⬆️
fs	78.30% <96.42%> (+0.20%)	⬆️
gd	78.30% <96.42%> (+0.20%)	⬆️
unittests	78.30% <96.42%> (+0.20%)	⬆️
vo	78.30% <96.42%> (+0.20%)	⬆️

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.

[mdorf]

This could use a bit of additional test coverage:

Not adequate for the new code as committed. The fix is cheap and the author has already written the assertions — commit them as test/middleware/test_trailing_slash_redirect.rb (the repo already has that convention via test_rack_attack.rb), as backend-free Rack::MockRequest unit tests covering:

POST/PUT/PATCH/DELETE /x/ → 308 with Location preserved
HEAD /x/ → 301 (and assert empty body, per the review finding)
root / and a no-trailing-slash path → pass through untouched
Optionally a 403 test on one annotator admin route. The verb-split test is the one I'd block on.

[alexskr]

Addressed in de5f8b3.

Added test/middleware/test_trailing_slash_redirect.rb — backend-free Rack::MockRequest unit tests (run in ~3ms, no triplestore/Solr/Redis), covering:

• Verb split (the one you'd block on): 301 for GET/HEAD, 308 for POST/PUT/PATCH/DELETE, with Location preserved.
• HEAD → 301 with empty body — and fixed the middleware to actually return an empty body for HEAD (it previously returned ["Moved Permanently"] for every verb).
• Root / and a no-trailing-slash path → pass through untouched.
• Location building: query string, percent-encoded path segments (%2F), and X-Forwarded-Proto scheme selection (incl. chained/invalid values).

The optional non-admin 403 coverage for an annotator admin route is in test/controllers/test_admin_only_endpoints.rb (the data-driven gate test).