w3c-fedid / custom-requests

This is a proposal to extend FedCM to allow RPs to make custom requests to the IdP
3 stars 1 forks source link

Passing arbitrary parameters to the ID assertion endpoint #2

Open cbiesinger opened 7 months ago

cbiesinger commented 7 months ago

(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 commented 7 months ago

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 w3c-fedid/continuation#1 .

cbiesinger commented 7 months ago

I have split out the scope parameter to w3c-fedid/continuation#4 because that seems more controversial.

cbiesinger commented 6 months ago

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 commented 6 months ago

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

SGTM

panva commented 6 months ago

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 commented 6 months ago
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 commented 6 months ago

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

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

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

cbiesinger commented 6 months ago

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 commented 6 months ago

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

panva commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

FilePickerAcceptType seems to use that.

cbiesinger commented 6 months ago

but I see that the URLSearchParams constructor uses your suggestion

bvandersloot-mozilla commented 5 months ago

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

aaronpk commented 5 months ago

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 commented 5 months ago

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

I don't think so. It would be a backwards incompatible change, so we'd have to coordinate with developers, which is doable but non-trivial. My plan is to get params in first (@cbiesinger got an origin trial in M126, I think), ship it in and once it settles start a migration and deprecation of nonce. Possible, but takes a lot of steps.

aaronpk commented 5 months ago

I just noticed the param_ prefix in this proposal. I get the concern about not colliding with the FedCM-defined property names, but it does mean I need another step of preprocessing this request before I can hand off the request to my existing OAuth authorization endpoint.

It also feels a bit weird because client_id is a parameter already used in OAuth, but in order to use any other OAuth parameters I'd have to find them with the param_ prefix in the post body request.

I'm not necessarily asking to change this, just wanted to get this written down while I'm thinking about it.

panva commented 5 months ago

I just noticed the param_ prefix in this proposal.

Well this is a shocker, I toyed with the flagged (authz) predecessor implementation of this and didn't realize either since it wasn't prefixed there.

Edit: @aaronpk We didn't realize because this was just edited in a week ago...

I'm not necessarily asking to change this

I am ;) Up until this there was a remote chance this API could directly be mapped to either the token_endpoint or the POST at authorization_endpoint (with a fedcm-specific response mode) if we chose to do so at our binding. With prefixes there isn't.

I get the concern about not colliding with the FedCM-defined property names

Solveable by setting a parameter precedence for the initial set of fedcm parameters and having future additions to fedcm not use conflicting parameter names with known prior art if they happen to have a different meaning. We have known parameter public registries to support the process.

npm1 commented 5 months ago

Yea I think ignoring any params conflicting with the FedCM specific ones could also work. Do any of these already conflict with some params commonly passed?

aaronpk commented 5 months ago

client_id is OAuth and OpenID Connect, and nonce is OpenID Connect, tho I believe the intent of these parameters is the same in FedCM and OAuth/OIDC so that shouldn't be a problem

panva commented 5 months ago

Yea I think ignoring any params conflicting with the FedCM specific ones could also work. Do any of these already conflict with some params commonly passed?

  • client_id
  • nonce
  • account_id
  • disclosure_text_shown
  • is_auto_selected
  • mode
  • fields
  • disclosure_shown_for

You can see the currently registered parameters in the IANA registry - the ones marked with either authorization request or token request.

client_id and nonce are registered parameters, however, client_id has the same meaning so that's okay?, and nonce should be removed (or at least made optional in the API during its deprecation) and also has the same meaning, although it is not applicable to non-OIDC flows so a removal is more appropriate.

wparad commented 5 months ago

I'd rather have a nested body (even one that had to be string parsed) compared to guessing which property keys have been abused.

panva commented 5 months ago

I'd rather have a nested body (even one that had to be string parsed) compared to guessing which property keys have been abused.

