Skip to content

Commit

Permalink
[ISSUE-16503] Fix createUser to use EntityResource (#16549)
Browse files Browse the repository at this point in the history
* Fix createUser to use EntityResource

* fix broken tests

* Fix Tests - 3
  • Loading branch information
mohityadav766 authored Jun 7, 2024
1 parent 38e2793 commit aeb020a
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -548,44 +548,60 @@ 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) {
addAuthMechanismToUser(user, create);
}
// 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(
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
public static Domain DOMAIN1;

// Users
public static User USER_WITH_CREATE_ACCESS;
public static User USER1;
public static EntityReference USER1_REF;
public static User USER2;
Expand All @@ -272,11 +273,9 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
public static Team
TEAM2; // Team 2 has team only policy and does not allow access to users not in team hierarchy
public static Team TEAM21; // Team under Team2

public static User DATA_STEWARD;
public static Persona DATA_ENGINEER;
public static Persona DATA_SCIENTIST;

public static Document ACTIVITY_FEED_KNOWLEDGE_PANEL;
public static Document MY_DATA_KNOWLEDGE_PANEL;
public static User USER_WITH_DATA_STEWARD_ROLE;
Expand All @@ -286,9 +285,10 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
public static EntityReference DATA_CONSUMER_REF;
public static Role DATA_CONSUMER_ROLE;
public static EntityReference DATA_CONSUMER_ROLE_REF;
public static Role CREATE_ACCESS_ROLE;
public static Role ROLE1;
public static EntityReference ROLE1_REF;

public static Policy CREATE_ACCESS_PERMISSION_POLICY;
public static Policy POLICY1;
public static Policy POLICY2;
public static Policy TEAM_ONLY_POLICY;
Expand Down Expand Up @@ -964,7 +964,8 @@ void get_entityWithDifferentFields_200_OK(TestInfo test) throws IOException {
if (supportsFollowers) {
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);
addFollower(entity.getId(), user1.getId(), OK, TEST_AUTH_HEADERS);
}
entity = validateGetWithDifferentFields(entity, false);
Expand Down Expand Up @@ -1618,7 +1619,8 @@ void put_addDeleteFollower_200(TestInfo test) throws IOException {
// Add follower to the entity
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);
addAndCheckFollower(entityId, user1.getId(), OK, 1, TEST_AUTH_HEADERS);

// Add the same user as follower and make sure no errors are thrown
Expand All @@ -1627,7 +1629,8 @@ void put_addDeleteFollower_200(TestInfo test) throws IOException {

// Add a new follower to the entity
User user2 =
userResourceTest.createEntity(userResourceTest.createRequest(test, 2), TEST_AUTH_HEADERS);
userResourceTest.createEntity(
userResourceTest.createRequest(test, 2), USER_WITH_CREATE_HEADERS);
addAndCheckFollower(entityId, user2.getId(), OK, 2, TEST_AUTH_HEADERS);

// Delete followers and make sure they are deleted
Expand All @@ -1648,7 +1651,8 @@ void put_addFollowerDeleteEntity_200(TestInfo test) throws IOException {
// Add follower to the entity
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);
addAndCheckFollower(entityId, user1.getId(), OK, 1, TEST_AUTH_HEADERS);

deleteEntity(entityId, ADMIN_AUTH_HEADERS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ public PolicyResourceTest() {
}

public void setupPolicies() throws IOException {
CREATE_ACCESS_PERMISSION_POLICY =
createEntity(createAccessControlPolicyWithCreateRule(), ADMIN_AUTH_HEADERS);
POLICY1 = createEntity(createRequest("policy1").withOwner(null), ADMIN_AUTH_HEADERS);
POLICY2 = createEntity(createRequest("policy2").withOwner(null), ADMIN_AUTH_HEADERS);
TEAM_ONLY_POLICY = getEntityByName("TeamOnlyPolicy", "", ADMIN_AUTH_HEADERS);
Expand Down Expand Up @@ -769,6 +771,19 @@ private CreatePolicy createAccessControlPolicyWithRules(String name, List<Rule>
.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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<UUID> users = Arrays.asList(user1.getId(), user2.getId());

CreatePersona create =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<UUID> users = Arrays.asList(user1.getId(), user2.getId());

RoleResourceTest roleResourceTest = new RoleResourceTest();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ public final class TestUtils {
public static final String TEST_USER_NAME = "test";
public static final Map<String, String> TEST_AUTH_HEADERS =
authHeaders(TEST_USER_NAME + "@open-metadata.org");
public static final String USER_WITH_CREATE_PERMISSION_NAME = "testWithCreateUserPermission";
public static final Map<String, String> USER_WITH_CREATE_HEADERS =
authHeaders(USER_WITH_CREATE_PERMISSION_NAME + "@open-metadata.org");

public static final UUID NON_EXISTENT_ENTITY = UUID.randomUUID();

Expand Down

0 comments on commit aeb020a

Please sign in to comment.