Skip to content

fix(backend): Validate file group ownership on submit-edited-data#6885

Merged
tombch merged 4 commits into
mainfrom
validate-file-groups-edited-data
Jul 17, 2026
Merged

fix(backend): Validate file group ownership on submit-edited-data#6885
tombch merged 4 commits into
mainfrom
validate-file-groups-edited-data

Conversation

@tombch

@tombch tombch commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

resolves #6732

Summary

  • Adds usage of validateFilesBelongToSubmittingGroups in submitEditedData for the SubmissionDatabaseService.
  • Adds tests confirming success and failure cases for this.

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

@claude claude Bot added the backend related to the loculus backend component label Jul 10, 2026
@tombch
tombch marked this pull request as ready for review July 10, 2026 13:21
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 2m 12s —— View job


Claude finished reviewing this PR

Todo list

  • Gather context (read diff, related code)
  • Understand the request (code review)
  • Review code quality, security, performance
  • Review test coverage
  • Post final review

Summary

This is a clean, minimal, well-targeted fix. It reuses the existing validateFilesBelongToSubmittingGroups helper (already used in updateProcessedData) rather than duplicating logic, and closes a real authorization gap: previously, a user editing their own sequence entry could reference a file ID belonging to another group without an ownership check, letting a private file from another group become associated with (and potentially released alongside) their own entry.

Observations

  • The new check in SubmissionDatabaseService.kt:1170-1175 is correctly placed after validateFilesExist/validateMultipartUploads, so file-existence errors still take priority over ownership errors — consistent with the existing updateProcessedData path.
  • It's also correctly placed after the accessionPreconditionValidator.validate { ... andThatUserIsAllowedToEditSequenceEntries(...) } block, so the group-ownership check happens only once permission on the sequence entry itself has been confirmed.
  • Test coverage is good: adds both a success case (file owned by the same group) and a failure case (file owned by another group, asserting the specific error message). The existing "no file uploaded" test was updated to pass a matching groupId so it continues to exercise the intended failure path rather than incidentally tripping the new group-ownership check.
  • Minor: validateFilesBelongToSubmittingGroups is written for batch use (map of accession version → file IDs) and is called here with a single-entry map. This mirrors how it looks in the batch path and isn't a real issue — no action needed, just noting there's no per-call overhead concern given submitEditedData operates on one entry at a time anyway.

No inline comments — didn't find anything worth flagging at that granularity.

Note on verification

I wasn't able to run ./gradlew ktlintCheck or ./gradlew test in this environment (network/tool permissions blocked git fetch and ./gradlew invocations). The change is small and follows the exact pattern of the pre-existing call site, so I'm confident in it from static review, but if you'd like CI-equivalent confirmation, let me know and update the allowed tools so I can run the build.

@clonder clonder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@tombch
tombch merged commit 6085812 into main Jul 17, 2026
43 checks passed
@tombch
tombch deleted the validate-file-groups-edited-data branch July 17, 2026 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend related to the loculus backend component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/submit-edited-data endpoint does not validate file group ownership

2 participants