8:46 a.m.
Hi, i think i may have found a bug in the identity provider mapping of
claims to roles.
it appears that if i have an identity provider with claims in the token,
which i want to map to a role in the identity broker, then it only does
this once during the first time login. if i remove the claim from the
identity provider token, then this successfully removes it from the broker
- but never remaps if i then add it again.
the scenario i am trying to create here is that the identity provider is
responsible for authentication where active directory groups appears as
claim in the token. the broker then map this claim to the role providing
the authorization.
this behaviour appears to be the same whether i map a broker role to a
custom claim or a realm role in the provider token.
hope this makes sense, thanks
Simon.
4:54 a.m.
further to this, i think i've located the responsible code in the class
org.keycloak.broker.oidc.mappers.ClaimToRoleMapper, which in my opinion
doesn't perform the expected behavior as previously highlighted. I'm
making a couple of assumptions that importNewUser method only gets called
on first broker login and that updateBrokeredUser gets called on every
login thereafter.
we can see in the code below that importNewUser grants the role if the
claim is found - which is fine. However, updateBrokeredUser only removes
if not found in the claim.
I'm proposing that we alter the method updateBrokeredUser to include the
addition of the role as well. I can't see how anyone is currently using
this code in any meaningful way as currently the code doesn't support the
addition of role after first login.
any thoughts? i'm happy to make the changes myself.
Simon.
@Override
public void importNewUser(KeycloakSession session, RealmModel realm,
UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
if (hasClaimValue(mapperModel, context)) {
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new IdentityBrokerException("Unable to
find role: " + roleName);
user.grantRole(role);
}
}
@Override
public void updateBrokeredUser(KeycloakSession session, RealmModel
realm, UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
if (!hasClaimValue(mapperModel, context)) {
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new IdentityBrokerException("Unable to
find role: " + roleName);
user.deleteRoleMapping(role);
}
}
On Wed, Oct 25, 2017 at 2:46 PM, Simon Payne <simonpayne58(a)gmail.com> wrote:
Hi, i think i may have found a bug in the identity provider mapping
of
claims to roles.
it appears that if i have an identity provider with claims in the token,
which i want to map to a role in the identity broker, then it only does
this once during the first time login. if i remove the claim from the
identity provider token, then this successfully removes it from the broker
- but never remaps if i then add it again.
the scenario i am trying to create here is that the identity provider is
responsible for authentication where active directory groups appears as
claim in the token. the broker then map this claim to the role providing
the authorization.
this behaviour appears to be the same whether i map a broker role to a
custom claim or a realm role in the provider token.
hope this makes sense, thanks
Simon.
2:56 a.m.
It makes sense to me. Feel free to create JIRA and ideally also submit
PR with the test.
Just one note: Make sure that "user.grantRole" is called during upgrade
just in case that user doesn't yet have this role. I guess you would
need to add some additional check like:
if (!user.hasRole(role)) {
user.grantRole(role);
}
That's because of performance (Calling of user.grantRole always
invalidates the user in the cache and require to reload him from DB,
which is not needed if he has the role already).
Marek
On 26/10/17 11:54, Simon Payne wrote:
further to this, i think i've located the responsible code in the
class
org.keycloak.broker.oidc.mappers.ClaimToRoleMapper, which in my opinion
doesn't perform the expected behavior as previously highlighted. I'm
making a couple of assumptions that importNewUser method only gets called
on first broker login and that updateBrokeredUser gets called on every
login thereafter.
we can see in the code below that importNewUser grants the role if the
claim is found - which is fine. However, updateBrokeredUser only removes
if not found in the claim.
I'm proposing that we alter the method updateBrokeredUser to include the
addition of the role as well. I can't see how anyone is currently using
this code in any meaningful way as currently the code doesn't support the
addition of role after first login.
any thoughts? i'm happy to make the changes myself.
Simon.
@Override
public void importNewUser(KeycloakSession session, RealmModel realm,
UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
if (hasClaimValue(mapperModel, context)) {
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new IdentityBrokerException("Unable to
find role: " + roleName);
user.grantRole(role);
}
}
@Override
public void updateBrokeredUser(KeycloakSession session, RealmModel
realm, UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
if (!hasClaimValue(mapperModel, context)) {
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new IdentityBrokerException("Unable to
find role: " + roleName);
user.deleteRoleMapping(role);
}
}
On Wed, Oct 25, 2017 at 2:46 PM, Simon Payne <simonpayne58(a)gmail.com> wrote:
> Hi, i think i may have found a bug in the identity provider mapping of
> claims to roles.
>
> it appears that if i have an identity provider with claims in the token,
> which i want to map to a role in the identity broker, then it only does
> this once during the first time login. if i remove the claim from the
> identity provider token, then this successfully removes it from the broker
> - but never remaps if i then add it again.
>
> the scenario i am trying to create here is that the identity provider is
> responsible for authentication where active directory groups appears as
> claim in the token. the broker then map this claim to the role providing
> the authorization.
>
> this behaviour appears to be the same whether i map a broker role to a
> custom claim or a realm role in the provider token.
>
> hope this makes sense, thanks
>
> Simon.
>
>
>
_______________________________________________
keycloak-user mailing list
keycloak-user(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-user
4:30 a.m.
i made the change on our system by rebuilding the keycloak-service at
3.2.1.final jar and deployed separately. The code was changed as follows
and after some testing appears to provide the functionality we need. I can
open a Jira ticket no problem as it would be useful to get it pushed into
the main build, as we have since upgraded to 3.3.0.final and lost the
changes.
@Override
public void importNewUser(KeycloakSession session, RealmModel realm,
UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
mapRole(realm, user, mapperModel, context);
}
@Override
public void updateBrokeredUser(KeycloakSession session, RealmModel
realm, UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
mapRole(realm, user, mapperModel, context);
}
private void mapRole(RealmModel realm, UserModel user,
IdentityProviderMapperModel mapperModel, BrokeredIdentityContext
context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new IdentityBrokerException("Unable to
find role: " + roleName);
if (hasClaimValue(mapperModel, context)) {
user.grantRole(role);
}else{
user.deleteRoleMapping(role);
}
}
On Mon, Nov 6, 2017 at 8:56 AM, Marek Posolda <mposolda(a)redhat.com> wrote:
It makes sense to me. Feel free to create JIRA and ideally also
submit PR
with the test.
Just one note: Make sure that "user.grantRole" is called during upgrade
just in case that user doesn't yet have this role. I guess you would need
to add some additional check like:
if (!user.hasRole(role)) {
user.grantRole(role);
}
That's because of performance (Calling of user.grantRole always
invalidates the user in the cache and require to reload him from DB, which
is not needed if he has the role already).
Marek
On 26/10/17 11:54, Simon Payne wrote:
> further to this, i think i've located the responsible code in the class
> org.keycloak.broker.oidc.mappers.ClaimToRoleMapper, which in my opinion
> doesn't perform the expected behavior as previously highlighted. I'm
> making a couple of assumptions that importNewUser method only gets called
> on first broker login and that updateBrokeredUser gets called on every
> login thereafter.
>
> we can see in the code below that importNewUser grants the role if the
> claim is found - which is fine. However, updateBrokeredUser only removes
> if not found in the claim.
>
> I'm proposing that we alter the method updateBrokeredUser to include the
> addition of the role as well. I can't see how anyone is currently using
> this code in any meaningful way as currently the code doesn't support the
> addition of role after first login.
>
> any thoughts? i'm happy to make the changes myself.
>
> Simon.
>
>
>
> @Override
> public void importNewUser(KeycloakSession session, RealmModel realm,
> UserModel user, IdentityProviderMapperModel mapperModel,
> BrokeredIdentityContext context) {
> String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
> if (hasClaimValue(mapperModel, context)) {
> RoleModel role = KeycloakModelUtils.getRoleFromString(realm,
> roleName);
> if (role == null) throw new IdentityBrokerException("Unable to
> find role: " + roleName);
> user.grantRole(role);
> }
> }
>
> @Override
> public void updateBrokeredUser(KeycloakSession session, RealmModel
> realm, UserModel user, IdentityProviderMapperModel mapperModel,
> BrokeredIdentityContext context) {
> String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
> if (!hasClaimValue(mapperModel, context)) {
> RoleModel role = KeycloakModelUtils.getRoleFromString(realm,
> roleName);
> if (role == null) throw new IdentityBrokerException("Unable to
> find role: " + roleName);
> user.deleteRoleMapping(role);
> }
>
> }
>
>
>
>
> On Wed, Oct 25, 2017 at 2:46 PM, Simon Payne <simonpayne58(a)gmail.com>
> wrote:
>
> Hi, i think i may have found a bug in the identity provider mapping of
>> claims to roles.
>>
>> it appears that if i have an identity provider with claims in the token,
>> which i want to map to a role in the identity broker, then it only does
>> this once during the first time login. if i remove the claim from the
>> identity provider token, then this successfully removes it from the
>> broker
>> - but never remaps if i then add it again.
>>
>> the scenario i am trying to create here is that the identity provider is
>> responsible for authentication where active directory groups appears as
>> claim in the token. the broker then map this claim to the role providing
>> the authorization.
>>
>> this behaviour appears to be the same whether i map a broker role to a
>> custom claim or a realm role in the provider token.
>>
>> hope this makes sense, thanks
>>
>> Simon.
>>
>>
>>
>> _______________________________________________
> keycloak-user mailing list
> keycloak-user(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-user
>
2:31 a.m.
Feel free to create JIRA and send PR. Few points:
- In your changes, I can't see the check I mentioned below for checking
if user is already in that role during upgrade. Again, it will be bad
for performance if we don't do it as "user.grantRole" will be called
during every broker login and will invalidate user from cache, which
will result in additional uneccessary DB requests. You can test with
Hibernate "show_sql" logging turned on to see the DB queries. This may
help to make sure that there are no DB queries during broker login in
case that user is already in that role.
- We will need automated test for the PR as well. Hopefully you can
reuse some existing broker test. Actually we have broker tests in both
old "integration-deprecated" testsuite and "integration-arquillian",
hopefully you are able to find the existing test for this mapper and add
the scenario for update.
Marek
On 08/11/17 11:30, Simon Payne wrote:
i made the change on our system by rebuilding the keycloak-service at
3.2.1.final jar and deployed separately. The code was changed as
follows and after some testing appears to provide the functionality we
need. I can open a Jira ticket no problem as it would be useful to
get it pushed into the main build, as we have since upgraded to
3.3.0.final and lost the changes.
@Override public void importNewUser(KeycloakSession session, RealmModel realm, UserModel
user, IdentityProviderMapperModel mapperModel, BrokeredIdentityContext context) {
mapRole(realm, user, mapperModel, context);
}
@Override public void updateBrokeredUser(KeycloakSession session, RealmModel realm,
UserModel user, IdentityProviderMapperModel mapperModel, BrokeredIdentityContext context)
{
mapRole(realm, user, mapperModel, context);
}
private void mapRole(RealmModel realm, UserModel user, IdentityProviderMapperModel
mapperModel, BrokeredIdentityContext context) {
String roleName = mapperModel.getConfig().get(ConfigConstants.ROLE);
RoleModel role = KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role ==null)throw new IdentityBrokerException("Unable to find role: "
+ roleName);
if (hasClaimValue(mapperModel, context)) {
user.grantRole(role);
}else{
user.deleteRoleMapping(role);
}
}
On Mon, Nov 6, 2017 at 8:56 AM, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@redhat.com>> wrote:
It makes sense to me. Feel free to create JIRA and ideally also
submit PR with the test.
Just one note: Make sure that "user.grantRole" is called during
upgrade just in case that user doesn't yet have this role. I guess
you would need to add some additional check like:
if (!user.hasRole(role)) {
user.grantRole(role);
}
That's because of performance (Calling of user.grantRole always
invalidates the user in the cache and require to reload him from
DB, which is not needed if he has the role already).
Marek
On 26/10/17 11:54, Simon Payne wrote:
further to this, i think i've located the responsible code in
the class
org.keycloak.broker.oidc.mappers.ClaimToRoleMapper, which in
my opinion
doesn't perform the expected behavior as previously
highlighted. I'm
making a couple of assumptions that importNewUser method only
gets called
on first broker login and that updateBrokeredUser gets called
on every
login thereafter.
we can see in the code below that importNewUser grants the
role if the
claim is found - which is fine. However, updateBrokeredUser
only removes
if not found in the claim.
I'm proposing that we alter the method updateBrokeredUser to
include the
addition of the role as well. I can't see how anyone is
currently using
this code in any meaningful way as currently the code doesn't
support the
addition of role after first login.
any thoughts? i'm happy to make the changes myself.
Simon.
@Override
public void importNewUser(KeycloakSession session, RealmModel
realm,
UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName =
mapperModel.getConfig().get(ConfigConstants.ROLE);
if (hasClaimValue(mapperModel, context)) {
RoleModel role =
KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new
IdentityBrokerException("Unable to
find role: " + roleName);
user.grantRole(role);
}
}
@Override
public void updateBrokeredUser(KeycloakSession session, RealmModel
realm, UserModel user, IdentityProviderMapperModel mapperModel,
BrokeredIdentityContext context) {
String roleName =
mapperModel.getConfig().get(ConfigConstants.ROLE);
if (!hasClaimValue(mapperModel, context)) {
RoleModel role =
KeycloakModelUtils.getRoleFromString(realm, roleName);
if (role == null) throw new
IdentityBrokerException("Unable to
find role: " + roleName);
user.deleteRoleMapping(role);
}
}
On Wed, Oct 25, 2017 at 2:46 PM, Simon Payne
<simonpayne58(a)gmail.com <mailto:simonpayne58@gmail.com>> wrote:
Hi, i think i may have found a bug in the identity
provider mapping of
claims to roles.
it appears that if i have an identity provider with claims
in the token,
which i want to map to a role in the identity broker, then
it only does
this once during the first time login. if i remove the
claim from the
identity provider token, then this successfully removes it
from the broker
- but never remaps if i then add it again.
the scenario i am trying to create here is that the
identity provider is
responsible for authentication where active directory
groups appears as
claim in the token. the broker then map this claim to the
role providing
the authorization.
this behaviour appears to be the same whether i map a
broker role to a
custom claim or a realm role in the provider token.
hope this makes sense, thanks
Simon.
_______________________________________________
keycloak-user mailing list
keycloak-user(a)lists.jboss.org
<mailto:keycloak-user@lists.jboss.org>
https://lists.jboss.org/mailman/listinfo/keycloak-user
<https://lists.jboss.org/mailman/listinfo/keycloak-user>
2885
days inactive
2900
days old
4 comments
2 participants
participants (2)
-
Marek Posolda -
Simon Payne