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

The name "token" seem to imply that only ID Tokens can be passed back to RPs #579

Closed
samuelgoto opened this issue May 10, 2024 · 13 comments
Closed

Comments

@samuelgoto
Copy link
Collaborator

samuelgoto commented May 10, 2024

It was raised numerous times (example) that our choice of words of token leads a reader/developer to assume that only ID Tokens can be passed back to the RP, when, in fact, it was designed to allow any arbitrary data to be passed back: it is a USVString, so it can be anything (e.g. a SAML XML response, a JSON.stringify(data), an access token, an mp3 file, a JPEG, whatever can be binary encoded into a string).

A few options that we have on this:

  1. rename token (backwards incompatible, so a bit hard)
  2. ignore the problem and solve with documentation
  3. introduce a more meaningful purpose-specific fields, e.g. code (less extensible, but more interoperable?)
  4. introduce a general purpose field, e.g. data or response (more extensible, but less interoperable?)
@timcappalli
Copy link
Member

I think we could do a type field to define the semantics of the token value. Could be as simple as tokenType with an enum.

@samuelgoto
Copy link
Collaborator Author

I think we could do a type field to define the semantics of the token value. Could be as simple as tokenType with an enum.

Yeah, that's another way that this could go, but that wouldn't address the confusion created by the name "token", would it?

@timcappalli
Copy link
Member

Ah right. This is in the context of other artifacts like code. Got it.

@panva
Copy link

panva commented May 10, 2024

rename token (backwards incompatible, so a bit hard)

That should not be a big concern given how little this spec adopted by the audience you're reaching out to.

introduce a more meaningful purpose-specific fields, e.g. code (less extensible, but more interoperable?)

-1, the last thing I'd want is to have to wait for browsers to adopt, support, and rollout additional fields to RPs when we define a new response shapes

introduce a general purpose field, e.g. data or response (more extensible, but less interoperable?)

+1 I thought that's what token is meant to be in the first place, in which case response as a name would be appropriate. Although we already have a response mode in OAuth which has a response field so then the RP would be getting the response.response in that one particular case ;)

@aaronpk
Copy link

aaronpk commented May 11, 2024

See my comment on this giant thread here where I used this field to return an OAuth authorization code which is exchanged for actual info from the IdP server-side.

@npm1
Copy link
Collaborator

npm1 commented May 13, 2024

FedCM is used in a large number of Chrome page loads, so even though the spec change would be ok to be backwards incompatible, from our perspective we want to avoid these changes if at all possible. That said, we could support passing 'token' OR 'response' if that helps with the confusion? If a developer passes both, we'd throw.

@samuelgoto
Copy link
Collaborator Author

That should not be a big concern given how little this spec adopted by the audience you're reaching out to.

It is still possible to make backwards compatible changes, but we don't take them lightly.

FedCM is used in a large number of Chrome page loads

To be concrete, 0.8% of page loads in Chrome:

https://twitter.com/RickByers/status/1781376930912333880
https://chromestatus.com/metrics/feature/timeline/popularity/4166

@anderspitman
Copy link

This is actually a very surprising number of page loads. Where is it being used?

@samuelgoto
Copy link
Collaborator Author

This is actually a very surprising number of page loads. Where is it being used?

Every site on the web that uses these variations of Sign in with Google:

https://developers.googleblog.com/en/federated-credential-management-fedcm-migration-for-google-identity-services/

@samuelgoto
Copy link
Collaborator Author

I think that this is maybe a duplicate of #578, so closing. Feel free to reopen if you disagree and there is something else that needs to be captured that isn't captured in #578.

@panva
Copy link

panva commented May 28, 2024

I think that this is maybe a duplicate of #578, so closing. Feel free to reopen if you disagree and there is something else that needs to be captured that isn't captured in #578.

I believe that entirely depends on what shape #578 is going to take. If it's going to define a new property (e.g. response, or data) that can be either any JSON primitive (string, object, etc) or whether it's going to allow such for the existing token property?

@samuelgoto
Copy link
Collaborator Author

I believe that entirely depends on what shape #578 is going to take. If it's going to define a new property (e.g. response, or data) that can be either any JSON primitive (string, object, etc) or whether it's going to allow such for the existing token property?

Ah, fair. Let me reopen this, and we can revisit as either moves along.

@samuelgoto samuelgoto reopened this May 28, 2024
@samuelgoto
Copy link
Collaborator Author

Ah, fair. Let me reopen this, and we can revisit as either moves along.

Ok, I moved this a bit further in a way that I believe still makes this issue obsolete, by making the following choice:

or whether it's going to allow such for the existing token property?

I made a concrete proposal in the following issue and will close this one here as as obsolete by the other. Will reopen this if my proposal in the other issue falls through.

#578 (comment)

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

No branches or pull requests

6 participants