feat: use oidc well-known url #2077
Conversation
muhlba91
requested review from
acasajus,
cquintana92 and
nguyenkims
as code owners
|
@viq @Queuecumber this is the follow-up PR from #2046 @nguyenkims @acasajus would you mind reviewing the PR? 😃 |
app/auth/views/oidc.py
Outdated
| @@ -60,19 +66,26 @@ def oidc_callback(): | |||
| flash("Please use another sign in method then", "warning") | |||
| return redirect("/") | |||
|
|
|||
| token_url = OIDC_TOKEN_URL | |||
There was a problem hiding this comment.
As most IDP support the well-known URL, should we remove OIDC_TOKEN_URL, OIDC_USER_INFO_URL, etc and only keep OIDC_WELL_KNOWN_URL?
There was a problem hiding this comment.
I think it may be useful to leave that capability as a fallback, since there probably is something out there that does not expose a proper well-known information.
There was a problem hiding this comment.
in general, i agree with @viq that a fallback option could be useful for IDPs that don't expose it correctly - there are always some weird systems out there...
personally, i favor this one as it keeps flexibility for users. (and we'd also not break existing configurations.)
however, from the maintenance point of view it'd be a bit easier to maintain if we rely only on the well-known endpoint.
i believe this is a trade-off core maintainers need to decide.
There was a problem hiding this comment.
I think using well-known url is a great initiative as this avoids RP from having to hard code several values and even though if it isn't obligatory in the standard, it's supported by most IDP and we can decide to keep it as the only option supported in SL.
There was a problem hiding this comment.
I agree with @nguyenkims. It's easier to maintain and it minimises the chances that some user badly configures the provider.
app/auth/views/oidc.py
Outdated
| @@ -16,14 +18,15 @@ | |||
| from app.email_utils import send_welcome_email | |||
| from app.log import LOG | |||
| from app.models import User, SocialAuth | |||
| from app.utils import encode_url, sanitize_email, sanitize_next_url | |||
| from app.utils import sanitize_email, sanitize_next_url | |||
|
|
|||
|
|
|||
| # need to set explicitly redirect_uri instead of leaving the lib to pre-fill redirect_uri | |||
| # when served behind nginx, the redirect_uri is localhost... and not the real url | |||
| _redirect_uri = URL + "/auth/oidc/callback" | |||
There was a problem hiding this comment.
Nitpicking. Can you rename this to redirect_uri? We don't need to keep the _ prefix since it's already the url we're going to use.
muhlba91
force-pushed
the
feat/oidc-well-known
branch
from
5e1e9d6 to
ccfc3e0
Compare
|
@acasajus @nguyenkims thank you for your feedback! |
follow-up of #2046 to be able to use the well-known OIDC URL defined as
OIDC_WELL_KNOWN_URLinstead of specifying all OIDC related URLs.if the
OIDC_WELL_KNOWN_URLis specified all other OIDC related URL env variables are ignored and the configuration is read from the well-known endpoint.additionally, the next URL is kept in the session (and cleared afterwards) to ensure the OIDC redirect URL is clean for the OIDC server to recognize correctly.