Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DO NOT MERGE: Craig's draft for v1.2 - still has work that should be brought across into 1.2 #47

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cdvoisin
Copy link
Collaborator

Introduce Self-Contained Passports

Introduce Self-Contained Passports
Copy link
Collaborator

@kwrodarmer kwrodarmer left a comment

Choose a reason for hiding this comment

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

This is a great move in the right direction. There are probably some things I will contribute afterward, but let's take this as the new base.

Copy link
Collaborator

@davidbernick davidbernick left a comment

Choose a reason for hiding this comment

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

This is exactly what we've been discussing -- no objections from me.

@@ -259,18 +301,50 @@ the Broker.

3. Access tokens MAY contain non-GA4GH Claims directly in the access token.

3. A Broker MAY issue [Self-Contained Passports](#term-self-contained-passport).

1. Self-Contained Passports MAY be large and are only for use via POST within
Copy link
Contributor

Choose a reason for hiding this comment

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

Items 1-3 are not requirements for brokers, restructure?

GA4GH-compatible service endpoints.

2. Self-Contained Passports MUST NOT be used with OAuth2 endpoints that require
an `Authorization` header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not even if something else than the self-contained passport (for instance, a regular OAuth2 access token) is placed to the Authorization header?


{
"ga4gh_passport:self_contained_v1": "<self-contained-passport-jws>",
"targets": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of allowing several targets in one request vs. sending several requests each with one target?

@mikael-linden
Copy link
Contributor

In ver 1.2, do we want to introduce a mechanism (potentially based on RFC8707) to request a downscoped passport from a broker so no root passport becomes exposed to a client?

@cdvoisin
Copy link
Collaborator Author

cdvoisin commented Aug 9, 2021 via email

@mikael-linden
Copy link
Contributor

How is the in the Passport Endpoint Response different from the regular ga4gh_passport_v1 claim received from /userinfo?

Where:

- `tokens`: REQUIRED as the map of tokens per `aud` in the request structure
and each `<self-contained-passport-jws>` is a JWS Compact String of a Self-Contained
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would need to specify the claims within the and how it is similar or different than what is returned from the /userinfo endpoint. For example, it has the visas but doesn't have email, picture, given name, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the Passport endpoint response is 1:1 with the ga4gh_passport_v1 claim from /userinfo, why do we need this new endpoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. It is designed to remove the need for bearer tokens to call back to /userinfo by issuing embedded tokens. More on requirements in the design doc.
  2. It is designed to be the mechanism to be leveraged to support visa downscoping, not yet shown in this PR. However there are other endpoints and mechanisms being explored in the design doc.
  3. It may offer a different set of claims than /userinfo to not expose as much PII. Once again, there may be other ways to control this using scopes.

Therefore, it is not clear if this PR currently has the right approach, and so this PR so far is for discussion on what such a self-contained passport minting endpoint might look like by exposing more details of how it translates into spec, but these design issues need to be resolved before creating or updating a PR like this to be ready to land.

@dvoet
Copy link

dvoet commented Sep 10, 2021

Are there going to be updates to the embedded access token (i.e. visa) polling section? I believe the RAS implementation has departed a bit from the spec in this respect by implementing a separate endpoint for visa (or even passport) validation and am wondering if that will be the approach in 1.2 as well.

@davidbernick
Copy link
Collaborator

davidbernick commented Sep 10, 2021 via email

@andrewpatto andrewpatto marked this pull request as draft October 7, 2021 21:56
@andrewpatto andrewpatto changed the title Initial draft for v1.2 DO NOT MERGE: Craig's draft for v1.2 - still has work that should be brought across into 1.2 Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants