Skip to content

Update composite roles on child role removal #11816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hmlnarik merged 1 commit into from
May 5, 2022
Merged

Update composite roles on child role removal #11816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,7 @@ jobs:
fetch-depth: 2

- name: Check whether HEAD^ contains HotRod storage relevant changes
run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e 'non-existent-folder' )" >> $GITHUB_ENV
# run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e '^model/hot-rod|^model/map|^model/build-processor|^testsuite/model' )" >> $GITHUB_ENV
run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e '^model/hot-rod|^model/map|^model/build-processor|^testsuite/model' )" >> $GITHUB_ENV

- name: Cache Maven packages
if: ${{ github.event_name != 'pull_request' || matrix.server != 'undertow-map-hot-rod' || env.GIT_HOTROD_RELEVANT_DIFF != 0 }}
Expand Down
1 change: 1 addition & 0 deletions ...p-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public void setName(String name) {
@ProtoField(number = 8)
public String clientId;

@ProtoDoc("@Field(index = Index.YES, store = Store.YES)")
@ProtoField(number = 9)
public Set<String> compositeRoles;

Expand Down
10 changes: 10 additions & 0 deletions ...a/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.keycloak.models.map.common.StringKeyConverter.UUIDKey;
import org.keycloak.models.map.storage.CriterionNotSupportedException;
import org.keycloak.models.map.storage.jpa.JpaModelCriteriaBuilder;
import org.keycloak.models.map.storage.jpa.hibernate.jsonb.JsonbType;
import org.keycloak.models.map.storage.jpa.role.entity.JpaRoleEntity;
import org.keycloak.storage.SearchableModelField;

Expand All @@ -61,6 +62,15 @@ public JpaRoleModelCriteriaBuilder compare(SearchableModelField<? super RoleMode
return new JpaRoleModelCriteriaBuilder((cb, root) ->
cb.equal(root.get(modelField.getName()), value[0])
);
} else if (modelField == SearchableFields.COMPOSITE_ROLE){
Copy link
Copy Markdown
Contributor Author

@mhajas mhajas May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized I am using a different comparison method here. I am using == while everywhere else is the equals method call. If I am not mistaken, both should work the way we want. Should I change this to use the equals method as well? Or it is okay to leave this as is?

Copy link
Copy Markdown
Contributor

@vramik vramik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, when thinking about it and reading[1], I believe we should be using == instead of equals. I'd create follow-up task to update it in all other places if we agree on it. I'll bring it up in the mtg.

[1] https://docs.oracle.com/javase/specs/jls/se9/html/jls-8.html#jls-8.9

Copy link
Copy Markdown
Contributor Author

@mhajas mhajas May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with changing it to == as we want to have only one instance of each SearchableField, however, SearchableFields are not enum. That is probably why there was equals method used.

Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for using ==, the fields should be the same instances as the predefined ones.

Copy link
Copy Markdown
Contributor

@vramik vramik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11843

validateValue(value, modelField, op, String.class);

return new JpaRoleModelCriteriaBuilder((cb, root) ->
cb.isTrue(cb.function("@>",
Boolean.TYPE,
cb.function("->", JsonbType.class, root.get("metadata"), cb.literal("fCompositeRoles")),
cb.literal(convertToJson(value[0]))))
Comment on lines +68 to +72
Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vramik @ahus1 Is this search supported by any index?

Copy link
Copy Markdown
Member

@ahus1 ahus1 May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmlnarik - probably not. At the same time I would assume that composite roles might soon move to its own table to follow along the discussion found here to allow adding or removing an entry to the composite roles without loading the rest of the entity: #11799

Depending on where that other PR will move to and how the performance improvements are, I wanted to call for a meeting on that topic to discuss how to handle situation where an attribute can contain a big collection of entries.

Copy link
Copy Markdown
Contributor

@vramik vramik May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmlnarik, that is correct what @ahus1 wrote, there is no index atm to support the search. I can also see the separation composite roles to its own table in near future (probably as a follow-up PR of this one)

Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to keep this as is in this PR. Could you please file a GH issue that points to this discussion?

Copy link
Copy Markdown
Contributor

@vramik vramik May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11844

);
} else {
throw new CriterionNotSupportedException(modelField, op);
}
Expand Down
3 changes: 3 additions & 0 deletions ...src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ public LdapRoleModelCriteriaBuilder compare(SearchableModelField<? super RoleMod
String field = modelFieldNameToLdap(roleMapperConfig, modelField);
return new LdapRoleModelCriteriaBuilder(roleMapperConfig,
() -> equal(field, value[0], LdapMapEscapeStrategy.DEFAULT, false));
} else if (modelField == RoleModel.SearchableFields.COMPOSITE_ROLE) {
// Not supported at the moment
return new LdapRoleModelCriteriaBuilder(roleMapperConfig, StringBuilder::new);
} else {
throw new CriterionNotSupportedException(modelField, op);
}
Expand Down
8 changes: 8 additions & 0 deletions model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,14 @@ public void preRemove(RealmModel realm) {
tx.delete(withCriteria(mcb));
}

public void preRemove(RealmModel realm, RoleModel role) {
// Remove reference from all composite roles
DefaultModelCriteria<RoleModel> mcb = criteria();
mcb = mcb.compare(SearchableFields.REALM_ID, Operator.EQ, realm.getId())
.compare(SearchableFields.COMPOSITE_ROLE, Operator.EQ, role.getId());
tx.read(withCriteria(mcb)).forEach(mapRoleEntity -> mapRoleEntity.removeCompositeRole(role.getId()));
}

@Override
public void close() {
}
Expand Down
3 changes: 3 additions & 0 deletions model/map/src/main/java/org/keycloak/models/map/role/MapRoleProviderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.CLIENT_BEFORE_REMOVE;
import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.REALM_BEFORE_REMOVE;
import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.ROLE_AFTER_REMOVE;
import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.ROLE_BEFORE_REMOVE;

public class MapRoleProviderFactory extends AbstractMapProviderFactory<MapRoleProvider, MapRoleEntity, RoleModel> implements RoleProviderFactory<MapRoleProvider>, InvalidationHandler {

Expand All @@ -51,6 +52,8 @@ public void invalidate(KeycloakSession session, InvalidableObjectType type, Obje
create(session).preRemove((RealmModel) params[0]);
} else if (type == CLIENT_BEFORE_REMOVE) {
create(session).removeRoles((ClientModel) params[1]);
} else if (type == ROLE_BEFORE_REMOVE) {
create(session).preRemove((RealmModel) params[0], (RoleModel) params[1]);
} else if (type == ROLE_AFTER_REMOVE) {
session.getKeycloakSessionFactory().publish(new RoleContainerModel.RoleRemovedEvent() {
@Override public RoleModel getRole() { return (RoleModel) params[1]; }
Expand Down
9 changes: 9 additions & 0 deletions model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public class MapFieldPredicates {
put(ROLE_PREDICATES, RoleModel.SearchableFields.DESCRIPTION, MapRoleEntity::getDescription);
put(ROLE_PREDICATES, RoleModel.SearchableFields.NAME, MapRoleEntity::getName);
put(ROLE_PREDICATES, RoleModel.SearchableFields.IS_CLIENT_ROLE, MapRoleEntity::isClientRole);
put(ROLE_PREDICATES, RoleModel.SearchableFields.COMPOSITE_ROLE, MapFieldPredicates::checkCompositeRoles);

put(USER_PREDICATES, UserModel.SearchableFields.REALM_ID, MapUserEntity::getRealmId);
put(USER_PREDICATES, UserModel.SearchableFields.USERNAME, MapUserEntity::getUsername);
Expand Down Expand Up @@ -326,6 +327,14 @@ private static MapModelCriteriaBuilder<Object, MapClientEntity, ClientModel> che
return mcb.fieldCompare(Boolean.TRUE::equals, getter);
}

private static MapModelCriteriaBuilder<Object, MapRoleEntity, RoleModel> checkCompositeRoles(MapModelCriteriaBuilder<Object, MapRoleEntity, RoleModel> mcb, Operator op, Object[] values) {
String roleIdS = ensureEqSingleValue(RoleModel.SearchableFields.COMPOSITE_ROLE, "composite_role_id", op, values);
Function<MapRoleEntity, ?> getter;
getter = re -> Optional.ofNullable(re.getCompositeRoles()).orElseGet(Collections::emptySet).contains(roleIdS);

return mcb.fieldCompare(Boolean.TRUE::equals, getter);
}

private static MapModelCriteriaBuilder<Object, MapUserEntity, UserModel> checkGrantedUserRole(MapModelCriteriaBuilder<Object, MapUserEntity, UserModel> mcb, Operator op, Object[] values) {
String roleIdS = ensureEqSingleValue(UserModel.SearchableFields.ASSIGNED_ROLE, "role_id", op, values);
Function<MapUserEntity, ?> getter;
Expand Down
1 change: 1 addition & 0 deletions server-spi/src/main/java/org/keycloak/models/RoleModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public static class SearchableFields {
public static final SearchableModelField<RoleModel> NAME = new SearchableModelField<>("name", String.class);
public static final SearchableModelField<RoleModel> DESCRIPTION = new SearchableModelField<>("description", String.class);
public static final SearchableModelField<RoleModel> IS_CLIENT_ROLE = new SearchableModelField<>("isClientRole", Boolean.class);
public static final SearchableModelField<RoleModel> COMPOSITE_ROLE = new SearchableModelField<>("compositeRoles", Boolean.class);
}

String getName();
Expand Down
56 changes: 56 additions & 0 deletions testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.Collection;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
Expand Down Expand Up @@ -227,6 +228,61 @@ public void testSearchRolesByDescription() {
});
}

@Test
public void testCompositeRolesUpdateOnChildRoleRemoval() {
final AtomicReference<String> parentRealmRoleId = new AtomicReference<>();
final AtomicReference<String> parentClientRoleId = new AtomicReference<>();

final AtomicReference<String> childRealmRoleId = new AtomicReference<>();
final AtomicReference<String> childClientRoleId = new AtomicReference<>();

withRealm(realmId, (session, realm) -> {
// Create realm role
RoleModel parentRealmRole = session.roles().addRealmRole(realm, "parentRealmRole");
parentRealmRoleId.set(parentRealmRole.getId());

// Create client role
ClientModel client = session.clients().addClient(realm,"clientWithRole");

RoleModel parentClientRole = session.roles().addClientRole(client, "parentClientRole");
parentClientRoleId.set(parentClientRole.getId());

// Create realm child role
RoleModel childRealmRole = session.roles().addRealmRole(realm, "childRealmRole");
childRealmRoleId.set(childRealmRole.getId());

RoleModel childClientRole = session.roles().addClientRole(client, "childClientRole");
childClientRoleId.set(childClientRole.getId());

// Add composites
parentRealmRole.addCompositeRole(childRealmRole);
parentRealmRole.addCompositeRole(childClientRole);

parentClientRole.addCompositeRole(childRealmRole);
parentClientRole.addCompositeRole(childClientRole);
return null;
});

withRealm(realmId, (session, realm) -> {
RoleModel parentRealmRole = session.roles().getRoleById(realm, parentRealmRoleId.get());
RoleModel parentClientRole = session.roles().getRoleById(realm, parentClientRoleId.get());
assertThat(parentRealmRole.getCompositesStream().collect(Collectors.toSet()), hasSize(2));
assertThat(parentClientRole.getCompositesStream().collect(Collectors.toSet()), hasSize(2));

session.roles().removeRole(session.roles().getRoleById(realm, childRealmRoleId.get()));
session.roles().removeRole(session.roles().getRoleById(realm, childClientRoleId.get()));
return null;
});

withRealm(realmId, (session, realm) -> {
RoleModel parentRealmRole = session.roles().getRoleById(realm, parentRealmRoleId.get());
RoleModel parentClientRole = session.roles().getRoleById(realm, parentClientRoleId.get());
assertThat(parentRealmRole.getCompositesStream().collect(Collectors.toSet()), empty());
assertThat(parentClientRole.getCompositesStream().collect(Collectors.toSet()), empty());
return null;
});
}

public void testRolesWithIdsPaginationSearchQueries(GetResult resultProvider) {
// test all parameters together
List<RoleModel> result = resultProvider.getResult("1", 4, 3);
Expand Down