Can you elaborate on "abused"? If the only param re-used by fedcm is client_id and further reuse is avoided by having a transparent development process which will look at prior art to avoid, what would a nested bag of attributes bring us? I believe that having existing oauth deployments need "just" a new response mode is a win.

cbiesinger commented 5 months ago

If we only disallow/ignore the known parameters, this is problematic from a compatibility perspective if we later add a new parameter.

We could disallow the existing parameters and reserve a prefix (like the sec- prefix for HTTP headers) for future parameters we will add.

But can you elaborate on why the param_ prefix is causing problems?

cbiesinger commented 5 months ago

In particular:

Up until this there was a remote chance this API could directly be mapped to either the token_endpoint or the POST at authorization_endpoint (with a fedcm-specific response mode) if we chose to do so at our binding. With prefixes there isn't.

Couldn't that mapping strip the param_ prefix?

aaronpk commented 5 months ago

The param_ prefix isn't a problem for net new implementations, but it is awkward to try to fit that into an existing OAuth authorization endpoint like @panva and I have done. It means more significant/awkward modification of the authorization endpoint to translate param_x to x before continuing with the rest of the existing processing.

samuelgoto commented 5 months ago

I am ;) Up until this there was a remote chance this API could directly be mapped to either the token_endpoint or the POST at authorization_endpoint (with a fedcm-specific response mode) if we chose to do so at our binding. With prefixes there isn't.

The concern that I have with not having the prefixes is that (a) browser engineers are going to have to look into this IANA Registry and at every other identity protocol that exists today and in the future (e.g. SAML) every time we add one and (b) vice versa, this IANA Registry is going to have to look into parameters that the browser has added prior and independently so that it doesn't collide.

It is a two way street that can corner two layers of the stack that should be able to move otherwise independently.

I understand how the translation form param_x to x isn't the most ergonomic things in the world, but I think that, if you look at this holistically, it is the right trade-off.

aaronpk commented 5 months ago

Backwards compatibility issues aside, couldn't it go the other way around? FedCM parameters get a prefix, everything else is sent as is:

cbiesinger commented 5 months ago

but we can't leave backwards compatibility aside

samuelgoto commented 5 months ago

FedCM parameters get a prefix, everything else is sent as is:

We considered this too.

Aside from backwards compatibility, I think our architectural design assumption is that the id_assertion_endpoint is a FedCM endpoint, not an OAuth one. The IdP has to write custom FedCM-specific code to handle it (e.g. check for CORS and Sec-Fetch-Dest): it speaks FedCM as input, and if you start from that assumption, it seems direct that it can make translations easily to its internal implementation.

aaronpk commented 5 months ago

That is how I implemented it in mine, which was to make a new endpoint with the FedCM processing (CORS, Sec-Fetch-Dest, origin check), then call the generateAuthorizationCode function that my OAuth authorization endpoint also calls.

panva commented 5 months ago

"What would an OpenID Connect binding for FedCM profile look like?" was the question posed at the IETF OAuth Interim meeting. Well, we have a shot at a clean binding with large chunk of existing deployments. A binding that actually fits the extension model of the OAuth protocol as well as the overall request flow of FedCM too. But the parameters must not get prefixed in order for that to be doable.

Adding new authorization-like endpoint that issues assertions is a much bigger undertaking than adding a response mode, both implementation and spec-wise.

It is clear to me why you want to distinguish the params as well as that you don't want the burden of having to look at registries. I think you make a way bigger deal out of it than it actually is.

How about having a way to change what gets prefixed - either the fedcm parameters (explicit option) or the arbitrary idp ones (the default for the evergreen which is backwards compat).

There is valuable opportunity in making the request somewhat compatible with deployments of both OAuth and SAML. Not just maintained deployments but more importantly also ones that are beyond serviceability. Everything except the assertion endpoint's parameter handling can be done as a layer on top of a deployment which a) adds Sec-Fetch-Dest check, b) forwards the request without mutating it (so that any possible request signing is not affected), c) plucks the resulting artifacts out of the response and sends its own JSON one.

