p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
Other
367 stars 65 forks source link

Update home idp lookup #83

Closed xgp closed 10 months ago

xgp commented 1 year ago

to current version https://github.com/sventorben/keycloak-home-idp-discovery

phamann commented 10 months ago

Hey, @xgp. While debugging our home realm discovery flow today, I noticed that the version of the home idp discovery extension you've imported into the project is quite out of date now (as this open issue also implys 😉).

I'm happy to raise a PR to get it updated, however, I suspect that it's not as simple as pulling the new files, so wanted to first ask:

I'll try have a look in the git history to understand whats changed. But any pointers would be greatly appreciated.

xgp commented 10 months ago

Thanks for raising this @phamann

The main thing that changed is how the IdP is looked up. @sventorben looks it up based on an attribute stored on the IdP entity, whereas we need to look it up based on an organization attribute. The code to do that is here in our current version https://github.com/p2-inc/keycloak-orgs/blob/main/src/main/java/io/phasetwo/service/auth/idp/HomeIdpDiscoveryAuthenticator.java#L198

I asked a while ago if they could make it a non-final class, so we could import it and then just override the lookup, but the answer was "no" at the time.

sventorben commented 10 months ago

There was a feature request recently to support organization attributes. You will find a description how supporting this can be achieved by a custom user attribute in this issue: https://github.com/sventorben/keycloak-home-idp-discovery/issues/189

Not sure if it helps in your case, though.

xgp commented 10 months ago

Thanks for taking a look @sventorben . I saw that addition of using a custom user attribute. It's close, but unfortunately it doesn't fit our use case, which requires doing a search in custom entities that are specific to this extension.

Our best case would be the ability to easily override the HomeIdpDiscoverer implementation. If that's something you would consider, let me know and I'll file an issue on your repo and make a PR for a suggested change.

xgp commented 10 months ago

Also @sventorben here was our original discussion about extension https://github.com/sventorben/keycloak-home-idp-discovery/issues/96 but I think the code has evolved/stabilized a lot since then. Let me know if there is some interest in allowing extension points.

sventorben commented 10 months ago

I think your usecase for the ATTEMPTED_USERNAME should be supported out of the box now, if you switch on Bypass login page in the configuration (see code at https://github.com/sventorben/keycloak-home-idp-discovery/blob/main/src/main/java/de/sventorben/keycloak/authentication/hidpd/HomeIdpDiscoveryAuthenticator.java#L39)

There also was a discussion opened yesterday to support a different way to discover the home idp: https://github.com/sventorben/keycloak-home-idp-discovery/discussions/228

So, there is definitely a need for some kind of extension point. I think that I need to understand some use cases a bit more in detail so that I can craft an SPI that could work for all cases.

xgp commented 10 months ago

Yes @sventorben ATTEMPTED_USERNAME works fine. I've added context on overriding the HomeIdPDiscoverer in your issue here https://github.com/sventorben/keycloak-home-idp-discovery/discussions/228#discussioncomment-6800254

phamann commented 10 months ago

@xgp while the above disucssion with @sventorben continues on the extension repo and issues, I have submitted a PR to keycloak-orgs to at least upgrade the forked code to the latest commit so we can benfit from the new params. Let me know if you're ok with this.

https://github.com/p2-inc/keycloak-orgs/pull/121

xgp commented 10 months ago

@phamann Thank you! This will give us the new params in the meantime while we wait on a decision to modify the upstream for our use case.

xgp commented 10 months ago

@sventorben This commit https://github.com/p2-inc/keycloak-orgs/commit/323cb0607113d4e2209caf69c642d65a2a60a4d2 by @phamann should give a good idea of how we are customizing your extension for our use case. It's focused mainly on the lookup of the IdPs https://github.com/p2-inc/keycloak-orgs/commit/323cb0607113d4e2209caf69c642d65a2a60a4d2#diff-481fc6d907ee6e1863cd931d95914761755b7cdf1eb3e0d65553072285c534eaR105-R112