Repository navigation
score client name change - #905
Merged
Merged
Conversation
leoraba
added a commit
to Pan-Canadian-Genome-Library/file-manager
that referenced
this pull request
Jun 15, 2026
* Disable CSRF * score client name change (overture-stack#905) * score client name change * Update introduction.rst * remove copypasta * re-add bash endline slash --------- Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com> * update migration script to fetch samples structure using batch JOIN query (overture-stack#907) --------- Co-authored-by: Jon Eubank <joneubank@gmail.com> Co-authored-by: Mitchell Shiell <59712867+MitchellShiell@users.noreply.github.com> Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>
joneubank
added a commit
to Pan-Canadian-Genome-Library/file-manager
that referenced
this pull request
Aug 13, 2026
* fix docker publishing tags in Jenkinsfile * version 5.2.0 * version 5.3.0 * Disable CSRF * score client name change (overture-stack#905) * score client name change * Update introduction.rst * remove copypasta * re-add bash endline slash --------- Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com> * update migration script to fetch samples structure using batch JOIN query (overture-stack#907) * Overture docs update (overture-stack#911) * Allow external validations to reference elements of an array (overture-stack#909) * Allow external validations to reference elements of an array * Throw validation exception when jsonPath points to non string or array value * Unit tests for ValidationService.getValuesAtJsonPath * Fix test file compilation * Move validation service unit tests to related dir, use JUnit 4 annotations for consistency * Fix song-server unit tests (overture-stack#912) * Allow external validations to reference elements of an array * Throw validation exception when jsonPath points to non string or array value * Unit tests for ValidationService.getValuesAtJsonPath * Fix test file compilation * Allow external validations to reference elements of an array * Include junit vintage engine dependency to pickup classes annotated with JUnit 4 * Move validation service unit tests to related dir, use JUnit 4 annotations for consistency * Upgrade testcontainers to 1.16 to support Apple Silicon * Fix failing server tests, postgres testcontainer to v14 to support latest migrations * Widen ValidationService.getValuesAtJsonPath to public GetValuesAtJsonPathTest lives in the validation package and calls this method directly, but it was package-private in the service package, which broke compilation of the entire song-server test module. * Fix ConstantsTest.testInfoTypes stale enum count assertion InfoTypes only has 4 constants (STUDY, FILE, SEQUENCING_READ, VARIANT_CALL) but the test asserted 7, with the assertEquals arguments also reversed. * Fix EntityTest.testFile fileType mismatch with FILE_TYPES constant file1 hardcoded fileType("mp4") while the equal-value comparison object and the getter assertion both used FILE_TYPES.get(0) ("mp3"), causing the equality check to fail. * Remove stale samples block from SerializationTest fixture The Payload model no longer has a samples field, and the test's expectedPayload object never sets one, but the expected-serialization fixture still hardcoded a samples array, causing a JSON diff failure. * Fix MockedSubmitTest Mockito matcher misuse validate(isA(JsonNode.class), "anyStudyId") mixed a matcher with a raw String argument, which Mockito rejects. Wrapped the raw values in eq(...), and for the two tests that stub/verify against the studyId actually passed to submit(), used that variable instead of the unrelated "anyStudyId" literal so the verify matches the real invocation. * Fix NonEnforcedSubmitControllerTest fixture staleness and md5 reuse Two issues surfaced by AbstractEnforcedTester (shared by all *SubmitControllerTest classes): 1. documents/variantcall-valid.json still had a stale top-level "samples" block. It's harmless on initial /submit (no schema forbids it there), but AnalysisServiceImpl's update-request schema explicitly disallows "samples", so any update built from data merged out of a submitted analysis (as several tests do) failed schema validation. 2. buildTestEnforcePayload() reused the fixture's hardcoded file md5sums on every call. AnalysisServiceImpl.checkDuplicateAnalysis checks file md5 existence globally (not scoped to study), so calling submit() more than once within a test run (this class has 7 test methods, each submitting) hit "info.already.exists" (409) from the second call onward. Randomized the md5sum per payload build using the test's existing seeded RandomGenerator. No production code changed. * Fix EnforcedSubmitControllerTest studyId/md5 bugs; revert samples removal Investigating EnforcedSubmitControllerTest showed the earlier removal of "samples" from variantcall-valid.json (previous commit) was wrong: samples is a normal, schema-validated part of a variantCall submit payload (testInvalidSample depends on it) - it's only forbidden in *update* requests. Restored it, and instead fixed the real problem: extracted a toUpdatableData() helper on AbstractEnforcedTester that strips "samples"/"files" before merging stored analysis data into an update request, and used it in both Enforced and NonEnforced tests (the underlying bug was the same in both). Also fixed two real bugs specific to EnforcedSubmitControllerTest: - testInvalidFile/testInvalidSample never overrode the fixture's hardcoded "studyId": "ABC123" to match the test's generated study, so submissions failed on studyId mismatch before schema validation had a chance to run. - Extracted randomizeFileChecksums() (same fix pattern as the NonEnforced commit) and applied it everywhere this test class loads a fixture with hardcoded file checksums, since submitting the same checksum twice in one run trips the global duplicate-file check. 11 of 18 tests in this class now pass. The remaining 7 all fail because the current schema/validation logic doesn't enforce constraints these tests expect (fileType/fileAccess/sampleType enums, "matchedNormal" field inclusion rules) - grep shows no schema or validator in the codebase currently implements these rules at all. Flagged in TEST_FIX_LOG.md for review rather than guessed at, since restoring that validation would be a functional change to production code, and this codebase's git history doesn't cheaply reveal whether these tests were ever passing. * Fix CorruptionSubmitControllerTest duplicate-file conflict testNonMutated resubmits the exact same payload (same file checksums) to verify that a non-mutated resubmission succeeds, but AnalysisServiceImpl.checkDuplicateAnalysis now rejects any resubmitted file checksum by default. Added an EndpointTester overload that passes the existing (already supported by SubmitController) allowDuplicates query param, and used it for the second submission. * Fix SubmitServiceTest duplicate-file conflicts and restore rollback Two issues: 1. The class overrode @TestExecutionListeners to only DependencyInjectionTestExecutionListener, which silently disabled TransactionalTestExecutionListener - so the class- and method-level @transactional annotations did nothing, letting each test method's DB writes persist into the next. Removed the override so the default listener set (including transactional rollback) applies. 2. testSaveIdMismatchAllSame/AllDifferent resubmit the same payload content (same file checksums) to verify a new analysisId is issued, and testNullSyncSequencingRead's two fixtures intentionally share the same file checksums - all three need allowDuplicates=true on the resubmission, same as the earlier CorruptionSubmitControllerTest fix. * Remove EnforcedSubmitControllerTest sample-validation tests Samples have been removed from the SONG analysis model entirely - there are no longer validation rules for tumour/normal or matched-tumour-normal relationships. Removed the 7 tests asserting that kind of schema violation (testInvalidSample and the matchedNormalFieldInclusionValidation_* suite), their now-unused runMatchedNormalTest helper, and the now-orphaned fixture files. Stripped the stale "samples" block out of variantcall-valid.json. * Fix Responses.contextMessage dropping %s substitutions It escaped every "%" in the message template to "%%" before calling String.format(..., args), which turns "%s" into "%%s" - "%%" is consumed as a literal "%" and the trailing "s" becomes inert text, so the format argument is silently dropped and the literal "%s" (or "%d", etc.) shows up in the final error message instead of the real value. Substitute args into the template first, then wrap the result in "[context] - ..." - no escaping needed since String.format doesn't re-interpret an already-substituted argument value. * Fix JWTSecurityTest: mock JWKS endpoint, fix provider and expiry The "secure" profile points jwk-set-uri at a real Keycloak instance, so every JWT-decoding test failed with Connection refused. Start a WireMock server before context refresh (via @DynamicPropertySource) and point jwk-set-uri at it; once the KeyPair bean is available, stub its response with a JWKS document built from that key pair's public half, so Spring Security can verify tokens signed by JWTGenerator. Two more issues surfaced once JWKS decoding worked: - auth.server.provider was "keycloak" (via the "secure" profile), which routes authorization through KeycloakAuthorizationService's UMA/RPT grant-fetch flow (another unreachable real endpoint) - but JWTGenerator embeds scopes directly in the JWT claims (the "ego" model). Overrode auth.server.provider=ego for this test. - JWTGenerator's "expired" tokens expired only 10s in the past, inside the JWT decoder's default 60s clock-skew tolerance, so they were still accepted. Pushed the expiry further out (5 minutes). Expired tokens are correctly rejected now, but with 401 Unauthorized (Spring Security's standard behavior for an invalid/expired token), not 403 Forbidden - there's no custom AuthenticationEntryPoint overriding this. Updated the 4 expired-token tests to expect 401 and renamed them from *_Forbidden to *_Unauthorized to match. * Remove stale samples fixtures from AnalysisTypeControllerTest/ExportServiceTest Samples have been removed from the SONG analysis model. Rewrote rendered-schema.json to match what AnalysisTypeService actually produces (no donor/sample/specimen definitions or "samples" property - confirmed against analysisBase.json, the real rendering template). Stripped the "samples" block from all 8 export test fixtures (variantcall-input/output 1-4), since Sample/Donor/Specimen no longer get the info-object field routing FileEntity has, so the expected-output fixtures' assumption about that never held once samples support was dropped. * Fix idSearch: remove dangling SQL clause and donor/specimen/sample join Two related bugs in the idSearch feature, both stemming from donor/specimen/sample no longer being part of the analysis model: 1. IdView's idSearch @NamedNativeQuery ended with a dangling "AND submitter_sample_id" (no operator/value), which made Postgres evaluate the text column as a boolean and throw on every call. IdSearchRequest never had a matching parameter for it - donor, specimen, and sample IDs are no longer valid search criteria, so removed the clause entirely rather than wiring up a replacement parameter. 2. The underlying idview database view still inner-joined donor -> specimen -> sample -> sampleset -> file -> analysis. Since submitted analyses no longer create donor/specimen/sample/ sampleset rows, that join chain could never match a current analysis - idSearch would silently return nothing even once the query stopped throwing. Added a migration redefining idview to join file -> analysis -> analysis_schema directly, and updated the IdView entity to match (dropped the now-nonexistent donorId/ specimenId/sampleId columns). * Fix ValidationService validating against the unrendered schema fragment ValidationService.validate() called analysisTypeService.getAnalysisType(analysisTypeId, true) - the `true` is `unrenderedOnly`, meaning every submission was schema-validated against the raw, unrendered custom schema fragment (e.g. just {"required":["experiment"], "properties":{"experiment":{...}}} for variantCall), never merged with analysisBase.json. That base envelope is where "analysisId":{"not":{}}, studyId, and the file fileMd5sum/fileAccess constraints all live, so none of that was ever actually enforced at submission time - only the "experiment" object was checked. Changed to getAnalysisType(analysisTypeId, false) so submissions validate against the fully-rendered schema. This alone fixes SubmitServiceTest's analysisId-not-rejected test and ValidationServiceTest's fileMd5 gap. Also fixed EnforcedSubmitControllerTest.testInvalidFile: it targets the pre-seeded legacy "variantCall" analysisType (from the historic V1_2__dynamic_schema_integration migration), which has no fileTypes option set, so the fileType allow-list check (ValidationService.validate lines 111-114, a separate mechanism from the JSON schema) never ran. Registered a new version of it with fileTypes restricted to what the fixture uses, and pointed all three submissions in the test at that version (schemas.enforceLatest=true requires it). * Remove dead spring-security-oauth-based auth classes and their tests AccessTokenConverterWithExpiry/ExpiringOauth2Authentication build an old org.springframework.security.oauth2.provider.OAuth2Authentication, but Scopes.extractGrantedScopes() (used by StudySecurity/ SystemSecurity) only recognizes the current JwtAuthenticationToken/BearerTokenAuthentication types - so TestStudySecurity/TestSystemSecurity, which build their Authentication via the old converter, always got an empty scope set regardless of input, failing every "should succeed" case. Confirmed these two classes have no other production usage (no bean wiring, no other callers), and that StudySecurity/SystemSecurity. authorize() are already covered end-to-end through the real JwtAuthenticationToken path by JWTSecurityTest, via the same @PreAuthorize("@studySecurity.authorize(...)")/ @PreAuthorize("@systemSecurity.authorize(...)") endpoints these tests were exercising through the dead code path. Removed both production classes and both test files rather than fix tests for a path nothing else uses. * Restore ValidationService.getValuesAtJsonPath validation exceptions Commit ac30d45 originally made this throw a ValidationException when a jsonPath resolves to a non-string scalar or an array containing non-string elements. b783eec reverted that to silently filtering non- strings out, which was the wrong fix - per direction, this should throw. Also restructured per feedback: the try/catch now wraps only JsonPath.read(...) (genuine JSON-path parsing failures - the only thing it was ever meant to guard), logging a debug message and returning an empty list for those. The type-checking logic that throws ValidationException now happens outside that try/catch, so the exception propagates directly instead of being caught-and-rethrown by a redundant `catch (ValidationException) { throw ... }` block, and the final "else" branch just throws instead of the dead throw-then-implicit-return shape. * Fix SchemaValidationTests stale violation counts, remove unused samples Samples have been removed from the SONG analysis model, so the 4 count-based assertions here were counting violations that no longer exist (missing submitterSampleId, invalid sampleType/gender/ tumourNormalDesignation/specimenType enum values). Verified each fixture's actual violations directly (logged via the existing log.error in the validate() helper) and updated the expected counts to match, documenting the exact expected violation messages in a comment above each assertion so this doesn't silently drift again. Also stripped the now-inert "samples" blocks out of the 5 fixture files these tests (and the shared sequencingread-valid.json, used by SubmitServiceTest/PayloadGenerator too) load, since they no longer contribute to validation either way. * Fix AnalysisServiceTest: wire options field through get_analysis_schema read path The options column was added to analysis_schema in V1_21 but the get_analysis_schema DB function backing the paginated getAnalysis() read path was never updated to select it, leaving AnalysisSchema.options null for analyses loaded through that path. Also fixes the earlier FileEntity.analysisId omission in buildAnalysis(), surfaced by the same test. * Fix AnalysisTypeControllerTest: return normalized options from commitAnalysisType commitAnalysisType() built newAnalysisOptions (falling back to the previous version's options, then to empty lists) and persisted that, but returned the raw, un-normalized options parameter in the response AnalysisType. This made the register response diverge from what a subsequent GET returns for the same analysisType. * Fix AnalysisTypeControllerTest.register_illegalFields_schemaViolation: drop stale samples check samples was removed from the analysis model (V1_22 migration) and the registration meta-schema no longer marks it as a reserved/illegal property name, so registering a schema with a "samples" property now succeeds as expected. Removed it from the list of fields this test checks are rejected. * Fix AnalysisTypeControllerTest experiment-violation message tests register_missingExperiment_schemaViolation and register_misspeltExperiment_schemaViolation asserted stale expected violation messages including "experiment"-related checks that commit 454c2b0 intentionally removed from the analysisTypeRegistration meta-schema. Updated both to the current 2-violation message, which now matches the already-passing register_extraFields_schemaViolation. * Fix AnalysisTypeControllerTest.register_emptySchema_schemaViolation: remove redundant @nonnull AnalysisTypeService.register()'s @nonnull on analysisTypeSchema made Lombok throw a raw NullPointerException before validateAnalysisTypeSchema() got a chance to run its own intended null check, which already produces a handled SCHEMA_VIOLATION server error. Removed the redundant annotation so the existing guard is reachable. * Upgrade song-server from JUnit 4 to 5 * Clarification on behaviour for external validation of non-string properties * Remove duplicate dependency declaration * Update Jenkinsfile Switch pipeline library branch * Update testcontainers to be compatible with docker 29 * Restore jenkins to master branch pipeline --------- Co-authored-by: Yelizar Alturmessov <yalturmessov@oicr.on.ca> * Versioning 5.4.0-SNAPSHOT * Versioning 5.4.0 * Activate all AuthZ classes only with the pcglauthz profile * Disable the JWTSecurityTest that are unused in PCGL * Update README for PCGL --------- Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com> Co-authored-by: Leonardo Rivera <leorivera_88@hotmail.com> Co-authored-by: Mitchell Shiell <59712867+MitchellShiell@users.noreply.github.com> Co-authored-by: Yelizar Alturmessov <yalturmessov@oicr.on.ca>
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.
score client name change