Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for known providers to OIDC #22176

Closed
wants to merge 1 commit into from
Closed

Add support for known providers to OIDC #22176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions ...c/deployment/src/main/java/io/quarkus/oidc/deployment/KnownProviderSupplierBuildItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package io.quarkus.oidc.deployment;

import java.util.Optional;
import java.util.function.Supplier;

import io.quarkus.builder.item.MultiBuildItem;
import io.quarkus.oidc.OidcTenantConfig;

/**
* An interface that abstracts details of a well known OIDC provider (google, github etc)
*/
public final class KnownProviderSupplierBuildItem extends MultiBuildItem {

final String name;
final Supplier<Optional<OidcTenantConfig>> supplier;

public KnownProviderSupplierBuildItem(String name, Supplier<Optional<OidcTenantConfig>> supplier) {
this.name = name;
this.supplier = supplier;
}

public Supplier<Optional<OidcTenantConfig>> getSupplier() {
return supplier;
}

public String getName() {
return name;
}
}
35 changes: 33 additions & 2 deletions extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/OidcBuildStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package io.quarkus.oidc.deployment;

import java.util.Collection;
import java.util.List;
import java.util.function.BooleanSupplier;
import java.util.stream.Collectors;

import javax.inject.Singleton;

Expand Down Expand Up @@ -38,6 +40,7 @@
import io.quarkus.oidc.runtime.OidcRecorder;
import io.quarkus.oidc.runtime.OidcTokenCredentialProducer;
import io.quarkus.oidc.runtime.TenantConfigBean;
import io.quarkus.oidc.runtime.providers.KnownOIDCProvidersRecorder;
import io.quarkus.runtime.TlsConfig;
import io.quarkus.vertx.core.deployment.CoreVertxBuildItem;
import io.quarkus.vertx.http.deployment.SecurityInformationBuildItem;
Expand Down Expand Up @@ -115,9 +118,13 @@ public SyntheticBeanBuildItem setup(
OidcConfig config,
OidcRecorder recorder,
CoreVertxBuildItem vertxBuildItem,
TlsConfig tlsConfig) {
TlsConfig tlsConfig,
List<KnownProviderSupplierBuildItem> knownProviders) {
return SyntheticBeanBuildItem.configure(TenantConfigBean.class).unremovable().types(TenantConfigBean.class)
.supplier(recorder.setup(config, vertxBuildItem.getVertx(), tlsConfig))
.supplier(recorder.setup(config, vertxBuildItem.getVertx(), tlsConfig,
knownProviders.stream()
.collect(Collectors.toMap(KnownProviderSupplierBuildItem::getName,
KnownProviderSupplierBuildItem::getSupplier))))
.destroyer(TenantConfigBean.Destroyer.class)
.scope(Singleton.class) // this should have been @ApplicationScoped but fails for some reason
.setRuntimeInit()
Expand Down Expand Up @@ -151,4 +158,28 @@ public boolean getAsBoolean() {
return config.enabled && config.defaultTokenCacheEnabled;
}
}

@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
KnownProviderSupplierBuildItem github(KnownOIDCProvidersRecorder recorder) {
return new KnownProviderSupplierBuildItem("github", recorder.github());
}

@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
KnownProviderSupplierBuildItem facebook(KnownOIDCProvidersRecorder recorder) {
return new KnownProviderSupplierBuildItem("facebook", recorder.facebook());
}

@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
KnownProviderSupplierBuildItem google(KnownOIDCProvidersRecorder recorder) {
return new KnownProviderSupplierBuildItem("google", recorder.google());
}

@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
KnownProviderSupplierBuildItem microsoft(KnownOIDCProvidersRecorder recorder) {
return new KnownProviderSupplierBuildItem("microsoft", recorder.microsoft());
}
}
7 changes: 7 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Optional;

