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.

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

Support new Organization Feature #340

Open
2 of 4 tasks
sventorben opened this issue Mar 27, 2024 · 9 comments
Open
2 of 4 tasks

Support new Organization Feature #340

sventorben opened this issue Mar 27, 2024 · 9 comments
Assignees
Labels
breaking-change Introduces a breaking change documentation Improvements or additions to documentation enhancement New feature or request

Comments

@sventorben
Copy link
Owner

sventorben commented Mar 27, 2024

Is there an existing feature request for this?

  • I have searched the existing issues

Is your feature related to a problem? Please describe.

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

keycloak/keycloak#23948 (comment)

TODOs:

  • Provide basic implementation
  • Needs tests
  • Needs documentation
@sventorben sventorben added documentation Improvements or additions to documentation enhancement New feature or request breaking-change Introduces a breaking change labels Mar 27, 2024
@sventorben sventorben self-assigned this Mar 27, 2024
@xgp
Copy link

xgp commented Apr 3, 2024

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

@sventorben
Copy link
Owner Author

sventorben commented Apr 3, 2024

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
Copy link

xgp commented Apr 3, 2024

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
Copy link
Owner Author

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
Copy link

xgp commented Apr 3, 2024

@sventorben
Copy link
Owner Author

@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
Copy link

xgp commented Apr 13, 2024

@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
Copy link
Owner Author

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";
    }
    // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Introduces a breaking change documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants