ga4gh / data-repository-service-schemas

A repository for the schemas used for the Data Repository Service.
Apache License 2.0
60 stars 53 forks source link

Feature/issue 348 post passport v2 #363

Closed briandoconnor closed 3 years ago

briandoconnor commented 3 years ago

DEADLINE: please comment on the PR #363 before the next Cloud call on Sept 13th. We will merge this PR then if there are no blocker comments.

See Issue #348 for the background on this PR

Note, I'm closing PR #349 and replacing it with this one to move to OpenAPI 3.

Some questions for the group:

  1. Is the PassportAuth SecuritySchemes done correctly? See data_repository_service.openapi.yaml and this documentation from OpenAPI: https://swagger.io/docs/specification/authentication/
  2. Is there a way to not have duplication information between the GET and POST versions of the same endpoint? I tried multiple approaches to this and none worked. I need someone better at OpenAPI 3 to help with this.
BinamB commented 3 years ago

I am not completely convinced that DRS should accept multiple passports. This deviates from the original idea of "One token to rule them all". I can see how this might be useful when someone might not want to repackage their passports but I think accepting multiple passports defeats the purpose of a passport. This wouldn't be any better than doing traditional OIDC with extra steps (i.e. just sending in a list of access tokens from different systems to a DRS server). The purpose of a passport is to "transport a researcher's digital identity and permissions across organizations, tools, and environments". A single passport is the best solution, it doesn't work if we're keeping track of multiple passports.

As an implementer, this is also concerning since this does not look like it would scale very well. I think there are already scalability concerns that we're trying to solve with a single passport. Passports do tend to get very large and sending multiple large passports to a DRS server might result in a poor user experience. I think scalability should be taken into consideration so that we don't have to come back to square one after there have been a couple of implementations.

briandoconnor commented 3 years ago

@BinamB, I agree with your general points. We talked during the GA4GH Cloud Meeting (can't remember the date) on your feedback above. Just putting some notes in here to record the info on the PR:

briandoconnor commented 3 years ago

DEADLINE: please comment on this PR before the next Cloud call on Sept 13th. We will merge this PR then if there are no blocker comments.

jb-adams commented 3 years ago

rebased this branch onto develop, bringing in the recently closed Service Info PR

BinamB commented 3 years ago

@kwrodarmer Im assuming this would look something like a key:value pair where the key is the issuer and value is the token. I see how this increases performance but I don't see how we can implement it in the DRS server. Correct me if I'm wrong but this would mean that each DRS object would have to map to an issuer to decide on which token to use?

kwrodarmer commented 3 years ago

Using something like you describe: [ "name1:jwt1", "name2:jwt2" ] would be a way to do it. Also, using a JSON object as { "name1" : "jwt1", "name2" : "jwt2" } would work.

each DRS object would have to map to an issuer to decide on which token to use?

I'm not sure I understand. Are you saying they don't?

kwrodarmer commented 3 years ago

As a follow on, there is an issue around labels on tokens - where do they come from, who assigns them, can they be reassigned, etc. This may prove to complicate matters too much to incorporate them into the current PR and DRS version. We have discussed it but not addressed it within the passport specification, so maybe we should revisit the issue in a wider scope. And @BinamB , maybe that is what you were saying before.

BinamB commented 3 years ago

Yeah, that is kinda what I was trying to say. It also complicates things when the system itself is trying to give access through their own Passports for something like developer access. But I think we're heading in a good direction by using a JSON instead of a list. Maybe we can save this for passports v2 but something I've been thinking about is instead of how we have a list of permissions right now in v1 we could have a JSON/Dictionary of it which would be a lot faster/easier for a DRS server to search through.