import io.quarkus.oidc.OidcTenantConfig;
import io.quarkus.oidc.runtime.providers.ProvidersConfig;
import io.quarkus.runtime.annotations.ConfigDocMapKey;
import io.quarkus.runtime.annotations.ConfigDocSection;
import io.quarkus.runtime.annotations.ConfigGroup;
Expand Down Expand Up @@ -37,6 +38,12 @@ public class OidcConfig {
@ConfigItem
public TokenCache tokenCache = new TokenCache();

/**
* Config for well known OIDC providers
*/
@ConfigItem
public ProvidersConfig provider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it has to be decoupled from OidcTenantConfig, it has to be part of OidcTenantConfig, tenant specific as needed, or rather, IMHO, something like quarkus.oidc.provider=github should point to an instance of OidcTenantConfig, merged with the OidcTenantConfig referencing it, please see below

Copy link
Member

@sberyozkin sberyozkin Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just realized, this way won't work with the dynamic TenantConfigResolver - one should be able to do, instead of defining this config statically in application.properties,

public class MyTenantConfigResolver implements TenantConfigResolver {
    public Uni<OidcTenantConfig> resolve(RoutingContext rc) 
          {
             OidcTenantConfig github = getDefaultOidcTenantGithubBlock();
             // overrride:
             github.getAuthentication().setScope("user:something");
             return github;
          }
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartwdouglas Stuart, how about having an OidcTenantConfig provider enum property with (github, etc, values).

This will allow OidcRecorder, when going over every OidcTenantConfig, pick up a referenced well-known provider OidcTenantConfig instance and merge with the current OidcTenantConfig - it will address all the concerns I've expressed in various comments here


/**
* Default TokenIntrospection and UserInfo cache configuration.
*/
Expand Down
13 changes: 11 additions & 2 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,24 @@ public Supplier<DefaultTokenIntrospectionUserInfoCache> setupTokenCache(OidcConf
return () -> new DefaultTokenIntrospectionUserInfoCache(config, vertx.get());
}

public Supplier<TenantConfigBean> setup(OidcConfig config, Supplier<Vertx> vertx, TlsConfig tlsConfig) {
public Supplier<TenantConfigBean> setup(OidcConfig config, Supplier<Vertx> vertx, TlsConfig tlsConfig,
Map<String, Supplier<Optional<OidcTenantConfig>>> knownProviders) {
final Vertx vertxValue = vertx.get();

String defaultTenantId = config.defaultTenant.getTenantId().orElse(DEFAULT_TENANT_ID);
TenantConfigContext defaultTenantContext = createStaticTenantContext(vertxValue, config.defaultTenant, tlsConfig,
defaultTenantId);

Map<String, TenantConfigContext> staticTenantsConfig = new HashMap<>();
for (Map.Entry<String, OidcTenantConfig> tenant : config.namedTenants.entrySet()) {
Map<String, OidcTenantConfig> tenants = new HashMap<>(config.namedTenants);
for (var e : knownProviders.entrySet()) {
var provider = e.getValue().get();
if (provider.isPresent()) {
tenants.put(e.getKey(), provider.get());
Copy link
Member

@sberyozkin sberyozkin Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but it is not correct - it creates a dedicated tenant per every social provider config, but it won't allow to override a specific property using OidcTenantConfig - with this approach we'd end up moving a good portion of OidcTenantConfig properties under the github and other provider specific blocks. I'd say it is a problem.
I understand that GitHub and other config groups you have added are meant to customize - but it creates a parallel stream for configuring things.

But also equally important, it does not allow to vary with the provider, each OIDC tenant allows to have:

quarkus.oidc.github1.provider=github
quarkus.oidc.github1.authentication.scopes=a

quarkus.oidc.github2.provider=github
quarkus.oidc.github2.authentication.scopes=b

It may not be something one would want to do in production but for tests it is very useful.

Copy link
Member

@sberyozkin sberyozkin Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so continuing from the earlier comment, so you have Map<String, Supplier<Optional<OidcTenantConfig>>> knownProviders.
But instead of creating tenants as you have typed here, the following will be more aligned with the OIDC tenancy:

if (oidcTenantConfig.provider.isPresent()) {
    // Merge - this ensures the github/etc properties are overridable  
    OidcTenantConfig thisIsCurrentTenantConfig = new OidcTenantConfig(knownProviders.get(oidcTenantConfig.provider.get()));
    // continue dealing with this tenant config
}

}
}

for (Map.Entry<String, OidcTenantConfig> tenant : tenants.entrySet()) {
OidcCommonUtils.verifyConfigurationId(defaultTenantId, tenant.getKey(), tenant.getValue().getTenantId());
staticTenantsConfig.put(tenant.getKey(),
createStaticTenantContext(vertxValue, tenant.getValue(), tlsConfig, tenant.getKey()));
Expand Down
34 changes: 34 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/Facebook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.quarkus.oidc.runtime.providers;

import java.util.List;

import io.quarkus.runtime.annotations.ConfigGroup;
import io.quarkus.runtime.annotations.ConfigItem;

@ConfigGroup
public class Facebook {

/**
* The client ID
*/
@ConfigItem
public String clientId;

/**
* The secret
*/
@ConfigItem
public String secret;

/**
* List of scopes
*/
@ConfigItem(defaultValue = "email,public_profile")
public List<String> scopes;

/**
* Fields to retrieve from the user info endpoint
*/
@ConfigItem(defaultValue = "id,name,email,first_name,last_name")
public String userInfoFields;
}
28 changes: 28 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/GitHub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.quarkus.oidc.runtime.providers;

import java.util.List;

import io.quarkus.runtime.annotations.ConfigGroup;
import io.quarkus.runtime.annotations.ConfigItem;

@ConfigGroup
public class GitHub {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, GitHub (and other providers) groups should extend OidcTenantConfig and override properties with GitHub/etc specific properties

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client-id, client-secret, scopes - these are all available in OidcTenantConfig, this is my earlier concern, it is not worth IMHO to start mirroring some of those properties here. By extending OidcTenantConfig they can be set as needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now think that these groups have to be removed, IMHO, the supplied OidcTenantConfig per every provider should be enough


/**
* The client ID
*/
@ConfigItem
public String clientId;

/**
* The secret
*/
@ConfigItem
public String secret;

/**
* List of scopes
*/
@ConfigItem(defaultValue = "user:email")
public List<String> scopes;
}
28 changes: 28 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/Google.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.quarkus.oidc.runtime.providers;

import java.util.List;

import io.quarkus.runtime.annotations.ConfigGroup;
import io.quarkus.runtime.annotations.ConfigItem;

@ConfigGroup
public class Google {

/**
* The client ID
*/
@ConfigItem
public String clientId;

/**
* The secret
*/
@ConfigItem
public String secret;

/**
* List of scopes
*/
@ConfigItem(defaultValue = "openid,email,profile")
public List<String> scopes;
}
111 changes: 111 additions & 0 deletions ...c/runtime/src/main/java/io/quarkus/oidc/runtime/providers/KnownOIDCProvidersRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package io.quarkus.oidc.runtime.providers;

import java.util.Optional;
import java.util.function.Supplier;

import io.quarkus.oidc.OidcTenantConfig;
import io.quarkus.oidc.runtime.OidcConfig;
import io.quarkus.runtime.annotations.Recorder;

@Recorder
public class KnownOIDCProvidersRecorder {

final OidcConfig config;

public KnownOIDCProvidersRecorder(OidcConfig config) {
this.config = config;
}

public Supplier<Optional<OidcTenantConfig>> github() {
return new Supplier<Optional<OidcTenantConfig>>() {
@Override
public Optional<OidcTenantConfig> get() {
if (config.provider.github.isEmpty()) {
return Optional.empty();
}
OidcTenantConfig ret = new OidcTenantConfig();
ret.clientId = Optional.of(config.provider.github.get().clientId);
ret.credentials.secret = Optional.of(config.provider.github.get().secret);
ret.authServerUrl = Optional.of("https://github.com/login/oauth");
//TODO: do we want to hard code this?
ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HYBRID won't work for GitHub - quarkus-oidc service apps can not handle opaque tokens from providers like GitHub since it has no introspection endpoint - I've opened an issue to make sure a userinfo check is enough

ret.discoveryEnabled = false;
ret.authorizationPath = Optional.of("authorize");
ret.tokenPath = Optional.of("access_token");
ret.userInfoPath = Optional.of("https://api.github.com/user");
ret.authentication.scopes = Optional.of(config.provider.github.get().scopes);
ret.authentication.userInfoRequired = true;
ret.authentication.setIdTokenRequired(false);
ret.authentication.setRedirectPath("/Login/githubLoginSuccess");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those links are too specific to my app. But in practice:

  • google/apple/ms works on a single endpoint without requiring UserInfo: everything is in the ID Token
  • github must be handled specially because UserInfo contains the user ID, name and user name, but we have to hit a second endpoint /user/emails to get the user's email.
  • facebook must be handled specially because UserInfo contains the user ID, name and email

Perhaps a better endpoint default would be: /{provider}-oidc-login-success?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naturally, a further layer of abstraction which knows how to fetch the damn fields from all providers and present them to the Quarkus user would be even better: then I only need a single success endpoint :)
But that can wait for another issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it has to be an application specific property

return Optional.of(ret);
}
};
}

public Supplier<Optional<OidcTenantConfig>> google() {
return new Supplier<Optional<OidcTenantConfig>>() {
@Override
public Optional<OidcTenantConfig> get() {
if (config.provider.google.isEmpty()) {
return Optional.empty();
}
OidcTenantConfig ret = new OidcTenantConfig();
ret.clientId = Optional.of(config.provider.google.get().clientId);
ret.credentials.secret = Optional.of(config.provider.google.get().secret);
ret.authServerUrl = Optional.of("https://accounts.google.com");
//TODO: do we want to hard code this?
ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;
ret.authentication.scopes = Optional.of(config.provider.google.get().scopes);
ret.authentication.setRedirectPath("/Login/oidcLoginSuccess");
return Optional.of(ret);
}
};
}

public Supplier<Optional<OidcTenantConfig>> microsoft() {
return new Supplier<Optional<OidcTenantConfig>>() {
@Override
public Optional<OidcTenantConfig> get() {
if (config.provider.microsoft.isEmpty()) {
return Optional.empty();
}
OidcTenantConfig ret = new OidcTenantConfig();
ret.clientId = Optional.of(config.provider.microsoft.get().clientId);
ret.credentials.secret = Optional.of(config.provider.microsoft.get().secret);
ret.authServerUrl = Optional.of("https://login.microsoftonline.com/common/v2.0");
//TODO: do we want to hard code this?
ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure, I'd have to test it on my app to see if it works.

ret.authentication.setRedirectPath("/Login/oidcLoginSuccess");
ret.token.setIssuer("any");
return Optional.of(ret);
}
};
}

public Supplier<Optional<OidcTenantConfig>> facebook() {
return new Supplier<Optional<OidcTenantConfig>>() {
@Override
public Optional<OidcTenantConfig> get() {
if (config.provider.facebook.isEmpty()) {
return Optional.empty();
}
OidcTenantConfig ret = new OidcTenantConfig();
ret.clientId = Optional.of(config.provider.facebook.get().clientId);
ret.credentials.secret = Optional.of(config.provider.facebook.get().secret);
ret.authServerUrl = Optional.of("https://www.facebook.com");
ret.authentication.scopes = Optional.of(config.provider.facebook.get().scopes);
ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;
ret.authentication.setRedirectPath("/Login/facebookLoginSuccess");
ret.discoveryEnabled = false;
ret.tokenPath = Optional.of("https://graph.facebook.com/v12.0/oauth/access_token");
ret.token.setIssuer("facebook");
ret.setAuthorizationPath("https://facebook.com/dialog/oauth/");
ret.setJwksPath("https://www.facebook.com/.well-known/oauth/openid/jwks/");
ret.setUserInfoPath("https://graph.facebook.com/me/?fields=" + config.provider.facebook.get().userInfoFields);
ret.authentication.setUserInfoRequired(true);
ret.authentication.setIdTokenRequired(false);
return Optional.of(ret);
}
};
}
}
27 changes: 27 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/Microsoft.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package io.quarkus.oidc.runtime.providers;

import java.util.List;

import io.quarkus.runtime.annotations.ConfigGroup;
import io.quarkus.runtime.annotations.ConfigItem;

@ConfigGroup
public class Microsoft {

/**
* The client ID
*/
@ConfigItem
public String clientId;

/**
* The secret
*/
@ConfigItem
public String secret;
/**
* List of scopes
*/
@ConfigItem(defaultValue = "openid,email,profile")
public List<String> scopes;
}
Loading