fix: enforce wiki space access on routed pages#597
Open
nkarkare wants to merge 1 commit intofrappe:developfrom
Open
fix: enforce wiki space access on routed pages#597nkarkare wants to merge 1 commit intofrappe:developfrom
nkarkare wants to merge 1 commit intofrappe:developfrom
Conversation
WalkthroughThis pull request implements role-based access control for Wiki Spaces and Wiki Pages. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wiki/utils.py (1)
39-53: Consider avoiding full-table scan in route resolver hot path.
get_wiki_space_for_routecurrently loads allWiki Spacerows and sorts in Python per call. For website requests, this can become costly as spaces grow; a prefix-candidate lookup (depth-based) avoids O(N) scans.⚡ Suggested approach (depth-based lookup)
def get_wiki_space_for_route(route: str, published_only: bool = False): @@ - normalized_route = route.strip("/") - spaces = frappe.get_all("Wiki Space", fields=["name", "route", "is_published"]) - - for space in sorted(spaces, key=lambda row: len((row.route or "").strip("/")), reverse=True): - space_route = (space.route or "").strip("/") - if not space_route: - continue - - if normalized_route != space_route and not normalized_route.startswith(f"{space_route}/"): - continue - - if published_only and not cint(space.is_published): - continue - - return frappe.get_cached_doc("Wiki Space", space.name) + normalized_route = route.strip("/") + parts = normalized_route.split("/") + for i in range(len(parts), 0, -1): + candidate = "/".join(parts[:i]) + filters = {"route": candidate} + if published_only: + filters["is_published"] = 1 + space_name = frappe.db.get_value("Wiki Space", filters, "name") + if space_name: + return frappe.get_cached_doc("Wiki Space", space_name) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/utils.py` around lines 39 - 53, get_wiki_space_for_route currently does a full-table scan via frappe.get_all("Wiki Space") and sorts in Python; change it to a depth-based prefix lookup to avoid O(N) per-request work: compute normalized_route and its path segments, then iterate from the longest prefix to shortest building candidate_route (e.g., join first i segments), and for each candidate call frappe.get_all or frappe.get_value filtering by route=candidate_route (and is_published when published_only) to find a match; when found return frappe.get_cached_doc("Wiki Space", <name>); keep references to normalized_route, published_only, and use frappe.get_cached_doc for the returned record.wiki/frappe_wiki/doctype/wiki_document/test_wiki_document.py (1)
178-212: Make user restoration failure-safe in the new tests.Use
self.addCleanup(...)(ortry/finally) afterfrappe.set_user(...)so session user is always restored, even if an assertion fails mid-test.✅ Suggested hardening
with ( patch.object(doc, "get_wiki_space", return_value=frappe._dict(name="restricted-space")), patch( "wiki.frappe_wiki.doctype.wiki_document.wiki_document.frappe.get_cached_doc", return_value=fake_space, ), ): frappe.set_user(user.name) + self.addCleanup(frappe.set_user, "Administrator") with self.assertRaises(frappe.PermissionError): doc.check_guest_access() - - frappe.set_user("Administrator") @@ with ( patch.object(doc, "get_wiki_space", return_value=frappe._dict(name="restricted-space")), patch( "wiki.frappe_wiki.doctype.wiki_document.wiki_document.frappe.get_cached_doc", return_value=fake_space, ), ): frappe.set_user(user.name) + self.addCleanup(frappe.set_user, "Administrator") doc.check_guest_access() - - frappe.set_user("Administrator")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/frappe_wiki/doctype/wiki_document/test_wiki_document.py` around lines 178 - 212, The tests test_role_restricted_space_blocks_user_without_allowed_role and test_role_restricted_space_allows_user_with_allowed_role call frappe.set_user(...) but do not guarantee restoration if an assertion raises; update each test to restore the session user safely by registering self.addCleanup(lambda: frappe.set_user("Administrator")) immediately after calling frappe.set_user(user.name) (or wrap the patch/context block in try/finally and call frappe.set_user("Administrator") in finally). Ensure the change is applied around the calls to doc.check_guest_access() and references to frappe.set_user and doc.check_guest_access so the session is always restored even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wiki/wiki/doctype/wiki_page/wiki_page.py`:
- Around line 30-41: In has_website_permission, normalize the user value before
the final guest check so that None cannot bypass access control; e.g., ensure
user is treated as a non-authenticated guest by handling user is None (or
coercing user = user or "Guest") and then perform the existing allow_guest and
guest comparison logic (affecting the existing return user != "Guest" behavior)
so pages don't incorrectly allow access when user is None.
---
Nitpick comments:
In `@wiki/frappe_wiki/doctype/wiki_document/test_wiki_document.py`:
- Around line 178-212: The tests
test_role_restricted_space_blocks_user_without_allowed_role and
test_role_restricted_space_allows_user_with_allowed_role call
frappe.set_user(...) but do not guarantee restoration if an assertion raises;
update each test to restore the session user safely by registering
self.addCleanup(lambda: frappe.set_user("Administrator")) immediately after
calling frappe.set_user(user.name) (or wrap the patch/context block in
try/finally and call frappe.set_user("Administrator") in finally). Ensure the
change is applied around the calls to doc.check_guest_access() and references to
frappe.set_user and doc.check_guest_access so the session is always restored
even on failures.
In `@wiki/utils.py`:
- Around line 39-53: get_wiki_space_for_route currently does a full-table scan
via frappe.get_all("Wiki Space") and sorts in Python; change it to a depth-based
prefix lookup to avoid O(N) per-request work: compute normalized_route and its
path segments, then iterate from the longest prefix to shortest building
candidate_route (e.g., join first i segments), and for each candidate call
frappe.get_all or frappe.get_value filtering by route=candidate_route (and
is_published when published_only) to find a match; when found return
frappe.get_cached_doc("Wiki Space", <name>); keep references to
normalized_route, published_only, and use frappe.get_cached_doc for the returned
record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94705e04-2dd4-4e84-adc5-c6543fc80a1f
📒 Files selected for processing (8)
wiki/frappe_wiki/doctype/wiki_document/test_wiki_document.pywiki/frappe_wiki/doctype/wiki_document/wiki_document.pywiki/utils.pywiki/wiki/doctype/wiki_page/test_wiki_page.pywiki/wiki/doctype/wiki_page/wiki_page.jsonwiki/wiki/doctype/wiki_page/wiki_page.pywiki/wiki/doctype/wiki_space/wiki_space.jsonwiki/wiki/doctype/wiki_space/wiki_space.py
Comment on lines
+30
to
+41
| def has_website_permission(self, ptype, user, verbose=False): | ||
| if not self.published: | ||
| return False | ||
|
|
||
| wiki_space = get_wiki_space_for_route(self.route) | ||
| if wiki_space and not has_wiki_space_access(wiki_space, user=user): | ||
| return False | ||
|
|
||
| if self.allow_guest: | ||
| return True | ||
|
|
||
| return user != "Guest" |
There was a problem hiding this comment.
Normalize user before guest check to avoid None bypass.
On Line 41, user=None currently passes the final check (None != "Guest"), which can incorrectly allow access on non-guest pages.
🔧 Proposed fix
-def has_website_permission(self, ptype, user, verbose=False):
+def has_website_permission(self, ptype, user=None, verbose=False):
+ user = user or frappe.session.user
if not self.published:
return False
wiki_space = get_wiki_space_for_route(self.route)
if wiki_space and not has_wiki_space_access(wiki_space, user=user):
return False
if self.allow_guest:
return True
return user != "Guest"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def has_website_permission(self, ptype, user, verbose=False): | |
| if not self.published: | |
| return False | |
| wiki_space = get_wiki_space_for_route(self.route) | |
| if wiki_space and not has_wiki_space_access(wiki_space, user=user): | |
| return False | |
| if self.allow_guest: | |
| return True | |
| return user != "Guest" | |
| def has_website_permission(self, ptype, user=None, verbose=False): | |
| user = user or frappe.session.user | |
| if not self.published: | |
| return False | |
| wiki_space = get_wiki_space_for_route(self.route) | |
| if wiki_space and not has_wiki_space_access(wiki_space, user=user): | |
| return False | |
| if self.allow_guest: | |
| return True | |
| return user != "Guest" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wiki/wiki/doctype/wiki_page/wiki_page.py` around lines 30 - 41, In
has_website_permission, normalize the user value before the final guest check so
that None cannot bypass access control; e.g., ensure user is treated as a
non-authenticated guest by handling user is None (or coercing user = user or
"Guest") and then perform the existing allow_guest and guest comparison logic
(affecting the existing return user != "Guest" behavior) so pages don't
incorrectly allow access when user is None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wiki Pageroutes respect the page-levelallow_guestflag instead of the DocType-wideallow_guest_to_viewshortcutrolestable toWiki Spaceso the allowlist is visible/configurable in the model, and cover the new checks with testsProblem
Direct wiki routes could bypass the access expectations set in the wiki UI:
Wiki Pageroutes were effectively public becauseallow_guest_to_viewshort-circuited route permission checks before page-levelallow_guestwas evaluatedWiki Pageroutes nor v3Wiki Documentroutes enforced a wiki-space role allowlist on direct page accessThat means a page could be hidden from the sidebar or marked non-guest, yet still render for users outside the intended wiki space roles when they visited the route directly.
Fix
Wiki Spacefor a route and evaluate its configuredrolesWikiPage.has_website_permission()and disable the DocType-wide guest bypass so legacy routes now respect:allow_guestWikiDocument.check_guest_access()to also enforce wiki-space role allowlists for v3 routesroleschild table toWiki SpaceTesting
python3 -m py_compile wiki/utils.py wiki/wiki/doctype/wiki_page/wiki_page.py wiki/frappe_wiki/doctype/wiki_document/wiki_document.py wiki/wiki/doctype/wiki_space/wiki_space.py wiki/wiki/doctype/wiki_page/test_wiki_page.py wiki/frappe_wiki/doctype/wiki_document/test_wiki_document.pyI could not run the full Frappe test suite locally in this environment because a bench/test runtime is not available here.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests