From aeb020ae3b0cbab3a2ee5995c61480cdd1eae405 Mon Sep 17 00:00:00 2001 From: Mohit Yadav <105265192+mohityadav766@users.noreply.github.com> Date: Fri, 7 Jun 2024 21:03:50 +0530 Subject: [PATCH] [ISSUE-16503] Fix createUser to use EntityResource (#16549) * Fix createUser to use EntityResource * fix broken tests * Fix Tests - 3 --- .../exception/CatalogExceptionMessage.java | 6 ++ .../service/resources/teams/UserResource.java | 57 ++++++++++++------- .../service/resources/EntityResourceTest.java | 18 +++--- .../policies/PolicyResourceTest.java | 15 +++++ .../resources/teams/PersonaResourceTest.java | 6 +- .../resources/teams/RoleResourceTest.java | 7 +++ .../resources/teams/TeamResourceTest.java | 7 ++- .../resources/teams/UserResourceTest.java | 21 +++++-- .../openmetadata/service/util/TestUtils.java | 3 + 9 files changed, 103 insertions(+), 37 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java index c098e1385170..433abb54f400 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java @@ -182,6 +182,12 @@ public static String notAdmin(String name) { return String.format("Principal: CatalogPrincipal{name='%s'} is not admin", name); } + public static String operationNotAllowed(String name, MetadataOperation operation) { + return String.format( + "Principal: CatalogPrincipal{name='%s'} operations [%s] not allowed", + name, operation.value()); + } + public static String notReviewer(String name) { return String.format("User '%s' is not a reviewer", name); } 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 5120b25ccce2..23d9954bf636 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 @@ -548,26 +548,41 @@ public Response createUser( @Context ContainerRequestContext containerRequestContext, @Valid CreateUser create) { User user = getUser(securityContext.getUserPrincipal().getName(), create); - if (Boolean.TRUE.equals(create.getIsAdmin())) { - authorizer.authorizeAdmin(securityContext); - } if (Boolean.TRUE.equals(create.getIsBot())) { addAuthMechanismToBot(user, create, uriInfo); } + // + try { + validateAndAddUserAuthForBasic(user, create); + } catch (RuntimeException ex) { + return Response.status(CONFLICT) + .type(MediaType.APPLICATION_JSON_TYPE) + .entity( + new ErrorMessage( + CONFLICT.getStatusCode(), CatalogExceptionMessage.ENTITY_ALREADY_EXISTS)) + .build(); + } + + // Add the roles on user creation + updateUserRolesIfRequired(user, containerRequestContext); + + // TODO do we need to authenticate user is creating himself? + Response createdUser = create(uriInfo, securityContext, user); + + // Send Invite mail to user + sendInviteMailToUserForBasicAuth(uriInfo, user, create); + + // Update response to remove auth fields + decryptOrNullify(securityContext, (User) createdUser.getEntity()); + return createdUser; + } + + private void validateAndAddUserAuthForBasic(User user, CreateUser create) { if (isBasicAuth()) { - 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) - .type(MediaType.APPLICATION_JSON_TYPE) - .entity( - new ErrorMessage( - CONFLICT.getStatusCode(), CatalogExceptionMessage.ENTITY_ALREADY_EXISTS)) - .build(); - } + // basic auth doesn't allow duplicate emails, since username part of the email is used as + // login name + validateEmailAlreadyExists(create.getEmail()); user.setName(user.getEmail().split("@")[0]); if (Boolean.FALSE.equals(create.getIsBot()) && create.getCreatePasswordType() == ADMIN_CREATE) { @@ -575,17 +590,18 @@ public Response createUser( } // else the user will get a mail if configured smtp } + } - // Add the roles on user creation + private void updateUserRolesIfRequired( + User user, ContainerRequestContext containerRequestContext) { if (Boolean.TRUE.equals(authorizerConfiguration.getUseRolesFromProvider()) && Boolean.FALSE.equals(user.getIsBot() != null && user.getIsBot())) { user.setRoles( validateAndGetRolesRef(getRolesFromAuthorizationToken(containerRequestContext))); } + } - // TODO do we need to authenticate user is creating himself? - - addHref(uriInfo, repository.create(uriInfo, user)); + private void sendInviteMailToUserForBasicAuth(UriInfo uriInfo, User user, CreateUser create) { if (isBasicAuth() && isEmailServiceEnabled) { try { authHandler.sendInviteMailToUser( @@ -598,9 +614,6 @@ public Response createUser( LOG.error("Error in sending invite to User" + ex.getMessage()); } } - Response response = Response.created(user.getHref()).entity(user).build(); - decryptOrNullify(securityContext, (User) response.getEntity()); - return response; } private boolean isBasicAuth() { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java index fc616ab77aa6..b0e0f88589e9 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java @@ -258,6 +258,7 @@ public abstract class EntityResourceTest .withOwner(USER1_REF); } + private CreatePolicy createAccessControlPolicyWithCreateRule() { + return new CreatePolicy() + .withName("CreatePermissionPolicy") + .withDescription("Create User Permission") + .withRules( + List.of( + new Rule() + .withName("CreatePermission") + .withResources(List.of(ALL_RESOURCES)) + .withOperations(List.of(MetadataOperation.CREATE)) + .withEffect(ALLOW))); + } + private void validateCondition(String expression) throws HttpResponseException { WebTarget target = getResource(collectionName + "/validation/condition/" + expression); TestUtils.get(target, ADMIN_AUTH_HEADERS); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/PersonaResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/PersonaResourceTest.java index 538145851308..42bfb0e62188 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/PersonaResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/PersonaResourceTest.java @@ -85,9 +85,11 @@ void post_teamWithUsersAndDefaultRoles_200_OK(TestInfo test) throws IOException // Add team to user relationships while creating a team UserResourceTest userResourceTest = new UserResourceTest(); User user1 = - userResourceTest.createEntity(userResourceTest.createRequest(test, 1), TEST_AUTH_HEADERS); + userResourceTest.createEntity( + userResourceTest.createRequest(test, 1), USER_WITH_CREATE_HEADERS); User user2 = - userResourceTest.createEntity(userResourceTest.createRequest(test, 2), TEST_AUTH_HEADERS); + userResourceTest.createEntity( + userResourceTest.createRequest(test, 2), USER_WITH_CREATE_HEADERS); List users = Arrays.asList(user1.getId(), user2.getId()); CreatePersona create = diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/RoleResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/RoleResourceTest.java index 4893ae45ec98..a367e397e6cc 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/RoleResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/RoleResourceTest.java @@ -74,6 +74,13 @@ public void setupRoles(TestInfo test) throws HttpResponseException { ROLE1 = createEntity(createRequest(test), ADMIN_AUTH_HEADERS); ROLE1_REF = ROLE1.getEntityReference(); + + CREATE_ACCESS_ROLE = + createEntity( + new CreateRole() + .withName("CreateAccessRole") + .withPolicies(List.of(CREATE_ACCESS_PERMISSION_POLICY.getFullyQualifiedName())), + ADMIN_AUTH_HEADERS); } /** Creates the given number of roles */ diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java index ffe71d5f4ca8..d533d2bfb3a1 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java @@ -50,6 +50,7 @@ import static org.openmetadata.service.util.TestUtils.ADMIN_AUTH_HEADERS; import static org.openmetadata.service.util.TestUtils.TEST_AUTH_HEADERS; import static org.openmetadata.service.util.TestUtils.TEST_USER_NAME; +import static org.openmetadata.service.util.TestUtils.USER_WITH_CREATE_HEADERS; import static org.openmetadata.service.util.TestUtils.UpdateType.CHANGE_CONSOLIDATED; import static org.openmetadata.service.util.TestUtils.UpdateType.MINOR_UPDATE; import static org.openmetadata.service.util.TestUtils.UpdateType.NO_CHANGE; @@ -195,9 +196,11 @@ void post_teamWithUsersAndDefaultRoles_200_OK(TestInfo test) throws IOException // Add team to user relationships while creating a team UserResourceTest userResourceTest = new UserResourceTest(); User user1 = - userResourceTest.createEntity(userResourceTest.createRequest(test, 1), TEST_AUTH_HEADERS); + userResourceTest.createEntity( + userResourceTest.createRequest(test, 1), USER_WITH_CREATE_HEADERS); User user2 = - userResourceTest.createEntity(userResourceTest.createRequest(test, 2), TEST_AUTH_HEADERS); + userResourceTest.createEntity( + userResourceTest.createRequest(test, 2), USER_WITH_CREATE_HEADERS); List users = Arrays.asList(user1.getId(), user2.getId()); RoleResourceTest roleResourceTest = new RoleResourceTest(); 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 6eee619f9861..d30d76a21be5 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 @@ -41,6 +41,7 @@ import static org.openmetadata.service.exception.CatalogExceptionMessage.PASSWORD_INVALID_FORMAT; import static org.openmetadata.service.exception.CatalogExceptionMessage.entityNotFound; import static org.openmetadata.service.exception.CatalogExceptionMessage.notAdmin; +import static org.openmetadata.service.exception.CatalogExceptionMessage.operationNotAllowed; import static org.openmetadata.service.exception.CatalogExceptionMessage.permissionNotAllowed; import static org.openmetadata.service.resources.teams.UserResource.USER_PROTECTED_FIELDS; import static org.openmetadata.service.security.SecurityUtil.authHeaders; @@ -51,6 +52,8 @@ import static org.openmetadata.service.util.TestUtils.INGESTION_BOT; import static org.openmetadata.service.util.TestUtils.TEST_AUTH_HEADERS; import static org.openmetadata.service.util.TestUtils.TEST_USER_NAME; +import static org.openmetadata.service.util.TestUtils.USER_WITH_CREATE_HEADERS; +import static org.openmetadata.service.util.TestUtils.USER_WITH_CREATE_PERMISSION_NAME; import static org.openmetadata.service.util.TestUtils.UpdateType.CHANGE_CONSOLIDATED; import static org.openmetadata.service.util.TestUtils.UpdateType.MINOR_UPDATE; import static org.openmetadata.service.util.TestUtils.UpdateType.REVERT; @@ -153,6 +156,14 @@ public UserResourceTest() { } public void setupUsers(TestInfo test) throws HttpResponseException { + CreateUser createUserWithAccess = + new CreateUser() + .withName(USER_WITH_CREATE_PERMISSION_NAME) + .withEmail(USER_WITH_CREATE_PERMISSION_NAME + "@open-metadata.org") + .withProfile(PROFILE) + .withRoles(List.of(CREATE_ACCESS_ROLE.getId())) + .withIsBot(false); + USER_WITH_CREATE_ACCESS = createEntity(createUserWithAccess, ADMIN_AUTH_HEADERS); CreateUser create = createRequest(test).withRoles(List.of(DATA_CONSUMER_ROLE.getId())); USER1 = createEntity(create, ADMIN_AUTH_HEADERS); USER1_REF = USER1.getEntityReference(); @@ -317,7 +328,9 @@ void post_validAdminUser_Non_Admin_401(TestInfo test) { .withIsAdmin(true); assertResponse( - () -> createAndCheckEntity(create, TEST_AUTH_HEADERS), FORBIDDEN, notAdmin(TEST_USER_NAME)); + () -> createAndCheckEntity(create, TEST_AUTH_HEADERS), + FORBIDDEN, + operationNotAllowed(TEST_USER_NAME, MetadataOperation.CREATE)); } @Test @@ -613,7 +626,7 @@ void patch_makeAdmin_as_nonAdmin_user_401(TestInfo test) throws HttpResponseExce User user = createEntity( createRequest(test, 6).withName("test2").withEmail("test2@email.com"), - authHeaders("test2@email.com")); + USER_WITH_CREATE_HEADERS); String userJson = JsonUtils.pojoToJson(user); user.setIsAdmin(Boolean.TRUE); assertResponse( @@ -871,7 +884,7 @@ void put_generateToken_bot_user_200_ok() throws HttpResponseException { .withEmail("ingestion-bot-jwt@email.com") .withRoles(List.of(ROLE1_REF.getId())) .withAuthenticationMechanism(authMechanism); - User user = createEntity(create, authHeaders("ingestion-bot-jwt@email.com")); + User user = createEntity(create, USER_WITH_CREATE_HEADERS); user = getEntity(user.getId(), "*", ADMIN_AUTH_HEADERS); assertEquals(1, user.getRoles().size()); TestUtils.put( @@ -922,7 +935,7 @@ void post_createUser_BasicAuth_AdminCreate_login_200_ok(TestInfo test) .withCreatePasswordType(CreateUser.CreatePasswordType.ADMIN_CREATE) .withPassword("Test@1234") .withConfirmPassword("Test@1234"), - authHeaders("testBasicAuth@email.com")); + USER_WITH_CREATE_HEADERS); // jwtAuth Response should be null always user = getEntity(user.getId(), ADMIN_AUTH_HEADERS); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/util/TestUtils.java b/openmetadata-service/src/test/java/org/openmetadata/service/util/TestUtils.java index cad0733a838c..49dd4f7d02c7 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/util/TestUtils.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/util/TestUtils.java @@ -104,6 +104,9 @@ public final class TestUtils { public static final String TEST_USER_NAME = "test"; public static final Map TEST_AUTH_HEADERS = authHeaders(TEST_USER_NAME + "@open-metadata.org"); + public static final String USER_WITH_CREATE_PERMISSION_NAME = "testWithCreateUserPermission"; + public static final Map USER_WITH_CREATE_HEADERS = + authHeaders(USER_WITH_CREATE_PERMISSION_NAME + "@open-metadata.org"); public static final UUID NON_EXISTENT_ENTITY = UUID.randomUUID();