Fix DicomNode permission leak between groups

[samuelvkwong]

Summary

• Fixed a bug in DicomNodeManager.accessible_by_user() where permissions from one group could leak to users of another group
• The root cause was separate .filter() calls creating independent JOINs, allowing conditions to match different DicomNodeGroupAccess records
• Combined all filter conditions into a single .filter() call to ensure they apply to the same access record

Test plan

• Run pytest adit/core/tests/test_models.py::TestDicomNodeManager to verify the fix and regression test
• Verify existing permission behavior still works correctly
• Test scenario: User in Group A (source-only) should NOT get destination access from Group B's permissions on the same server

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed filtering logic to correctly handle group-based access permissions and access types, ensuring proper permission isolation between groups

• Tests

  • Added regression test to verify users only retain permissions from their assigned groups

[gemini-code-assist]

Summary of Changes

Hello @samuelvkwong, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a security vulnerability in the DicomNodeManager that could lead to incorrect permission assignments. By refactoring the query logic to combine filtering conditions, it ensures that user access to DICOM nodes is strictly confined to their assigned groups, thereby preventing unauthorized access and enhancing data security. A new test case has been added to validate the fix and prevent future regressions.

Highlights

• Permission Leak Fix: Addressed a critical bug in DicomNodeManager.accessible_by_user() where permissions could incorrectly leak between different user groups, potentially granting unauthorized access to DICOM nodes.
• Root Cause Identification: Identified that the issue stemmed from using separate .filter() calls, which led to independent database JOINs and allowed conditions to match different DicomNodeGroupAccess records.
• Solution Implementation: Refactored the accessible_by_user() method to combine all group and access type filter conditions into a single .filter() call, ensuring that all conditions apply to the same access record and preventing permission leakage.
• Regression Test: Added a new regression test case to test_models.py to specifically verify that permissions do not leak between groups, ensuring the fix is robust and prevents future regressions.

Changelog

• adit/core/models.py
  • Refactored the accessible_by_user method to consolidate filter conditions into a single Django ORM .filter() call.
  • Removed separate .filter() calls for group and access type to prevent unintended database JOINs and permission leakage.
  • Added comments explaining the rationale behind the change.
• adit/core/tests/test_models.py
  • Added test_accessible_by_user_does_not_leak_permissions_between_groups to verify that permissions are correctly isolated between user groups.
  • Created a scenario with a user in one group and a shared DICOM server with different access rights for another group to confirm no leakage occurs.

Activity

• The pull request was created by samuelvkwong to address a permission leak.
• A fix has been implemented in adit/core/models.py to consolidate filter logic.
• A new regression test has been added in adit/core/tests/test_models.py to validate the fix.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

The changes refactor the permission filtering logic in DicomNodeManager.accessible_by_user to combine group-based and access-type filters into a single filter operation, ensuring both conditions apply to the same access record. A regression test is added to verify that permissions from one group do not leak to users in a different group.

Changes

Cohort / File(s)	Summary
Permission Filtering Logic adit/core/models.py, adit/core/tests/test_models.py	Refactored filtering in DicomNodeManager.accessible_by_user to apply group and access type filters simultaneously rather than sequentially; added regression test ensuring cross-group permission isolation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A filter combined is a filter refined,
No leaking between groups—permissions aligned!
One query to bind them all tight,
Cross-group confusion set right. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix DicomNode permission leak between groups' directly and specifically describes the main bug fix in the changeset.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/dicom-node-group-permission-leak

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.}

[claude]

Code Review

Summary

This PR fixes a real and subtle Django ORM bug. The analysis is correct and the fix is the right approach. Here is my detailed feedback.

---

Bug Fix Assessment: Correct

The root cause description is accurate. When you chain separate .filter() calls on a queryset that involves a related table (accesses), Django performs a new JOIN for each .filter() call. This means the group condition and the access-type condition can match different rows in DicomNodeGroupAccess, producing the permission leak described. Combining all conditions into a single .filter() call forces them to apply to the same row, which is the correct behavior.

---

adit/core/models.py

Good

• The fix is minimal and targeted — only the logic that was broken is changed.
• The **access_filter unpacking pattern cleanly avoids duplicating the self.filter() calls.
• The inline comment explaining why a single .filter() is used is very helpful and should stay.

Minor concern — all_groups branch still has the same structure as before, now fixed

The all_groups=True path was also vulnerable to the same bug (chained .filter() calls), and it is now correctly fixed in the same way. Worth confirming the regression test also covers this path.

Looking at the test, it only exercises all_groups=False (the default). Consider adding a second assertion or a separate test that passes all_groups=True to verify the fix holds in that branch as well.

---

adit/core/tests/test_models.py

Good

• The regression test is well-structured and clearly documents the failure scenario in its docstring.
• The assertion message on the final assert provides a useful diagnostic if the test fails.
• Using DicomNodeGroupAccess.objects.create() directly (rather than a factory) is appropriate here since the test needs explicit control over source/destination flags.

Gaps to address

1. all_groups=True is not tested. The current test only covers the active-group path. The all_groups=True code path had the exact same bug, and there is no regression test for it. Add a parallel test where the user belongs to Group A and the query uses all_groups=True, asserting that destination access from Group B does not leak through.

2. Source access leak not covered. The test verifies that a user in Group A (source-only) does not get destination access from Group B (which has destination). It does not verify the symmetric case: a user in Group B (destination-only, no source) does not gain source access from Group A. This may or may not be a real risk depending on how the data is set up, but adding it would make the regression test comprehensive.

3. user.active_group is set before user.groups.add(). This is unlikely to cause issues in practice (the save happens after both), but for clarity, setting the group membership first and active_group second is a more natural order, since active_group conceptually depends on the user being in that group.

---

Security Impact

The bug, if exploited, allows a user to query a DICOM node as a destination (i.e., send data to it) or as a source (retrieve data from it) when they should not have that access. In a medical imaging context this is a meaningful access-control violation. The fix is necessary and correct.

---

Suggested small additions to the test

# Test all_groups=True does not leak permissions
accessible_destinations_all = DicomNode.objects.accessible_by_user(
    user, "destination", all_groups=True
)
assert not accessible_destinations_all.filter(pk=server.pk).exists(), (
    "Permission leaked via all_groups=True: user got destination access "
    "from Group B even though they are only in Group A"
)

This can be added as an additional assertion in the existing test, or as a separate test method.

---

Overall

The fix is correct and the PR is well-described. The main thing missing before merging is test coverage for the all_groups=True code path. Everything else is a minor polish item.

[gemini-code-assist]

Code Review

This pull request effectively resolves a critical permission leak bug in DicomNodeManager.accessible_by_user by consolidating separate .filter() calls into a single call, which correctly applies all conditions to the same database join. The fix is well-reasoned and the inclusion of a specific regression test is a great practice, ensuring this issue does not reappear. I have one minor suggestion to further refactor the implementation for improved code clarity and maintainability. Overall, this is a well-executed and important fix.

[gemini-code-assist]

To improve maintainability and adhere to the Don't Repeat Yourself (DRY) principle, you can construct the group filter arguments separately and then make a single .filter() call. This avoids repeating the self.filter(...) logic within the if/else block and makes the code more modular.

Suggested change

	if all_groups:
	accessible_nodes = self.filter(
	accesses__group__in=user.groups.all(), **access_filter
	)
	else:
	accessible_nodes = self.filter(accesses__group=user.active_group, **access_filter)
	group_kwargs = (
	{"accesses__group__in": user.groups.all()}
	if all_groups
	else {"accesses__group": user.active_group}
	)
	accessible_nodes = self.filter(**group_kwargs, **access_filter)