Skip to content

Commit

Permalink
[#3132] improvement(core): Use a more elegant validation of User, Gro…
Browse files Browse the repository at this point in the history
…up and Role NameIdentifier (#3143)

### What changes were proposed in this pull request?

add `checkUser`, `checkGroup`, `checkRole` in `NameIdentifier`

### Why are the changes needed?

Fix: #3132 

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

ut

---------

Co-authored-by: yangliwei <[email protected]>
  • Loading branch information
lw-yang and lw-yang authored Apr 24, 2024
1 parent de720e9 commit 1200886
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,40 @@ public static Namespace ofGroupNamespace(String metalake) {
public static Namespace ofUserNamespace(String metalake) {
return Namespace.of(metalake, Entity.SYSTEM_CATALOG_RESERVED_NAME, Entity.USER_SCHEMA_NAME);
}

public static void checkUser(NameIdentifier ident) {
NameIdentifier.check(ident != null, "User identifier must not be null");
checkUserNamespace(ident.namespace());
}

public static void checkGroup(NameIdentifier ident) {
NameIdentifier.check(ident != null, "Group identifier must not be null");
checkGroupNamespace(ident.namespace());
}

public static void checkRole(NameIdentifier ident) {
NameIdentifier.check(ident != null, "Role identifier must not be null");
checkRoleNamespace(ident.namespace());
}

public static void checkUserNamespace(Namespace namespace) {
Namespace.check(
namespace != null && namespace.length() == 3,
"User namespace must have 3 levels, the input namespace is %s",
namespace);
}

public static void checkGroupNamespace(Namespace namespace) {
Namespace.check(
namespace != null && namespace.length() == 3,
"Group namespace must have 3 levels, the input namespace is %s",
namespace);
}

public static void checkRoleNamespace(Namespace namespace) {
Namespace.check(
namespace != null && namespace.length() == 3,
"Role namespace must have 3 levels, the input namespace is %s",
namespace);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.datastrato.gravitino.Entity;
import com.datastrato.gravitino.HasIdentifier;
import com.datastrato.gravitino.NameIdentifier;
import com.datastrato.gravitino.authorization.AuthorizationUtils;
import com.datastrato.gravitino.exceptions.NoSuchEntityException;
import com.datastrato.gravitino.meta.GroupEntity;
import com.datastrato.gravitino.storage.relational.mapper.GroupMetaMapper;
Expand Down Expand Up @@ -67,11 +68,8 @@ private Long getGroupIdByMetalakeIdAndName(Long metalakeId, String groupName) {
}

public GroupEntity getGroupByIdentifier(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkGroup(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
GroupPO groupPO = getGroupPOByMetalakeIdAndName(metalakeId, identifier.name());
Expand All @@ -82,11 +80,7 @@ public GroupEntity getGroupByIdentifier(NameIdentifier identifier) {

public void insertGroup(GroupEntity groupEntity, boolean overwritten) {
try {
Preconditions.checkArgument(
groupEntity.namespace() != null
&& !groupEntity.namespace().isEmpty()
&& groupEntity.namespace().levels().length == 3,
"The namespace of groupEntity should not be null and should have three level.");
AuthorizationUtils.checkGroup(groupEntity.nameIdentifier());

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(groupEntity.namespace().level(0));
Expand Down Expand Up @@ -130,11 +124,8 @@ public void insertGroup(GroupEntity groupEntity, boolean overwritten) {
}

public boolean deleteGroup(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkGroup(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
Long groupId = getGroupIdByMetalakeIdAndName(metalakeId, identifier.name());
Expand All @@ -152,11 +143,7 @@ public boolean deleteGroup(NameIdentifier identifier) {

public <E extends Entity & HasIdentifier> GroupEntity updateGroup(
NameIdentifier identifier, Function<E, E> updater) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkGroup(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import com.datastrato.gravitino.Entity;
import com.datastrato.gravitino.NameIdentifier;
import com.datastrato.gravitino.authorization.AuthorizationUtils;
import com.datastrato.gravitino.exceptions.NoSuchEntityException;
import com.datastrato.gravitino.meta.RoleEntity;
import com.datastrato.gravitino.storage.relational.mapper.GroupRoleRelMapper;
Expand All @@ -15,7 +16,6 @@
import com.datastrato.gravitino.storage.relational.utils.ExceptionUtils;
import com.datastrato.gravitino.storage.relational.utils.POConverters;
import com.datastrato.gravitino.storage.relational.utils.SessionUtils;
import com.google.common.base.Preconditions;
import java.util.List;

/** The service class for role metadata. It provides the basic database operations for role. */
Expand Down Expand Up @@ -70,11 +70,7 @@ public List<RolePO> listRolesByGroupId(Long groupId) {

public void insertRole(RoleEntity roleEntity, boolean overwritten) {
try {
Preconditions.checkArgument(
roleEntity.namespace() != null
&& !roleEntity.namespace().isEmpty()
&& roleEntity.namespace().levels().length == 3,
"The namespace of RoleEntity must have three levels.");
AuthorizationUtils.checkRole(roleEntity.nameIdentifier());

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(roleEntity.namespace().level(0));
Expand All @@ -99,11 +95,8 @@ public void insertRole(RoleEntity roleEntity, boolean overwritten) {
}

public RoleEntity getRoleByIdentifier(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier of Role must have three levels.");
AuthorizationUtils.checkRole(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
RolePO rolePO = getRolePOByMetalakeIdAndName(metalakeId, identifier.name());
Expand All @@ -112,11 +105,8 @@ public RoleEntity getRoleByIdentifier(NameIdentifier identifier) {
}

public boolean deleteRole(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier of Role must have three levels.");
AuthorizationUtils.checkRole(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
Long roleId = getRoleIdByMetalakeIdAndName(metalakeId, identifier.name());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.datastrato.gravitino.Entity;
import com.datastrato.gravitino.HasIdentifier;
import com.datastrato.gravitino.NameIdentifier;
import com.datastrato.gravitino.authorization.AuthorizationUtils;
import com.datastrato.gravitino.exceptions.NoSuchEntityException;
import com.datastrato.gravitino.meta.UserEntity;
import com.datastrato.gravitino.storage.relational.mapper.UserMetaMapper;
Expand Down Expand Up @@ -67,11 +68,8 @@ private Long getUserIdByMetalakeIdAndName(Long metalakeId, String userName) {
}

public UserEntity getUserByIdentifier(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkUser(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
UserPO userPO = getUserPOByMetalakeIdAndName(metalakeId, identifier.name());
Expand All @@ -82,11 +80,7 @@ public UserEntity getUserByIdentifier(NameIdentifier identifier) {

public void insertUser(UserEntity userEntity, boolean overwritten) {
try {
Preconditions.checkArgument(
userEntity.namespace() != null
&& !userEntity.namespace().isEmpty()
&& userEntity.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkUser(userEntity.nameIdentifier());

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(userEntity.namespace().level(0));
Expand Down Expand Up @@ -130,11 +124,8 @@ public void insertUser(UserEntity userEntity, boolean overwritten) {
}

public boolean deleteUser(NameIdentifier identifier) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkUser(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
Long userId = getUserIdByMetalakeIdAndName(metalakeId, identifier.name());
Expand All @@ -151,11 +142,7 @@ public boolean deleteUser(NameIdentifier identifier) {

public <E extends Entity & HasIdentifier> UserEntity updateUser(
NameIdentifier identifier, Function<E, E> updater) {
Preconditions.checkArgument(
identifier != null
&& !identifier.namespace().isEmpty()
&& identifier.namespace().levels().length == 3,
"The identifier should not be null and should have three level.");
AuthorizationUtils.checkUser(identifier);

Long metalakeId =
MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*
* Copyright 2024 Datastrato Pvt Ltd.
* This software is licensed under the Apache License version 2.
*/
package com.datastrato.gravitino.authorization;

import com.datastrato.gravitino.NameIdentifier;
import com.datastrato.gravitino.Namespace;
import com.datastrato.gravitino.exceptions.IllegalNameIdentifierException;
import com.datastrato.gravitino.exceptions.IllegalNamespaceException;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

class TestAuthorizationUtils {

String metalake = "metalake";

@Test
void testCreateNameIdentifier() {
NameIdentifier user = AuthorizationUtils.ofUser(metalake, "user");
NameIdentifier group = AuthorizationUtils.ofGroup(metalake, "group");
NameIdentifier role = AuthorizationUtils.ofRole(metalake, "role");

Assertions.assertEquals(AuthorizationUtils.ofUserNamespace(metalake), user.namespace());
Assertions.assertEquals("user", user.name());
Assertions.assertEquals(AuthorizationUtils.ofGroupNamespace(metalake), group.namespace());
Assertions.assertEquals("group", group.name());
Assertions.assertEquals(AuthorizationUtils.ofRoleNamespace(metalake), role.namespace());
Assertions.assertEquals("role", role.name());
}

@Test
void testCreateNameIdentifierWithInvalidArgs() {
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUser(metalake, null));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUser(metalake, ""));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroup(metalake, null));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroup(metalake, ""));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRole(metalake, null));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRole(metalake, ""));
}

@Test
void testCreateNamespace() {
Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake);
Assertions.assertEquals(3, namespace.length());
Assertions.assertEquals(metalake, namespace.level(0));
Assertions.assertEquals("system", namespace.level(1));
Assertions.assertEquals("user", namespace.level(2));

namespace = AuthorizationUtils.ofGroupNamespace(metalake);
Assertions.assertEquals(3, namespace.length());
Assertions.assertEquals(metalake, namespace.level(0));
Assertions.assertEquals("system", namespace.level(1));
Assertions.assertEquals("group", namespace.level(2));

namespace = AuthorizationUtils.ofRoleNamespace(metalake);
Assertions.assertEquals(3, namespace.length());
Assertions.assertEquals(metalake, namespace.level(0));
Assertions.assertEquals("system", namespace.level(1));
Assertions.assertEquals("role", namespace.level(2));
}

@Test
void testCreateNamespaceWithInvalidArgs() {
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofUserNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofUserNamespace(""));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofGroupNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofGroupNamespace(""));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofRoleNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.ofRoleNamespace(""));
}

@Test
void testCheckNameIdentifier() {
NameIdentifier user = AuthorizationUtils.ofUser(metalake, "user");
NameIdentifier group = AuthorizationUtils.ofGroup(metalake, "group");
NameIdentifier role = AuthorizationUtils.ofRole(metalake, "role");

Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkUser(user));
Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroup(group));
Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRole(role));

Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkUser(null));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkGroup(null));
Assertions.assertThrows(
IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkRole(null));
Assertions.assertThrows(
IllegalNameIdentifierException.class,
() -> AuthorizationUtils.checkUser(NameIdentifier.of("")));
Assertions.assertThrows(
IllegalNameIdentifierException.class,
() -> AuthorizationUtils.checkGroup(NameIdentifier.of("")));
Assertions.assertThrows(
IllegalNameIdentifierException.class,
() -> AuthorizationUtils.checkRole(NameIdentifier.of("")));
}

@Test
void testCheckNamespace() {
Namespace userNamespace = AuthorizationUtils.ofUserNamespace(metalake);
Namespace groupNamespace = AuthorizationUtils.ofGroupNamespace(metalake);
Namespace roleNamespace = AuthorizationUtils.ofRoleNamespace(metalake);

Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkUserNamespace(userNamespace));
Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroupNamespace(groupNamespace));
Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRoleNamespace(roleNamespace));

Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.checkUserNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.checkGroupNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class, () -> AuthorizationUtils.checkRoleNamespace(null));
Assertions.assertThrows(
IllegalNamespaceException.class,
() -> AuthorizationUtils.checkUserNamespace(Namespace.of("a", "b")));
Assertions.assertThrows(
IllegalNamespaceException.class,
() -> AuthorizationUtils.checkGroupNamespace(Namespace.of("a")));
Assertions.assertThrows(
IllegalNamespaceException.class,
() -> AuthorizationUtils.checkRoleNamespace(Namespace.of("a", "b", "c", "d")));
}
}

0 comments on commit 1200886

Please sign in to comment.