fix(views): deny global view access on dedicated portals

[PascalRepond]

Dedicated portals (e.g., folia.unifr.ch) could serve documents from other organisations via the /global/ URL path, causing search engines to index cross-organisation content. The route converter now checks if the request comes from a dedicated portal and rejects the global view with a 404.

• Closes A dedicated portal should not give access to the global view #909.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds server-name based validation to block the global view on dedicated portals, updates two organisation serverName entries, and adds a unit test that verifies the global view is inaccessible when a dedicated portal mapping exists.

Changes

Cohort / File(s)	Summary
Route Converter Logic sonar/route_converters.py	Added imports (urlparse, request, OrganisationSearch). OrganisationCodeConverter.to_python now derives server_name from the request and raises ValidationError for SONAR_APP_DEFAULT_ORGANISATION when OrganisationSearch.get_organisation_pid_by_server_name(server_name) is truthy. Copyright year updated.
Test Coverage tests/ui/documents/test_documents_views.py	Added from unittest import mock and new test test_global_view_blocked_on_dedicated_portal that mocks OrganisationSearch.get_organisation_pid_by_server_name, asserts /global returns 404 on a dedicated portal and org-specific view returns 200.
Organisation Data data/organisations/data.json	Updated serverName values for organisations fictivededicated and hepvs from .test.com to .test.rero.ch.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: denying global view access on dedicated portals by checking server origin in the route converter.
Description check	✅ Passed	The description clearly explains the problem, the solution, and references the closed issue, all relating to blocking global view access on dedicated portals.
Linked Issues check	✅ Passed	The PR fully implements the requirement from issue #909: the route converter now checks if requests originate from dedicated portals and rejects global view access with a 404 response.
Out of Scope Changes check	✅ Passed	All changes are in scope: route converter logic to block global view access, test coverage for the new behavior, and test data updates to reflect dedicated portal server names.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

tests/ui/documents/test_documents_views.py (1)

120-138: Consider adding a test case for the ext.py homepage guard (root path /)

The existing assertions exercise the route_converters.py guard (via /global). The ext.py guard — which fires when the request hits the default / route — has no explicit test. Since mock.patch on sonar.route_converters.OrganisationSearch.get_organisation_pid_by_server_name patches the method on the shared class object, the same mock context would also cover ext.py's lazy-imported reference; only an additional request to / is needed.

✅ Suggested additional assertion

     with mock.patch(
         "sonar.route_converters.OrganisationSearch.get_organisation_pid_by_server_name",
         return_value="org",
     ):
         assert client.get(url_for("index", view="global")).status_code == 404
