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

In any mode, make the browser automatically generate clientId based on the RP's origin when omitted #586

Closed
samuelgoto opened this issue May 16, 2024 · 15 comments

Comments

@samuelgoto
Copy link
Collaborator

samuelgoto commented May 16, 2024

When an RP uses any with #240 and #585 it is, by design, not an RP that is registered with the IdP ahead of time, so things like clientId isn't something that they need to specify (e.g. similarly, they wouldn't have registered terms of service and privacy policies, as described in #581).

So, instead of making every RP pass clientId: window.location.origin, maybe it would be helpful for the browser to generate that value, so that it can decrease the number of ways RPs could get wrong.

const identityCredential = await navigator.credentials.get({
  identity: {
    context: "signin",
      providers: [{
        type: "indieauth",
        // clientId: window.location.origin+"/", when omitted, defaults to window.location.origin when `type` or `any` is available
     }],
  },
});
@cbiesinger
Copy link
Collaborator

since we already send an Origin header, why not just make clientId optional in this case?

@aaronpk
Copy link

aaronpk commented May 16, 2024

That's an interesting idea, that implies that the OAuth server would effectively use the Origin header as the client_id. I suppose that does work, tho only in a web context, so it would have to have some processing rule like "if Origin header is present, client_id=Origin, otherwise look in the query string".

@cbiesinger
Copy link
Collaborator

oh, what you're saying is existing servers assume that clientId==origin?

@aaronpk
Copy link

aaronpk commented May 16, 2024

Right now, the client_id is included in the query string because it's a redirect. So existing IndieAuth/OAuth servers are always expecting a client_id parameter in the request. You could make the argument that the Origin header here works as a replacement, but that will require changing the OAuth server specifically for FedCM.

@cbiesinger
Copy link
Collaborator

I am surprised by your implication that they don't otherwise need changes for FedCM. I mean, don't they need to check things like sec-fetch-dest matching "webidentity", etc.?

@npm1
Copy link
Collaborator

npm1 commented May 16, 2024

I'm curious how client ID is currently determined in BYO IDP cases if not using the Origin

@aaronpk
Copy link

aaronpk commented May 16, 2024

That's stuff at the new FedCM endpoints, which is all net new code. Changing how client_id works means changing more of the underlying OAuth part, which may not be practical if you're trying to build this on top of an existing OAuth server.

@aaronpk
Copy link

aaronpk commented May 16, 2024

I'm curious how client ID is currently determined in BYO IDP cases if not using the Origin

The client provides its URL in the initial redirect to the authorization endpoint. Since the Origin header only exists in browsers, non-browser clients need a solution too.

@aaronpk
Copy link

aaronpk commented May 16, 2024

For example, the client would construct this URL and redirect the browser to it:

https://authorization-server.com/authorize?client_id=https://example-app.com/&....

Since it's a redirect, the origin header that the authorization server sees is the authorization server itself of course.

@cbiesinger
Copy link
Collaborator

That's stuff at the new FedCM endpoints, which is all net new code. Changing how client_id works means changing more of the underlying OAuth part, which may not be practical if you're trying to build this on top of an existing OAuth server.

it seems like the fedcm endpoint could fill in the client_id then?

@aaronpk
Copy link

aaronpk commented May 16, 2024

Ok looking at how I implemented this, I did make a new endpoint for the assertion endpoint (rather than trying to reuse the authorization endpoint). That's where it does the check for the Sec-Fetch-Dest header. I also had already added a "if origin hostname matches client_id hostname" check. When all the validations pass, it calls into the same function that the OAuth authorization endpoint calls to create the authorization code. So the client_id query parameter here is redundant, since I could make it pass the Origin header into the generateAuthorizationCode function as the client_id.

@ThisIsMissEm
Copy link

Wouldn't this tie FedCM too much to an implementation detail in IndieAuth (@aaronpk I think we discussed a better solution for client_id for both)

@cbiesinger
Copy link
Collaborator

Ok looking at how I implemented this, I did make a new endpoint for the assertion endpoint (rather than trying to reuse the authorization endpoint). That's where it does the check for the Sec-Fetch-Dest header. I also had already added a "if origin hostname matches client_id hostname" check. When all the validations pass, it calls into the same function that the OAuth authorization endpoint calls to create the authorization code. So the client_id query parameter here is redundant, since I could make it pass the Origin header into the generateAuthorizationCode function as the client_id.

Based on this, should we close this as wontfix or do you still think we should do something here?

(maybe we should make clientId optional? I believe it's currently required)

@aaronpk
Copy link

aaronpk commented May 22, 2024

I'm leaning towards not recommending FedCM do anything automatic with client_id here.

We've been talking about changing the client_id parameter in IndieAuth to be a full URL to a JSON document with the client metadata (indieweb/indieauth#133) rather than just the origin, which wouldn't be compatible with this new suggestion either.

@samuelgoto
Copy link
Collaborator Author

I'm leaning towards not recommending FedCM do anything automatic with client_id here.

Ok, I'm going to close this for now and reopen in case the need appears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants