diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java b/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java index 04531fe56ed..9b3519c78a1 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java @@ -25,6 +25,7 @@ public class ResourceRegistry { mapFieldOperation(MetadataOperation.EDIT_ROLE, "defaultRoles"); mapFieldOperation(MetadataOperation.EDIT_ROLE, "roles"); mapFieldOperation(MetadataOperation.EDIT_POLICY, "policies"); + mapFieldOperation(MetadataOperation.EDIT_TEAMS, "teams"); // TODO tier, lineage, statues, reviewers, tests, queries, data profile, sample data } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java index a3604112412..c0afafa22a3 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java @@ -52,6 +52,9 @@ public class SystemRepository { public Settings getConfigWithKey(String key) { try { Settings fetchedSettings = dao.getConfigWithKey(key); + if (fetchedSettings == null) { + return null; + } if (fetchedSettings.getConfigType() == SettingsType.EMAIL_CONFIGURATION) { SmtpSettings emailConfig = (SmtpSettings) fetchedSettings.getConfigValue(); emailConfig.setPassword("***********"); @@ -60,7 +63,7 @@ public class SystemRepository { return fetchedSettings; } catch (Exception ex) { - LOG.error("Error while trying fetch Settings " + ex.getMessage()); + LOG.error("Error while trying fetch Settings ", ex); } return null; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java index fb675bbb69d..687c71d1e73 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java @@ -490,10 +490,10 @@ public class UserResource extends EntityResource { if (Boolean.TRUE.equals(create.getIsBot())) { addAuthMechanismToBot(user, create, uriInfo); } - // Basic Auth Related + if (isBasicAuth()) { - // basic auth doesn't allow duplicate emails try { + // basic auth doesn't allow duplicate emails, since username part of the email is used as login name validateEmailAlreadyExists(create.getEmail()); } catch (RuntimeException ex) { return Response.status(CONFLICT) @@ -501,7 +501,6 @@ public class UserResource extends EntityResource { .entity(new ErrorMessage(CONFLICT.getStatusCode(), CatalogExceptionMessage.ENTITY_ALREADY_EXISTS)) .build(); } - // this is also important since username is used for a lot of stuff user.setName(user.getEmail().split("@")[0]); if (Boolean.FALSE.equals(create.getIsBot()) && create.getCreatePasswordType() == ADMIN_CREATE) { addAuthMechanismToUser(user, create); @@ -545,11 +544,9 @@ public class UserResource extends EntityResource { public Response createOrUpdateUser( @Context UriInfo uriInfo, @Context SecurityContext securityContext, @Valid CreateUser create) throws IOException { User user = getUser(securityContext, create); - - // If entity does not exist, this is a create operation, else update operation - ResourceContext resourceContext = getResourceContextByName(user.getFullyQualifiedName()); - dao.prepareInternal(user); + + ResourceContext resourceContext = getResourceContextByName(user.getFullyQualifiedName()); if (Boolean.TRUE.equals(create.getIsAdmin()) || Boolean.TRUE.equals(create.getIsBot())) { authorizer.authorizeAdmin(securityContext); } else if (!securityContext.getUserPrincipal().getName().equals(user.getName())) { @@ -725,8 +722,9 @@ public class UserResource extends EntityResource { JsonObject patchOpObject = patchOp.asJsonObject(); if (patchOpObject.containsKey("path") && patchOpObject.containsKey("value")) { String path = patchOpObject.getString("path"); - if (path.equals("/isAdmin") || path.equals("/isBot")) { + if (path.equals("/isAdmin") || path.equals("/isBot") || path.equals("/roles")) { authorizer.authorizeAdmin(securityContext); + continue; } // if path contains team, check if team is joinable by any user if (patchOpObject.containsKey("op") @@ -742,8 +740,7 @@ public class UserResource extends EntityResource { String teamId = value.getString("id"); dao.validateTeamAddition(id, UUID.fromString(teamId)); if (!dao.isTeamJoinable(teamId)) { - // Only admin can join closed teams - authorizer.authorizeAdmin(securityContext); + authorizer.authorizeAdmin(securityContext); // Only admin can join closed teams } } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java index e472277cb38..9f6928d9e99 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java @@ -124,7 +124,7 @@ public class CompiledRule extends Rule { while (iterator.hasNext()) { MetadataOperation operation = iterator.next(); if (matchOperation(operation) && matchExpression(policyContext, subjectContext, resourceContext)) { - LOG.info("operation {} allowed", operation); + LOG.debug("operation {} allowed", operation); iterator.remove(); } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java index 96f3ba07bbf..30ae54b163c 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java @@ -35,7 +35,12 @@ public class ResourceContext implements ResourceContextInterface { @Override public EntityReference getOwner() throws IOException { resolveEntity(); - return entity == null ? null : entity.getOwner(); + if (entity == null) { + return null; + } else if (Entity.USER.equals(entityRepository.getEntityType())) { + return entity.getEntityReference(); // Owner for a user is same as the user + } + return entity.getOwner(); } @Override diff --git a/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json b/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json index 5fae5a1cf98..87eb0109c1e 100644 --- a/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json +++ b/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json @@ -25,6 +25,7 @@ "EditSampleData", "EditStatus", "EditTags", + "EditTeams", "EditTier", "EditTests", "EditUsers" @@ -395,7 +396,8 @@ "EditCustomFields", "EditDescription", "EditDisplayName", - "EditRole" + "EditRole", + "EditTeams" ] }, { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java index c43b44866e5..153f6730e3f 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java @@ -133,7 +133,9 @@ import org.openmetadata.service.util.TestUtils.UpdateType; @Slf4j @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class UserResourceTest extends EntityResourceTest { - final Profile PROFILE = new Profile().withImages(new ImageList().withImage(URI.create("http://image.com"))); + private static final Profile PROFILE = + new Profile().withImages(new ImageList().withImage(URI.create("http://image.com"))); + private static final TeamResourceTest TEAM_TEST = new TeamResourceTest(); public UserResourceTest() { super(USER, User.class, UserList.class, "users", UserResource.FIELDS); @@ -289,17 +291,16 @@ public class UserResourceTest extends EntityResourceTest { @Test void post_validUserWithTeams_200_ok(TestInfo test) throws IOException { // Create user with different optional fields - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team1 = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS); - Team team2 = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS); + Team team1 = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS); + Team team2 = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 2), ADMIN_AUTH_HEADERS); List teams = Arrays.asList(team1.getId(), team2.getId()); CreateUser create = createRequest(test).withTeams(teams); User user = createAndCheckEntity(create, ADMIN_AUTH_HEADERS); // Ensure Team has relationship to this user - team1 = teamResourceTest.getEntity(team1.getId(), "users", ADMIN_AUTH_HEADERS); + team1 = TEAM_TEST.getEntity(team1.getId(), "users", ADMIN_AUTH_HEADERS); assertEquals(user.getId(), team1.getUsers().get(0).getId()); - team2 = teamResourceTest.getEntity(team2.getId(), "users", ADMIN_AUTH_HEADERS); + team2 = TEAM_TEST.getEntity(team2.getId(), "users", ADMIN_AUTH_HEADERS); assertEquals(user.getId(), team2.getUsers().get(0).getId()); } @@ -322,9 +323,8 @@ public class UserResourceTest extends EntityResourceTest { @Test void get_listUsersWithTeams_200_ok(TestInfo test) throws IOException { - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team1 = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS); - Team team2 = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS); + Team team1 = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS); + Team team2 = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 2), ADMIN_AUTH_HEADERS); List teams = of(team1.getId(), team2.getId()); List team = of(team1.getId()); @@ -447,8 +447,7 @@ public class UserResourceTest extends EntityResourceTest { @Test void get_listUsersWithFalseBotFilterPagination(TestInfo test) throws IOException { - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS); + Team team = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS); Map queryParams = Map.of("isBot", "false", "team", team.getName()); @@ -494,8 +493,7 @@ public class UserResourceTest extends EntityResourceTest { @Test void get_listUsersWithTeamsPagination(TestInfo test) throws IOException { - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team1 = teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS); + Team team1 = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS); List team = of(team1.getId()); // create 15 users and add them to team1 @@ -560,57 +558,22 @@ public class UserResourceTest extends EntityResourceTest { assertDoesNotThrow(() -> PasswordUtil.validatePassword(randomPwd), PASSWORD_INVALID_FORMAT); } - /** - * @see EntityResourceTest put_addDeleteFollower_200 test for tests related to GET user with owns field parameter - * @see EntityResourceTest put_addDeleteFollower_200 for tests related getting user with follows list - * @see TableResourceTest also tests GET user returns owns list - */ @Test - void patch_userNameChange_as_another_user_401(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure user display can't be changed using patch by another user + void patch_makeAdmin_as_nonAdmin_user_401(TestInfo test) throws HttpResponseException, JsonProcessingException { + // Ensure a non admin user can't make another user admin User user = createEntity( - createRequest(test, 7).withName("test23").withDisplayName("displayName").withEmail("test23@email.com"), - authHeaders("test23@email.com")); - String userJson = JsonUtils.pojoToJson(user); - user.setDisplayName("newName"); - assertResponse( - () -> patchEntity(user.getId(), userJson, user, TEST_AUTH_HEADERS), - FORBIDDEN, - permissionNotAllowed(TEST_USER_NAME, List.of(MetadataOperation.EDIT_DISPLAY_NAME))); - } - - @Test - void patch_makeAdmin_as_another_user_401(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure username can't be changed using patch - User user = - createEntity( - createRequest(test, 6).withName("test2").withDisplayName("displayName").withEmail("test2@email.com"), - authHeaders("test2@email.com")); + createRequest(test, 6).withName("test2").withEmail("test2@email.com"), authHeaders("test2@email.com")); String userJson = JsonUtils.pojoToJson(user); user.setIsAdmin(Boolean.TRUE); assertResponse(() -> patchEntity(user.getId(), userJson, user, TEST_AUTH_HEADERS), FORBIDDEN, notAdmin("test")); } - @Test - void patch_userNameChange_as_same_user_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure username can't be changed using patch - User user = - createEntity( - createRequest(test, 6).withName("test1").withDisplayName("displayName").withEmail("test1@email.com"), - authHeaders("test1@email.com")); - String userJson = JsonUtils.pojoToJson(user); - String newDisplayName = "newDisplayName"; - user.setDisplayName(newDisplayName); // Update the name - user = patchEntity(user.getId(), userJson, user, ADMIN_AUTH_HEADERS); // Patch the user - assertEquals(newDisplayName, user.getDisplayName()); - } - @Test void patch_teamAddition_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { - TeamResourceTest teamResourceTest = new TeamResourceTest(); + // Admin can add user to a team by patching `teams` attribute EntityReference team1 = - teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS).getEntityReference(); + TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS).getEntityReference(); User user = createEntity( createRequest(test, 10) @@ -634,13 +597,12 @@ public class UserResourceTest extends EntityResourceTest { User user = createEntity(createRequest(test).withProfile(null), ADMIN_AUTH_HEADERS); assertListNull(user.getDisplayName(), user.getIsBot(), user.getProfile(), user.getTimezone()); - TeamResourceTest teamResourceTest = new TeamResourceTest(); EntityReference team1 = - teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS).getEntityReference(); + TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS).getEntityReference(); EntityReference team2 = - teamResourceTest.createEntity(teamResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS).getEntityReference(); + TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 2), ADMIN_AUTH_HEADERS).getEntityReference(); EntityReference team3 = - teamResourceTest.createEntity(teamResourceTest.createRequest(test, 3), ADMIN_AUTH_HEADERS).getEntityReference(); + TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 3), ADMIN_AUTH_HEADERS).getEntityReference(); List teams = Arrays.asList(team1, team2); Profile profile = new Profile().withImages(new ImageList().withImage(URI.create("http://image.com"))); @@ -725,10 +687,61 @@ public class UserResourceTest extends EntityResourceTest { patchEntityAndCheck(user, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); } + @Test + void patch_userAuthorizationTests(TestInfo test) throws IOException { + // + // A user can update many attributes for himself. These tests validate what is allowed and not allowed + // + Team team = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 1), ADMIN_AUTH_HEADERS); + Team teamNotJoinable = + TEAM_TEST.createEntity(TEAM_TEST.createRequest(test, 2).withIsJoinable(false), ADMIN_AUTH_HEADERS); + User user1 = createEntity(createRequest(test, 1).withTeams(listOf(TEAM2.getId())), ADMIN_AUTH_HEADERS); + Map user1Auth = authHeaders(user1.getName()); + String json = JsonUtils.pojoToJson(user1); + + // User can't set himself as admin + user1.withIsAdmin(true); + assertResponse(() -> patchEntity(user1.getId(), json, user1, user1Auth), FORBIDDEN, notAdmin(user1.getName())); + + // User can't set himself as bot + user1.withIsAdmin(false).withIsBot(true); + assertResponse(() -> patchEntity(user1.getId(), json, user1, user1Auth), FORBIDDEN, notAdmin(user1.getName())); + + // User can't change the roles + user1.withIsBot(null).withRoles(listOf(DATA_CONSUMER_ROLE_REF)); + assertResponse(() -> patchEntity(user1.getId(), json, user1, user1Auth), FORBIDDEN, notAdmin(user1.getName())); + + // User can change for authorized as himself the teams and other attributes + ChangeDescription change = getChangeDescription(user1.getVersion()); + user1.withRoles(null).withDescription("description").withDisplayName("display"); + user1.getTeams().add(team.getEntityReference()); + fieldUpdated(change, "description", "", "description"); + fieldAdded(change, "displayName", "display"); + fieldAdded(change, "teams", listOf(team.getEntityReference())); + User updatedUser1 = patchEntityAndCheck(user1, json, user1Auth, MINOR_UPDATE, change); + + // A user can't join a team that is not open for joining. Only an Admin can join such teams. + String json1 = JsonUtils.pojoToJson(updatedUser1); + List previousTeams = new ArrayList<>(updatedUser1.getTeams()); + updatedUser1.getTeams().add(teamNotJoinable.getEntityReference()); + assertResponse( + () -> patchEntity(user1.getId(), json1, updatedUser1, authHeaders(user1.getName())), + FORBIDDEN, + notAdmin(user1.getName())); + + // A user (without privileges) can't change the attributes of another user + // Note the authHeaders from another user different from user1 in the following patch operation + updatedUser1.withTeams(previousTeams); + updatedUser1.getTeams().add(TEAM21.getEntityReference()); + assertResponse( + () -> patchEntity(user1.getId(), json1, updatedUser1, authHeaders(USER2.getName())), + FORBIDDEN, + permissionNotAllowed(USER2.getName(), listOf(MetadataOperation.EDIT_TEAMS))); + } + @Test void delete_validUser_as_admin_200(TestInfo test) throws IOException { - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team = teamResourceTest.createEntity(teamResourceTest.createRequest(test), ADMIN_AUTH_HEADERS); + Team team = TEAM_TEST.createEntity(TEAM_TEST.createRequest(test), ADMIN_AUTH_HEADERS); List teamIds = Collections.singletonList(team.getId()); // Create user with teams @@ -744,7 +757,7 @@ public class UserResourceTest extends EntityResourceTest { deleteAndCheckEntity(user, ADMIN_AUTH_HEADERS); // Make sure the user is no longer following the table - team = teamResourceTest.getEntity(team.getId(), "users", ADMIN_AUTH_HEADERS); + team = TEAM_TEST.getEntity(team.getId(), "users", ADMIN_AUTH_HEADERS); assertDeleted(team.getUsers(), true); tableResourceTest.checkFollowerDeleted(table.getId(), user.getId(), ADMIN_AUTH_HEADERS); @@ -945,8 +958,7 @@ public class UserResourceTest extends EntityResourceTest { @Test void testImportInvalidCsv() throws IOException { // Headers - name,displayName,description,email,timezone,isAdmin,teams,roles - TeamResourceTest teamResourceTest = new TeamResourceTest(); - Team team = teamResourceTest.createEntity(teamResourceTest.createRequest("team-invalidCsv"), ADMIN_AUTH_HEADERS); + Team team = TEAM_TEST.createEntity(TEAM_TEST.createRequest("team-invalidCsv"), ADMIN_AUTH_HEADERS); // Invalid team String resultsHeader = recordToString(EntityCsv.getResultHeaders(UserCsv.HEADERS)); @@ -970,12 +982,11 @@ public class UserResourceTest extends EntityResourceTest { void testUserImportExport() throws IOException { // Create team hierarchy - team with children t1, t1 has t11 // "name", "displayName", "description", "teamType", "parents", "owner", "isJoinable", "defaultRoles", & "policies" - TeamResourceTest teamResourceTest = new TeamResourceTest(); String team = "teamImportExport,,,Division,Organization,,,,"; String team1 = "teamImportExport1,,,Department,teamImportExport,,,,"; String team11 = "teamImportExport11,,,Group,teamImportExport1,,,,"; String csv = EntityCsvTest.createCsv(TeamCsv.HEADERS, listOf(team, team1, team11), null); - CsvImportResult result = teamResourceTest.importCsv(ORG_TEAM.getName(), csv, false); + CsvImportResult result = TEAM_TEST.importCsv(ORG_TEAM.getName(), csv, false); assertEquals(0, result.getNumberOfRowsFailed()); // Create users in the team hierarchy diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/policies/accessControl/resourceDescriptor.json b/openmetadata-spec/src/main/resources/json/schema/entity/policies/accessControl/resourceDescriptor.json index 3cb43356554..cabf82a28ee 100644 --- a/openmetadata-spec/src/main/resources/json/schema/entity/policies/accessControl/resourceDescriptor.json +++ b/openmetadata-spec/src/main/resources/json/schema/entity/policies/accessControl/resourceDescriptor.json @@ -35,6 +35,7 @@ "EditSampleData", "EditStatus", "EditTags", + "EditTeams", "EditTier", "EditTests", "EditUsers" diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/teams/user.json b/openmetadata-spec/src/main/resources/json/schema/entity/teams/user.json index 5649a951a30..4906074a8db 100644 --- a/openmetadata-spec/src/main/resources/json/schema/entity/teams/user.json +++ b/openmetadata-spec/src/main/resources/json/schema/entity/teams/user.json @@ -87,13 +87,11 @@ }, "isBot": { "description": "When true indicates a special type of user called Bot.", - "type": "boolean", - "boolean": false + "type": "boolean" }, "isAdmin": { "description": "When true indicates user is an administrator for the system with superuser privileges.", - "type": "boolean", - "boolean": false + "type": "boolean" }, "authenticationMechanism": { "$ref": "#/definitions/authenticationMechanism"