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

metadataSeed.authorization_endpoint not prioritized over remote metadata #1803

Open
jgarplind opened this issue Jan 8, 2025 · 4 comments · May be fixed by #1804
Open

metadataSeed.authorization_endpoint not prioritized over remote metadata #1803

jgarplind opened this issue Jan 8, 2025 · 4 comments · May be fixed by #1804
Milestone

Comments

@jgarplind
Copy link

jgarplind commented Jan 8, 2025

We have a niche use case for which I've come to conclude that we would like to use metadataSeed.

Use case

For one particular client, the user knows themselves by an id of a particular series, let's say a number. The auth server, on the other hand, only knows the user as a GUID. To map from the number to the GUID, an intermediary which is aware of both the number and the GUID must be used.

Ideally, we would want to change the authorization_endpoint to point to this intermediary, only for this client, without impacting other users of our identity server. The intermediary would the handle the input of the number, convert it to the GUID, and redirect the user onward to the normal flow on the auth server.

EDIT: Turns out this is a bad idea since the authorization_endpoint is also used for refreshing the token, something we don't want to use the proxy for. So we'll have to look for another solution. Either way, the bug remains.

Tested approaches

I found two approaches within the library's API:

  1. Overriding metadata.authorization_endpoint. This comes with two drawbacks:
    1. I don't think that the behavior is well-specified, so nothing stops this library from in the future trying to retrieve the remote metadata and prioritizing it over the locally specified metadata if available.
    2. It appears that as implemented now, once metadata is added to the local definition, the call fetch the remote metadata is never sent. This means that we have to define all of the metadata locally, which is obviously a source of unwanted duplication.
  2. I then found out about metadataSeed which sound like an excellent fit, but it seems that the remote value is prioritized over the seeded value? The implementation goes like Object.assign({}, this._settings.metadataSeed, metadata), and as per Object.assign docs, later occurrences (i.e. metadata) override earlier occurrences (in this case this._settings.metadataSeed). When doing debug logging, we can see this in action:
    1. [MetadataService] getMetadata: merging remote JSON with seed metadata
    2. [MetadataService] _getMetadataProperty('authorization_endpoint'): resolved
    3. [OidcClient] createSigninRequest: Received authorization endpoint authorization endpoint from remote source

Is the prioritization of remote metadata over metadataSeed a straight up bug in the MetadataService, or am I missing something?

@jgarplind jgarplind changed the title Ability to override metadata.authorization_endpoint while getting rest of metadata from the metadata endpoint metadataSeed.authorization_endpoint not prioritized over remote metadata Jan 8, 2025
@jgarplind jgarplind linked a pull request Jan 8, 2025 that will close this issue
2 tasks
@outout14
Copy link

+1.
Had the same issue trying to override the token_endpoint from the fetched metadata.

@jgarplind
Copy link
Author

If you need a short-term solution, you can use a custom fork/patch as per this PR. I've tested it myself: https://github.com/authts/oidc-client-ts/pull/1804/files

@pamapa
Copy link
Member

pamapa commented Jan 17, 2025

Is the prioritization of remote metadata over metadataSeed a straight up bug in the MetadataService, or am I missing something?

Thanks for providing context about this merging use case. The initial idea was that what the IdP returns as metadata is correct and would enrich the seed. Which does not work in your case. On the other hand i really makes sense to let seed win, as it is anyway optional. I am just thinking about not breaking existing applications. I guess i just have to reflect this with a version minor bump, instead of a bugix...

@pamapa pamapa added this to the 3.1.1 milestone Jan 17, 2025
@jgarplind
Copy link
Author

Thanks for considering it!

As mentioned above, it ended up being a poor solution for us, since authorization_endpoint has multiple uses, only one of which we wished to overwrite.

But yes, from my perspetive it is intuitive for metadataSeed to enrich the IdP response rather than the other way around.

My perspective is grounded in the assumption that the IdP response will be complete and return with no failure. Others may use metadataSeed as a fallback in case the IdP response is incomplete, or missing, supposedly? I don't know if this is pure library specifics, or if there is some spec to fall back to. Either way I trust your judgement, and don't see a pressing need to merge this ASAP unless you feel confident it is correct.

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

Successfully merging a pull request may close this issue.

3 participants