Added requirement for redirect URIs to be the same hostname as client

[jaxoncreed]

Continuation of #16 because of a mess up with forked repos

[michielbdejong]

@zenomt and @dmitrizagidulin can you review please?

[zenomt]

i think the premise of this change is mistaken. a client_id isn't necessarily, and probably isn't going to be, a URI (much less a webid), but instead a non-URI, unique, opaque string. this is illustrated in the most recent application-workflow-detailed.md of #27.

it's not appropriate to place any constraint on the form of the client_id (for example, that it "must be identical to the value of the origin header") beyond what is already specified by OIDC (which is that it's a string and it's unique for every registration).

further, the redirect_uri should be entirely up to the application. the only requirements on it should be the ones already spelled out in OIDC/OAuth: that it is scheme https, any non-http scheme, or http://localhost[:port][path-abempty].

[RubenVerborgh]

it's not appropriate to place any constraint on the form of the client_id (for example, that it "must be identical to the value of the origin header") beyond what is already specified by OIDC

I wouldn't know why it is “not appropriate”, given that we also place constraints on OIDC to authenticate people with a WebID.

But I do think that the burden of proof is on @jaxoncreed to argue why indeed it needs to be a WebID, and to document that in this PR.

further, the redirect_uri should be entirely up to the application.

Why though? It seems perfectly reasonable to constrain this to the same domain, because this is the domain I'm giving permission.

[zenomt]

placing a constraint on the client_id is inappropriate because the form of the client_id is an implementation detail of the OP (except for the requirement that it's unique for every registration). OPs are free to make the client_id be whatever is convenient (for example, a key to a database table, or a structured blob encoding information about the client, or anything, as long as it's unique). this is discussed a lot more in #12.

regarding the redirect_uri: since that's where the OP sends the credentials (or the code), i've argued in #12 and #23 that the redirect_uri should be the application identifier.

[RubenVerborgh]

the form of the client_id is an implementation detail of the OP

But nothing stops us from adding more constraints on it for Solid-compatibility?

It would be a problem if making it a URL would be incompatible with the OIDC spec. But it does not seem to be incompatible.

the redirect_uri should be the application identifier

Very likely not (talking from a SemWeb perspective now); the URI of a a redirect page identifies a redirect page, not an app.

[zenomt]

But nothing stops us from adding more constraints on it for Solid-compatibility?

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider. currently the changes to make an OIDC Provider be a WebID-OIDC Provider are backward-compatible additions, like "put the WebID in a new webid claim, otherwise if the sub is a URI, it's the WebID", and for the proof-token thing, "have a way to set a cnf claim in the id_token". the other things that make it be WebID-OIDC are outside the OP, like that the WebID says the OP is an authorized issuer.

the uniqueness of the client_id is an important security aspect of OIDC/OAuth. if the client_id were a URL, it would still need to be a unique URL somehow (fragment?) for each registration.

for the OP to assign a client_id taking the form of a URL that has the same origin as the redirect_uri, that would require all redirect_uris being registered to be in the same origin (OIDC allows multiple redirect_uris, and they don't currently need to share an origin).

requiring all redirect_uris for a client to share an origin, and for that origin to be the same as something (there is no Origin header when redirecting to/loading the OP's login page, and dynamic registration could be done by a back-end server that could omit or set any Origin) would eliminate an entire class of possible application architectures, where different components of the application are in different origins (maybe some parts are in AWS Lambda/Azure Functions/OpenWhisk/etc, maybe logging in is a static github pages page, etc).

[RubenVerborgh]

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider.

+1

if the client_id were a URL, it would still need to be a unique URL somehow (fragment?) for each registration.

This is a major blocker indeed; makes the suggestion incompatible with OIDC.

Thanks for clarifying!

[RubenVerborgh]

As remarked by @zenomt, the fact that the Client ID would need to be the application WebID is incompatible with the OIDC spec, which mandates a unique Client ID per registration.

[kjetilk]

So, what do you say, @jaxoncreed ? Do you agree with @zenomt and @RubenVerborgh 's assessment?

[justinwb]

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider. currently the changes to make an OIDC Provider be a WebID-OIDC Provider are backward-compatible additions, like "put the WebID in a new webid claim, otherwise if the sub is a URI, it's the WebID", and for the proof-token thing, "have a way to set a cnf claim in the id_token". the other things that make it be WebID-OIDC are outside the OP, like that the WebID says the OP is an authorized issuer.

Anything that hinders compatibility with other OIDC provider implementations should be considered a non-starter. Really important that we foster interop.

[jaxoncreed]

Yes, I actually do agree with @zenomt at the moment. It's reflected in another pull request I have (#27) for the spec but not this one. I believe this one can be thrown out.

[justinwb]

Yes, I actually do agree with @zenomt at the moment. It's reflected in another pull request I have (#27) for the spec but not this one. I believe this one can be thrown out.

@jaxoncreed to be clear - this pull request can be closed without merging in favor of #27 ?

[michielbdejong]

See also #12 (comment)

[kjetilk]

OK; I understand that this can be closed. If that was wrong, we can always reopen :-)

[jaxoncreed]

You're correct. It can be closed.