wip: feat(core): restrict module access globally or per group#277
Draft
pchopinet wants to merge 16 commits into
Draft
wip: feat(core): restrict module access globally or per group#277pchopinet wants to merge 16 commits into
pchopinet wants to merge 16 commits into
Conversation
📝 WalkthroughWalkthroughThis pull request implements a comprehensive per-module access control system. Five workspace modules are marked as restrictable, a new ModuleAccessRule model stores per-module and per-group access rules, and access is enforced at authentication, middleware, and feature levels using a centralized service layer. ChangesModule Access Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspace/core/views.py (1)
57-58:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvalid Python 3 exception handling syntax.
except TypeError, ValueError:is Python 2 syntax and will raise aSyntaxErrorin Python 3. The correct syntax uses parentheses to catch multiple exception types.🐛 Proposed fix
try: limit = int(request.query_params.get("limit", 10)) - except TypeError, ValueError: + except (TypeError, ValueError): limit = 10🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/views.py` around lines 57 - 58, The except clause uses Python 2 syntax "except TypeError, ValueError:" which is invalid in Python 3; update the exception handler to use the tuple form "except (TypeError, ValueError):" so both exceptions are correctly caught and the fallback assignment to limit = 10 still runs (locate the except block in workspace/core/views.py that sets limit).
🧹 Nitpick comments (7)
workspace/core/activity_registry.py (1)
93-111: 💤 Low valueConsider returning early if viewer user is not found.
When
viewer_idis provided but the user doesn't exist (deleted account), the current logic falls through and yields all providers. While this is unlikely in normal operation (viewer_id comes from authenticated requests), explicitly returning an empty iterator or raising would be more defensively correct.♻️ Optional defensive fix
def _accessible_providers(self, viewer_id): """Yield provider infos the viewer may access. When viewer_id is None (system/unfiltered context) no filtering is applied. Otherwise providers for modules the viewer cannot access are skipped. """ if viewer_id is None: yield from self._providers.values() return from django.contrib.auth import get_user_model from workspace.core.services.module_access import can_access_module viewer = get_user_model().objects.filter(pk=viewer_id).first() + if viewer is None: + return # Unknown viewer: yield nothing rather than everything for info in self._providers.values(): - if viewer is not None and not can_access_module(viewer, info.slug): + if not can_access_module(viewer, info.slug): continue yield info🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/activity_registry.py` around lines 93 - 111, The _accessible_providers method currently treats a non-existent viewer_id as an unfiltered context because viewer becomes None and the access check is skipped; change it to return early (yield nothing) when viewer_id is not None but the user lookup returns no user: after viewer = ... .first() add a guard that if viewer_id is not None and viewer is None then return (or raise), so subsequent iteration uses can_access_module(viewer, info.slug) safely and does not accidentally expose all providers.workspace/core/tests/test_module_access_hiding.py (1)
12-17: ⚡ Quick winSeveral cache-sensitive test classes only clear Django cache in
tearDown().
workspace/core/tests/test_module_access_hiding.py,workspace/core/tests/test_module_access_middleware.py,workspace/core/tests/test_module_access_permission.py,workspace/core/tests/test_module_access_service.py, andworkspace/core/tests/test_views_sse.pyall exercise cached module-access state, but they clear cache only after each test. Because Django cache survives acrossTestCaseclasses, the first test in any of these classes can inherit stale entries from an earlier class. Clear the cache insetUp()as well, or registerself.addCleanup(cache.clear)before warming any access decisions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/tests/test_module_access_hiding.py` around lines 12 - 17, The tests clear the Django cache only in tearDown(), which allows stale cache entries to leak into the first test of a different TestCase; in the setUp() method (the one that creates self.factory and self.user) either call cache.clear() at the start or register self.addCleanup(cache.clear) before any access-warming occurs so each test starts with a clean cache; update the setUp() in the TestCase class that defines setUp/tearDown (refer to the setUp and tearDown methods and the cache.clear call) to include cache.clear() or self.addCleanup(cache.clear).workspace/core/tests/test_module_access_middleware.py (1)
31-48: ⚡ Quick winThe allow-path tests are too weak in
workspace/core/tests/test_module_access_middleware.pyandworkspace/core/tests/test_module_access_permission.py.These cases only assert
status_code != 403, which still passes on unrelated 404/500/401 responses. That means they do not actually prove middleware/auth let the request through. Assert the expected downstream outcome for each endpoint instead so these tests verify successful access, not just the absence of one specific denial code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/tests/test_module_access_middleware.py` around lines 31 - 48, The tests (test_enabled_module_not_blocked, test_non_restrictable_module_never_blocked, test_superuser_bypasses_block) currently only assert resp.status_code != 403 which can hide 404/500/401 failures; update each to assert the actual successful downstream outcome (e.g. assert resp.status_code == 200 or use assertContains/assertIn on expected response body) for the specific endpoints used (/api/v1/mail/accounts and /api/v1/modules) so the tests verify real access rather than just absence of 403.workspace/core/tests/test_activity.py (1)
470-493: 💤 Low valueConsider using explicit module paths for patch targets.
The tests use
patch.object(info.provider_cls, "get_recent_events", ...)whereinfo.provider_clsis retrieved from the registry at runtime. While this works correctly, using explicit module paths like@patch('workspace.chat.activity.ChatActivityProvider.get_recent_events')would make dependencies clearer and improve maintainability.The current approach has a trade-off: it tests exactly what the registry uses (good for integration testing) but couples the test to the registry's runtime state (less explicit). For unit tests of filtering behavior, explicit paths are typically preferred.
📝 Example refactor for test_recent_events_excludes_disabled_source
- info = activity_registry.get_all()["chat"] - with patch.object( - info.provider_cls, - "get_recent_events", - return_value=list(fake_events), - ): + with patch( + "workspace.chat.activity.ChatActivityProvider.get_recent_events", + return_value=list(fake_events), + ): events = activity_registry.get_recent_events( self.viewer.id, viewer_id=self.viewer.id, source="chat" )Apply the same pattern to
test_recent_events_returns_enabled_source.Also applies to: 495-515
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/tests/test_activity.py` around lines 470 - 493, The test uses patch.object(info.provider_cls, "get_recent_events") which ties the test to runtime registry state; change the patch target to an explicit import path for the provider class (e.g. patch 'workspace.chat.activity.ChatActivityProvider.get_recent_events') in test_recent_events_excludes_disabled_source (and the sibling test_recent_events_returns_enabled_source) so the mocked method is clear and stable; update test imports as needed and use patch as a decorator or context manager with the string target instead of patch.object(info.provider_cls, ...).Source: Coding guidelines
workspace/core/services/module_access.py (2)
21-27: 💤 Low valueConsider caching
restrictable_module_slugs()result.This function is called multiple times (lines 48, 68, 81) but iterates all app configs each time. Since the set of restrictable modules is static after app initialization, consider caching the result to avoid repeated iteration.
💡 Example implementation with module-level cache
+_restrictable_cache = None + def restrictable_module_slugs() -> set[str]: """Return the slugs of modules whose AppConfig opts into restriction.""" + global _restrictable_cache + if _restrictable_cache is not None: + return _restrictable_cache - return { + _restrictable_cache = { config.label for config in apps.get_app_configs() if getattr(config, "restrictable", False) } + return _restrictable_cache🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/services/module_access.py` around lines 21 - 27, The function restrictable_module_slugs() currently iterates apps.get_app_configs() on every call which is wasteful; cache its result (since app configs are static after initialization) by memoizing it — either add a module-level variable (e.g., _RESTRICTABLE_MODULE_SLUGS) that is computed once and returned thereafter or decorate restrictable_module_slugs with functools.lru_cache(maxsize=1); update the function body to return the cached set and ensure any tests/import-time initialization still triggers the initial computation.
85-88: ⚡ Quick winAvoid calling
slug_gettertwice per item.The condition calls
slug_getter(item)twice for each item. Store the result in a variable to improve efficiency.♻️ Proposed refactor
enabled = enabled_module_slugs(user) return [ item for item in items - if slug_getter(item) not in restrictable or slug_getter(item) in enabled + if (slug := slug_getter(item)) not in restrictable or slug in enabled ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/services/module_access.py` around lines 85 - 88, The list comprehension calls slug_getter(item) twice per item; compute the slug once and reuse it. Replace the comprehension with one that stores slug = slug_getter(item) (e.g., use the walrus operator "(slug := slug_getter(item))" in the comprehension or switch to an explicit for-loop) and then test "slug not in restrictable or slug in enabled"; update references to the symbols slug_getter, items, restrictable, and enabled accordingly.workspace/core/migrations/0001_initial.py (1)
28-28: 💤 Low valueConsider adding
help_textto document the field.The
module_slugfield lacks ahelp_textparameter. Adding documentation helps administrators understand the field's purpose in the Django admin interface.📝 Suggested addition
- ("module_slug", models.CharField(max_length=64)), + ("module_slug", models.CharField(max_length=64, help_text="Slug of the workspace module to control access for")),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspace/core/migrations/0001_initial.py` at line 28, The module_slug CharField currently has no help_text; add descriptive help_text to the field definition (the models.CharField(max_length=64) for module_slug) in the model class that defines module_slug (not by hand-editing historical migration) — e.g. module_slug = models.CharField(max_length=64, help_text="Short identifier used for..."), then run makemigrations to produce an updated migration that records the help_text change so Django admin shows the documentation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workspace/core/services/module_access.py`:
- Line 53: Replace the absolute import in module_access.py with a relative
import: change the current "from workspace.core.models import ModuleAccessRule"
to use the parent-package relative form so that ModuleAccessRule is imported via
"from ..models import ModuleAccessRule"; update any other similar absolute
imports in this file to follow the same relative pattern to match service-module
import guidelines.
---
Outside diff comments:
In `@workspace/core/views.py`:
- Around line 57-58: The except clause uses Python 2 syntax "except TypeError,
ValueError:" which is invalid in Python 3; update the exception handler to use
the tuple form "except (TypeError, ValueError):" so both exceptions are
correctly caught and the fallback assignment to limit = 10 still runs (locate
the except block in workspace/core/views.py that sets limit).
---
Nitpick comments:
In `@workspace/core/activity_registry.py`:
- Around line 93-111: The _accessible_providers method currently treats a
non-existent viewer_id as an unfiltered context because viewer becomes None and
the access check is skipped; change it to return early (yield nothing) when
viewer_id is not None but the user lookup returns no user: after viewer = ...
.first() add a guard that if viewer_id is not None and viewer is None then
return (or raise), so subsequent iteration uses can_access_module(viewer,
info.slug) safely and does not accidentally expose all providers.
In `@workspace/core/migrations/0001_initial.py`:
- Line 28: The module_slug CharField currently has no help_text; add descriptive
help_text to the field definition (the models.CharField(max_length=64) for
module_slug) in the model class that defines module_slug (not by hand-editing
historical migration) — e.g. module_slug = models.CharField(max_length=64,
help_text="Short identifier used for..."), then run makemigrations to produce an
updated migration that records the help_text change so Django admin shows the
documentation.
In `@workspace/core/services/module_access.py`:
- Around line 21-27: The function restrictable_module_slugs() currently iterates
apps.get_app_configs() on every call which is wasteful; cache its result (since
app configs are static after initialization) by memoizing it — either add a
module-level variable (e.g., _RESTRICTABLE_MODULE_SLUGS) that is computed once
and returned thereafter or decorate restrictable_module_slugs with
functools.lru_cache(maxsize=1); update the function body to return the cached
set and ensure any tests/import-time initialization still triggers the initial
computation.
- Around line 85-88: The list comprehension calls slug_getter(item) twice per
item; compute the slug once and reuse it. Replace the comprehension with one
that stores slug = slug_getter(item) (e.g., use the walrus operator "(slug :=
slug_getter(item))" in the comprehension or switch to an explicit for-loop) and
then test "slug not in restrictable or slug in enabled"; update references to
the symbols slug_getter, items, restrictable, and enabled accordingly.
In `@workspace/core/tests/test_activity.py`:
- Around line 470-493: The test uses patch.object(info.provider_cls,
"get_recent_events") which ties the test to runtime registry state; change the
patch target to an explicit import path for the provider class (e.g. patch
'workspace.chat.activity.ChatActivityProvider.get_recent_events') in
test_recent_events_excludes_disabled_source (and the sibling
test_recent_events_returns_enabled_source) so the mocked method is clear and
stable; update test imports as needed and use patch as a decorator or context
manager with the string target instead of patch.object(info.provider_cls, ...).
In `@workspace/core/tests/test_module_access_hiding.py`:
- Around line 12-17: The tests clear the Django cache only in tearDown(), which
allows stale cache entries to leak into the first test of a different TestCase;
in the setUp() method (the one that creates self.factory and self.user) either
call cache.clear() at the start or register self.addCleanup(cache.clear) before
any access-warming occurs so each test starts with a clean cache; update the
setUp() in the TestCase class that defines setUp/tearDown (refer to the setUp
and tearDown methods and the cache.clear call) to include cache.clear() or
self.addCleanup(cache.clear).
In `@workspace/core/tests/test_module_access_middleware.py`:
- Around line 31-48: The tests (test_enabled_module_not_blocked,
test_non_restrictable_module_never_blocked, test_superuser_bypasses_block)
currently only assert resp.status_code != 403 which can hide 404/500/401
failures; update each to assert the actual successful downstream outcome (e.g.
assert resp.status_code == 200 or use assertContains/assertIn on expected
response body) for the specific endpoints used (/api/v1/mail/accounts and
/api/v1/modules) so the tests verify real access rather than just absence of
403.
🪄 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 Plus
Run ID: a2777e1a-1e96-4850-a7ba-67da9925b434
📒 Files selected for processing (28)
workspace/ai/apps.pyworkspace/calendar/apps.pyworkspace/chat/apps.pyworkspace/core/activity_registry.pyworkspace/core/admin.pyworkspace/core/apps.pyworkspace/core/authentication.pyworkspace/core/context_processors.pyworkspace/core/middleware.pyworkspace/core/migrations/0001_initial.pyworkspace/core/migrations/__init__.pyworkspace/core/models.pyworkspace/core/services/module_access.pyworkspace/core/signals.pyworkspace/core/tests/test_activity.pyworkspace/core/tests/test_module_access_admin.pyworkspace/core/tests/test_module_access_hiding.pyworkspace/core/tests/test_module_access_middleware.pyworkspace/core/tests/test_module_access_model.pyworkspace/core/tests/test_module_access_permission.pyworkspace/core/tests/test_module_access_service.pyworkspace/core/tests/test_restrictable_modules.pyworkspace/core/tests/test_views_sse.pyworkspace/core/views.pyworkspace/core/views_sse.pyworkspace/mail/apps.pyworkspace/notes/apps.pyworkspace/settings.py
| return set(restrictable) | ||
|
|
||
| # Imported here to avoid a circular import (the model imports this module). | ||
| from workspace.core.models import ModuleAccessRule |
There was a problem hiding this comment.
Use relative import as per coding guidelines.
Service files under workspace/*/services/*.py should use relative imports to import from the parent module. As per coding guidelines, this import should be from ..models import ModuleAccessRule.
♻️ Proposed fix
- from workspace.core.models import ModuleAccessRule
+ from ..models import ModuleAccessRule📝 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
| from workspace.core.models import ModuleAccessRule | |
| from ..models import ModuleAccessRule |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workspace/core/services/module_access.py` at line 53, Replace the absolute
import in module_access.py with a relative import: change the current "from
workspace.core.models import ModuleAccessRule" to use the parent-package
relative form so that ModuleAccessRule is imported via "from ..models import
ModuleAccessRule"; update any other similar absolute imports in this file to
follow the same relative pattern to match service-module import guidelines.
Source: Coding guidelines
pchopinet
changed the title
4 tasks
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
Adds per-module access control so an administrator can disable or limit access to workspace modules (notes, chat, calendar, mail) either globally or per
auth.Group, configured entirely from the Django admin. Thefilesandaimodules are intentionally never restrictable (filesunderpins notes/WebDAV;aiis surfaced inside other modules).A single mechanism covers both intents:
What changed
ModuleAccessRule(module_slug, group, is_enabled)model in thecoreapp (first model there), managed via the Django admin with a slug dropdown limited to restrictable modules. Two constraints guarantee one global rule per module plus one rule per (module, group).restrictable = Trueflag on eachAppConfig, aggregated by a single helper (no hardcoded module list).permission_classes.Notes
notes,chat,calendar,mail. Not restrictable:files,ai, and all infrastructure modules (core,dashboard,users,notifications)./dav) is intentionally left unrestricted; sincefilesis never restrictable this is moot.Test plan
mailin the admin; confirm it disappears from nav/commands/search and that/mailand/api/v1/mail/*return 403 for a normal user (browser, token, and basic auth).mailback to a specific group and confirm only members regain access.