Fixed issuer path trimming #58
Conversation
| .then(providerFromHeaders => providerFromHeaders || discoverFromProfile(webId)) | ||
|
|
||
| .then(providerUri => { | ||
| // drop the path (provider origin only) |
There was a problem hiding this comment.
Is this really appropriate (I'm just checking here, I honestly have no idea what the 'correct behaviour' should be)? i.e. I know we don't want to strip off any trailing slashes from the origin (i.e. if the URI is https://provider.com/, then we do not want https://provider.com), but do we also want to keep the entire path if there is one (i.e. if the URI was https://provider.com/some/path, do we really want https://provider.com/some/path, or do we actually need https://provider.com/)...? (Regardless, it seems any such logic belongs in the validateProviderUri() function, if indeed we do need any logic to mess around with the providerUri value.)
There was a problem hiding this comment.
Why do we not want to strip off a trailing slash? I can see that the spec doesn't forbid its presence, but what is the function of keeping it?
There was a problem hiding this comment.
if the URI was https://provider.com/some/path, do we really want https://provider.com/some/path, or do we actually need https://provider.com/)...?
Yes, we would want it to be https://provider.com/some/path. There is no reason someone would put an extra path in their WebId. We do not want to automagically equate an issuer at https://provider.com/some/path with one at https://provider.com/ as they could be two separate issuers. So I don't think any additional logic is needed.
what is the function of keeping it?
This particular section is checking if ISSUER_LISTED_IN_THE_WEBID === ISSUER_IN_THE_TOKEN. Right now, for ESS both the issuer in the webId and the issuer in the token are https://example.com/, but the statement I removed changes the issuer.
michielbdejong
left a comment
michielbdejong
left a comment
There was a problem hiding this comment.
How does this relate to nodeSolidServer/oidc-rs#10 and https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest ?
Your PR title claims it fixes a problem, but from the fact that you removed some code and inverted a unit test, it seems you it's an undocumented change to the desired behaviour?
Please describe the issue you are fixing here.
To me, it sounds like removing the path (if present) is useful, just like removing an extra trailing slash (if present)?
|
if current nss behaviour is wrong because of something we missed, then should the solid test suite also start testing this new requirement? |
|
@michielbdejong While https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest says that any terminating backslash should be removed before adding /.well-known/openid-connect, this section of the code has nothing to do with openid config discovery. oidc-auth-manager incorrectly assumed that issuers MUST be an origin, when the spec indicates otherwise. https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata indicates that an issuer can be any URL as long as it is "identical to the iss Claim value in ID Tokens issued from this Issuer." Because the |
|
got it, thanks! |
No description provided.