sventorben / keycloak-home-idp-discovery

Keycloak: Home IdP Discovery - discover home identity provider or realm by email domain
MIT License
246 stars 45 forks source link

Support new Organization Feature #340

Open sventorben opened 4 months ago

sventorben commented 4 months ago

Is there an existing feature request for this?

Is your feature related to a problem? Please describe.

Placeholder to support the Keycloak’s Organization Feature coming with KC 25.

https://github.com/keycloak/keycloak/discussions/23948#discussioncomment-8921009

TODOs:

sventorben commented 4 months ago

Adding some links here for reference:

xgp commented 4 months ago

@sventorben Do you think this is an opportunity to implement the "discoverer" as an SPI as we discussed?

sventorben commented 4 months ago

Maybe, yes. Still not sure what it would look like though. I am not sure as something simple like this would work:

public interface Discoverer { 
  List<IdentityProviderModel> discoverForUser(String username)
}

What else would be needed? AuthenticationFlowContext maybe?

xgp commented 4 months ago

We override that method in HomeIdpDiscoverer right now. The other issue is that we require some additional configuration, which we add as ProviderConfigProperty to HomeIdpDiscoveryConfigProperties. So, solving both of those would be sufficient for us.

sventorben commented 4 months ago

The config is bound to the authenticator instance that initiates the discovery process. Do you need the additional config parameters for discovery or for other means? Can you point me to the code in your code base, please? I feel I need a better understanding of this.

xgp commented 4 months ago

Here are the additions we made: https://github.com/p2-inc/keycloak-orgs/blob/main/src/main/java/io/phasetwo/service/auth/idp/HomeIdpDiscoveryConfigProperties.java#L20-L36

These are used in the discovery.

sventorben commented 4 months ago

@xgp Can you take a look at #346 please. This is what I came up with - would like some initial feedback. Consider it to be an early draft.

The HomeIdpDiscoveryAuthenticatorFactory along with the EmailHomeIdpDiscoveryAuthenticatorFactoryDiscovererConfig should give some hints on how to add custom properties to configure the discovery logic. And the HomeIdpDiscoverySpi is the extension point to come up with your own discovery logic.

xgp commented 4 months ago

@sventorben this meets all of the needs we currently have. Thanks so much for putting this together, and being so thoughtful about developer support and making sure stability is a future priority. If you think it would help you, I can do an implementation of our org discoverer based on your branch and report back if there are any issues.

I understand the desire not to "support" others' extensions of your classes, and thus are choosing to make them final. But, it would be nice, at least in our case, to be able to extend EmailHomeIdpDiscoverer and EmailHomeIdpDiscovererConfig, as we are overriding very little of those, and I'd prefer not to duplicate code.

sventorben commented 4 months ago

If you think it would help you, I can do an implementation of our org discoverer based on your branch and report back if there are any issues.

That would be great.

to be able to extend EmailHomeIdpDiscoverer and EmailHomeIdpDiscovererConfig

I am not a big fan of extension in terms of inheritance here. I had another look at your code and would prefer to go with composition instead. Take a look at my latest commit to the branch. Would that work for you?

Your implementation should look something like this in the end:

public class OrgsHomeIdpDiscoveryAuthenticatorFactory extends AbstractHomeIdpDiscoveryAuthenticatorFactory {
    public OrgsHomeIdpDiscoveryAuthenticatorFactory() {
        super(new DiscovererConfig() {
            @Override
            public List<ProviderConfigProperty> getProperties() {
                // return your custom properties here
                return null;
            }

            @Override
            public String getProviderId() {
                return "orgs";
            }
        });
    }

    @Override
    public String getId() {
        return "orgs";
    }

    // ...
}

public class OrgsDiscovererFactory implements HomeIdpDiscovererFactory {

    @Override
    public HomeIdpDiscoverer create(KeycloakSession keycloakSession) {
        return new EmailHomeIdpDiscoverer(new Users(keycloakSession), new IdentityProviders() {
            @Override
            public List<IdentityProviderModel> withMatchingDomain(AuthenticationFlowContext context, List<IdentityProviderModel> candidates, Domain domain) {
                // filter the candidates here or simply lookup by your own logic via domain parameter
                YourCustomConfig config = new YourCustomConfig(context.getAuthenticatorConfig());
                return null;
            }
        });
    }

    @Override
    public String getId() {
        return "orgs";
    }
    // ...
}