This could new breathe life to IdPs that would otherwise be dead weight after 3rd party cookie deprecation or whatever comes next.

samuelgoto commented 5 months ago

Everything except the assertion endpoint's parameter handling can be done as a layer on top of a deployment which a) adds Sec-Fetch-Dest check, b) forwards the request without mutating it (so that any possible request signing is not affected), c) plucks the resulting artifacts out of the response and sends its own JSON one.

I think what's important is to maintain the architectural design choice of keeping the id_assertion_endpoint a FedCM endpoint, not an OAuth one. I think it is key to allow it to evolve independently and autonomously, rather than in lock step with another protocol.

b) forwards the request without mutating it (so that any possible request signing is not affected), c) plucks the resulting artifacts out of the response and sends its own JSON one.

I think that should be an non-goal: making the FedCM request forwardable without mutations. If you run code that can intercept and forward, you can also mutate. I'm highly skeptical that what is holding an IdP back is being able to mutate the request before passing it along, and I'm fairly convinced that conflating it will pull both layers back.

As I said, I think the id_assertion_endpoint is a FedCM endpoint, and making it be shared with another protocol is an unnecessary architectural design constraint.

bvandersloot-mozilla commented 5 months ago

This is part of why I like the model of the alternate operating mode of FedCM more. There are no new server API endpoints dedicated to handling the browser API needed. Instead the RP can send a request to the IDP (which I would happily extend to include a Javascript dict) and the IDP can respond in its page, giving data back (which I would also happily extend) to the RP (and access to third party cookies if that is more convenient).

I think the architecture of FedCM as is and the general naming of things, like nonce, client_id, and token really look from the outside like an identity protocol, rather than browser plumbing.

Also, this is a point where the form encoding is annoying- we can't include structure in the params because of it.

bvandersloot-mozilla commented 5 months ago

but we can't leave backwards compatibility aside

Yes we absolutely can :)

panva commented 5 months ago

How about having a way to change what gets prefixed - either the fedcm parameters (explicit option) or the arbitrary idp ones (the default for the evergreen which is backwards compat).

@samuelgoto The above suggestion does not impede the architectural design choice, does not break backwards compatibility, and it makes integration with FedCM much easier for existing infrastructure.

This is part of why I like the model of the alternate operating mode of FedCM more.

@bvandersloot-mozilla I was not aware of this and will give it a proper read in the coming week.

I think the architecture of FedCM as is and the general naming of things, like nonce, client_id, and token really look from the outside like an identity protocol, rather than browser plumbing.

This a 💯.

but we can't leave backwards compatibility aside

Yes we absolutely can :)

👍

bc-pi commented 5 months ago

I typically agree with @panva on just about everything but, while I empathize with the perspective, I'm not sure I share the same level of enthusiasm.

Stepping back just a bit though, can we all maybe agree that the aesthetics of automatically prefixing application/x-www-form-urlencoded parameters with param_ are not great and probably not the kind of thing that we'd want to enshrine "forever" in a standard?

cbiesinger commented 5 months ago

Yes we absolutely can :)

I can't tell if I'm missing a joke but this is an API used by 0.9% of pageloads by a number of IdPs, so any breaking change needs to be carefully managed and communicated; certainly API owners would not let us just break this many pages even if we wanted to.

Stepping back just a bit though, can we all maybe agree that the aesthetics of automatically prefixing application/x-www-form-urlencoded parameters with param_ are not great and probably not the kind of thing that we'd want to enshrine "forever" in a standard?

I truly don't understand the problem?

(https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes is a somewhat related precedent)

panva commented 5 months ago

I can't tell if I'm missing a joke but this is an API used by 0.9% of pageloads by a number of IdPs, so any breaking change needs to be carefully managed and communicated;

