[DR-3324] Only fetch DRS object via passport if one exists

[okotsopoulos]

https://broadworkbench.atlassian.net/browse/DR-3324

DrsHub asks TDR Production for the supported authorization mechanisms for a controlled access GTEx file:

curl -X 'OPTIONS' \
  'https://data.terra.bio/ga4gh/drs/v1/objects/v2_e559f418-7452-31c2-94fe-ed412157363d' \
  -H 'accept: application/json'

{
  "supported_types": [
    "PassportAuth",
    "BearerAuth"
  ],
  "passport_auth_issuers": [
    "https://stsstg.nih.gov/"
  ],
  "bearer_auth_issuers": null
}

Seeing that PassportAuth is a supported type (dictated by the presence of both a PHS ID and consent code on the source snapshot), it tries to obtain a passport to use for fetching the DRS object.

Previous behavior

In cases where DRS OPTIONS indicate PassportAuth as a supported means of authorization, if a user Passport is not available, then DrsHub passed an empty string as a Passport when attempting object resolution.

But the empty string is not a valid Passport: when DrsHub passed it to TDR, TDR passed it to the ECM client for decoding. ECM then throws an unhandled error:

{
  "message" : "500 Internal Server Error: \"{\"msg\":\"400 Bad Request: \\\"{<EOL>  \\\"message\\\" : \\\"invalid passport jwt\\\",<EOL>  \\\"statusCode\\\" : 400<EOL>}\\\"\",\"status_code\":500}\"",
  "statusCode" : 500
}

Furthermore, DrsHub did not fall back to fetching the object via bearer token.

New behavior

In cases where DRS OPTIONS indicate PassportAuth as a supported means of authorization, DrsHub only attempts to fetch the object via POSTed passport if such a passport exists.

If any part of this fails, DrsHub now falls back to fetching the object via bearer token.

Tests

I added targeted unit tests covering DRS object fetching, but there were no previous unit tests for DrsResolutionService: I believe this code remains largely uncovered by automated tests, but am also still finding my way around this repo.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ecd54b9) 84.11% compared to head (9a288a8) 84.56%.

Additional details and impacted files

@@             Coverage Diff              @@
##                dev      #80      +/-   ##
============================================
+ Coverage     84.11%   84.56%   +0.44%     
- Complexity      228      231       +3     
============================================
  Files            36       36              
  Lines           806      810       +4     
  Branches         75       76       +1     
============================================
+ Hits            678      685       +7     
+ Misses           96       94       -2     
+ Partials         32       31       -1     

Files	Coverage Δ
...io/terra/drshub/services/DrsResolutionService.java	86.76% <100.00%> (+2.67%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[nmalfroy]

This looks great! And thanks for the general doc cleanup too!

[nmalfroy]

Too bad...I wish that the code gen would make postObject take a typed object. I'mm actually kinda surprised that it doesn't

[okotsopoulos]

I agree with you… this seems like a similar issue to what we encountered with calling the TPS client in TDR. I was surprised that it didn't enforce the presence of a required argument.

[nmalfroy]

I've been poking a little...It's because (I think) the code gen only creates models for objects in the components.schemas part of the openapi yaml and these are in components.requestBodies. Trying to see if there's a way to have it generate models for those too

[pshapiro4broad]

IMO this is exactly the reason why an API that returns a collection shouldn't use Optional. The calling code needs to check for both the optional being present and the collection being non-empty. Returning an empty collection instead would make this code simpler. But it looks like Optional is threaded through many places here so it wouldn't be easy to change this.