Add support for well-known OIDC providers

[sberyozkin]

Replaces #22176, Fixes #20783.

This PR builds on Stuart's idea but instead uses OidcTenantConfig references to well-known providers from the current OidcTenantConfig. Any of the statically or dynamically configured ODC tenants can refer to them and override as many properties as needed and add as many properties as needed in scope of the current tenant.

It will allow the following variations:

1. Default tenant:

quarkus.oidc.provider=github
quarkus.oidc.client-id=myclientid
//etc

2. Single custom tenant which may read better

quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=myclientid
//etc

3. More than one tenant

quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=myclientid

quarkus.oidc.apple.provider=apple
quarkus.oidc.apple.client-id=myclientid

4. In TenantConfigResolver:

OidcTenantConfig config = new OidcTenantConfig();
config.setProvider(Provider.GITHUB);
// more properties as needed
return Uni.createFrom().item(config);

This PR supports GitHub and Apple only in the beginning. Not sure about Microsoft - I believe some of its providers also have tenant specific authServerUrl. I think it makes sense to add them one by one - if we see all users dealing with a given provider have to set at least 2 identical properties including authServerIUrl then it would qualify.

PR is based on the idea of merging the current OidcTenantConfig with the referenced provider's OidcTenantConfig. At the moment the copying of properties is done selectively to handle the supported providers (GitHub for now) - it is safe as this merge operation can only happen if a recognized enum provider value is configured. I had to remove some default values for some properties and handle defaults directly in the code as otherwise it is not clear if it was the user who set this property or it was defaulted to some value.

Going forward it would be good to let the users provide arbitrary OidcTenantConfig blocks, example, myKeycloakWithHttpsAndProxyEtc which can be referenced as quarlus.oidc.provider.

I've updated the current GitHub and Apple OIDC web-app doc fragments, but we'll have a proper doc dedicated to configuring various providers once we have a few more issues reported by Steph addressed...

CC @FroMage

[FroMage]

LGTM, but note that some config lost their default values, so they're not documented anymore. IMO you should add them as documentation if they still have default values, no?

[sberyozkin]

@FroMage Good point, I'll update JavaDocs today. (I'll be off till next Wed)

[sberyozkin]

Hi @FroMage and @pedroigor, thanks, I've updated JavaDocs.

By the way, by default, both github and apple configurations request an email scope. Should we limit it to the name only by default ? (read:user (profile) scope in case of GitHub, name for Apple) ? I guess in most web-app application the name is always required to welcome the current logged in user, but email is only required for the applications which add extra features like notifying the users etc. (this is also a GDPR related space, keeping the emails). So I'd be inclined to restrict to the name scope only by default but the extra scopes can be added if needed easily.

I'd also like to wait until @stuartwdouglas returns from PTO in case he'd like to add something since he created the original PR

Happy New Year :-)

[gsmet]

I added a couple of small suggestions.

[gsmet]

@sberyozkin could you have a look at my comments? This is a very nice addition for 2.7 and it would be cool to get it in.

[gsmet]

@sberyozkin friendly ping. CR1 is next week and if we want this in, we need it merged.

[sberyozkin]

Hi @gsmet sorry for a delay, only returned this morning, let me deal with the comments, thanks, I just wanted to wait for Stuart to have a quick look as this PR replaces the one he started with. I'll merge on Monday evening/Tue morning if no comments will follow, 19th is Wed, so it should make it just in time

Hi @gsmet, I believe I've addressed the comments, and might add something for Facebook, please request more changes if needed

[FroMage]

I'm about to test this.

[FroMage]

@sberyozkin I've tested this, and it almost works. I've added missing providers, and changed some scopes and turned them into lists.

My only issue is with the microsoft provider, it appears that this is not picked up:

        ret.getToken().setIssuer("any");

Because I'm getting a 401 in my tests, due to Quarkus checking the issuer. At least I'm sure it's that, because you don't log any auth failures so it would take me a lot of debugging to figure it out (we really have to fix this).

I'm sure it's that because if I re-enable:

# Must be any because it appears to be a random UUID
quarkus.oidc.microsoft.token.issuer=any

Then my test passes.

[sberyozkin]

@FroMage Thanks, Let me update OidcUtilsTest.

I'm a bit confused about Microsoft - it is not Azure ? See how Azure is configured for ex here: #20363, note TenantId at the end but it is not used in your microsoft code.

Re GitHub - should read:user remain as I see you have similar scopes ? Can you check #22572 (comment) ? I.e - should we support an email scope for github and other providers by default ? It seems to me we should not as it is not a base level scope like the one which will get the user profile such as read:user for GitHub - not all websites need an email but all may need a user name ?

[sberyozkin]

@FroMage never mind, it is all can be easily overridden, lets keep it in the form which works best OOB for the application which drives these enhancements :-)

[sberyozkin]

@FroMage I added the tests and microsoft will also work for you now, as well as facebook.

[FroMage]

I'm a bit confused about Microsoft - it is not Azure ? See how Azure is configured for ex here: #20363, note TenantId at the end but it is not used in your microsoft code.

I did set it up in Azure, is all I know. I could not find TenantId in your linked issue.

Re GitHub - should read:user remain as I see you have similar scopes ? Can you check #22572 (comment) ? I.e - should we support an email scope for github and other providers by default ? It seems to me we should not as it is not a base level scope like the one which will get the user profile such as read:user for GitHub - not all websites need an email but all may need a user name ?

My opinion is the opposite: most websites won't need a user name, they will ALL require a user ID though, but OIDC gives that OOTB. Most of them will require an email though, to be able to communicate with the user, and getting it from OIDC allows them to skip validating the email.

[FroMage]

Yup, that works, thanks, much better!

[gsmet]

@stuartwdouglas could you have a look at that one? I will like to get it merged for 2.7.0.CR1 which will be released on Wednesday morning Paris time so we need to iterate quickly on it.

[sberyozkin]

I'm planning to merge tomorrow morning

[stuartwdouglas]

LGTM

[FroMage]

Damnit, I'm actually missing a few properties.

[FroMage]

@sberyozkin those are not merged:

        ret.getAuthentication().setExtraParams(new HashMap<>());
        ret.getAuthentication().getExtraParams().put("response_mode", "form_post");
        ret.getAuthentication().setForceRedirectHttpsScheme(true);

[sberyozkin]

@FroMage please set form_post directly for now before we introduce a tested support for form_post, re forceRedirectHttpsScheme - is that specific to your setup ?

[FroMage]

please set form_post directly for now before we introduce a tested support for form_post

Ah. #22029 OK.

re forceRedirectHttpsScheme - is that specific to your setup ?

Yes and no. No, because Apple will just never work without https so we might as well make those links as it wants them. But that's also related to ngrok which doesn't add the proper HTTP proto headers so I was unable to test whether those headers would get us the right proto.

[sberyozkin]

@FroMage Sure, lets add forceRedirectHttpsScheme, please update alongside the facebook setup; (if you are Ok with doing the PR, can you also tweak OidcUtils to copy forceRedirectHttpsScheme during the merge of configs and update both of OidcUtils apple tests - to verify this property is accepted and overridable)

[FroMage]

OK, done at #23061