diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java index f58e90bb687..42abfcfb822 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java @@ -205,7 +205,7 @@ public abstract class EntityRepository { * * @see TableRepository#storeRelationships(Table) for an example implementation */ - public abstract void storeRelationships(T entity); + public abstract void storeRelationships(T entity) throws IOException; /** * PATCH operations can't overwrite certain fields, such as entity ID, fullyQualifiedNames etc. Instead of throwing an @@ -492,8 +492,8 @@ public abstract class EntityRepository { public final DeleteResponse delete(String updatedBy, String id, boolean recursive, boolean hardDelete) throws IOException { - DeleteResponse response = deleteInternal(updatedBy, id, recursive, hardDelete); - postDelete((T) response.getEntity()); + DeleteResponse response = deleteInternal(updatedBy, id, recursive, hardDelete); + postDelete(response.getEntity()); return response; } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java index df22c0926a8..978af8e6a52 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java @@ -286,16 +286,6 @@ public class RoleRepository extends EntityRepository { updatedDefaultRole = updatedDefaultRole.withDefaultRole(false); new RoleUpdater(origDefaultRole, updatedDefaultRole, Operation.PATCH).update(); } - List users = getAllUsers(); - if (users.isEmpty()) { - return; - } - LOG.info("Creating 'user --- has ---> role' relationship for {} role", role.getName()); - for (User user : users) { - daoCollection - .relationshipDAO() - .insert(user.getId(), role.getId(), Entity.USER, Entity.ROLE, Relationship.HAS.ordinal()); - } } private void setDefaultToFalse(Role role) { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java index 45d5243015b..c6b1afb7bfb 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java @@ -15,14 +15,12 @@ package org.openmetadata.catalog.jdbi3; import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; -import com.fasterxml.jackson.core.JsonProcessingException; import java.io.IOException; import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -31,6 +29,7 @@ import org.openmetadata.catalog.Entity; import org.openmetadata.catalog.entity.teams.Team; import org.openmetadata.catalog.entity.teams.User; import org.openmetadata.catalog.exception.CatalogExceptionMessage; +import org.openmetadata.catalog.jdbi3.EntityRepository.EntityUpdater; import org.openmetadata.catalog.resources.teams.UserResource; import org.openmetadata.catalog.type.ChangeDescription; import org.openmetadata.catalog.type.EntityReference; @@ -70,24 +69,13 @@ public class UserRepository extends EntityRepository { /** Ensures that the default roles are added for POST, PUT and PATCH operations. */ @Override public void prepare(User user) throws IOException { - // Get roles assigned to the user. - Set roleIds = listOrEmpty(user.getRoles()).stream().map(EntityReference::getId).collect(Collectors.toSet()); - // Get default role set up globally. - daoCollection.roleDAO().getDefaultRolesIds().forEach(roleIdStr -> roleIds.add(UUID.fromString(roleIdStr))); - - // Assign roles. - List rolesRef = new ArrayList<>(roleIds.size()); - for (UUID roleId : roleIds) { - rolesRef.add(daoCollection.roleDAO().findEntityReferenceById(roleId)); - } - rolesRef.sort(EntityUtil.compareEntityReference); - user.setRoles(rolesRef); + // Role and teams are already validated } @Override public void restorePatchAttributes(User original, User updated) { // Patch can't make changes to following fields. Ignore the changes - updated.withId(original.getId()).withName(original.getName()); + updated.withId(original.getId()).withName(original.getName()).withInheritedRoles(original.getInheritedRoles()); } private List getTeamDefaultRoles(User user) throws IOException { @@ -106,21 +94,23 @@ public class UserRepository extends EntityRepository { public void storeEntity(User user, boolean update) throws IOException { // Relationships and fields such as href are derived and not stored as part of json List roles = user.getRoles(); + List inheritedRoles = user.getInheritedRoles(); List teams = user.getTeams(); // Don't store roles, teams and href as JSON. Build it on the fly based on relationships - user.withRoles(null).withTeams(null).withHref(null); + user.withRoles(null).withTeams(null).withHref(null).withInheritedRoles(null); store(user.getId(), user, update); // Restore the relationships - user.withRoles(roles).withTeams(teams); + user.withRoles(roles).withTeams(teams).withInheritedRoles(inheritedRoles); } @Override - public void storeRelationships(User user) { + public void storeRelationships(User user) throws IOException { assignRoles(user, user.getRoles()); assignTeams(user, user.getTeams()); + user.setInheritedRoles(getInheritedRoles(user)); } @Override @@ -138,10 +128,10 @@ public class UserRepository extends EntityRepository { public User setFields(User user, Fields fields) throws IOException { user.setProfile(fields.contains("profile") ? user.getProfile() : null); user.setTeams(fields.contains("teams") ? getTeams(user) : null); - user.setRoles(fields.contains("roles") ? getRoles(user) : null); user.setOwns(fields.contains("owns") ? getOwns(user) : null); user.setFollows(fields.contains("follows") ? getFollows(user) : null); - return user; + user.setRoles(fields.contains("roles") ? getRoles(user) : null); + return user.withInheritedRoles(fields.contains("roles") ? getInheritedRoles(user) : null); } public boolean isTeamJoinable(String teamId) throws IOException { @@ -202,10 +192,20 @@ public class UserRepository extends EntityRepository { return validatedTeams; } - /* Add all the roles that user has been assigned, to User entity */ + private List getDefaultRole() throws IOException { + List defaultRoleIds = daoCollection.roleDAO().getDefaultRolesIds(); + return EntityUtil.populateEntityReferences(defaultRoleIds, Entity.ROLE); + } + + /* Add all the roles that user has been assigned and inherited from the team to User entity */ private List getRoles(User user) throws IOException { List roleIds = findTo(user.getId(), Entity.USER, Relationship.HAS, Entity.ROLE); - List roles = EntityUtil.populateEntityReferences(roleIds, Entity.ROLE); + return EntityUtil.populateEntityReferences(roleIds, Entity.ROLE); + } + + /* Add all the roles that user has been assigned and inherited from the team to User entity */ + private List getInheritedRoles(User user) throws IOException { + List roles = getDefaultRole(); roles.addAll(getTeamDefaultRoles(user)); return roles.stream().distinct().collect(Collectors.toList()); // Remove duplicates } @@ -353,13 +353,19 @@ public class UserRepository extends EntityRepository { @Override public void entitySpecificUpdate() throws IOException { - updateRoles(original.getEntity(), updated.getEntity()); - updateTeams(original.getEntity(), updated.getEntity()); - recordChange("profile", original.getEntity().getProfile(), updated.getEntity().getProfile(), true); - recordChange("timezone", original.getEntity().getTimezone(), updated.getEntity().getTimezone()); - recordChange("isBot", original.getEntity().getIsBot(), updated.getEntity().getIsBot()); - recordChange("isAdmin", original.getEntity().getIsAdmin(), updated.getEntity().getIsAdmin()); - recordChange("email", original.getEntity().getEmail(), updated.getEntity().getEmail()); + User origUser = original.getEntity(); + User updatedUser = updated.getEntity(); + + updateRoles(origUser, updatedUser); + updateTeams(origUser, updatedUser); + recordChange("profile", origUser.getProfile(), updatedUser.getProfile(), true); + recordChange("timezone", origUser.getTimezone(), updatedUser.getTimezone()); + recordChange("isBot", origUser.getIsBot(), updatedUser.getIsBot()); + recordChange("isAdmin", origUser.getIsAdmin(), updatedUser.getIsAdmin()); + recordChange("email", origUser.getEmail(), updatedUser.getEmail()); + + // Add inherited roles to the entity after update + updatedUser.setInheritedRoles(getInheritedRoles(updatedUser)); } private void updateRoles(User origUser, User updatedUser) throws IOException { @@ -378,7 +384,7 @@ public class UserRepository extends EntityRepository { recordListChange("roles", origRoles, updatedRoles, added, deleted, EntityUtil.entityReferenceMatch); } - private void updateTeams(User origUser, User updatedUser) throws JsonProcessingException { + private void updateTeams(User origUser, User updatedUser) throws IOException { // Remove teams from original and add teams from updated deleteTo(origUser.getId(), Entity.USER, Relationship.HAS, Entity.TEAM); assignTeams(updatedUser, updatedUser.getTeams()); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/RoleResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/RoleResource.java index 17f4982a056..fa2de1381ff 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/RoleResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/RoleResource.java @@ -177,7 +177,7 @@ public class RoleResource extends EntityResource { ResultList roles; if (defaultParam) { - // The number of default roles is usually 1, and hence does not require pagination. + // The number of default roles is 1, and hence does not require pagination. roles = dao.getDefaultRolesResultList(uriInfo, fields); } else if (before != null) { // Reverse paging roles = dao.listBefore(uriInfo, fields, filter, limitParam, before); // Ask for one extra entry diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java index 729fe349bf5..59623667c1d 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java @@ -82,6 +82,7 @@ public class UserResource extends EntityResource { public User addHref(UriInfo uriInfo, User user) { Entity.withHref(uriInfo, user.getTeams()); Entity.withHref(uriInfo, user.getRoles()); + Entity.withHref(uriInfo, user.getInheritedRoles()); Entity.withHref(uriInfo, user.getOwns()); Entity.withHref(uriInfo, user.getFollows()); return user; diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java index 55b76deea16..b433cdfc651 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java @@ -14,8 +14,10 @@ package org.openmetadata.catalog.security; import static org.openmetadata.catalog.Entity.FIELD_OWNER; +import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -119,9 +121,10 @@ public class DefaultAuthorizer implements Authorizer { EntityRepository userRepository = Entity.getEntityRepository(Entity.USER); Fields fieldsRolesAndTeams = userRepository.getFields("roles, teams"); User user = getUserFromAuthenticationContext(ctx, fieldsRolesAndTeams); + List allRoles = getAllRoles(user); if (entityReference == null) { // In some cases there is no specific entity being acted upon. Eg: Lineage. - return RoleEvaluator.getInstance().hasPermissions(user.getRoles(), null, operation); + return RoleEvaluator.getInstance().hasPermissions(allRoles, null, operation); } Object entity = Entity.getEntity(entityReference, new Fields(List.of("tags", FIELD_OWNER)), Include.NON_DELETED); @@ -130,7 +133,7 @@ public class DefaultAuthorizer implements Authorizer { if (Entity.shouldHaveOwner(entityReference.getType()) && owner != null && isOwnedByUser(user, owner)) { return true; // Entity is owned by the user. } - return RoleEvaluator.getInstance().hasPermissions(user.getRoles(), entity, operation); + return RoleEvaluator.getInstance().hasPermissions(allRoles, entity, operation); } catch (IOException | EntityNotFoundException ex) { return false; } @@ -149,8 +152,9 @@ public class DefaultAuthorizer implements Authorizer { EntityRepository userRepository = Entity.getEntityRepository(Entity.USER); Fields fieldsRolesAndTeams = userRepository.getFields("roles, teams"); User user = getUserFromAuthenticationContext(ctx, fieldsRolesAndTeams); + List allRoles = getAllRoles(user); if (entityReference == null) { - return RoleEvaluator.getInstance().getAllowedOperations(user.getRoles(), null); + return RoleEvaluator.getInstance().getAllowedOperations(allRoles, null); } Object entity = Entity.getEntity(entityReference, new Fields(List.of("tags", FIELD_OWNER)), Include.NON_DELETED); EntityReference owner = Entity.getEntityInterface(entity).getOwner(); @@ -158,7 +162,7 @@ public class DefaultAuthorizer implements Authorizer { // Entity does not have an owner or is owned by the user - allow all operations. return Stream.of(MetadataOperation.values()).collect(Collectors.toList()); } - return RoleEvaluator.getInstance().getAllowedOperations(user.getRoles(), entity); + return RoleEvaluator.getInstance().getAllowedOperations(allRoles, entity); } catch (IOException | EntityNotFoundException ex) { return Collections.emptyList(); } @@ -227,7 +231,7 @@ public class DefaultAuthorizer implements Authorizer { EntityRepository userRepository = Entity.getEntityRepository(Entity.USER); if (ctx.getUser() != null) { // If a requested field is not present in the user, then add it - for (String field : fields.getList()) { + for (String field : fields.getFieldList()) { if (!ctx.getUserFields().contains(field)) { userRepository.setFields(ctx.getUser(), userRepository.getFields(field)); ctx.getUserFields().add(fields); @@ -253,4 +257,10 @@ public class DefaultAuthorizer implements Authorizer { LOG.debug("User entry: {} already exists.", user); } } + + private List getAllRoles(User user) { + List allRoles = new ArrayList<>(listOrEmpty(user.getRoles())); + allRoles.addAll(listOrEmpty(user.getInheritedRoles())); + return allRoles.stream().distinct().collect(Collectors.toList()); // Remove duplicates + } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java index 0370ff19cde..1e0261c39cb 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java @@ -29,6 +29,7 @@ import java.util.function.BiPredicate; import java.util.regex.Pattern; import java.util.stream.Collectors; import javax.ws.rs.WebApplicationException; +import lombok.Getter; import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -233,7 +234,7 @@ public final class EntityUtil { @RequiredArgsConstructor public static class Fields { public static final Fields EMPTY_FIELDS = new Fields(null, null); - private final List fieldList; + @Getter private final List fieldList; public Fields(List allowedFields, String fieldsParam) { if (nullOrEmpty(fieldsParam)) { @@ -260,10 +261,6 @@ public final class EntityUtil { public boolean contains(String field) { return fieldList.contains(field); } - - public List getList() { - return fieldList; - } } public static List getIDList(List refList) { diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/teams/role.json b/catalog-rest-service/src/main/resources/json/schema/entity/teams/role.json index 339d4a45d3e..57d7328bfc7 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/teams/role.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/teams/role.json @@ -55,7 +55,7 @@ "$ref": "../../type/entityReference.json#/definitions/entityReferenceList" }, "defaultRole": { - "description": "If `true`, this role is set as default role and will be assigned to all users.", + "description": "If `true`, this role is set as default system role and will be inherited by all the users.", "type": "boolean", "default": false }, diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/teams/team.json b/catalog-rest-service/src/main/resources/json/schema/entity/teams/team.json index 259d232c0b2..feb1d760b8b 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/teams/team.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/teams/team.json @@ -69,7 +69,7 @@ "default": false }, "defaultRoles": { - "description": "Roles to be assigned to all users that are part of this team.", + "description": "Default roles of a team. These roles will be inherited by all the users that are part of this team.", "$ref": "../../type/entityReference.json#/definitions/entityReferenceList" } }, diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/teams/user.json b/catalog-rest-service/src/main/resources/json/schema/entity/teams/user.json index 2acb0a912c8..7d3b863ad01 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/teams/user.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/teams/user.json @@ -84,6 +84,10 @@ "roles": { "description": "Roles that the user has been assigned.", "$ref": "../../type/entityReference.json#/definitions/entityReferenceList" + }, + "inheritedRoles": { + "description": "Roles that a user is inheriting either as part of system default role or through membership in teams that have set team default roles.", + "$ref": "../../type/entityReference.json#/definitions/entityReferenceList" } }, "additionalProperties": false, diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java index f5878a25885..ccd19c4ad68 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java @@ -1426,6 +1426,7 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { } } + /** Helper function to generate JSON PATCH, submit PATCH API request and validate response. */ protected final T patchEntityAndCheck( T updated, String originalJson, @@ -1433,22 +1434,10 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { UpdateType updateType, ChangeDescription expectedChange) throws IOException { - return patchEntityAndCheck(updated, originalJson, authHeaders, updateType, expectedChange, updated); - } - - /** Helper function to generate JSON PATCH, submit PATCH API request and validate response. */ - protected final T patchEntityAndCheck( - T updated, - String originalJson, - Map authHeaders, - UpdateType updateType, - ChangeDescription expectedChange, - T update) - throws IOException { EntityInterface entityInterface = getEntityInterface(updated); // Validate information returned in patch response has the updates - T returned = patchEntity(entityInterface.getId(), originalJson, update, authHeaders); + T returned = patchEntity(entityInterface.getId(), originalJson, updated, authHeaders); entityInterface = getEntityInterface(returned); compareEntities(updated, returned, authHeaders); diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java index 236e8eb204a..e9d0c738627 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java @@ -96,14 +96,15 @@ public class RoleResourceTest extends EntityResourceTest { for (User user : users) { UUID prevDefaultRoleId = prevDefaultRole.getId(); boolean prevDefaultRoleExists = - user.getRoles().stream().anyMatch(role -> role.getId().equals(prevDefaultRoleId)); + user.getInheritedRoles().stream().anyMatch(role -> role.getId().equals(prevDefaultRoleId)); if (prevDefaultRoleExists) { fail( String.format( "Previous default role %s has not been removed for user %s", prevDefaultRole.getName(), user.getName())); } - boolean defaultRoleExists = user.getRoles().stream().anyMatch(role -> role.getId().equals(defaultRole.getId())); + boolean defaultRoleExists = + user.getInheritedRoles().stream().anyMatch(role -> role.getId().equals(defaultRole.getId())); if (!defaultRoleExists) { fail(String.format("Default role %s was not set for user %s", defaultRole.getName(), user.getName())); } @@ -211,7 +212,6 @@ public class RoleResourceTest extends EntityResourceTest { validateRole(role, role.getDescription(), role.getDisplayName(), updatedBy); assertListNull(role.getPolicies(), role.getUsers()); - // .../roles?fields=policy,users String fields = "policies,teams,users"; role = byName diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java index ff24487d11a..793cb4dfbea 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java @@ -55,8 +55,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; import java.util.function.Predicate; -import java.util.stream.Collectors; -import javax.json.JsonPatch; +import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.apache.http.client.HttpResponseException; import org.junit.jupiter.api.MethodOrderer; @@ -64,7 +63,6 @@ import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.api.TestMethodOrder; -import org.openmetadata.catalog.CatalogApplicationTest; import org.openmetadata.catalog.Entity; import org.openmetadata.catalog.api.teams.CreateUser; import org.openmetadata.catalog.entity.data.Table; @@ -142,93 +140,103 @@ public class UserResourceTest extends EntityResourceTest { @Order(Integer.MAX_VALUE) // Run this test last to avoid side effects of default role creation to fail other tests. @Test void userDefaultRoleAssignment(TestInfo test) throws IOException { - // Given a global default role has been set, ... + // Find a nonDefault role RoleResourceTest roleResourceTest = new RoleResourceTest(); List roles = roleResourceTest.listEntities(Collections.emptyMap(), ADMIN_AUTH_HEADERS).getData(); - UUID nonDefaultRoleId = roles.stream().filter(role -> !role.getDefaultRole()).findAny().orElseThrow().getId(); - UUID defaultRoleId = - roles.stream() - .filter(Role::getDefaultRole) - .findAny() - .orElseThrow() - .getId(); // DataConsumer is global default role. + Role nonDefaultRole = roles.stream().filter(role -> !role.getDefaultRole()).findAny().orElseThrow(); + EntityReference nonDefaultRoleRef = new RoleEntityInterface(nonDefaultRole).getEntityReference(); + + // Find a default role + EntityReference defaultRoleRef = getDefaultRole(); + assertNotNull(defaultRoleRef); // ... when a user is created without any roles, then the global default role should be assigned. CreateUser create = createRequest(test, 1); - User user1 = createUserAndCheckRoles(create, Arrays.asList(defaultRoleId)); + User user1 = createUserAndCheckRoles(create, Collections.emptyList(), Arrays.asList(defaultRoleRef)); - // ... when a user is created with a non default role, then the global default role should be assigned along with - // the non default role. - create = createRequest(test, 2).withRoles(List.of(nonDefaultRoleId)); - User user2 = createUserAndCheckRoles(create, Arrays.asList(nonDefaultRoleId, defaultRoleId)); + // ... when a user is created with a non default role, then the global default role + // should be assigned along with the non default role. + create = createRequest(test, 2).withRoles(List.of(nonDefaultRole.getId())); + User user2 = createUserAndCheckRoles(create, Arrays.asList(nonDefaultRoleRef), Arrays.asList(defaultRoleRef)); // ... when a user is created with both global default and non-default role, then both roles should be assigned. - create = createRequest(test, 3).withRoles(List.of(nonDefaultRoleId, defaultRoleId)); - User user3 = createUserAndCheckRoles(create, Arrays.asList(nonDefaultRoleId, defaultRoleId)); + create = createRequest(test, 3).withRoles(List.of(nonDefaultRole.getId(), defaultRoleRef.getId())); + User user3 = + createUserAndCheckRoles( + create, Arrays.asList(nonDefaultRoleRef, defaultRoleRef), Arrays.asList(defaultRoleRef)); - // Given the default role has changed, ... - UUID prevDefaultRoleId = defaultRoleId; - Role defaultRole = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1000), ADMIN_AUTH_HEADERS); - String defaultRoleJson = JsonUtils.pojoToJson(defaultRole); - defaultRole.setDefaultRole(true); - defaultRoleId = defaultRole.getId(); // New global default role. - assertNotEquals(prevDefaultRoleId, defaultRoleId); - roleResourceTest.patchEntity(defaultRoleId, defaultRoleJson, defaultRole, ADMIN_AUTH_HEADERS); + // Change the default roles and make sure the change is reflected in all users + Role newDefaultRole = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1000), ADMIN_AUTH_HEADERS); + EntityReference newDefaultRoleRef = new RoleEntityInterface(newDefaultRole).getEntityReference(); + String defaultRoleJson = JsonUtils.pojoToJson(newDefaultRole); + newDefaultRole.setDefaultRole(true); + roleResourceTest.patchEntity(newDefaultRole.getId(), defaultRoleJson, newDefaultRole, ADMIN_AUTH_HEADERS); // ... when user1 exists, then ensure that the default role changed. user1 = getEntity(user1.getId(), ADMIN_AUTH_HEADERS); - assertRoles(user1, Arrays.asList(defaultRoleId)); + assertRoles(user1, Collections.emptyList(), Arrays.asList(newDefaultRoleRef)); // ... when user2 exists, then ensure that the default role changed. user2 = getEntity(user2.getId(), ADMIN_AUTH_HEADERS); - assertRoles(user2, Arrays.asList(nonDefaultRoleId, defaultRoleId)); + assertRoles(user2, Arrays.asList(nonDefaultRoleRef), Arrays.asList(newDefaultRoleRef)); // ... when user3 exists, then ensure that the default role changed. user3 = getEntity(user3.getId(), ADMIN_AUTH_HEADERS); - assertRoles(user3, Arrays.asList(nonDefaultRoleId, defaultRoleId)); + assertRoles(user3, Arrays.asList(nonDefaultRoleRef), Arrays.asList(newDefaultRoleRef)); - Role role1 = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1001), ADMIN_AUTH_HEADERS); - Role role2 = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1002), ADMIN_AUTH_HEADERS); + // Create team1 with defaultRole role1 + Role role1 = + roleResourceTest.createEntity( + roleResourceTest.createRequest("teamDefaultRole1", "", "", null), ADMIN_AUTH_HEADERS); + EntityReference role1Ref = new RoleEntityInterface(role1).getEntityReference(); TeamResourceTest teamResourceTest = new TeamResourceTest(); Team team1 = teamResourceTest.createEntity( teamResourceTest.createRequest(test, 1).withDefaultRoles(List.of(role1.getId())), ADMIN_AUTH_HEADERS); + EntityReference team1Ref = new TeamEntityInterface(team1).getEntityReference(); + + // Create team2 with defaultRole role2 + Role role2 = + roleResourceTest.createEntity( + roleResourceTest.createRequest("teamDefaultRole2", "", "", null), ADMIN_AUTH_HEADERS); + EntityReference role2Ref = new RoleEntityInterface(role2).getEntityReference(); + Team team2 = teamResourceTest.createEntity( teamResourceTest.createRequest(test, 2).withDefaultRoles(List.of(role1.getId(), role2.getId())), ADMIN_AUTH_HEADERS); + EntityReference team2Ref = new TeamEntityInterface(team2).getEntityReference(); - // Given user1 is not part of any team and have no roles assigned, when user1 joins team1, then user1 gets assigned - // the global default role and the team1 default role, role1. + // user1 has defaultRole + // Add user1 to team1 to inherit default of roles of team1(role1) String originalUser1 = JsonUtils.pojoToJson(user1); - user1.setTeams(List.of(new TeamEntityInterface(team1).getEntityReference())); - user1 = patchUser(originalUser1, user1, ADMIN_AUTH_HEADERS); - assertRoles(user1, Arrays.asList(defaultRoleId, role1.getId())); + user1.setTeams(List.of(team1Ref)); + user1 = patchEntity(user1.getId(), originalUser1, user1, ADMIN_AUTH_HEADERS); + assertRoles(user1, Collections.emptyList(), Arrays.asList(newDefaultRoleRef, role1Ref)); - // Given user1 is part of team1, when user1 joins team2, then user1 gets assigned the global default role, the - // team1 default role, role1 and team2 default role, role2. + // user1 now has default role and role1 (through team1) + // Add user1 to team2 to inherit default of roles team1 (role1 and role2) originalUser1 = JsonUtils.pojoToJson(user1); - user1.setTeams( - List.of( - new TeamEntityInterface(team1).getEntityReference(), new TeamEntityInterface(team2).getEntityReference())); - user1 = patchUser(originalUser1, user1, ADMIN_AUTH_HEADERS); - assertRoles(user1, Arrays.asList(defaultRoleId, role1.getId(), role2.getId())); + user1.setTeams(List.of(team1Ref, team2Ref)); + user1 = patchEntity(user1.getId(), originalUser1, user1, ADMIN_AUTH_HEADERS); + assertRoles(user1, Collections.emptyList(), Arrays.asList(newDefaultRoleRef, role1Ref, role2Ref)); - // Given user2 has a non default role assigned, when user2 joins team2, then user2 should get assigned the global - // default role, team2 default roles, role1 and role2, and retain its non-default role. + // user2 has defaultRole and nonDefaultRole + // Add user2 to team2 and inherit default roles role1 and role2 of team2 String originalUser2 = JsonUtils.pojoToJson(user2); - user2.setTeams(List.of(new TeamEntityInterface(team2).getEntityReference())); - user2 = patchUser(originalUser2, user2, ADMIN_AUTH_HEADERS); - assertRoles(user2, Arrays.asList(defaultRoleId, role1.getId(), role2.getId(), nonDefaultRoleId)); + user2.setTeams(List.of(team2Ref)); + user2 = patchEntity(user2.getId(), originalUser2, user2, ADMIN_AUTH_HEADERS); + assertRoles(user2, Arrays.asList(nonDefaultRoleRef), Arrays.asList(newDefaultRoleRef, role1Ref, role2Ref)); // Given user2 has a non default role assigned, when user2 leaves team2, then user2 should get assigned the global // default role and retain its non-default role. - // To be fixed in https://github.com/open-metadata/OpenMetadata/issues/2969 - // originalUser2 = JsonUtils.pojoToJson(user2); - // user2.setTeams(null); - // user2 = patchUser(originalUser2, user2, ADMIN_AUTH_HEADERS); - // assertRoles(user2, Arrays.asList(defaultRoleId, nonDefaultRoleId)); + originalUser2 = JsonUtils.pojoToJson(user2); + ChangeDescription change = getChangeDescription(user2.getVersion()); + change.getFieldsDeleted().add(new FieldChange().withName("teams").withOldValue(List.of(team2Ref))); + user2.setTeams(null); + user2 = patchEntityAndCheck(user2, originalUser2, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + assertRoles(user2, Arrays.asList(nonDefaultRoleRef), Arrays.asList(newDefaultRoleRef)); } @Test @@ -279,7 +287,7 @@ public class UserResourceTest extends EntityResourceTest { } @Test - void put_validUser_200_ok(TestInfo test) throws IOException { + void put_validUser_200_ok() throws IOException { // Create user with different optional fields CreateUser create = createRequest("user.xyz", null, null, null); User user = updateAndCheckEntity(create, CREATED, ADMIN_AUTH_HEADERS, UpdateType.CREATED, null); @@ -342,19 +350,9 @@ public class UserResourceTest extends EntityResourceTest { RoleResourceTest roleResourceTest = new RoleResourceTest(); Role role1 = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS); Role role2 = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS); - UUID defaultRoleId = - roleResourceTest.listEntities(Collections.emptyMap(), ADMIN_AUTH_HEADERS).getData().stream() - .filter(Role::getDefaultRole) - .findAny() - .orElseThrow() - .getId(); // DataConsumer is default role. - EntityReference defaultRoleRef = - new RoleEntityInterface(roleResourceTest.getEntity(defaultRoleId, RoleResource.FIELDS, ADMIN_AUTH_HEADERS)) - .getEntityReference(); - List roles = Arrays.asList(role1.getId(), role2.getId()); CreateUser create = createRequest(test).withRoles(roles); - List createdRoles = Arrays.asList(role1.getId(), role2.getId(), defaultRoleRef.getId()); + List createdRoles = Arrays.asList(role1.getId(), role2.getId()); CreateUser created = createRequest(test).withRoles(createdRoles); User user = createAndCheckEntity(create, ADMIN_AUTH_HEADERS, created); @@ -500,7 +498,9 @@ public class UserResourceTest extends EntityResourceTest { String userJson = JsonUtils.pojoToJson(user); user.setDisplayName("newName"); assertResponse( - () -> patchUser(userJson, user, authHeaders("test100@email.com")), FORBIDDEN, noPermission("test100")); + () -> patchEntity(user.getId(), userJson, user, authHeaders("test100@email.com")), + FORBIDDEN, + noPermission("test100")); } @Test @@ -513,7 +513,7 @@ public class UserResourceTest extends EntityResourceTest { String userJson = JsonUtils.pojoToJson(user); user.setIsAdmin(Boolean.TRUE); Map authHeaders = authHeaders("test100@email.com"); - assertResponse(() -> patchUser(userJson, user, authHeaders), FORBIDDEN, notAdmin("test100")); + assertResponse(() -> patchEntity(user.getId(), userJson, user, authHeaders), FORBIDDEN, notAdmin("test100")); } @Test @@ -526,7 +526,7 @@ public class UserResourceTest extends EntityResourceTest { String userJson = JsonUtils.pojoToJson(user); String newDisplayName = "newDisplayName"; user.setDisplayName(newDisplayName); // Update the name - user = patchUser(userJson, user, ADMIN_AUTH_HEADERS); // Patch the user + user = patchEntity(user.getId(), userJson, user, ADMIN_AUTH_HEADERS); // Patch the user assertEquals(newDisplayName, user.getDisplayName()); } @@ -553,12 +553,6 @@ public class UserResourceTest extends EntityResourceTest { Profile profile = new Profile().withImages(new ImageList().withImage(URI.create("http://image.com"))); RoleResourceTest roleResourceTest = new RoleResourceTest(); - List roles = roleResourceTest.listEntities(Collections.emptyMap(), ADMIN_AUTH_HEADERS).getData(); - UUID defaultRoleId = - roles.stream().filter(Role::getDefaultRole).findAny().orElseThrow().getId(); // DataConsumer is default role. - EntityReference defaultRoleRef = - new RoleEntityInterface(roleResourceTest.getEntity(defaultRoleId, RoleResource.FIELDS, ADMIN_AUTH_HEADERS)) - .getEntityReference(); EntityReference role1 = new RoleEntityInterface( roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS)) @@ -570,23 +564,13 @@ public class UserResourceTest extends EntityResourceTest { String origJson = JsonUtils.pojoToJson(user); String timezone = "America/Los_Angeles"; - user.withRoles(Arrays.asList(role1, defaultRoleRef)) + user.withRoles(Arrays.asList(role1)) .withTeams(teams) .withTimezone(timezone) .withDisplayName("displayName") .withProfile(profile) .withIsBot(false) .withIsAdmin(false); - User update = - JsonUtils.readValue(origJson, User.class) - .withRoles(Arrays.asList(role1)) - .withTeams(teams) - .withTimezone(timezone) - .withDisplayName("displayName") - .withProfile(profile) - .withIsBot(false) - .withIsAdmin(false); - ChangeDescription change = getChangeDescription(user.getVersion()); change.getFieldsAdded().add(new FieldChange().withName("roles").withNewValue(Arrays.asList(role1))); change.getFieldsAdded().add(new FieldChange().withName("teams").withNewValue(teams)); @@ -594,7 +578,7 @@ public class UserResourceTest extends EntityResourceTest { change.getFieldsAdded().add(new FieldChange().withName("displayName").withNewValue("displayName")); change.getFieldsAdded().add(new FieldChange().withName("profile").withNewValue(profile)); change.getFieldsAdded().add(new FieldChange().withName("isBot").withNewValue(false)); - user = patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change, update); + user = patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); // // Replace the attributes @@ -609,22 +593,13 @@ public class UserResourceTest extends EntityResourceTest { .getEntityReference(); origJson = JsonUtils.pojoToJson(user); - user.withRoles(Arrays.asList(role2, defaultRoleRef)) + user.withRoles(Arrays.asList(role2)) .withTeams(teams1) .withTimezone(timezone1) .withDisplayName("displayName1") .withProfile(profile1) .withIsBot(true) .withIsAdmin(false); - update = - JsonUtils.readValue(origJson, User.class) - .withRoles(Arrays.asList(role2)) - .withTeams(teams1) - .withTimezone(timezone1) - .withDisplayName("displayName1") - .withProfile(profile1) - .withIsBot(true) - .withIsAdmin(false); change = getChangeDescription(user.getVersion()); change.getFieldsDeleted().add(new FieldChange().withName("roles").withOldValue(Arrays.asList(role1))); @@ -639,28 +614,19 @@ public class UserResourceTest extends EntityResourceTest { .add(new FieldChange().withName("displayName").withOldValue("displayName").withNewValue("displayName1")); change.getFieldsUpdated().add(new FieldChange().withName("profile").withOldValue(profile).withNewValue(profile1)); change.getFieldsUpdated().add(new FieldChange().withName("isBot").withOldValue(false).withNewValue(true)); - user = patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change, update); + user = patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); // // Remove the attributes // origJson = JsonUtils.pojoToJson(user); - user.withRoles(Arrays.asList(defaultRoleRef)) + user.withRoles(null) .withTeams(null) .withTimezone(null) .withDisplayName(null) .withProfile(null) .withIsBot(null) .withIsAdmin(false); - update = - JsonUtils.readValue(origJson, User.class) - .withRoles(null) - .withTeams(null) - .withTimezone(null) - .withDisplayName(null) - .withProfile(null) - .withIsBot(null) - .withIsAdmin(false); // Note non-empty display field is not deleted change = getChangeDescription(user.getVersion()); @@ -670,7 +636,7 @@ public class UserResourceTest extends EntityResourceTest { change.getFieldsDeleted().add(new FieldChange().withName("displayName").withOldValue("displayName1")); change.getFieldsDeleted().add(new FieldChange().withName("profile").withOldValue(profile1)); change.getFieldsDeleted().add(new FieldChange().withName("isBot").withOldValue(true).withNewValue(null)); - patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change, update); + patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); } @Test @@ -703,30 +669,21 @@ public class UserResourceTest extends EntityResourceTest { entityNotFound("user", user.getId())); } - private User createUserAndCheckRoles(CreateUser create, List expectedRolesIds) throws HttpResponseException { + @SneakyThrows + private User createUserAndCheckRoles( + CreateUser create, List expectedRoles, List expectedInheritedRoles) { User user = createEntity(create, ADMIN_AUTH_HEADERS); - assertRoles(user, expectedRolesIds); + assertRoles(user, expectedRoles, expectedInheritedRoles); + + user = getEntity(user.getId(), "roles", ADMIN_AUTH_HEADERS); + assertRoles(user, expectedRoles, expectedInheritedRoles); return user; } - private void assertRoles(User user, List expectedRolesIds) throws HttpResponseException { - user = getEntity(user.getId(), ADMIN_AUTH_HEADERS); - List actualRolesIds = - user.getRoles().stream().map(EntityReference::getId).sorted().collect(Collectors.toList()); - Collections.sort(expectedRolesIds); - assertEquals(expectedRolesIds, actualRolesIds); - } - - private User patchUser(UUID userId, String originalJson, User updated, Map headers) - throws JsonProcessingException, HttpResponseException { - String updatedJson = JsonUtils.pojoToJson(updated); - JsonPatch patch = JsonUtils.getJsonPatch(originalJson, updatedJson); - return TestUtils.patch(CatalogApplicationTest.getResource("users/" + userId), patch, User.class, headers); - } - - private User patchUser(String originalJson, User updated, Map headers) - throws JsonProcessingException, HttpResponseException { - return patchUser(updated.getId(), originalJson, updated, headers); + private void assertRoles( + User user, List expectedRoles, List expectedInheritedRoles) { + assertEntityReferenceList(expectedRoles, user.getRoles()); + assertEntityReferenceList(expectedInheritedRoles, user.getInheritedRoles()); } @Override @@ -787,7 +744,8 @@ public class UserResourceTest extends EntityResourceTest { } @Override - public void validateCreatedEntity(User user, CreateUser createRequest, Map authHeaders) { + public void validateCreatedEntity(User user, CreateUser createRequest, Map authHeaders) + throws HttpResponseException { validateCommonEntityFields( getEntityInterface(user), createRequest.getDescription(), @@ -804,12 +762,7 @@ public class UserResourceTest extends EntityResourceTest { for (UUID roleId : listOrEmpty(createRequest.getRoles())) { expectedRoles.add(new EntityReference().withId(roleId).withType(Entity.ROLE)); } - if (expectedRoles.isEmpty()) { - // Default role of data consumer is added when roles are not assigned to a user during create - TestUtils.assertEntityReferenceList(List.of(DATA_CONSUMER_ROLE_REFERENCE), user.getRoles()); - } else { - TestUtils.assertEntityReferenceList(expectedRoles, user.getRoles()); - } + assertRoles(user, expectedRoles, List.of(DATA_CONSUMER_ROLE_REFERENCE)); List expectedTeams = new ArrayList<>(); for (UUID teamId : listOrEmpty(createRequest.getTeams())) { @@ -867,4 +820,13 @@ public class UserResourceTest extends EntityResourceTest { assertCommonFieldChange(fieldName, expected, actual); } } + + private EntityReference getDefaultRole() throws HttpResponseException { + RoleResourceTest roleResourceTest = new RoleResourceTest(); + List roles = roleResourceTest.getDefaultRoles(); + if (roleResourceTest.getDefaultRoles().size() == 0) { + return null; + } + return new RoleEntityInterface(roles.get(0)).getEntityReference(); + } } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java index 6b93cc3939c..7fc879d617f 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java @@ -378,6 +378,7 @@ public final class TestUtils { return; } if (expected != null) { + actual = listOrEmpty(actual); assertEquals(expected.size(), actual.size()); for (EntityReference e : expected) { TestUtils.existsInEntityReferenceList(actual, e.getId(), true);