Fix double oauth2_provider mountpoint in oidc view

[dollarklavs]

Fixes the doubling of mountpoint path in the OIDC endpoints values for .well-known/openid-configuration/

According to the django-oauth-toolkit documentation for OIDC_ISS_ENDPOINT this settings variable should enable discovery at OIDC_ISS_ENDPOINT + /.well-known/openid-configuration/. This behaviour is backed by the OIDC specs.

But if you use the variable as described you'll end up with the correct URL for the issuer value but incorrect URL's for the values of authorization_endpoint, token_endpoint, userinfo_endpoint, and jwks_uri in the json response when calling /.well-known/openid-configuration/.

So if the OIDC_ISS_ENDPOINT is http://localhost:8001/some-initial-path/o the issuer will be http://localhost:8001/some-initial-path/o but authorization_endpoint will be http://localhost:8001/some-initial-path/o/some-initial-path/o/authorize/. Same pattern for token_endpoint, userinfo_endpoint, and jwks_uri

Fixes #955

Description of the Change

Instead of concatting OIDC_ISS_ENDPOINT with the output ofreverse for the different OIDC views, first use urlparse to strip OIDC_ISS_ENDPOINT to only scheme + netloc (eg. http + :// + localhost:8000).
Also updates tests to expect an OIDC_ISS_ENDPOINT that ends in /o.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[codecov]

Codecov Report

Merging #957 (a58b895) into master (27bd0af) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #957   +/-   ##
=======================================
  Coverage   96.60%   96.61%           
=======================================
  Files          31       31           
  Lines        1710     1713    +3     
=======================================
+ Hits         1652     1655    +3     
  Misses         58       58           

Impacted Files	Coverage Δ
oauth2_provider/views/oidc.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27bd0af...a58b895. Read the comment docs.

[auvipy]

can you please add the changelog entry?

[dollarklavs]

can you please add the changelog entry?

It's been added👌

[rtpg]

@dollarklavs is it safe to say that, given this is affecting the connection discovery view, that in the current release this is just outright broken (since the URLs are wrong), and so people upgrading don't need to take special precautions when upgrading if they are using this setting (since supposedly the feature just isn't working)?

If there's some big-ish changes we might want to add something to the changelog to make it clear that this change affects people using this specific setting.

Things otherwise look good to me as well

[dollarklavs]

@dollarklavs is it safe to say that, given this is affecting the connection discovery view, that in the current release this is just outright broken (since the URLs are wrong), and so people upgrading don't need to take special precautions when upgrading if they are using this setting (since supposedly the feature just isn't working)?

If there's some big-ish changes we might want to add something to the changelog to make it clear that this change affects people using this specific setting.

Things otherwise look good to me as well

@rtpg I would think that OIDC in the current release is broken. So yeah no need to take special precautions - but I/we could probably have overlooked a use case that incorporates the previous implementation.
If extra information should be added to the changelog it could be
#955 Avoid doubling of oauth2_provider urls mountpath in json response for OIDC view ConnectDiscoveryInfoView. Breaks existing OIDC discovery output. Would that be enough?

[rtpg]

I think that would be good, yeah. So if somoene is looking over the release notes and are using OIDC they'll click through and maybe realize the change does (or more likely doesn't!) affect them. Thanks for the detailed reply.

[dollarklavs]

I think that would be good, yeah. So if somoene is looking over the release notes and are using OIDC they'll click through and maybe realize the change does (or more likely doesn't!) affect them. Thanks for the detailed reply.

@rtpg Changelog updated🥳

I reviewed and aprpoved, and previously there was only a changelog request.

[n2ygk]

@dollarklavs @rtpg I'm concerned that you think OIDC in the current release is broken as I've been using it as have others. I hope this PR hasn't caused a breaking change. @MattBlack85 and I will need to re-review before we publish a release.

[rtpg]

Going to try and summarize the issue, because I think the issue itself is relatively limited in scope:

• if you are not setting OIDC_ISS_ENDPOINT then the URLS listed in the discovery view will correctly list the URLs for various views according to what you have in your url configuration (so if I set the base oauth2_providers.urls to foobar/ then http://127.0.0.1:8000/foobar/.well-known/openid-configuration/ will properly report all of those)
• However, as you might know, this ends up working by getting the host from the request (so doing a request via localhost will return those URLS etc). Maybe this is part of the reason for this setting to exist in the first place?
• If I decided that I want people to get a canonical hostname and declare the OpenID connect absolute path explicitly, by setting:

OAUTH2_PROVIDER = {
   ...,
  "OIDC_ISS_ENDPOINT": "http://localhost:8000/foobar",
}

then what I get back from the above .well-known view is

{"issuer": "http://127.0.0.1:8000/foobar",
 "authorization_endpoint": "http://127.0.0.1:8000/foobar/foobar/authorize/",
 "token_endpoint": "http://127.0.0.1:8000/foobar/foobar/token/",
 "userinfo_endpoint": "http://127.0.0.1:8000/foobar/foobar/userinfo/",
 ...
}

(note the doubling of foobar in the URL). To be clear, the endpoints being returned in this response are invalid, this setting doesn't change the routing or anything. So if you were getting these URLs they would be unusable to you, a priori. Now I'm not super clear on the reason one would set this OIDC_ISS_ENDPOINT setting in the first place (the canonical thing is a guess), but it definitely feels like in the current configuration there are few ways t set this setting that doesn't cause the discovery view to return invalid data.

But if you don't use this setting, everything seems fine! Hence this not being a major issue yet, I think. Hope this helps