+        # Also verify the homepage root path (handled by ext.py, not the converter)
+        assert client.get("/").status_code == 404

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/ui/documents/test_documents_views.py` around lines 120 - 138, Add an
assertion in test_global_view_blocked_on_dedicated_portal that also requests the
root path to exercise the ext.py homepage guard: while mocking
sonar.route_converters.OrganisationSearch.get_organisation_pid_by_server_name
(as already done in the second mock context) add a client.get(url_for("index",
view=None) or client.get(url_for("index")) request for "/" and assert it returns
404, then in the third mock context ensure the org view still returns 200 for
completeness; this ensures the ext.py lazy-imported homepage guard is tested
alongside the route_converters guard.

sonar/ext.py (1)

227-232: Guard logic is correct — optional: extract shared helper to remove duplication

The same two-liner pattern appears in both ext.py (line 230–231) and route_converters.py (line 42–43):

server_name = urlparse(request.url).hostname
OrganisationSearch().get_organisation_pid_by_server_name(server_name)

While the two guards cover different URL patterns (/ vs /<org_code:view>), extracting this into a small helper (e.g., in sonar/modules/organisations/api.py or a dedicated utils module) would make the intent explicit and keep future changes in one place.

Additionally, urlparse(request.url).hostname can theoretically return None; passing None to the ES query is safe (fails open), but or "" would be a slightly more defensive choice and was mentioned in the related review on route_converters.py.

♻️ Proposed helper extraction

# In a shared location (e.g. sonar/modules/organisations/api.py or a utils module):
+ def is_dedicated_portal(request_url):
+     """Return True when the server name maps to a dedicated portal."""
+     from urllib.parse import urlparse
+     server_name = urlparse(request_url).hostname or ""
+     return bool(OrganisationSearch().get_organisation_pid_by_server_name(server_name))

Then in ext.py:

-            server_name = urlparse(request.url).hostname
-            if OrganisationSearch().get_organisation_pid_by_server_name(server_name):
-                abort(404)
+            if is_dedicated_portal(request.url):
+                abort(404)

And identically in route_converters.py:

-            server_name = urlparse(request.url).hostname
-            if OrganisationSearch().get_organisation_pid_by_server_name(server_name):
-                raise ValidationError
+            if is_dedicated_portal(request.url):
+                raise ValidationError

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@sonar/ext.py` around lines 227 - 232, Extract the duplicated guard into a
small helper (e.g., in sonar.modules.organisations.api or a new utils module)
that encapsulates the logic of resolving the request host and checking for an
organisation PID; specifically, create a helper that uses
urlparse(request.url).hostname or "" to get the server_name and calls
OrganisationSearch().get_organisation_pid_by_server_name(server_name) and
returns a boolean, then replace the two-liner in ext.py (the block around
OrganisationSearch().get_organisation_pid_by_server_name and urlparse) and the
identical block in route_converters.py with a call to that helper and call
abort(404) when it returns true.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@sonar/ext.py`:
- Around line 227-232: Extract the duplicated guard into a small helper (e.g.,
in sonar.modules.organisations.api or a new utils module) that encapsulates the
logic of resolving the request host and checking for an organisation PID;
specifically, create a helper that uses urlparse(request.url).hostname or "" to
get the server_name and calls
OrganisationSearch().get_organisation_pid_by_server_name(server_name) and
returns a boolean, then replace the two-liner in ext.py (the block around
OrganisationSearch().get_organisation_pid_by_server_name and urlparse) and the
identical block in route_converters.py with a call to that helper and call
abort(404) when it returns true.

In `@tests/ui/documents/test_documents_views.py`:
- Around line 120-138: Add an assertion in
test_global_view_blocked_on_dedicated_portal that also requests the root path to
exercise the ext.py homepage guard: while mocking
sonar.route_converters.OrganisationSearch.get_organisation_pid_by_server_name
(as already done in the second mock context) add a client.get(url_for("index",
view=None) or client.get(url_for("index")) request for "/" and assert it returns
404, then in the third mock context ensure the org view still returns 200 for
completeness; this ensures the ext.py lazy-imported homepage guard is tested
alongside the route_converters guard.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5e9609 and 3717493.

⛔ Files ignored due to path filters (1)

• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• sonar/ext.py
• sonar/route_converters.py
• tests/ui/documents/test_documents_views.py

[coderabbitai]

♻️ Duplicate comments (1)

tests/ui/documents/test_documents_views.py (1)

120-120: ARG001 for organisation is intentionally not fixed.

The Ruff ARG001 warning for the unused organisation parameter is a known pattern in this repo — the rule is disabled for test files. No action needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/ui/documents/test_documents_views.py` at line 120, The unused parameter
warning (ARG001) for the organisation argument in the test function
test_global_view_blocked_on_dedicated_portal is intentional per repository test
linter rules; do not remove or modify the organisation parameter—leave the test
signature as-is and make no code changes in that function regarding the unused
parameter.

🧹 Nitpick comments (3)

tests/ui/documents/test_documents_views.py (1)

120-138: The ext.py homepage guard (root / path) has no test coverage.

The mock targets sonar.route_converters.OrganisationSearch.get_organisation_pid_by_server_name, which correctly exercises the route-converter path (/global/). However, the parallel check added in sonar/ext.py (Lines 230–232) — which fires for the root / path via the defaults={"view": ...} shortcut, bypassing the converter entirely — is never exercised with the dedicated-portal mock. The existing test_index (Line 141) only confirms a non-mocked 200, so the abort(404) branch in ext.py remains untested.

Consider adding a case to this test (or extending test_index) that patches the class as imported in sonar.ext:

