[keycloak-dev] Multi tenant review

[Juraci Paixão Kröhling]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Stian,

I went ahead and did the changes based on the following assumptions:

3) KeycloakSecurityContext should have a "realm" property. Added test
for this and changed the callers to pass the realm to the constructor.

4) Leave the adapter unconfigured in case of configuration mismatch
(ie: resolver specified but invalid + keycloak.json present). The
cases are described as comments in the code

6 and 7) I assume this is not a problem. I cannot find a situation
where this would lead to a non-existing user in one tenant to
successfully access an unauthorized resource. In any case, I've added
a couple of test cases, where an user from tenant1 tries to login to
tenant2 and where an already logged in user from tenant1 tries to
access tenant2.

A new PR was sent with all the changes from your review.

- - Juca.

On 10/29/2014 11:23 AM, Juraci Paixão Kröhling wrote:
> 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.
> 
>> 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.
> 
>> 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 .
> 
> - Juca.
> 
> 
> 
> 
> _______________________________________________ keycloak-dev
> mailing list keycloak-dev at lists.jboss.org 
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCgAGBQJUUO1BAAoJEDnJtskdmzLMIv4H/R0NTJ46mK0dbDXkwNb68xQS
W6zeQ+jzBmAvwu7/i8/kSu7AXSPaGxR/EBbjSBKj6l5zVQLjlPQ0CFUzQSDGDASy
WKyMEKk64Df0dNZlkuqHHFaCyuZ+3rz2k9XiGRmpIXej/uzUjvXUpIKBmUOpDxR6
dmqViOkRBuLFbBvwbCgid85Mip92wd2YXUHl0dmPNZN/j9bJPp8GYTHinJgv1pZs
j6hTYduasl/T4jcgWWHYKftuOpbpOWtusjCwuGwxIS1TsIYNZt4G88LL6tWTD9WF
8LznY1cxXuRA7YAvGKOrZY/e9yECQGJokWSZxkQrjNuvzLx1TXkaU7bM7E1oPXs=
=Vpjp
-----END PGP SIGNATURE-----