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

Passing arbitrary parameters to the ID assertion endpoint #2

Closed
cbiesinger opened this issue Apr 11, 2024 · 66 comments
Closed

Passing arbitrary parameters to the ID assertion endpoint #2

cbiesinger opened this issue Apr 11, 2024 · 66 comments

Comments

@cbiesinger
Copy link

cbiesinger commented Apr 11, 2024

(this has been split out of w3c-fedid/FedCM#477 )

Also, there is currently no general-purpose way to pass information from the RP to the ID assertion endpoint. It is already possible to pass arbitrary data through the “nonce” or the “client_id” field; however, this is inconvenient and unstructured. An explicit way to pass key/value pairs would be better.

@cbiesinger
Copy link
Author

cbiesinger commented Apr 11, 2024

Proposal

We are proposing letting the RP specify additional fields that will get sent to the IdP:

partial dictionary IdentityProviderConfig {
  // A map of key-value pairs that are opaque to the browser and only
  // passed to the IdP after the user's acknowledgement.
  record<USVString, USVString> params;
};

An RP could use them like this:

let {token} = await navigator.credentials.get({
  identity: {
    providers: [{
      clientId: "1234",
      nonce: "234234",
      loginHint: "previous@user.com",
      configURL: "https://idp.example/fedcm.json",
      // A string with parameters that need to be passed from the
      // RP to the IdP but that don't really play any role with
      // the browser.
      params: {
        "IDP_SPECIFIC_PARAM": "1",
        "foo": "BAR",
        "ETC": "MOAR",
        "response_type": "id_token",
        "scope": "photos:read photos:write",
      }
    },
  }
  // If possible, return without prompting the user, if not possible
  // prompt the user.
  mediation: "optional",
});

These parameters will be sent to the ID assertion endpoint (that is, they will only be shared with the IdP after user interaction) with a param_ prefix.

For example, the ID assertion request body might look like this:

POST /fedcm_assertion_endpoint HTTP/1.1
Host: idp.example
Origin: https://rp.example/
Content-Type: application/x-www-form-urlencoded
Cookie: 0x23223
Sec-Fetch-Dest: webidentity

account_id=123&client_id=client1234&nonce=234234&disclosure_text_shown=false&param_reponse_type=id_token&param_IDP_SPECIFIC_PARAM=1&param_foo=BAR&param_ETC=MOAR

Considerations

Parameters are prefixed with param_ in the request body so that they can not conflict with built-in keys like account_id and disclosure_text_shown.

This proposal works best in conjunction with the continuation API proposal described in #1 .

@cbiesinger cbiesinger changed the title Passing custom data to the ID assertion endpoint (scopes and others) Passing custom data to the ID assertion endpoint Apr 15, 2024
@cbiesinger
Copy link
Author

I have split out the scope parameter to #4 because that seems more controversial.

@cbiesinger
Copy link
Author

The discussion in today's fedid CG meeting was in favor of adding params, but was skeptical about responseType. I think it's best to remove that field from this proposal.

@samuelgoto
Copy link
Collaborator

I think it's best to remove that field from this proposal.

SGTM

@panva
Copy link

panva commented Apr 26, 2024

Some OAuth 2.0 registered extension parameters, e.g. resource can be passed to the authorization endpoint multiple times.

To that end a definition like so

  record<USVString, USVString> params;

cannot be used to achieve a application/x-www-form-urlencoded encoded body which an OAuth 2.0 authorization server understands

resource=urn:example:1&resource=urn:example:2

@panva
Copy link

panva commented Apr 26, 2024

partial dictionary IdentityProviderConfig {
  sequence<USVString> responseType;
  record<USVString, USVString> params;
};

sequence<USVString> responseType; is redundant if I can pass any parameter, to that end so would the existing nonce and the proposed https://github.com/fedidcg/FedCM/issues/559 Scope API.

@samuelgoto
Copy link
Collaborator

sequence responseType; is redundant if I can pass any parameter, to that end so would the existing nonce and the proposed #4 Scope API.

Yep. @cbiesinger is removing responseType from this proposal. See above:

https://github.com/fedidcg/FedCM/issues/556#issuecomment-2072677672

@cbiesinger
Copy link
Author

The scope API is partially redundant, but it also affects the browser UI (which is why it is a separate proposal).

You are correct about nonce, but I am not sure we will be able to remove it for backwards compatibility reasons.

@cbiesinger
Copy link
Author

@panva thanks for the information. however I am not sure that exact compatibility with existing endpoints is a goal

@panva
Copy link

panva commented Apr 29, 2024

@panva thanks for the information. however I am not sure that exact compatibility with existing endpoints is a goal

I am sure that was not my point. I simply ask to use a type that allows prior art that is deployed in the wild to be used.

@obfuscoder
Copy link

Would it be possible to allow for an array of strings for param values? This could be constructed as a union to values as strings abut also array of strings. This would allow for the multiple resource example given by @panva

@samuelgoto
Copy link
Collaborator

Would it be possible to allow for an array of strings for param values?

I think the proposal is to accept an object, which is a set of key-value pairs (where the keys and values are strings), so I think the answer is "yes".

@cbiesinger
Copy link
Author

As of tomorrow's canary, responseType will no longer be supported in Chrome

Regarding arrays of strings, it is certainly doable if we decide to support it.

@domenic
Copy link

domenic commented May 10, 2024

To be a bit clearer on the usual API shape for accepting these sorts of things: it's

sequence<sequence<USVString>> or record<USVString, USVString>

with extra processing on the first variant. See https://fetch.spec.whatwg.org/#concept-headers-fill for the processing algorithm.

This allows the formats

{
  "key": "value",
  "key2": "value2"
}

as well as

[
  ["key", "value"]
  ["key", "value2"]
]

but notably does not allow

{
  "key": ["value1", "value2"]
}

or

[
  ["key", ["value1", "value2"]]
]

@cbiesinger
Copy link
Author

@domenic I'm curious why not record<USVString, (USVString or sequence<USVString>)>?

FilePickerAcceptType seems to use that.

@cbiesinger
Copy link
Author

cbiesinger commented May 10, 2024

but I see that the URLSearchParams constructor uses your suggestion

@bvandersloot-mozilla
Copy link

Is there a reason to maintain the nonce when this is added?

@aaronpk
Copy link

aaronpk commented May 21, 2024

No I don't think so. I mentioned in another issue (can't find it at the moment) that unless FedCM defines some specific processing behavior to use/validate the nonce parameter, it shouldn't be defined as a parameter in FedCM.

@samuelgoto
Copy link
Collaborator

samuelgoto commented May 28, 2024

Managing backwards incompatible changes is part of the challenge of shipping an experimental API that doesn't have the implementation experience, standard set, or another browser shipping

Yeah, I think we are roughly on the same page: it is a risk that we are happy that we took, and are happy to manage it now.

Part of managing that risk is acknowledging that they are often going to be necessary and but also that they are not free, so being smart and deliberate about the choices.

LMK if you disagree with anything in this response here: https://github.com/fedidcg/FedCM/issues/556#issuecomment-2135670331

@panva
Copy link

panva commented May 28, 2024

The point that I'm trying to make is that the id_assertion_endpoint is a FedCM one, not an OAuth one: it is not a design goal to make it backwards compatible to OAuth without any modification.

Yes, I do believe the circumstance that there stands to be a chance of a clean oauth profile for fedcm and other existing prior idp art is entirely accidental.

I think it is unsafe to suggest so

I agree it's unsafe, but also noone suggested it. An oauth binding for fedcm would't dare omitting the mandatory idp provisions set forth by fedcm regardless of whether its parameters are prefixed or not.

one has to process a FedCM request intentionally, rather than by accident.

100% agree. But one can intentionally (or unintentionally) mess up fedcm's provisions regardless of how custom vs fedcm-specific params are transmitted. Bad implementations are inevitable. They are also not a strong argument against an affordance for respecting years of prior idp art.

@samuelgoto
Copy link
Collaborator

They are also not a strong argument against an affordance for respecting years of prior idp art.

I think we are starting to talk past each other now: I'm not suggesting that we ignore prior art, I'm suggesting that this is an entirely new layer that's being introduced, and that the parameters are specific to that layer. I already made this point, so don't think it is a matter of clarification.

@timcappalli
Copy link

Proposal on the 2024-06-11 call: what if this was POST w/ JSON payload vs parameters? Then the params_ prefix could be dropped from the key names.

@samuelgoto
Copy link
Collaborator

Proposal on the 2024-06-11 call: what if this was POST w/ JSON payload vs parameters? Then the params_ prefix could be dropped from the key names.

Another observation that was discussed with @aaronpk and @bc-pi in the CG call was that the feedback from IdPs that they would be really worried about opening up their oauth endpoints to start taking traffic from the FedCM flows, because that could lead to unintended consequences to a very security sensitive piece of infrastructure. For example, FedCM requires the id_assertion_endpoint to take certain CORS parameters and check for certain Sec-Fetch-Dest parameters, which is not well understood whether it could lead to security risks to existing oauth endpoints. So, so far, we got the feedback that we are better off keeping the id_assertion_endpoint deliberately isolated from the oauth authorization endpoints, so that we can reason about them in isolation.

@aaronpk
Copy link

aaronpk commented Jul 16, 2024

Clarifying question for FedCM implementations and intent going forward... are the arbitrary params values only ever strings or could they be other JSON data types including complex objects? Does the browser care? e.g.

let {token} = await navigator.credentials.get({
  identity: {
    providers: [{
      clientId: "1234",
      nonce: "234234",
      loginHint: "previous@user.com",
      configURL: "https://idp.example/fedcm.json",
      params: {
        "response_type": "id_token",
        "scope": "photos:read photos:write",
        "foo": true,
        "authorization_details": [
           {
             "type": "payment_initiation", 
             "amount": {"currency": "EUR", "amount": 123.5} 
           }
        ]
      }
    },
  }
});

@cbiesinger
Copy link
Author

The values are currently defined as strings in the IDL, so if you try to pass an object, you will get standard JS behavior to convert to string, which will likely result in "[Object object]" or somesuch. (you can of course call JSON.stringify yourself)

Proposal on the 2024-06-11 call: what if this was POST w/ JSON payload vs parameters? Then the params_ prefix could be dropped from the key names.

As a note, because of how CORS works (https://fetch.spec.whatwg.org/#cors-safelisted-request-header) this would make the ID assertion fetch require a CORS preflight request. This isn't necessarily a problem but I wanted to point that out.

@bvandersloot-mozilla
Copy link

It seems reasonable to support arbitrary structure. That is what the discussion in w3c-fedid/idp-registration#13 led to. But things get weird when you start having numbers that parse out of JSON imprecisely, like JSON.parse(12345678901234567890) == 12345678901234567000.

@cbiesinger
Copy link
Author

Hmm if we start supporting arbitrary JSON objects and (presumably) serialize that into a string to send as a parameter value -- maybe we should go a step further and just send params=${JSON.stringify(identity.providers[N].params)?

@samuelgoto
Copy link
Collaborator

samuelgoto commented Jul 23, 2024

params=${JSON.stringify(identity.providers[N].params)

Yeah, that occurred to me, and it seems right. That's what we are going to do for the digital credentials API.

It also addresses the question of what prefix to use for params: none at all.

@cbiesinger
Copy link
Author

Adding agenda+ to discuss the proposal in the last two comments & to hear if there was an outcome at the IETF meeting

@elf-pavlik
Copy link

I understand that the discussion is about https://www.w3.org/TR/fedcm/#idp-api-id-assertion-endpoint

Which currently states:

It will also contain the following parameters in the request body application/x-www-form-urlencoded

Reading the proposal

Hmm if we start supporting arbitrary JSON objects and (presumably) serialize that into a string to send as a parameter value -- maybe we should go a step further and just send params=${JSON.stringify(identity.providers[N].params)?

I don't understand why the content type of the request body can't be changed to application/json

I'm sorry if I missed something obvious.

@cbiesinger
Copy link
Author

It is possible, although there are two/three downsides to that proposal:

  • Sending the body as JSON means we trigger a CORS preflight (this isn't necessarily a problem, but makes things a bit more complicated and adds latency)
  • There's a few IDPs who already adopted FedCM which would need to update their code
  • JSON post bodies are relatively unusual

But yeah we could go that route. The easiest way to do that might be to require IDPs to opt in to that when they are ready and eventually remove support for urlencoded.

@cbiesinger
Copy link
Author

We discussed this at today's FedID CG call. The outcome was that everyone is happier now that we no longer prefix parameters and support arbitrary values (not just strings). There was no particular preference for application/json vs urlencoded as far as I can tell.

@npm1
Copy link

npm1 commented Sep 3, 2024

I suppose if there are no benefits to using application/json then we can keep the current as it a) avoids backwards incompatible change and b) avoids CORS preflights (which would need to be supported by IDPs, although if needed I don't think it would be a big deal)

@samuelgoto
Copy link
Collaborator

I don't understand why the content type of the request body can't be changed to application/json

I think this has its own merits and challenges on its own, outside of the discussion of how this issue. I kicked off w3c-fedid/FedCM#644 to decouple the discussion, so that we can make forward progress on this issue independently, and move to text/json as a separate step.

@samuelgoto samuelgoto transferred this issue from w3c-fedid/FedCM Sep 10, 2024
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 18, 2024
This implements the consensus from the FedID CG described here:
w3c-fedid/custom-requests#2 (comment)

For easier migration for IDPs, this still sends the previous
param_ prefixed parameters as well for now.

Bug: 40262526
Change-Id: I2fe16a2776cecc76b58f339c1a33221f1781aaac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5870964
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1357291}
cbiesinger added a commit to cbiesinger/WebID that referenced this issue Oct 2, 2024
samuelgoto pushed a commit to w3c-fedid/FedCM that referenced this issue Oct 8, 2024
* Specify the params API

Bug: w3c-fedid/custom-requests#2

* Update spec/index.bs

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

* any

---------

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@cbiesinger
Copy link
Author

Closing this now that the spec PR is merged.

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