Potential over validation of OIDC URI about identitymodel

[josephdecock]

The spec doesn't require validation of paths, but it does specifically allow paths to be part of the issuer to support multi tenancy. If you make the assumption that such multi tenant issuers would organize their endpoints underneath the issuer, I think there is value in validating the paths. And while that is an assumption/not explicitly part of the spec, I would be reluctant to change the default behavior.

What if we kept the current behavior as default, but add a flag to disable validation of paths?

[josephdecock]

Looking at this a little closer, there actually already is the DiscoveryPolicy.AdditionalEndpointBaseAddresses that will accomplish this. I verified the usage by putting the sample discovery document above into the file discovery_endpoints_not_relative.json, and then running this test (which passes without changes):

        [Fact]
        public async Task AdditionalEndpointBaseAddresses_should_be_respected()
        {
            var discoFileName = FileName.Create("discovery_endpoints_not_relative.json");
            var document = File.ReadAllText(discoFileName);
            var handler = new NetworkHandler(request => document, HttpStatusCode.OK);
            var client = new HttpClient(handler)
            {
                BaseAddress = new Uri("https://auth.domain.tld/oauth2/openid/jellyfin-aer")
            };

            var discoRequest = new DiscoveryDocumentRequest();
            discoRequest.Policy.AdditionalEndpointBaseAddresses.Add("https://auth.domain.tld");

            var disco = await client.GetDiscoveryDocumentAsync(discoRequest);

            disco.IsError.Should().BeFalse();
            disco.AuthorizeEndpoint.Should().Be("https://auth.domain.tld/ui/oauth2");
            disco.TokenEndpoint.Should().Be("https://auth.domain.tld/oauth2/token");
            disco.JwksUri.Should().Be("https://auth.domain.tld/oauth2/openid/jellyfin-aer/public_key.jwk");
        }

So, I think we don't need to add any new options here. We can just update the error message to help users understand the situation better, and maybe even mention that AdditionalEndpointBaseAddresses exists.

[leastprivilege]

It's been a while, but you know that you can plugin your own validators?

[Firstyear]

That may be so, but the defaults matter and if they are "over-verifying" then people will disable them. So shouldn't it do the right things by default?

[leastprivilege]

That would be up for discussion - it is always hard to change defaults in a library that has hundreds of millions of downloads. Feel free to write up a proposal in form of a PR.

[Firstyear]

I'm not a C# developer, I'm the person who writes the IDP that's triggering this behaviour. I raised this to hopefully help and improve the defaults you offer, but I won't be submitting any PR's. That would be up to @9p4 or yourself.

[brockallen]

We would like to suggest that
That the error return it's expected values, not just the endpoint value (e.g. Endpoint is on a different host than authority: {endpoint} expected {host} )

@josephdecock I could see adding this requested change in our current round of work.

I'm unsure if we should change the behavior of the rest. My sense is that no one would know enough to ever enable the strong validation check.

[9p4]

There is already a flag to disable the validation of paths, but this is not ideal since this issue is about the same domain (with different paths).

[Firstyear]

This would resolve our issues for our users, while retaining the origin validation that is a valuable security check.

[Firstyear]

There is already a flag to disable the validation of paths.

It also disables origin checking.

[josephdecock]

The new error message is now released in preview 3 of our upcoming 7.0.0 release!