[keycloak-dev] Multi tenant review

[Stian Thorgersen]

----- Original Message -----
> From: "Juraci Paixão Kröhling" <jcosta at redhat.com>
> To: "Stian Thorgersen" <stian at redhat.com>, "keycloak dev" <keycloak-dev at lists.jboss.org>
> Sent: Wednesday, 29 October, 2014 11:23:08 AM
> Subject: Re: Multi tenant review
> 
> Stian,
> 
> Agree with all the comments. I'm doing the changes, but I've got a
> question and a few comments. See below.
> 
> On 10/28/2014 04:54 PM, Stian Thorgersen wrote:
> > 3. CatalinaSessionTokenStore.checkCurrentToken - can you figure out the
> > realm if the session was serialized? when adapter is clustered we support
> > serializing the session
> 
> I'm then changing one of the SecurityContext's to include the realm, so
> that it gets de-serialized with this information. Now, the question is
> whether it is more appropriate to add it to KeycloakSecurityContext or
> RefreshableKeycloakSecurityContext. On the superclass
> (KeycloakSecurityContext), I have access only to IdToken and
> AccessToken. I believe both have ways to retrieve the realm (issuer, I
> believe), but I don't know how reliable this is. I remember seeing a
> post from Bill on keycloak-user that it should be changed to an URL, not
> the realm name. On the subclass, however, I have access to the
> KeycloakDeployment, which provides the realm on the exact way that it
> was originally configured.

TBH I have no idea - Marek can hopefully elaborate on this

> 
> > 4. KeycloakAuthenticatorValve.initInternal - just do a warn + unconfigured
> > to make it behave same as if keycloak-server.json is missing, and drop the
> > comment
> 
> Ok, but just to confirm: I should add a new warn with the same text as
> the other and *not* attempt to load a keycloak.json, right? The question
> is because there might be a non-existing resolver specified *and* a
> keycloak.json . As it is, there will be a warn on the log, saying that
> the resolver will not be used, but as there is a keycloak.json, a
> KeycloakDeployment is available.
> 
> Based on your comment, I assume that this is a configuration problem,
> hence, it should not attempt to load keycloak.json if a resolver is
> specified.

Yes, it should ignore 'keycloak.json' in this case. It could be debatable whether or not throwing an exception would be a better option, but I think it's simpler to just match existing functionality

> 
> > 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?
> 
> I don't think it's a significant problem. It will happen when one user
> has access to two realms, is logged in on the first and then tries to
> access the same application on a second tenant. On this case, the cookie
> sent is for the first tenant, so, we should consider it invalid for this
> request, meaning, the principal for this cookie for this realm is
> unknown (return null). On this scenario, Keycloak will redirect the user
> for logging in on the second realm, which will effectively override the
> first cookie. This scenario is tested on
> MultiTenancyTest#testTenantsWithoutLoggingOut .

Hmm.. So basically a user can only be logged-in to one tenant at the time? I think we'd need some fix for that.

> 
> - Juca.
> 
> 
> 
> 
>