Fix/role based endpoint rules#216
Merged
Merged
Conversation
…min policy verification Closes #210
annikaholmqvist94
requested review from
TatjanaTrajkovic,
johanbriger and
lindaeskilsson
April 19, 2026 09:10
📝 WalkthroughWalkthroughSecurity authorization rules were changed to an explicit method+path role matrix; comment entities gained a typed Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommentService
participant CommentPolicy
participant Database
Note over Client,Database: Create comment flow
Client->>CommentService: create(commentDto, currentUser)
CommentService->>CommentService: determine type from currentUser.role
CommentService->>Database: save Comment(with type)
Database-->>CommentService: saved Comment
Note over Client,Database: Read comments with visibility filtering
Client->>CommentService: getByRecord(recordId, currentUser)
CommentService->>Database: findByMedicalRecordIdOrderByCreatedAtAsc(recordId)
Database-->>CommentService: List<Comment>
loop each comment
CommentService->>CommentPolicy: isVisibleTo(currentUser, comment)
alt visible
CommentPolicy-->>CommentService: true
else filtered
CommentPolicy-->>CommentService: false
end
end
CommentService-->>Client: filtered List<Comment>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/vet1177/policy/MedicalRecordPolicy.java (1)
39-67:⚠️ Potential issue | 🟠 MajorAuthorize the role before returning state-specific business errors.
Lines 40, 55, and 94 run final-state checks before the OWNER denial branches, so an OWNER acting on a closed record gets
BusinessRuleExceptioninstead of the intendedForbiddenException. Move the business-rule checks after role/scope authorization so unauthorized users do not receive record-state information.🛡️ Proposed fix
public void canUpdate(User user, MedicalRecord record) { - if (record.getStatus().isFinal()) - throw new BusinessRuleException("Stängda ärenden kan inte uppdateras"); - switch (user.getRole()) { case OWNER -> throw new ForbiddenException("Ägare får inte uppdatera ärenden"); case VET -> { if (!sameClinic(user, record)) throw new ForbiddenException("Du har inte tillgång till ärenden på en annan klinik"); } case ADMIN -> {} } + + if (record.getStatus().isFinal()) + throw new BusinessRuleException("Stängda ärenden kan inte uppdateras"); } public void canUpdateStatus(User user, MedicalRecord record, RecordStatus newStatus) { - if (record.getStatus().isFinal()) - throw new BusinessRuleException("Stängda ärenden kan inte uppdateras"); - switch (user.getRole()) { case OWNER -> throw new ForbiddenException("Ägare får inte ändra status på ärenden"); case VET -> { if (!sameClinic(user, record)) throw new ForbiddenException("Du har inte tillgång till ärenden på en annan klinik"); } case ADMIN -> {} } + + if (record.getStatus().isFinal()) + throw new BusinessRuleException("Stängda ärenden kan inte uppdateras"); } public void canClose(User user, MedicalRecord record) { - if (record.getStatus().isFinal()) - throw new BusinessRuleException("Ärendet är redan stängt"); - switch (user.getRole()) { case OWNER -> throw new ForbiddenException("Ägare får inte stänga ärenden"); case VET -> { if (!sameClinic(user, record)) throw new ForbiddenException("Du har inte tillgång till ärenden på en annan klinik"); } case ADMIN -> {} } + + if (record.getStatus().isFinal()) + throw new BusinessRuleException("Ärendet är redan stängt"); }Also applies to: 93-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/policy/MedicalRecordPolicy.java` around lines 39 - 67, The final-state check (record.getStatus().isFinal()) should not run before authorization because OWNERs (and unauthorized users) would see BusinessRuleException instead of ForbiddenException; update canUpdate and canUpdateStatus so you first enforce role/scope checks (handle OWNER -> throw ForbiddenException, VET -> verify sameClinic(user, record) and throw ForbiddenException if not, ADMIN allowed) and only after the caller is authorized perform the final-state check and throw BusinessRuleException; refer to the methods canUpdate and canUpdateStatus and symbols OWNER, VET, ADMIN, sameClinic(...) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/example/vet1177/policy/MedicalRecordPolicy.java`:
- Around line 39-67: The final-state check (record.getStatus().isFinal()) should
not run before authorization because OWNERs (and unauthorized users) would see
BusinessRuleException instead of ForbiddenException; update canUpdate and
canUpdateStatus so you first enforce role/scope checks (handle OWNER -> throw
ForbiddenException, VET -> verify sameClinic(user, record) and throw
ForbiddenException if not, ADMIN allowed) and only after the caller is
authorized perform the final-state check and throw BusinessRuleException; refer
to the methods canUpdate and canUpdateStatus and symbols OWNER, VET, ADMIN,
sameClinic(...) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6c7f37e-ecdc-4a3b-b5f8-0a04ea247f0d
📒 Files selected for processing (4)
src/main/java/org/example/vet1177/policy/MedicalRecordPolicy.javasrc/main/java/org/example/vet1177/security/SecurityConfig.javasrc/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.javasrc/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java
…ecord and pet ownership alignment
…e based on user role
…te validation logic
…ICAL_NOTE visibility rules
…nd visibility filtering logic
Contributor
Author
|
Roller och vad de får
URL-regler (grovmaskigt rollfilter i SecurityConfig)
403 = fel roll. 401 = ogiltig/utgången token. Er nuvarande response-interceptor hanterar bara 401 — lägg gärna till en 403-branch som visar en toast ("Du saknar behörighet"). Nya kommentartyper (viktigt för CaseDetail.jsx) CommentResponse innehåller nu type: "OWNER_MESSAGE" | "VET_CLINICAL_NOTE".
Frontend-saker att åtgärda mot regler
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/vet1177/policy/AttachmentPolicy.java (1)
54-64:⚠️ Potential issue | 🟠 MajorKeep OWNER deletion denial explicit.
Line 60 reintroduces
OWNERinto the allowed self-delete branch, which conflicts with the PR objective that OWNER cannot delete attachments. IfMedicalRecordPolicy.canUpdatecurrently blocks OWNER, this branch is unreachable; if that policy changes later, this becomes a privilege regression. SinceAttachmentController.deleteAttachment()delegates straight to policy and the providedSecurityConfigsnippet only requires authentication,AttachmentPolicy.canDeleteshould state the OWNER rule directly.🔒 Proposed authorization fix
switch (user.getRole()) { case ADMIN -> { // Admin har fulla rättigheter att radera. } - case VET, OWNER -> { - // VET och OWNER får endast radera bilagor de själva har laddat upp. + case VET -> { + // VET får endast radera bilagor de själva har laddat upp. if (attachment.getUploadedBy() == null || !attachment.getUploadedBy().getId().equals(user.getId())) { throw new ForbiddenException("Du kan endast radera bilagor du själv har laddat upp."); } } + case OWNER -> throw new ForbiddenException("Djurägare får inte radera bilagor."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/policy/AttachmentPolicy.java` around lines 54 - 64, AttachmentPolicy.canDelete currently groups OWNER into the self-delete branch which reintroduces deletion rights for owners; change the switch in AttachmentPolicy.canDelete so that OWNER is not included in the allowed branch (use case VET -> { ... } only) and add an explicit case OWNER -> throw new ForbiddenException("Du kan inte radera bilagor som ägs av posten" or the project-standard owner-deny message) so OWNER denial is explicit; keep the existing call to MedicalRecordPolicy.canUpdate(user, attachment.getMedicalRecord()) and ensure AttachmentController.deleteAttachment and SecurityConfig behavior remain unchanged.
🧹 Nitpick comments (4)
src/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java (1)
82-84: Minor: duplicatedotherOwnersetup and inconsistent message assertions.Three tests (Line 82-84, 118-120, 161-163) re-construct the same
otherOwner. Lift it intosetUp()to shrink noise. Additionally, thecanView/canClose/canUpdateStatus/canAssignVet/canViewClinictests only assert the exception type, whereas thecanCreate/canUpdatetests also assert the Swedish message. Consider assertinghasMessage(...)consistently so regressions in user-facing error strings are caught uniformly.Also applies to: 118-120, 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java` around lines 82 - 84, Multiple tests recreate the same User instance otherOwner; move that construction (new User(...) and setPrivateField(otherOwner, "id", ...)) into the test class setUp() method and reuse the field otherOwner in tests (e.g., canCreate_ownerOfOtherPet_shouldThrowForbidden, canView_ownerOfOtherPet_shouldThrowForbidden, canUpdate_ownerOfOtherPet_shouldThrowForbidden) to reduce duplication; also make exception assertions consistent by adding hasMessage("Du saknar åtkomst") (or the exact Swedish message used in canCreate/canUpdate) to the existing assertThrows/exception checks in canView/canClose/canUpdateStatus/canAssignVet/canViewClinic tests so all forbidden tests verify both exception type and message.src/main/resources/schema.sql (1)
66-71: Constraincomment_typeto valid enum values.The column is non-null, but direct SQL/imports can still write values that do not map to
CommentType, causing read-time failures in JPA. Add a check constraint alongside the default.Suggested schema hardening
- comment_type VARCHAR(32) NOT NULL DEFAULT 'OWNER_MESSAGE', + comment_type VARCHAR(32) NOT NULL DEFAULT 'OWNER_MESSAGE' + CHECK (comment_type IN ('OWNER_MESSAGE', 'VET_CLINICAL_NOTE')),For existing deployments:
DO $$ BEGIN ALTER TABLE comment ADD CONSTRAINT chk_comment_comment_type CHECK (comment_type IN ('OWNER_MESSAGE', 'VET_CLINICAL_NOTE')); EXCEPTION WHEN duplicate_object THEN NULL; END $$;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/schema.sql` around lines 66 - 71, Add a CHECK constraint to the comment table to restrict comment_type to valid enum values (e.g., 'OWNER_MESSAGE', 'VET_CLINICAL_NOTE') to prevent invalid values being written by direct SQL/imports; update the migration/schema by adding a constraint named e.g. chk_comment_comment_type on table comment that enforces comment_type IN (…), and for existing deployments wrap the addition in a safe block (DO $$ ... EXCEPTION WHEN duplicate_object THEN NULL; END $$) or use an IF NOT EXISTS pattern so the ALTER TABLE doesn't fail if the constraint already exists.src/main/java/org/example/vet1177/services/CommentService.java (1)
125-130: Avoid loading every comment just to count visible comments.
countByRecord(...)now materializes the full ordered comment list and entity graph before counting. With the current visibility rule, this can stay as a DB count.Proposed direction
- return commentRepository.findByMedicalRecordIdOrderByCreatedAtAsc(recordId) - .stream() - .filter(c -> commentPolicy.isVisibleTo(currentUser, c)) - .count(); + if (currentUser.getRole() == Role.OWNER) { + return commentRepository.countByMedicalRecordIdAndType(recordId, CommentType.OWNER_MESSAGE); + } + return commentRepository.countByMedicalRecordId(recordId);Add the repository method:
long countByMedicalRecordIdAndType(UUID recordId, CommentType type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/CommentService.java` around lines 125 - 130, The current code materializes all comments via commentRepository.findByMedicalRecordIdOrderByCreatedAtAsc(...) then filters with commentPolicy.isVisibleTo(...) to get a count; instead add a repository method long countByMedicalRecordIdAndType(UUID recordId, CommentType type) and use it so counting happens in the DB. Keep the existing commentPolicy.canView(currentUser, record) call, determine the CommentType that corresponds to the visibility rule used by commentPolicy.isVisibleTo(...) (the same type used in the filter), and replace the stream/count block with a single call to commentRepository.countByMedicalRecordIdAndType(recordId, thatType).src/main/java/org/example/vet1177/entities/Comment.java (1)
26-28: Enforce the non-nulltypeinvariant in the setter.The DB/JPA mapping says this is non-null, but
setType(null)can still create an invalid in-memory entity before flush. Guard it at the entity boundary.Proposed fix
import jakarta.persistence.*; import java.time.Instant; +import java.util.Objects; import java.util.UUID;public CommentType getType() { return type; } - public void setType(CommentType type) { this.type = type; } + public void setType(CommentType type) { this.type = Objects.requireNonNull(type, "type"); }Also applies to: 60-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/Comment.java` around lines 26 - 28, The entity's non-null invariant for the CommentType field is only enforced by JPA annotations but not in-memory; update the Comment class's setType(CommentType type) to validate input (e.g., Objects.requireNonNull or an explicit null check) and throw an IllegalArgumentException with a clear message if null is passed, ensuring the in-memory entity cannot be put into an invalid state; apply the same null-guard pattern to any other enum/setter methods in this class that are annotated non-null (e.g., other setters for non-null enum fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/policy/CommentPolicy.java`:
- Around line 70-72: The visibility check in CommentPolicy.isVisibleTo currently
lets OWNERs see any non-VET_CLINICAL_NOTE (including null/unrecognized types);
change the logic to "fail closed" for owners by first checking if user.getRole()
== Role.OWNER and, if so, only return true when comment.getType() ==
CommentType.OWNER_MESSAGE; otherwise preserve the non-owner behavior. Update
isVisibleTo in CommentPolicy to explicitly handle Role.OWNER first and compare
comment.getType() against CommentType.OWNER_MESSAGE.
In `@src/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java`:
- Around line 76-149: Tests in MedicalRecordPolicyTest contradict the PR
objective that OWNERs must be blocked by the policy; update the two
owner-positive tests (canCreate_ownerOfPet_shouldNotThrow and
canUpdate_ownerOfRecord_shouldNotThrow) so they assert a ForbiddenException is
thrown when calling MedicalRecordPolicy.canCreate(owner, pet, clinic) and
MedicalRecordPolicy.canUpdate(owner, openRecord) respectively, using the same
localized forbidden messages the policy produces (e.g., the messages used
elsewhere for owner-forbidden cases) or the exact strings from
MedicalRecordPolicy; if you prefer the other route, instead update
MedicalRecordPolicy.canCreate / MedicalRecordPolicy.canUpdate to universally
throw ForbiddenException for Role.OWNER before the owner-equality checks so the
existing tests remain valid—choose one consistent approach and make the tests
and policy match (reference: MedicalRecordPolicy.canCreate,
MedicalRecordPolicy.canUpdate, and the test methods named above).
---
Outside diff comments:
In `@src/main/java/org/example/vet1177/policy/AttachmentPolicy.java`:
- Around line 54-64: AttachmentPolicy.canDelete currently groups OWNER into the
self-delete branch which reintroduces deletion rights for owners; change the
switch in AttachmentPolicy.canDelete so that OWNER is not included in the
allowed branch (use case VET -> { ... } only) and add an explicit case OWNER ->
throw new ForbiddenException("Du kan inte radera bilagor som ägs av posten" or
the project-standard owner-deny message) so OWNER denial is explicit; keep the
existing call to MedicalRecordPolicy.canUpdate(user,
attachment.getMedicalRecord()) and ensure AttachmentController.deleteAttachment
and SecurityConfig behavior remain unchanged.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/entities/Comment.java`:
- Around line 26-28: The entity's non-null invariant for the CommentType field
is only enforced by JPA annotations but not in-memory; update the Comment
class's setType(CommentType type) to validate input (e.g.,
Objects.requireNonNull or an explicit null check) and throw an
IllegalArgumentException with a clear message if null is passed, ensuring the
in-memory entity cannot be put into an invalid state; apply the same null-guard
pattern to any other enum/setter methods in this class that are annotated
non-null (e.g., other setters for non-null enum fields).
In `@src/main/java/org/example/vet1177/services/CommentService.java`:
- Around line 125-130: The current code materializes all comments via
commentRepository.findByMedicalRecordIdOrderByCreatedAtAsc(...) then filters
with commentPolicy.isVisibleTo(...) to get a count; instead add a repository
method long countByMedicalRecordIdAndType(UUID recordId, CommentType type) and
use it so counting happens in the DB. Keep the existing
commentPolicy.canView(currentUser, record) call, determine the CommentType that
corresponds to the visibility rule used by commentPolicy.isVisibleTo(...) (the
same type used in the filter), and replace the stream/count block with a single
call to commentRepository.countByMedicalRecordIdAndType(recordId, thatType).
In `@src/main/resources/schema.sql`:
- Around line 66-71: Add a CHECK constraint to the comment table to restrict
comment_type to valid enum values (e.g., 'OWNER_MESSAGE', 'VET_CLINICAL_NOTE')
to prevent invalid values being written by direct SQL/imports; update the
migration/schema by adding a constraint named e.g. chk_comment_comment_type on
table comment that enforces comment_type IN (…), and for existing deployments
wrap the addition in a safe block (DO $$ ... EXCEPTION WHEN duplicate_object
THEN NULL; END $$) or use an IF NOT EXISTS pattern so the ALTER TABLE doesn't
fail if the constraint already exists.
In `@src/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java`:
- Around line 82-84: Multiple tests recreate the same User instance otherOwner;
move that construction (new User(...) and setPrivateField(otherOwner, "id",
...)) into the test class setUp() method and reuse the field otherOwner in tests
(e.g., canCreate_ownerOfOtherPet_shouldThrowForbidden,
canView_ownerOfOtherPet_shouldThrowForbidden,
canUpdate_ownerOfOtherPet_shouldThrowForbidden) to reduce duplication; also make
exception assertions consistent by adding hasMessage("Du saknar åtkomst") (or
the exact Swedish message used in canCreate/canUpdate) to the existing
assertThrows/exception checks in
canView/canClose/canUpdateStatus/canAssignVet/canViewClinic tests so all
forbidden tests verify both exception type and message.
🪄 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: 13782a56-1d09-4138-b136-aab0e207ad12
📒 Files selected for processing (11)
src/main/java/org/example/vet1177/dto/response/comment/CommentResponse.javasrc/main/java/org/example/vet1177/entities/Comment.javasrc/main/java/org/example/vet1177/entities/CommentType.javasrc/main/java/org/example/vet1177/policy/AttachmentPolicy.javasrc/main/java/org/example/vet1177/policy/CommentPolicy.javasrc/main/java/org/example/vet1177/security/SecurityConfig.javasrc/main/java/org/example/vet1177/services/CommentService.javasrc/main/resources/schema.sqlsrc/test/java/org/example/vet1177/policy/CommentPolicyTest.javasrc/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.javasrc/test/java/org/example/vet1177/services/CommentServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/vet1177/entities/CommentType.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/vet1177/security/SecurityConfig.java
Comment on lines
+76
to
+149
| @Test | ||
| void canCreate_ownerOfPet_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canCreate(owner, pet, clinic)); | ||
| } | ||
|
|
||
| @Test | ||
| void canCreate_ownerOfOtherPet_shouldThrowForbidden() throws Exception { | ||
| User otherOwner = new User("Bertil Annan", "b@mail.se", "hash", Role.OWNER); | ||
| setPrivateField(otherOwner, "id", UUID.randomUUID()); | ||
|
|
||
| assertThatThrownBy(() -> policy.canCreate(otherOwner, pet, clinic)) | ||
| .isInstanceOf(ForbiddenException.class) | ||
| .hasMessage("Du kan inte skapa ärende för någon annans djur"); | ||
| } | ||
|
|
||
| @Test | ||
| void canCreate_vetOnOwnClinic_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canCreate(vet, pet, clinic)); | ||
| } | ||
|
|
||
| @Test | ||
| void canCreate_vetOnOtherClinic_shouldThrowForbidden() { | ||
| assertThatThrownBy(() -> policy.canCreate(vetOtherClinic, pet, clinic)) | ||
| .isInstanceOf(ForbiddenException.class) | ||
| .hasMessage("Du kan inte skapa ärende för en annan klinik"); | ||
| } | ||
|
|
||
| @Test | ||
| void canCreate_admin_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canCreate(admin, pet, clinic)); | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------------- | ||
| // canUpdate — OWNER får uppdatera eget ärende (öppet) | ||
| // ------------------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void canUpdate_ownerOfRecord_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canUpdate(owner, openRecord)); | ||
| } | ||
|
|
||
| @Test | ||
| void canUpdate_ownerOfOtherRecord_shouldThrowForbidden() throws Exception { | ||
| User otherOwner = new User("Bertil Annan", "b@mail.se", "hash", Role.OWNER); | ||
| setPrivateField(otherOwner, "id", UUID.randomUUID()); | ||
|
|
||
| assertThatThrownBy(() -> policy.canUpdate(otherOwner, openRecord)) | ||
| .isInstanceOf(ForbiddenException.class) | ||
| .hasMessage("Du har inte tillgång till detta ärende"); | ||
| } | ||
|
|
||
| @Test | ||
| void canUpdate_vetOnOwnClinic_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canUpdate(vet, openRecord)); | ||
| } | ||
|
|
||
| @Test | ||
| void canUpdate_vetOnOtherClinic_shouldThrowForbidden() { | ||
| assertThatThrownBy(() -> policy.canUpdate(vetOtherClinic, openRecord)) | ||
| .isInstanceOf(ForbiddenException.class) | ||
| .hasMessage("Du har inte tillgång till ärenden på en annan klinik"); | ||
| } | ||
|
|
||
| @Test | ||
| void canUpdate_admin_shouldNotThrow() { | ||
| assertThatNoException().isThrownBy(() -> policy.canUpdate(admin, openRecord)); | ||
| } | ||
|
|
||
| @Test | ||
| void canUpdate_closedRecord_shouldThrowBusinessRule() { | ||
| assertThatThrownBy(() -> policy.canUpdate(vet, closedRecord)) | ||
| .isInstanceOf(BusinessRuleException.class) | ||
| .hasMessage("Stängda ärenden kan inte uppdateras"); | ||
| } |
There was a problem hiding this comment.
OWNER create/update tests contradict the stated PR objective.
The PR description explicitly states that OWNER should no longer be able to create or update medical records, and that MedicalRecordPolicy.canCreate / canUpdate should throw ForbiddenException for OWNER. However, these tests assert the opposite:
canCreate_ownerOfPet_shouldNotThrow(Line 77-79) — asserts OWNER can create for own pet.canUpdate_ownerOfRecord_shouldNotThrow(Line 113-115) — asserts OWNER can update own open record.
Looking at the current MedicalRecordPolicy source, the OWNER branches in both methods still only throw when the user isn't the pet/record owner — they do not universally block OWNER as the PR description claims. So one of two things is true, and both are problematic:
- The policy code was not actually tightened as described in the PR — the claimed security restriction (OWNER blocked at the policy layer) is missing, and relying on URL rules alone is defense-in-depth that the PR explicitly promised in the policy layer.
- Or the tests encode pre-change expectations and will lock in the wrong behavior going forward.
Please reconcile MedicalRecordPolicy.canCreate / canUpdate with the PR objective and flip these assertions accordingly. Expected tests for the new behavior would look like:
🔧 Suggested expectation change (if policy is updated per PR description)
`@Test`
-void canCreate_ownerOfPet_shouldNotThrow() {
- assertThatNoException().isThrownBy(() -> policy.canCreate(owner, pet, clinic));
+void canCreate_owner_shouldAlwaysThrowForbidden() {
+ assertThatThrownBy(() -> policy.canCreate(owner, pet, clinic))
+ .isInstanceOf(ForbiddenException.class);
}
`@Test`
-void canUpdate_ownerOfRecord_shouldNotThrow() {
- assertThatNoException().isThrownBy(() -> policy.canUpdate(owner, openRecord));
+void canUpdate_owner_shouldAlwaysThrowForbidden() {
+ assertThatThrownBy(() -> policy.canUpdate(owner, openRecord))
+ .isInstanceOf(ForbiddenException.class);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/vet1177/policy/MedicalRecordPolicyTest.java` around
lines 76 - 149, Tests in MedicalRecordPolicyTest contradict the PR objective
that OWNERs must be blocked by the policy; update the two owner-positive tests
(canCreate_ownerOfPet_shouldNotThrow and canUpdate_ownerOfRecord_shouldNotThrow)
so they assert a ForbiddenException is thrown when calling
MedicalRecordPolicy.canCreate(owner, pet, clinic) and
MedicalRecordPolicy.canUpdate(owner, openRecord) respectively, using the same
localized forbidden messages the policy produces (e.g., the messages used
elsewhere for owner-forbidden cases) or the exact strings from
MedicalRecordPolicy; if you prefer the other route, instead update
MedicalRecordPolicy.canCreate / MedicalRecordPolicy.canUpdate to universally
throw ForbiddenException for Role.OWNER before the owner-equality checks so the
existing tests remain valid—choose one consistent approach and make the tests
and policy match (reference: MedicalRecordPolicy.canCreate,
MedicalRecordPolicy.canUpdate, and the test methods named above).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/security/SecurityConfig.java`:
- Around line 95-107: Update SecurityConfig to add URL-layer deny rules for
creating/updating medical records and attachment operations: add requestMatchers
to block POST /api/medical-records, PUT /api/medical-records/* (base record
update) and attachment upload/delete endpoints (e.g.,
/api/medical-records/*/attachments/**) so OWNER cannot reach them at the URL
layer; ensure the general PUT matcher for "/api/medical-records/*" is placed
after the specific PUT matchers ("/close", "/assign-vet", "/status") so they
take precedence; also update the surrounding comment text in SecurityConfig to
reflect that OWNER is blocked at the URL layer for these operations rather than
allowed to create/update or manage attachments.
🪄 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: ce041763-99d9-4c00-9699-8ededeeda164
📒 Files selected for processing (3)
src/main/java/org/example/vet1177/policy/CommentPolicy.javasrc/main/java/org/example/vet1177/security/SecurityConfig.javasrc/test/java/org/example/vet1177/policy/CommentPolicyTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/vet1177/policy/CommentPolicy.java
Comment on lines
+95
to
+107
| // ─── VET/ADMIN: ärende-status/tilldelning/stängning ─── | ||
| .requestMatchers(HttpMethod.PUT, "/api/medical-records/*/close").hasAnyRole("VET", "ADMIN") | ||
| .requestMatchers(HttpMethod.PUT, "/api/medical-records/*/assign-vet").hasAnyRole("VET", "ADMIN") | ||
| .requestMatchers(HttpMethod.PUT, "/api/medical-records/*/status").hasAnyRole("VET", "ADMIN") | ||
|
|
||
| // ─── VET/ADMIN: klinik-vy ─── | ||
| .requestMatchers(HttpMethod.GET, "/api/medical-records/clinic/**").hasAnyRole("VET", "ADMIN") | ||
|
|
||
| // ─── Alla inloggade + policy finjusterar ─── | ||
| // Här ligger medvetet: skapa/uppdatera egna ärenden, se/skapa/radera egna bilagor, | ||
| // kommentarer, aktivitetsloggar, /api/pets/**. URL-lagret kan inte uttrycka ägarskap | ||
| // eller "samma klinik" — det gör policy. OWNER kan skapa/uppdatera egna ärenden; | ||
| // kliniska anteckningar döljs via CommentType-filter i CommentService. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which medical-record and attachment endpoints actually exist in controllers,
# to confirm the URL patterns the new matchers should cover.
rg -nP --type=java -C2 '@(Post|Put|Delete|Get)Mapping' -g '**/controller/**' -g '**/MedicalRecord*.java' -g '**/Attachment*.java'Repository: ithsjava25/project-backend-org-random-coders
Length of output: 21256
🏁 Script executed:
cat -n src/main/java/org/example/vet1177/security/SecurityConfig.java | sed -n '80,125p'Repository: ithsjava25/project-backend-org-random-coders
Length of output: 3294
Missing URL-layer rules for medical-record create/update and attachment operations contradict the PR intent.
The PR description states OWNER is blocked at the URL layer for POST /api/medical-records, PUT /api/medical-records/{id} (base record update), and attachment upload/delete. None of these rules exist in the matcher list; all fall through to .anyRequest().authenticated(), so authorization is delegated entirely to the policy layer.
Additionally, the comment at lines 104–107 contradicts the stated PR goal. It says "OWNER kan skapa/uppdatera egna ärenden" and "se/skapa/radera egna bilagor" — implying policy-layer control — yet the PR description explicitly requires URL-layer rejection of OWNER for these operations.
These endpoints should be blocked at the URL layer for defense-in-depth:
🛡️ Add missing URL matchers
// ─── VET/ADMIN: ärende-status/tilldelning/stängning ───
.requestMatchers(HttpMethod.PUT, "/api/medical-records/*/close").hasAnyRole("VET", "ADMIN")
.requestMatchers(HttpMethod.PUT, "/api/medical-records/*/assign-vet").hasAnyRole("VET", "ADMIN")
.requestMatchers(HttpMethod.PUT, "/api/medical-records/*/status").hasAnyRole("VET", "ADMIN")
+ // ─── VET/ADMIN: skapa/uppdatera medicinska ärenden ───
+ .requestMatchers(HttpMethod.POST, "/api/medical-records").hasAnyRole("VET", "ADMIN")
+ .requestMatchers(HttpMethod.PUT, "/api/medical-records/*").hasAnyRole("VET", "ADMIN")
+
+ // ─── VET/ADMIN: bilagor (ladda upp/radera) ───
+ .requestMatchers(HttpMethod.POST, "/api/attachments/record/**").hasAnyRole("VET", "ADMIN")
+ .requestMatchers(HttpMethod.DELETE, "/api/attachments/**").hasAnyRole("VET", "ADMIN")
+
// ─── VET/ADMIN: klinik-vy ───
.requestMatchers(HttpMethod.GET, "/api/medical-records/clinic/**").hasAnyRole("VET", "ADMIN")
// ─── Alla inloggade + policy finjusterar ───
- // Här ligger medvetet: skapa/uppdatera egna ärenden, se/skapa/radera egna bilagor,
- // kommentarer, aktivitetsloggar, /api/pets/**. URL-lagret kan inte uttrycka ägarskap
- // eller "samma klinik" — det gör policy. OWNER kan skapa/uppdatera egna ärenden;
- // kliniska anteckningar döljs via CommentType-filter i CommentService.
+ // Här ligger medvetet: se egna ärenden/bilagor, kommentarer, aktivitetsloggar,
+ // /api/pets/**. URL-lagret kan inte uttrycka ägarskap eller "samma klinik" —
+ // det gör policy. Kliniska anteckningar döljs via CommentType-filter i CommentService.
.anyRequest().authenticated()Note: The PUT /api/medical-records/* rule must come after the specific sub-path matchers (*/close, */assign-vet, */status) so the more specific patterns match first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/security/SecurityConfig.java` around lines
95 - 107, Update SecurityConfig to add URL-layer deny rules for
creating/updating medical records and attachment operations: add requestMatchers
to block POST /api/medical-records, PUT /api/medical-records/* (base record
update) and attachment upload/delete endpoints (e.g.,
/api/medical-records/*/attachments/**) so OWNER cannot reach them at the URL
layer; ensure the general PUT matcher for "/api/medical-records/*" is placed
after the specific PUT matchers ("/close", "/assign-vet", "/status") so they
take precedence; also update the surrounding comment text in SecurityConfig to
reflect that OWNER is blocked at the URL layer for these operations rather than
allowed to create/update or manage attachments.
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.
Sammanställning vad vi exakt ändrat för krav per roll
OWNER strikt tillstramat
VET rollen tydligare bekräftad i URL-lagret
policy.
ADMIN oförändrat
authenticated(), respektive @PreAuthorize/AdminPolicy).
Publika endpoints oförändrat
Tester som berördes
klinik) och ADMIN.
Summary by CodeRabbit
New Features
Improvements
Tests