with mock.patch(
    "sonar.modules.organisations.api.OrganisationSearch.get_organisation_pid_by_server_name",
    return_value="org",
):
    assert client.get("/").status_code == 404

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/ui/documents/test_documents_views.py` around lines 120 - 138, The
root-path guard in sonar/ext.py (the defaults={"view": ...} branch) is untested;
extend test_global_view_blocked_on_dedicated_portal (or add to test_index) to
patch the class imported in sonar.ext by mocking
sonar.modules.organisations.api.OrganisationSearch.get_organisation_pid_by_server_name
to return "org" and then assert that client.get("/") returns 404, ensuring the
abort(404) branch in ext.py is exercised.

sonar/ext.py (2)

227-232: Consider extracting the dedicated-portal guard into a shared helper to avoid duplication.

The identical three-line block (urlparse → .hostname → get_organisation_pid_by_server_name) exists in both sonar/ext.py (Line 230–232) and sonar/route_converters.py (Lines 42–44). Extracting it (e.g., to sonar/modules/organisations/api.py or a small sonar/utils.py) would give a single point of maintenance.

♻️ Sketch of a shared helper

# sonar/modules/organisations/api.py  (or a dedicated utils module)
+def is_dedicated_portal(request_url: str) -> bool:
+    """Return True if request_url originates from a dedicated portal."""
+    from urllib.parse import urlparse
+    server_name = urlparse(request_url).hostname
+    return bool(OrganisationSearch().get_organisation_pid_by_server_name(server_name))

Then in both call-sites:

-            server_name = urlparse(request.url).hostname
-            if OrganisationSearch().get_organisation_pid_by_server_name(server_name):
-                abort(404)  # (or raise ValidationError in route_converters.py)
+            if is_dedicated_portal(request.url):
+                abort(404)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@sonar/ext.py` around lines 227 - 232, Extract the duplicate dedicated-portal
guard (the urlparse(request.url).hostname lookup +
OrganisationSearch().get_organisation_pid_by_server_name(...) check that leads
to abort(404) when view ==
current_app.config.get("SONAR_APP_DEFAULT_ORGANISATION")) into a shared helper
function (e.g., is_dedicated_portal_server(request) or
server_owned_by_organisation(server_name)) placed in a common module (like
sonar.modules.organisations.api or sonar.utils); have the helper accept the
request or server_name, perform urlparse(request.url).hostname and call
OrganisationSearch().get_organisation_pid_by_server_name, returning a boolean,
and then replace the three-line block in both sonar/ext.py and
sonar/route_converters.py with a call to the helper and abort(404) when it
returns True.

---

227-232: New per-request Elasticsearch query on every default-org view hit.

Every request to / (or any /global/… path) now fires OrganisationSearch().get_organisation_pid_by_server_name(server_name). Because the server-name → org-pid mapping is essentially static (changes only when an org's serverName is edited), caching this lookup—even with a short TTL—would avoid hitting ES on every page load.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@sonar/ext.py` around lines 227 - 232, The per-request ES hit comes from
calling OrganisationSearch().get_organisation_pid_by_server_name(server_name)
inside the default-org view check; replace that direct call with a cached lookup
(e.g. a module-level helper like
get_organisation_pid_by_server_name_cached(server_name) that uses
functools.lru_cache or your app cache with a short TTL) and call that helper
from the block that checks
current_app.config.get("SONAR_APP_DEFAULT_ORGANISATION") and computes
server_name; also add cache invalidation when an organisation's serverName is
edited (clear the cached entry or the lru_cache) so updates are reflected.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@tests/ui/documents/test_documents_views.py`:
- Line 120: The unused parameter warning (ARG001) for the organisation argument
in the test function test_global_view_blocked_on_dedicated_portal is intentional
per repository test linter rules; do not remove or modify the organisation
parameter—leave the test signature as-is and make no code changes in that
function regarding the unused parameter.

---

Nitpick comments:
In `@sonar/ext.py`:
- Around line 227-232: Extract the duplicate dedicated-portal guard (the
urlparse(request.url).hostname lookup +
OrganisationSearch().get_organisation_pid_by_server_name(...) check that leads
to abort(404) when view ==
current_app.config.get("SONAR_APP_DEFAULT_ORGANISATION")) into a shared helper
function (e.g., is_dedicated_portal_server(request) or
server_owned_by_organisation(server_name)) placed in a common module (like
sonar.modules.organisations.api or sonar.utils); have the helper accept the
request or server_name, perform urlparse(request.url).hostname and call
OrganisationSearch().get_organisation_pid_by_server_name, returning a boolean,
and then replace the three-line block in both sonar/ext.py and
sonar/route_converters.py with a call to the helper and abort(404) when it
returns True.
- Around line 227-232: The per-request ES hit comes from calling
OrganisationSearch().get_organisation_pid_by_server_name(server_name) inside the
default-org view check; replace that direct call with a cached lookup (e.g. a
module-level helper like get_organisation_pid_by_server_name_cached(server_name)
that uses functools.lru_cache or your app cache with a short TTL) and call that
helper from the block that checks
current_app.config.get("SONAR_APP_DEFAULT_ORGANISATION") and computes
server_name; also add cache invalidation when an organisation's serverName is
edited (clear the cached entry or the lru_cache) so updates are reflected.

In `@tests/ui/documents/test_documents_views.py`:
- Around line 120-138: The root-path guard in sonar/ext.py (the
defaults={"view": ...} branch) is untested; extend
test_global_view_blocked_on_dedicated_portal (or add to test_index) to patch the
class imported in sonar.ext by mocking
sonar.modules.organisations.api.OrganisationSearch.get_organisation_pid_by_server_name
to return "org" and then assert that client.get("/") returns 404, ensuring the
abort(404) branch in ext.py is exercised.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5e9609 and 3717493.

⛔ Files ignored due to path filters (1)

• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• sonar/ext.py
• sonar/route_converters.py
• tests/ui/documents/test_documents_views.py