Feature secure access#42
Open
Linsss123 wants to merge 24 commits into
Open
Conversation
… in presigned URLs
…ccess checks in AttachmentDownloadController
…ccess checks in AttachmentDownloadController
…mentDownloadController, update S3Properties and README with presign-max-ttl-seconds
…update test dependencies in pom.xml
… add explicit dependency for spring-boot-test-autoconfigure in pom.xml
…ccess denial, and validation scenarios
…erTest to cover additional download scenarios
…cess and validation cases
…roller to cover new upload and presign scenarios, enhance validation coverage, and refine Content-Disposition checks.
…io with 502 response
…and enforce max TTL configuration
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements secure presigned URL generation for file downloads. It adds JWT authentication and role-based authorization checks to the attachment download endpoint, introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as AttachmentDownloadController
participant AuthService as AuthorizationService
participant AuditService as AuditService
participant MinioService as MinioStorageService
participant Repo as AttachmentRepository
Client->>Controller: POST /api/files/{id}/presign (ttl=3600)
Controller->>Repo: getAttachment(id)
Repo-->>Controller: Attachment
Controller->>AuthService: canAccessAttachment(user, attachment)
alt Authorization Denied
AuthService-->>Controller: false
Controller->>AuditService: log(ACCESS_DENIED, context)
Controller-->>Client: 403 Forbidden
else Authorization Granted
AuthService-->>Controller: true
Controller->>MinioService: generatePresignedGetUrlWithContentDisposition(objectKey, ttl, fileName)
MinioService-->>Controller: presignedUrl
Controller->>AuditService: log(FILE_PRESIGNED, context)
Controller-->>Client: 200 OK {url, expiresInSeconds}
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/main/java/org/example/alfs/controllers/AttachmentDownloadController.java (2)
126-127: Minor: 404 vs 403 discloses attachment existence.Because the 404 (not found) check runs before authorization, an authenticated REPORTER can distinguish existing-but-forbidden attachments from non-existent ones by probing IDs. Low-severity info leak given the JWT requirement, but you may want to return 404 for both cases (or check access first) to prevent enumeration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/AttachmentDownloadController.java` around lines 126 - 127, The current AttachmentDownloadController fetches the attachment via attachmentRepository.findById(...) and throws a 404 before authorization, allowing authenticated users to probe IDs; change the flow so you perform the authorization check (e.g., verify ownership/permission using your access-check method) before revealing existence or, alternatively always return 404 for both not-found and forbidden cases: locate the code around AttachmentDownloadController.findById usage and either move the access check to run on the retrieved entity before deciding the response or replace the 403 path with a generic ResponseStatusException(HttpStatus.NOT_FOUND, ...) so callers cannot distinguish forbidden vs missing attachments.
58-73: PotentialLazyInitializationExceptionwhen OSIV is disabled.
Attachment.ticketisFetchType.LAZY. WhenauthorizationService.canAccessAttachment()accessesticket.getReporter().getId()and whenAuditService.log()receivesatt.getTicket(), both operations occur outside a transaction in a non-@Transactionalcontroller. These lazy navigations work currently becausespring.jpa.open-in-viewdefaults totrue, but will fail withLazyInitializationExceptionif OSIV is later disabled (a common performance optimization).Avoid this by either:
- Fetching ticket and reporter eagerly via a custom repository method with
JOIN FETCH- Wrapping
authorizationService.canAccessAttachment()and the subsequentauditService.log()calls in@Transactional(readOnly = true)Also applies to the
presignendpoint (lines 126–144).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/AttachmentDownloadController.java` around lines 58 - 73, AttachmentDownloadController currently triggers lazy access to Attachment.ticket outside a transaction (authorizationService.canAccessAttachment and auditService.log use att.getTicket()), which will throw LazyInitializationException if OSIV is disabled; fix by ensuring the ticket and its reporter are loaded before leaving the repository layer or by executing the access+audit inside a read-only transaction: either add a custom repository method (e.g., attachmentRepository.findByIdWithTicketAndReporter(id) using JOIN FETCH to eagerly load ticket and ticket.reporter) and use that in place of attachmentRepository.findById(id), or annotate the controller methods (the download handler and the presign handler) with `@Transactional`(readOnly = true) so authorizationService.canAccessAttachment(...) and auditService.log(...) run inside a transaction. Ensure references to attachmentRepository.findById, authorizationService.canAccessAttachment, and auditService.log are updated accordingly.src/test/java/org/example/alfs/services/AuthorizationServiceTest.java (1)
12-88: Solid coverage; minor gap worth adding.The test matrix covers the important branches well. Two small additions would harden it:
- An explicit
attachment_with_null_ticket_should_be_deniedcase exercising theticket != nullguard (currently only theattachment == nullcase covers this branch indirectly).- JUnit 5 idiom:
public class AuthorizationServiceTestcan be package-private (class AuthorizationServiceTest). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AuthorizationServiceTest.java` around lines 12 - 88, Add a test that asserts AuthorizationService.canAccessAttachment denies access when an Attachment has a null Ticket (to exercise the ticket != null guard); create an Attachment by reusing/adding a helper (or calling attachmentWithReporter with a modification) and add a test method like attachment_with_null_ticket_should_be_denied that calls authorizationService.canAccessAttachment(user, attachmentWithNullTicket) and asserts false. Also optionally make the test class package-private by changing "public class AuthorizationServiceTest" to "class AuthorizationServiceTest" (non-blocking).src/test/java/org/example/alfs/controllers/AttachmentDownloadControllerTest.java (1)
113-132: Denial-path audit assertions are too loose.Both
presign_access_denied_returns_403_and_audit_logged(Line 129) anddownload_access_denied_returns_403_and_audit_logged(Line 214) useverify(auditService).log(any(), any(), any(), any(), any(), any()), which will pass even if a refactor silently emits the wrongAuditAction(e.g.,FILE_PRESIGNEDinstead ofACCESS_DENIED). SinceACCESS_DENIEDis the security-relevant signal here, consider asserting on the first argument.♻️ Tighter assertion
- verify(auditService).log(any(), any(), any(), any(), any(), any()); + verify(auditService).log(eq(AuditAction.ACCESS_DENIED), any(), any(), any(), any(), any());Same suggestion applies symmetrically to the success tests (
eq(AuditAction.FILE_PRESIGNED)/eq(AuditAction.FILE_DOWNLOAD_REQUESTED)).Also applies to: 202-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/controllers/AttachmentDownloadControllerTest.java` around lines 113 - 132, The audit verification in presign_access_denied_returns_403_and_audit_logged (and the symmetric download_access_denied_returns_403_and_audit_logged) is too loose; replace the generic verify(auditService).log(any(), ...) with a verification that the first argument equals the ACCESS_DENIED action (e.g., verify(auditService).log(eq(AuditAction.ACCESS_DENIED), any(), any(), any(), any(), any())); likewise tighten the success-path tests to assert the specific success actions (eq(AuditAction.FILE_PRESIGNED) and eq(AuditAction.FILE_DOWNLOAD_REQUESTED)) so the assertions in AttachmentDownloadControllerTest target the correct AuditAction rather than using any().src/test/java/org/example/alfs/services/AuditServiceDataJpaTest.java (1)
29-54: Consider capturing and asserting on the persistedAuditLogfields.The test verifies
save(...)is invoked, but doesn't confirm the controller actually populated theAuditLogwith the expected action, actor, ticket, or metadata. UsingArgumentCaptor<AuditLog>(orverify(..).save(argThat(...))) would ensure regressions inAuditService.log(...)— e.g., dropping the actor or using the wrongAuditAction— are caught.♻️ Example refactor using `ArgumentCaptor`
- // Assert - verify(auditLogRepository, times(1)).save(any(AuditLog.class)); + // Assert + ArgumentCaptor<AuditLog> captor = ArgumentCaptor.forClass(AuditLog.class); + verify(auditLogRepository, times(1)).save(captor.capture()); + AuditLog saved = captor.getValue(); + assertEquals(AuditAction.FILE_PRESIGNED, saved.getAction()); + assertEquals(reporter, saved.getActor()); + assertEquals(t, saved.getTicket()); + assertTrue(saved.getMetadata().contains("attachmentId=1"));Also, the class is named
AuditServiceDataJpaTestbut doesn't use@DataJpaTest(or any Spring testing support) — it's a pure Mockito unit test. Consider renaming toAuditServiceTestto avoid misleading future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AuditServiceDataJpaTest.java` around lines 29 - 54, Add assertions that the persisted AuditLog contains the expected populated fields by capturing the saved entity: use an ArgumentCaptor<AuditLog> (or verify(...).save(argThat(...))) when verifying auditLogRepository.save(...) and assert AuditLog.getAction()==AuditAction.FILE_PRESIGNED, getActor()/getActor().getId()==reporter.getId(), getTicket()/getTicket().getId()==t.getId(), and metadata/message matches "presign issued: attachmentId=1, ttl=120"; also update the test class name from AuditServiceDataJpaTest to AuditServiceTest (or add `@DataJpaTest` if you intend an integration test) so the class name accurately reflects that this is a Mockito unit test.README.md (1)
61-86: Mixed-language documentation: consider consistency.The existing README sections (steps 1-4) are in English, but the newly added sections for presign/download (steps 5-6 and notes) are in Swedish. For a public-facing README, picking one language (or providing both) improves discoverability for reviewers/consumers. Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 61 - 86, The new README sections "5) Secure download via presigned URL (Week 2)", "6) Direkt download-endpoint (GET)" and "Notes" are in Swedish while earlier steps are English—update those headings and all inline text (examples, descriptions, and the "Upload (POST /api/files/upload)"/endpoint mentions and cURL snippets) to English to match the rest of the document (or alternatively provide both English and Swedish versions); ensure endpoint names (/api/files/{id}/presign, POST /api/files/upload, GET /api/files/{id}/download), response JSON example, and authorization notes remain accurate and use consistent terminology (presigned URL, JWT, TTL, Content-Disposition, roles ADMIN/INVESTIGATOR/REPORTER).
🤖 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/alfs/controllers/AttachmentDownloadController.java`:
- Around line 117-122: Update the stale Javadoc on the presign endpoint in
AttachmentDownloadController to reflect that the method now performs
authentication and authorization; mention that it calls
securityUtils.getCurrentUser() for authentication and uses AuthorizationService
for access checks before returning a presigned MinIO URL, and remove or replace
the phrase "ingen auth ännu" so readers aren't misled about current behavior.
In `@src/main/java/org/example/alfs/services/storage/MinioStorageService.java`:
- Around line 97-105: The filename is injected directly into
response-content-disposition without sanitization and uses URLEncoder which
produces '+' for spaces instead of RFC 5987 percent-encoding; fix
generatePresignedGetUrlWithContentDisposition by first applying a filename
sanitizer (e.g., implement sanitizeFileName to trim, strip CR/LF/control chars,
remove path separators and dangerous characters), escape the value used inside
filename="..." as a quoted-string (implement escapeForQuotedString to
backslash-escape quotes and backslashes), and create a proper RFC 5987
percent-encoded value for filename* (implement rfc5987Encode that UTF-8
percent-encodes bytes and replaces '+' with '%20' rather than relying on
URLEncoder). Replace the current safeName/URLEncoder usage with
sanitizeFileName(suppliedName), escapeForQuotedString(safeName) for
filename="...", and rfc5987Encode(safeName) for filename*=UTF-8''..., and remove
or update any existing sanitize helper if needed.
In `@src/main/resources/application.properties`:
- Around line 7-15: The comment text in application.properties contains mojibake
(non-UTF-8 characters) causing Maven resource filtering to fail; open the file
and re-save it as UTF-8 (or replace the comment text) and remove/replace the
invalid characters in the header comments so they are ASCII-safe (e.g., replace
"miljöer" and "giltighetstid" chars), then verify keys like storage.s3.endpoint,
storage.s3.bucket, storage.s3.presign-max-ttl-seconds remain unchanged and
commit the UTF-8-saved file.
---
Nitpick comments:
In `@README.md`:
- Around line 61-86: The new README sections "5) Secure download via presigned
URL (Week 2)", "6) Direkt download-endpoint (GET)" and "Notes" are in Swedish
while earlier steps are English—update those headings and all inline text
(examples, descriptions, and the "Upload (POST /api/files/upload)"/endpoint
mentions and cURL snippets) to English to match the rest of the document (or
alternatively provide both English and Swedish versions); ensure endpoint names
(/api/files/{id}/presign, POST /api/files/upload, GET /api/files/{id}/download),
response JSON example, and authorization notes remain accurate and use
consistent terminology (presigned URL, JWT, TTL, Content-Disposition, roles
ADMIN/INVESTIGATOR/REPORTER).
In
`@src/main/java/org/example/alfs/controllers/AttachmentDownloadController.java`:
- Around line 126-127: The current AttachmentDownloadController fetches the
attachment via attachmentRepository.findById(...) and throws a 404 before
authorization, allowing authenticated users to probe IDs; change the flow so you
perform the authorization check (e.g., verify ownership/permission using your
access-check method) before revealing existence or, alternatively always return
404 for both not-found and forbidden cases: locate the code around
AttachmentDownloadController.findById usage and either move the access check to
run on the retrieved entity before deciding the response or replace the 403 path
with a generic ResponseStatusException(HttpStatus.NOT_FOUND, ...) so callers
cannot distinguish forbidden vs missing attachments.
- Around line 58-73: AttachmentDownloadController currently triggers lazy access
to Attachment.ticket outside a transaction
(authorizationService.canAccessAttachment and auditService.log use
att.getTicket()), which will throw LazyInitializationException if OSIV is
disabled; fix by ensuring the ticket and its reporter are loaded before leaving
the repository layer or by executing the access+audit inside a read-only
transaction: either add a custom repository method (e.g.,
attachmentRepository.findByIdWithTicketAndReporter(id) using JOIN FETCH to
eagerly load ticket and ticket.reporter) and use that in place of
attachmentRepository.findById(id), or annotate the controller methods (the
download handler and the presign handler) with `@Transactional`(readOnly = true)
so authorizationService.canAccessAttachment(...) and auditService.log(...) run
inside a transaction. Ensure references to attachmentRepository.findById,
authorizationService.canAccessAttachment, and auditService.log are updated
accordingly.
In
`@src/test/java/org/example/alfs/controllers/AttachmentDownloadControllerTest.java`:
- Around line 113-132: The audit verification in
presign_access_denied_returns_403_and_audit_logged (and the symmetric
download_access_denied_returns_403_and_audit_logged) is too loose; replace the
generic verify(auditService).log(any(), ...) with a verification that the first
argument equals the ACCESS_DENIED action (e.g.,
verify(auditService).log(eq(AuditAction.ACCESS_DENIED), any(), any(), any(),
any(), any())); likewise tighten the success-path tests to assert the specific
success actions (eq(AuditAction.FILE_PRESIGNED) and
eq(AuditAction.FILE_DOWNLOAD_REQUESTED)) so the assertions in
AttachmentDownloadControllerTest target the correct AuditAction rather than
using any().
In `@src/test/java/org/example/alfs/services/AuditServiceDataJpaTest.java`:
- Around line 29-54: Add assertions that the persisted AuditLog contains the
expected populated fields by capturing the saved entity: use an
ArgumentCaptor<AuditLog> (or verify(...).save(argThat(...))) when verifying
auditLogRepository.save(...) and assert
AuditLog.getAction()==AuditAction.FILE_PRESIGNED,
getActor()/getActor().getId()==reporter.getId(),
getTicket()/getTicket().getId()==t.getId(), and metadata/message matches
"presign issued: attachmentId=1, ttl=120"; also update the test class name from
AuditServiceDataJpaTest to AuditServiceTest (or add `@DataJpaTest` if you intend
an integration test) so the class name accurately reflects that this is a
Mockito unit test.
In `@src/test/java/org/example/alfs/services/AuthorizationServiceTest.java`:
- Around line 12-88: Add a test that asserts
AuthorizationService.canAccessAttachment denies access when an Attachment has a
null Ticket (to exercise the ticket != null guard); create an Attachment by
reusing/adding a helper (or calling attachmentWithReporter with a modification)
and add a test method like attachment_with_null_ticket_should_be_denied that
calls authorizationService.canAccessAttachment(user, attachmentWithNullTicket)
and asserts false. Also optionally make the test class package-private by
changing "public class AuthorizationServiceTest" to "class
AuthorizationServiceTest" (non-blocking).
🪄 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: 35e4490f-b0d5-4084-a1e4-e9d25645bba0
📒 Files selected for processing (14)
README.mdpom.xmlsrc/main/java/org/example/alfs/config/S3Properties.javasrc/main/java/org/example/alfs/config/SecurityConfig.javasrc/main/java/org/example/alfs/controllers/AttachmentDownloadController.javasrc/main/java/org/example/alfs/dto/attachment/PresignedUrlResponseDTO.javasrc/main/java/org/example/alfs/enums/AuditAction.javasrc/main/java/org/example/alfs/services/AuthorizationService.javasrc/main/java/org/example/alfs/services/storage/MinioStorageService.javasrc/main/resources/application.propertiessrc/test/java/org/example/alfs/controllers/AttachmentControllerUploadTest.javasrc/test/java/org/example/alfs/controllers/AttachmentDownloadControllerTest.javasrc/test/java/org/example/alfs/services/AuditServiceDataJpaTest.javasrc/test/java/org/example/alfs/services/AuthorizationServiceTest.java
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 by CodeRabbit
New Features
Security