fix(rest): guard attachment checkStatus approval behind CLEARING_ADMIN role#4047
Open
Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Open
fix(rest): guard attachment checkStatus approval behind CLEARING_ADMIN role#4047Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Conversation
Contributor
Author
|
Hey @GMishx, found this while going through the REST attachment flow; any user with write access on a release can set |
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
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.
While digging through the attachment update flow in the REST layer, I noticed that
Sw360AttachmentServicelets any user with write access on a release set an attachment'scheckStatustoACCEPTEDorREJECTED; no role check at all. This turns out to be a pretty serious gap becauseComponentDatabaseHandlercallsautosetReleaseClearingStateon every release update, and that method promotes the release straight toClearingState.APPROVEDthe moment it finds aCLEARING_REPORTorCOMPONENT_LICENSE_INFO_XMLattachment whosecheckStatusisACCEPTED. So a regular user who created a release could just upload a file, patch the attachment status toACCEPTEDvia the REST API, and their release would show up as fully cleared, without a clearing expert ever looking at it.The fix is small: before applying
ACCEPTEDorREJECTEDto an attachment in bothuploadAttachment()andupdateAttachment(), we now check that the caller holds at leastCLEARING_ADMIN. If they don't, we throwAccessDeniedExceptionwhich the existingRestExceptionHandlermaps to HTTP 403. Nothing else in the flow needed changing;autosetReleaseClearingStateitself is fine, it just needed trustworthy input.Issue: NA
Suggest Reviewer
@GMishx
(this touches the clearing state promotion logic so anyone familiar with the attachment approval flow would be a good fit.)
How To Test?
USER(non-clearing-admin).CLEARING_REPORTviaPOST /api/releases/{id}/attachments.checkStatustoACCEPTEDviaPATCH /api/releases/{id}/attachment/{attachmentContentId}with the same user token; you should get HTTP 403.CLEARING_ADMINtoken; it should go through and the releaseclearingStateshould becomeAPPROVED.checkStatustoNOTCHECKEDstill works for any user with write access (unchanged behaviour).No new automated tests added in this PR, but the above steps are quick to reproduce manually against a local SW360 instance.
Checklist
Must: