Fixes #4638 - Separate assigned roles and inherited roles for user entity (#4639)

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java

@@ -205,7 +205,7 @@ public abstract class EntityRepository<T> {
    *
    * @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<T> {
 
   public final DeleteResponse<T> delete(String updatedBy, String id, boolean recursive, boolean hardDelete)
       throws IOException {
-    DeleteResponse response = deleteInternal(updatedBy, id, recursive, hardDelete);
+    DeleteResponse<T> response = deleteInternal(updatedBy, id, recursive, hardDelete);
-    postDelete((T) response.getEntity());
+    postDelete(response.getEntity());
     return response;
   }
 

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java

@@ -286,16 +286,6 @@ public class RoleRepository extends EntityRepository<Role> {
         updatedDefaultRole = updatedDefaultRole.withDefaultRole(false);
         new RoleUpdater(origDefaultRole, updatedDefaultRole, Operation.PATCH).update();
       }
-      List<User> 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) {

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<User> {
   /** 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.
+    // Role and teams are already validated
-    Set<UUID> 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<EntityReference> rolesRef = new ArrayList<>(roleIds.size());
-    for (UUID roleId : roleIds) {
-      rolesRef.add(daoCollection.roleDAO().findEntityReferenceById(roleId));
-    }
-    rolesRef.sort(EntityUtil.compareEntityReference);
-    user.setRoles(rolesRef);
   }
 
   @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<EntityReference> getTeamDefaultRoles(User user) throws IOException {
@@ -106,21 +94,23 @@ public class UserRepository extends EntityRepository<User> {
   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<EntityReference> roles = user.getRoles();
+    List<EntityReference> inheritedRoles = user.getInheritedRoles();
     List<EntityReference> 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<User> {
   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<User> {
     return validatedTeams;
   }
 
-  /* Add all the roles that user has been assigned, to User entity */
+  private List<EntityReference> getDefaultRole() throws IOException {
+    List<String> 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<EntityReference> getRoles(User user) throws IOException {
     List<String> roleIds = findTo(user.getId(), Entity.USER, Relationship.HAS, Entity.ROLE);
-    List<EntityReference> 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<EntityReference> getInheritedRoles(User user) throws IOException {
+    List<EntityReference> roles = getDefaultRole();
     roles.addAll(getTeamDefaultRoles(user));
     return roles.stream().distinct().collect(Collectors.toList()); // Remove duplicates
   }
@@ -353,13 +353,19 @@ public class UserRepository extends EntityRepository<User> {
 
     @Override
     public void entitySpecificUpdate() throws IOException {
-      updateRoles(original.getEntity(), updated.getEntity());
+      User origUser = original.getEntity();
-      updateTeams(original.getEntity(), updated.getEntity());
+      User updatedUser = updated.getEntity();
-      recordChange("profile", original.getEntity().getProfile(), updated.getEntity().getProfile(), true);
+
-      recordChange("timezone", original.getEntity().getTimezone(), updated.getEntity().getTimezone());
+      updateRoles(origUser, updatedUser);
-      recordChange("isBot", original.getEntity().getIsBot(), updated.getEntity().getIsBot());
+      updateTeams(origUser, updatedUser);
-      recordChange("isAdmin", original.getEntity().getIsAdmin(), updated.getEntity().getIsAdmin());
+      recordChange("profile", origUser.getProfile(), updatedUser.getProfile(), true);
-      recordChange("email", original.getEntity().getEmail(), updated.getEntity().getEmail());
+      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<User> {
       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());

catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/RoleResource.java

@@ -177,7 +177,7 @@ public class RoleResource extends EntityResource<Role, RoleRepository> {
 
     ResultList<Role> 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

catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java

@@ -82,6 +82,7 @@ public class UserResource extends EntityResource<User, UserRepository> {
   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;

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<User> userRepository = Entity.getEntityRepository(Entity.USER);
       Fields fieldsRolesAndTeams = userRepository.getFields("roles, teams");
       User user = getUserFromAuthenticationContext(ctx, fieldsRolesAndTeams);
+      List<EntityReference> 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<User> userRepository = Entity.getEntityRepository(Entity.USER);
       Fields fieldsRolesAndTeams = userRepository.getFields("roles, teams");
       User user = getUserFromAuthenticationContext(ctx, fieldsRolesAndTeams);
+      List<EntityReference> 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<User> 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<EntityReference> getAllRoles(User user) {
+    List<EntityReference> allRoles = new ArrayList<>(listOrEmpty(user.getRoles()));
+    allRoles.addAll(listOrEmpty(user.getInheritedRoles()));
+    return allRoles.stream().distinct().collect(Collectors.toList()); // Remove duplicates
+  }
 }

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<String> fieldList;
+    @Getter private final List<String> fieldList;
 
     public Fields(List<String> 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<String> getList() {
-      return fieldList;
-    }
   }
 
   public static List<UUID> getIDList(List<EntityReference> refList) {

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
     },

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"
     }
   },

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,

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java

@@ -1426,6 +1426,7 @@ public abstract class EntityResourceTest<T, K> 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<T, K> 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<String, String> authHeaders,
-      UpdateType updateType,
-      ChangeDescription expectedChange,
-      T update)
-      throws IOException {
     EntityInterface<T> 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);

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java

@@ -96,14 +96,15 @@ public class RoleResourceTest extends EntityResourceTest<Role, CreateRole> {
       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<Role, CreateRole> {
     validateRole(role, role.getDescription(), role.getDisplayName(), updatedBy);
     assertListNull(role.getPolicies(), role.getUsers());
 
-    // .../roles?fields=policy,users
     String fields = "policies,teams,users";
     role =
         byName

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 lombok.SneakyThrows;
-import javax.json.JsonPatch;
 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<User, CreateUser> {
   @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<Role> roles = roleResourceTest.listEntities(Collections.emptyMap(), ADMIN_AUTH_HEADERS).getData();
-    UUID nonDefaultRoleId = roles.stream().filter(role -> !role.getDefaultRole()).findAny().orElseThrow().getId();
+    Role nonDefaultRole = roles.stream().filter(role -> !role.getDefaultRole()).findAny().orElseThrow();
-    UUID defaultRoleId =
+    EntityReference nonDefaultRoleRef = new RoleEntityInterface(nonDefaultRole).getEntityReference();
-        roles.stream()
+
-            .filter(Role::getDefaultRole)
+    // Find a default role
-            .findAny()
+    EntityReference defaultRoleRef = getDefaultRole();
-            .orElseThrow()
+    assertNotNull(defaultRoleRef);
-            .getId(); // DataConsumer is global default role.
 
     // ... 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
+    // ... when a user is created with a non default role, then the global default role
-    // the non default role.
+    // should be assigned along with the non default role.
-    create = createRequest(test, 2).withRoles(List.of(nonDefaultRoleId));
+    create = createRequest(test, 2).withRoles(List.of(nonDefaultRole.getId()));
-    User user2 = createUserAndCheckRoles(create, Arrays.asList(nonDefaultRoleId, defaultRoleId));
+    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));
+    create = createRequest(test, 3).withRoles(List.of(nonDefaultRole.getId(), defaultRoleRef.getId()));
-    User user3 = createUserAndCheckRoles(create, Arrays.asList(nonDefaultRoleId, defaultRoleId));
+    User user3 =
+        createUserAndCheckRoles(
+            create, Arrays.asList(nonDefaultRoleRef, defaultRoleRef), Arrays.asList(defaultRoleRef));
 
-    // Given the default role has changed, ...
+    // Change the default roles and make sure the change is reflected in all users
-    UUID prevDefaultRoleId = defaultRoleId;
+    Role newDefaultRole = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1000), ADMIN_AUTH_HEADERS);
-    Role defaultRole = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1000), ADMIN_AUTH_HEADERS);
+    EntityReference newDefaultRoleRef = new RoleEntityInterface(newDefaultRole).getEntityReference();
-    String defaultRoleJson = JsonUtils.pojoToJson(defaultRole);
+    String defaultRoleJson = JsonUtils.pojoToJson(newDefaultRole);
-    defaultRole.setDefaultRole(true);
+    newDefaultRole.setDefaultRole(true);
-    defaultRoleId = defaultRole.getId(); // New global default role.
+    roleResourceTest.patchEntity(newDefaultRole.getId(), defaultRoleJson, newDefaultRole, ADMIN_AUTH_HEADERS);
-    assertNotEquals(prevDefaultRoleId, defaultRoleId);
-    roleResourceTest.patchEntity(defaultRoleId, defaultRoleJson, defaultRole, 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);
+    // Create team1 with defaultRole role1
-    Role role2 = roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1002), ADMIN_AUTH_HEADERS);
+    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
+    // user1 has defaultRole
-    // the global default role and the team1 default role, role1.
+    // 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.setTeams(List.of(team1Ref));
-    user1 = patchUser(originalUser1, user1, ADMIN_AUTH_HEADERS);
+    user1 = patchEntity(user1.getId(), originalUser1, user1, ADMIN_AUTH_HEADERS);
-    assertRoles(user1, Arrays.asList(defaultRoleId, role1.getId()));
+    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
+    // user1 now has default role and role1 (through team1)
-    // team1 default role, role1 and team2 default role, role2.
+    // Add user1 to team2 to inherit default of roles team1 (role1 and role2)
     originalUser1 = JsonUtils.pojoToJson(user1);
