roryqi commented on code in PR #11848:
URL: https://github.com/apache/gravitino/pull/11848#discussion_r3511053538
##########
common/src/main/java/org/apache/gravitino/dto/authorization/UserDTO.java:
##########
@@ -33,6 +33,12 @@ public class UserDTO implements User {
@JsonProperty("name")
private String name;
+ @JsonProperty("externalId")
Review Comment:
Should u use nullable annotation? I don't remember clearly. You can look for
other references.
##########
api/src/main/java/org/apache/gravitino/authorization/User.java:
##########
@@ -34,6 +35,21 @@ public interface User extends Auditable {
*/
String name();
+ /**
+ * The external identifier from an upstream identity system, such as a SCIM
provider.
+ *
+ * @return The external identifier, or null if not set.
+ */
+ @Nullable
+ String externalId();
Review Comment:
Could u use Optional instead of Nullable?
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java:
##########
@@ -75,6 +103,43 @@ User addUser(String metalake, String user)
*/
User getUser(String metalake, String user) throws NoSuchUserException,
NoSuchMetalakeException;
+ /**
+ * Gets a User by external identifier.
+ *
+ * @param metalake The Metalake of the User.
+ * @param externalId The external identifier of the User.
+ * @return The getting User instance.
+ * @throws NoSuchUserException If the User with the given external id does
not exist.
+ * @throws NoSuchMetalakeException If the Metalake with the given name does
not exist.
+ * @throws RuntimeException If getting the User encounters storage issues.
+ */
+ User getUserByExternalId(String metalake, String externalId)
+ throws NoSuchUserException, NoSuchMetalakeException;
+
+ /**
+ * Enables a User without removing role bindings.
+ *
+ * @param metalake The Metalake of the User.
+ * @param externalId The external identifier of the User.
+ * @return The updated User instance.
+ * @throws NoSuchUserException If the User with the given external id does
not exist.
+ * @throws NoSuchMetalakeException If the Metalake with the given name does
not exist.
+ */
+ User enableUser(String metalake, String externalId)
Review Comment:
Why do we use `externalId` instead of `user name`?
##########
core/src/main/java/org/apache/gravitino/EntityStore.java:
##########
@@ -167,6 +168,66 @@ <E extends Entity & HasIdentifier> E update(
<E extends Entity & HasIdentifier> E get(NameIdentifier ident, EntityType
entityType, Class<E> e)
throws NoSuchEntityException, IOException;
+ /**
+ * Get the entity from the underlying storage by external id within the
namespace.
+ *
+ * @param namespace the namespace of the entity
+ * @param entityType the general type of the entity
+ * @param type the detailed type of the entity
+ * @param externalId the external id of the entity
+ * @param <E> the class of entity
+ * @return the entity retrieved from the underlying storage
+ * @throws NoSuchEntityException if the entity does not exist
+ * @throws IOException if the retrieve operation fails
+ */
+ <E extends Entity & HasIdentifier> E getByExternalId(
+ Namespace namespace, EntityType entityType, Class<E> type, String
externalId)
+ throws NoSuchEntityException, IOException;
+
+ /**
+ * Update the enabled state of a user by external id within the user
namespace.
+ *
+ * @param namespace the user namespace of the metalake
+ * @param externalId the external id of the user
+ * @param enabled the expected enabled state
+ * @return the updated user entity
+ * @throws NoSuchEntityException if the user does not exist
+ * @throws IOException if the update operation fails
+ */
+ default UserEntity updateUserEnabledByExternalId(
Review Comment:
Why does entity store contain this interface?
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java:
##########
@@ -51,6 +51,21 @@ public interface AccessControlDispatcher {
User addUser(String metalake, String user)
throws UserAlreadyExistsException, NoSuchMetalakeException;
+ /**
+ * Adds a new User with an external identifier and enabled state.
+ *
+ * @param metalake The Metalake of the User.
+ * @param user The name of the User.
+ * @param externalId The external identifier from an upstream identity
system.
+ * @param enabled Whether the User is enabled.
+ * @return The added User instance.
+ * @throws UserAlreadyExistsException If a User with the same name already
exists.
+ * @throws NoSuchMetalakeException If the Metalake with the given name does
not exist.
+ * @throws RuntimeException If adding the User encounters storage issues.
+ */
+ User addUser(String metalake, String user, String externalId, boolean
enabled)
Review Comment:
Why do we choose add a method instead of adding a parameter?
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -145,6 +152,44 @@ public static NameIdentifier ofUser(String metalake,
String user) {
metalake, Entity.SYSTEM_CATALOG_RESERVED_NAME,
Entity.USER_SCHEMA_NAME, user);
}
+ /**
+ * Creates a name identifier for locking or addressing a user by external id.
+ *
+ * <p>The path uses a reserved prefix in the leaf segment so it does not
collide with {@link
+ * #ofUser(String, String)} when the external id equals a user name.
+ *
+ * @param metalake the metalake name
+ * @param externalId the external id of the user
+ * @return the name identifier of the user external id path
+ */
+ public static NameIdentifier ofUserExternalId(String metalake, String
externalId) {
Review Comment:
Why does the external id become a name identifier?
##########
core/src/main/java/org/apache/gravitino/EntityStore.java:
##########
@@ -167,6 +168,66 @@ <E extends Entity & HasIdentifier> E update(
<E extends Entity & HasIdentifier> E get(NameIdentifier ident, EntityType
entityType, Class<E> e)
throws NoSuchEntityException, IOException;
+ /**
+ * Get the entity from the underlying storage by external id within the
namespace.
+ *
+ * @param namespace the namespace of the entity
+ * @param entityType the general type of the entity
+ * @param type the detailed type of the entity
+ * @param externalId the external id of the entity
+ * @param <E> the class of entity
+ * @return the entity retrieved from the underlying storage
+ * @throws NoSuchEntityException if the entity does not exist
+ * @throws IOException if the retrieve operation fails
+ */
+ <E extends Entity & HasIdentifier> E getByExternalId(
Review Comment:
Why does entity store contain this interface? This is a specific interface.
It shouldn't occur in the entity store class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
