Skip to content

Rework oidc provider construction#281

Merged
jevolk merged 4 commits into
matrix-construct:mainfrom
Jeidnx:oidc_discovery_fix
Feb 12, 2026
Merged

Rework oidc provider construction#281
jevolk merged 4 commits into
matrix-construct:mainfrom
Jeidnx:oidc_discovery_fix

Conversation

@Jeidnx
Copy link
Copy Markdown
Contributor

@Jeidnx Jeidnx commented Feb 1, 2026

Rework the initialization code for oidc providers:

  • Refactor the code to be more maintainable
  • split github into its own init function since its a very special case
  • Remove default values for {authorization,introspection,token} urls, since there is no standard default that could be used. All providers i know of support the discovery standard, which should be preferred instead of configuring these manually.
  • Adds automatic construction of the callback url.
  • Fixes oAuth can't parse client's sso urls #276

Comment thread src/service/oauth/providers.rs Outdated
Comment thread src/service/oauth/providers.rs
Comment thread src/service/oauth/providers.rs Outdated
@Jeidnx Jeidnx force-pushed the oidc_discovery_fix branch from 3c155a6 to fce2e5a Compare February 2, 2026 16:47
@Jeidnx
Copy link
Copy Markdown
Contributor Author

Jeidnx commented Feb 2, 2026

Sorry i may have been a bit harsh with the words. I was still in my Software Engineering mindset and honestly a little frustrated since it took me some time to get this software to compile properly. (I will probably open an issue or a PR for the problems i encountered)

I refactored a lot of the configuration code, in my opinion this should be way better to read and maintain.
Github seems to be a very special case, so it only made sense that it gets it own function. The other branded providers comply with the discovery spec.
The callback_url really shouldn't be configurable, i fixed that and indicated it in the docs.

If you have any questions about the code please don't hesitate to ask

@Jeidnx Jeidnx marked this pull request as ready for review February 2, 2026 16:48
@Jeidnx Jeidnx force-pushed the oidc_discovery_fix branch from fce2e5a to f0a3df6 Compare February 3, 2026 11:37
@Jeidnx Jeidnx changed the title fix: assemble oidc discovery url correctly Rework oidc provider construction Feb 3, 2026
@Jeidnx Jeidnx force-pushed the oidc_discovery_fix branch from f0a3df6 to 03e6637 Compare February 3, 2026 12:00
@Jeidnx
Copy link
Copy Markdown
Contributor Author

Jeidnx commented Feb 3, 2026

Not sure why the CI fails on unchanged code. Runs fine on main: https://github.com/matrix-construct/tuwunel/actions/runs/21559255457/job/62120577532

@x86pup
Copy link
Copy Markdown
Member

x86pup commented Feb 3, 2026

I think this is some new issue with the typos tool we use for detecting typos, not the first time something like this has happened. We will handle it ourselves don't worry. Thank you for the contribution.

@aazf
Copy link
Copy Markdown

aazf commented Feb 4, 2026

@Jeidnx If possible, pls add username claim from OAuth provider.
some of provider return the username, not the preferred_username.

@jevolk jevolk force-pushed the oidc_discovery_fix branch 2 times, most recently from f0a3e96 to a7731f6 Compare February 6, 2026 07:13
@jevolk
Copy link
Copy Markdown
Member

jevolk commented Feb 6, 2026

When testing GitHub I 404'ed after the initial redirect. At a glance it might be that the base_path is not being applied. I can have a look but I have to merge the other pending items to main branch first.

@Jeidnx Jeidnx force-pushed the oidc_discovery_fix branch from a7731f6 to 8e3bc4b Compare February 6, 2026 18:41
@Jeidnx
Copy link
Copy Markdown
Contributor Author

Jeidnx commented Feb 6, 2026

i rebased on main and made some changes to the github provider. I'm pretty sure this time the endpoints are the same as they where previously. Still couldn't find any documentation for them though. Btw did you find any or how do you know the endpoints?

I also decided to remove the base_path. There really isn't a reason to have this, because the only thing it would affect is the discovery url. If there is a provider where the discovery url is non standard, there is an explicit option to overwrite just that.

@jevolk
Copy link
Copy Markdown
Member

jevolk commented Feb 6, 2026

I also decided to remove the base_path. There really isn't a reason to have this, because the only thing it would affect is the discovery url.

Unfortunately we can't remove this because it would break released code. The only way to effectively deprecate it at this point is to still apply the value somewhere expected when still used.

@jevolk
Copy link
Copy Markdown
Member

jevolk commented Feb 6, 2026

Hypothetically, would adjusting the path literals from the original code leading to Url::join() like f6c3c31 fix the specific issuer_url issue?

I like the automatic callback_url from this pr as well, that's a great idea.

@Jeidnx
Copy link
Copy Markdown
Contributor Author

Jeidnx commented Feb 8, 2026

Not quite, for Url::join() it matters both if there is a slash at the end of the Url, and if there is one at the start of the input. I've assembled a minimal patch that according to my testing fixes the discovery issue. If that is what you want, feel free to apply that and close this PR:
fix_discovery.patch

jevolk and others added 4 commits February 12, 2026 02:06
Signed-off-by: Jason Volk <jason@zemos.net>
Signed-off-by: Jason Volk <jason@zemos.net>
Co-authored-by: jeidnx <git@domainhier.de>
Signed-off-by: Jason Volk <jason@zemos.net>
@jevolk jevolk force-pushed the oidc_discovery_fix branch from 8e3bc4b to 2e19a30 Compare February 12, 2026 03:24
@jevolk jevolk merged commit 2e19a30 into matrix-construct:main Feb 12, 2026
130 of 146 checks passed
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 this pull request may close these issues.

oAuth can't parse client's sso urls

4 participants