Isn't a substantial amount of these driven by GIS's own SDKs/provided scripts?

samuelgoto commented 5 months ago

I can't tell if I'm missing a joke but this is an API used by 0.9% of pageloads by a number of IdPs, so any breaking change needs to be carefully managed and communicated; certainly API owners would not let us just break this many pages even if we wanted to.

Just to be concrete: it is possible to make backwards compatible changes, but they aren't free, so should have (really) good reasons to and do as few times as possible (e.g. batch them).

aaronpk commented 5 months ago

(https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes is a somewhat related precedent)

The big difference here is the author of the HTML adds the data- prefix, rather than the browser adding it automatically.

cbiesinger commented 5 months ago

I can't tell if I'm missing a joke but this is an API used by 0.9% of pageloads by a number of IdPs, so any breaking change needs to be carefully managed and communicated;

Isn't a substantial amount of these driven by GIS's own SDKs/provided scripts?

Even for GIS changes are not free, and I'm more concerned about other IDPs who may get scared off by FedCM making too many breaking changes. That said -- as Sam said, there are probably still few enough IdPs that we can make changes if we have good reasons and manage the rollout.

(https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes is a somewhat related precedent)

The big difference here is the author of the HTML adds the data- prefix, rather than the browser adding it automatically.

We could certainly require the caller of FedCM to add a param or data prefix if that were helpful?

samuelgoto commented 5 months ago

Isn't a substantial amount of these driven by GIS's own SDKs/provided scripts?

There are a few things that makes backwards compatibility hard (but not impossible): (a) there are more IdPs than GIS using FedCM (e.g. https://indiatimes.com/.well-known/web-identity, https://mobage.jp/.well-known/web-identity and https://szn.cz/.well-known/web-identity to name a few publicly announced ones), (b) there are a few more big ones that are actively developing against it (https://paypal.com/.well-known/web-identity to name a few that publicly announced, more that haven't) and (c) we know of a few web developers that save the GSI JS SDK (rather than load it dynamically).

So, backwards compatibility changes are (still) possible (and I'm open to them), but I wouldn't assume that they are free.

Just to be transparent about how we think about backwards incompatible changes, we often refer to Users first, developers second, browser engines third, technical purity fourth to evaluate options.

panva commented 5 months ago

We could certainly require the caller of FedCM to add a param or data prefix if that were helpful?

Or this?

How about having a way to change what gets prefixed - either the fedcm parameters (explicit option) or the arbitrary idp ones (the default for the evergreen which is backwards compat).

samuelgoto commented 5 months ago

To get back to my original point, aside of backwards compatibility, which, as I said, is possible but not free:

@samuelgoto The above suggestion does not impede the architectural design choice, does not break backwards compatibility, and it makes integration with FedCM much easier for existing infrastructure.

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. I think it is unsafe to suggest so: one has to process a FedCM request intentionally, rather than by accident.

There is a series of key privacy and security checks (e.g. the Sec-Fetch-Dest header, the account_id that was selected, whether the account was is_auto_selected, whether the disclosure_text_shown, etc) that need to be made in the id_assertion_endpoint, and we want to reserve the right to evolve it autonomously and independently.

These are key parameters that shouldn't be ignored and have to be processed intentionally: they are the parameters to the id_assertion_endpoint based on the choices that the user has made in the FedCM prompt.

I understand that there is an ergonomics benefit of prefixing these and un-prefixing the params, but, IMHO, Security first, Ergonomics second, Technical Purity last.

bvandersloot-mozilla commented 5 months ago

I can't tell if I'm missing a joke but this is an API used by 0.9% of pageloads by a number of IdPs, so any breaking change needs to be carefully managed and communicated; certainly API owners would not let us just break this many pages even if we wanted to.

No joking here. 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. This is also what the adoptees were signing up for when using an experimental API.

**Edited because I accidentally closed via misclick

samuelgoto commented 5 months ago

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