-    user1.setTeams(
+    user1.setTeams(List.of(team1Ref, team2Ref));
-        List.of(
+    user1 = patchEntity(user1.getId(), originalUser1, user1, ADMIN_AUTH_HEADERS);
-            new TeamEntityInterface(team1).getEntityReference(), new TeamEntityInterface(team2).getEntityReference()));
+    assertRoles(user1, Collections.emptyList(), Arrays.asList(newDefaultRoleRef, role1Ref, role2Ref));
-    user1 = patchUser(originalUser1, user1, ADMIN_AUTH_HEADERS);
-    assertRoles(user1, Arrays.asList(defaultRoleId, role1.getId(), role2.getId()));
 
-    // Given user2 has a non default role assigned, when user2 joins team2, then user2 should get assigned the global
+    // user2 has defaultRole and nonDefaultRole
-    // default role, team2 default roles, role1 and role2, and retain its non-default role.
+    // 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.setTeams(List.of(team2Ref));
-    user2 = patchUser(originalUser2, user2, ADMIN_AUTH_HEADERS);
+    user2 = patchEntity(user2.getId(), originalUser2, user2, ADMIN_AUTH_HEADERS);
-    assertRoles(user2, Arrays.asList(defaultRoleId, role1.getId(), role2.getId(), nonDefaultRoleId));
+    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);
-    //    originalUser2 = JsonUtils.pojoToJson(user2);
+    ChangeDescription change = getChangeDescription(user2.getVersion());
-    //    user2.setTeams(null);
+    change.getFieldsDeleted().add(new FieldChange().withName("teams").withOldValue(List.of(team2Ref)));
-    //    user2 = patchUser(originalUser2, user2, ADMIN_AUTH_HEADERS);
+    user2.setTeams(null);
-    //    assertRoles(user2, Arrays.asList(defaultRoleId, nonDefaultRoleId));
+    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<User, CreateUser> {
   }
 
   @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<User, CreateUser> {
     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<UUID> roles = Arrays.asList(role1.getId(), role2.getId());
     CreateUser create = createRequest(test).withRoles(roles);
-    List<UUID> createdRoles = Arrays.asList(role1.getId(), role2.getId(), defaultRoleRef.getId());
+    List<UUID> 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<User, CreateUser> {
     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<User, CreateUser> {
     String userJson = JsonUtils.pojoToJson(user);
     user.setIsAdmin(Boolean.TRUE);
     Map<String, String> 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<User, CreateUser> {
     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<User, CreateUser> {
     Profile profile = new Profile().withImages(new ImageList().withImage(URI.create("http://image.com")));
 
     RoleResourceTest roleResourceTest = new RoleResourceTest();
-    List<Role> 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<User, CreateUser> {
     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<User, CreateUser> {
     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,16 +593,7 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
             .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")
@@ -639,22 +614,13 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
         .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)
@@ -670,7 +636,7 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
     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<User, CreateUser> {
         entityNotFound("user", user.getId()));
   }
 
-  private User createUserAndCheckRoles(CreateUser create, List<UUID> expectedRolesIds) throws HttpResponseException {
+  @SneakyThrows
+  private User createUserAndCheckRoles(
+      CreateUser create, List<EntityReference> expectedRoles, List<EntityReference> 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<UUID> expectedRolesIds) throws HttpResponseException {
+  private void assertRoles(
-    user = getEntity(user.getId(), ADMIN_AUTH_HEADERS);
+      User user, List<EntityReference> expectedRoles, List<EntityReference> expectedInheritedRoles) {
-    List<UUID> actualRolesIds =
+    assertEntityReferenceList(expectedRoles, user.getRoles());
-        user.getRoles().stream().map(EntityReference::getId).sorted().collect(Collectors.toList());
+    assertEntityReferenceList(expectedInheritedRoles, user.getInheritedRoles());
-    Collections.sort(expectedRolesIds);
-    assertEquals(expectedRolesIds, actualRolesIds);
-  }
-
-  private User patchUser(UUID userId, String originalJson, User updated, Map<String, String> 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<String, String> headers)
-      throws JsonProcessingException, HttpResponseException {
-    return patchUser(updated.getId(), originalJson, updated, headers);
   }
 
   @Override
@@ -787,7 +744,8 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
   }
 
   @Override
-  public void validateCreatedEntity(User user, CreateUser createRequest, Map<String, String> authHeaders) {
+  public void validateCreatedEntity(User user, CreateUser createRequest, Map<String, String> authHeaders)
+      throws HttpResponseException {
     validateCommonEntityFields(
         getEntityInterface(user),
         createRequest.getDescription(),
@@ -804,12 +762,7 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
     for (UUID roleId : listOrEmpty(createRequest.getRoles())) {
       expectedRoles.add(new EntityReference().withId(roleId).withType(Entity.ROLE));
     }
-    if (expectedRoles.isEmpty()) {
+    assertRoles(user, expectedRoles, List.of(DATA_CONSUMER_ROLE_REFERENCE));
-      // 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());
-    }
 
     List<EntityReference> expectedTeams = new ArrayList<>();
     for (UUID teamId : listOrEmpty(createRequest.getTeams())) {
@@ -867,4 +820,13 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
       assertCommonFieldChange(fieldName, expected, actual);
     }
   }
+
+  private EntityReference getDefaultRole() throws HttpResponseException {
+    RoleResourceTest roleResourceTest = new RoleResourceTest();
+    List<Role> roles = roleResourceTest.getDefaultRoles();
+    if (roleResourceTest.getDefaultRoles().size() == 0) {
+      return null;
+    }
+    return new RoleEntityInterface(roles.get(0)).getEntityReference();
+  }
 }

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);