[keycloak-user] identity broker role mapping bug?
Marek Posolda
mposolda at redhat.com
Thu Nov 9 03:31:59 EST 2017
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 at redhat.com
> <mailto:mposolda at 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 at gmail.com <mailto:simonpayne58 at 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 at lists.jboss.org
> <mailto:keycloak-user at lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/keycloak-user
> <https://lists.jboss.org/mailman/listinfo/keycloak-user>
>
>
>
>
More information about the keycloak-user
mailing list