[keycloak-dev] Multi tenant review

Stian Thorgersen stian at redhat.com
Tue Oct 28 11:54:35 EDT 2014


Hi,

I merged the PR into multi-tenant-adapter branch. It looks good, but I've got a few comments/suggestions:

Adapter:

1. Change <name> in integration/adapter-core/pom.xml - can you send this as a separate PR directly to master?
2. With the updates to AdapterDeploymentContext it's always required to pass request, so we can just remove getDeployment and always use resolveDeployment
3. CatalinaSessionTokenStore.checkCurrentToken - can you figure out the realm if the session was serialized? when adapter is clustered we support serializing the session
4. KeycloakAuthenticatorValve.initInternal - just do a warn + unconfigured to make it behave same as if keycloak-server.json is missing, and drop the comment
5. KeycloakServletExtension.handleDeployment - same as above
6. CatalinaCookieTokenStore.checkPrincipalFromCookie - if cookie has different realm I'd say the log should at least be a warn, is this not a significant problem?
7. CatalinaSessionTokenStore.checkCurrentToken - same as above isn't cookie with different realm a pretty big issue?

Docs:

1. Move multi-tenancy documentation into adapters section

Examples:

1. Remove multi-tenant example (it's just to complex for us to maintain it)
2. Rename simple-multi-tenant example to multi-tenant
3. In simple-multi-tenant/README.MD can you remove instructions on starting server (we have separate instructions for that, and assume a user can do it by the time they're looking at examples), also remove comments about docker (we need to focus examples and not confuse users by introducing other concepts). Also, why does it instruct the user to start on 8180?
4. Remove/improve "hackish" comment in PathBasedKeycloakConfigResolver
5. PathBasedKeycloakConfigResolver loads adapter settings from json on every request - this will be relatively expensive


More information about the keycloak-dev mailing list