Copilot commented on code in PR #10971:
URL: https://github.com/apache/gravitino/pull/10971#discussion_r3213072408
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -685,5 +702,7 @@ private void initGravitinoServerComponents() {
BuiltInJobTemplateEventListener builtInJobTemplateListener =
new BuiltInJobTemplateEventListener(jobManager, entityStore,
idGenerator);
eventListenerManager.addEventListener("builtin-job-template",
builtInJobTemplateListener);
+
+ this.idpManager = IdpManagerFactory.create();
Review Comment:
`initGravitinoServerComponents()` unconditionally calls
`IdpManagerFactory.create()`, which throws if no `IdpManager` service provider
is on the runtime classpath. This can prevent Gravitino from starting in
deployments/dev runs that don’t ship the `idp-basic` plugin (even when built-in
IdP is not needed). Consider making IdP manager loading conditional (e.g.,
based on config/authenticator) or providing a default no-op implementation /
lazy-load with a clear 503/501 behavior for `/idp/*` endpoints when the plugin
is absent.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/authorization/IdpManager.java:
##########
@@ -0,0 +1,331 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.idp.basic.authorization;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.dto.IdpGroupDTO;
+import org.apache.gravitino.dto.IdpUserDTO;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.NoSuchGroupException;
+import org.apache.gravitino.exceptions.NoSuchUserException;
+import org.apache.gravitino.exceptions.UserAlreadyExistsException;
+import org.apache.gravitino.idp.basic.password.PasswordHasher;
+import org.apache.gravitino.idp.basic.password.PasswordHasherFactory;
+import org.apache.gravitino.storage.IdGenerator;
+import org.apache.gravitino.storage.relational.po.IdpGroupPO;
+import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO;
+import org.apache.gravitino.storage.relational.po.IdpUserPO;
+import org.apache.gravitino.storage.relational.service.IdpGroupMetaService;
+import org.apache.gravitino.storage.relational.service.IdpUserMetaService;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+/**
+ * Built-in IdP manager implementation loaded from the {@code idp-basic}
plugin.
+ *
+ * <p>This implementation manages both built-in IdP users and groups and
restricts mutation
+ * operations to Gravitino service admins.
+ */
+public class IdpManager implements
org.apache.gravitino.authorization.IdpManager {
+ private static final long INITIAL_VERSION = 1L;
+
+ private final Config config;
+ private final IdGenerator idGenerator;
+ private final IdpUserMetaService userMetaService;
+ private final IdpGroupMetaService groupMetaService;
+ private final PasswordHasher passwordHasher;
+
+ public IdpManager() {
+ this(
+ GravitinoEnv.getInstance().config(),
+ GravitinoEnv.getInstance().idGenerator(),
+ IdpUserMetaService.getInstance(),
+ IdpGroupMetaService.getInstance(),
+ PasswordHasherFactory.create());
+ }
+
+ IdpManager(
+ Config config,
+ IdGenerator idGenerator,
+ IdpUserMetaService userMetaService,
+ IdpGroupMetaService groupMetaService,
+ PasswordHasher passwordHasher) {
+ this.config = config;
+ this.idGenerator = idGenerator;
+ this.userMetaService = userMetaService;
+ this.groupMetaService = groupMetaService;
+ this.passwordHasher = passwordHasher;
+ }
+
+ @Override
+ public IdpUserDTO createUser(String userName, String password) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ validatePassword(password);
+ if (userMetaService().findUser(userName).isPresent()) {
+ throw new UserAlreadyExistsException("Built-in IdP user %s already
exists", userName);
+ }
+
+ userMetaService()
+ .createUser(
+ IdpUserPO.builder()
+ .withUserId(nextId())
+ .withUserName(userName)
+ .withPasswordHash(passwordHasher.hash(password))
+ .withCurrentVersion(INITIAL_VERSION)
+ .withLastVersion(INITIAL_VERSION)
+ .withDeletedAt(0L)
+ .build());
+ return getUser(userName);
+ }
+
+ @Override
+ public IdpUserDTO getUser(String userName) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ IdpUserPO userPO =
+ userMetaService()
+ .findUser(userName)
+ .orElseThrow(
+ () -> new NoSuchUserException("Built-in IdP user %s does not
exist", userName));
+ return toUserDTO(userPO);
+ }
+
+ @Override
+ public boolean deleteUser(String userName) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ Optional<IdpUserPO> user = userMetaService().findUser(userName);
+ if (!user.isPresent()) {
+ return false;
+ }
+
+ return userMetaService().deleteUser(user.get(),
System.currentTimeMillis());
+ }
+
+ @Override
+ public IdpUserDTO resetPassword(String userName, String password) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ validatePassword(password);
+ IdpUserPO userPO =
+ userMetaService()
+ .findUser(userName)
+ .orElseThrow(
+ () -> new NoSuchUserException("Built-in IdP user %s does not
exist", userName));
+ if (passwordHasher.verify(password, userPO.getPasswordHash())) {
+ throw new IllegalArgumentException(
+ "The new password must be different from the old password");
+ }
+
+ userMetaService()
+ .updatePassword(userPO, passwordHasher.hash(password),
userPO.getCurrentVersion() + 1);
+ return getUser(userName);
+ }
+
+ @Override
+ public IdpGroupDTO createGroup(String groupName) {
+ ensureServiceAdmin();
+ validateGroupName(groupName);
+ if (groupMetaService().findGroup(groupName).isPresent()) {
+ throw new GroupAlreadyExistsException("Built-in IdP group %s already
exists", groupName);
+ }
+
+ groupMetaService()
+ .createGroup(
+ IdpGroupPO.builder()
+ .withGroupId(nextId())
+ .withGroupName(groupName)
+ .withCurrentVersion(INITIAL_VERSION)
+ .withLastVersion(INITIAL_VERSION)
+ .withDeletedAt(0L)
+ .build());
+ return getGroup(groupName);
+ }
+
+ @Override
+ public IdpGroupDTO getGroup(String groupName) {
+ ensureServiceAdmin();
+ validateGroupName(groupName);
+ IdpGroupPO groupPO =
+ groupMetaService()
+ .findGroup(groupName)
+ .orElseThrow(
Review Comment:
`getGroup()` does not call `ensureServiceAdmin()`, so non-admin callers can
read built-in IdP groups and their user membership lists. If built-in IdP is
intended to be service-admin managed, consider enforcing the admin check for
reads as well (or add explicit authorization + documentation if reads are
intentionally public).
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/authorization/IdpManager.java:
##########
@@ -0,0 +1,331 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.idp.basic.authorization;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.dto.IdpGroupDTO;
+import org.apache.gravitino.dto.IdpUserDTO;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.NoSuchGroupException;
+import org.apache.gravitino.exceptions.NoSuchUserException;
+import org.apache.gravitino.exceptions.UserAlreadyExistsException;
+import org.apache.gravitino.idp.basic.password.PasswordHasher;
+import org.apache.gravitino.idp.basic.password.PasswordHasherFactory;
+import org.apache.gravitino.storage.IdGenerator;
+import org.apache.gravitino.storage.relational.po.IdpGroupPO;
+import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO;
+import org.apache.gravitino.storage.relational.po.IdpUserPO;
+import org.apache.gravitino.storage.relational.service.IdpGroupMetaService;
+import org.apache.gravitino.storage.relational.service.IdpUserMetaService;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+/**
+ * Built-in IdP manager implementation loaded from the {@code idp-basic}
plugin.
+ *
+ * <p>This implementation manages both built-in IdP users and groups and
restricts mutation
+ * operations to Gravitino service admins.
+ */
+public class IdpManager implements
org.apache.gravitino.authorization.IdpManager {
+ private static final long INITIAL_VERSION = 1L;
+
+ private final Config config;
+ private final IdGenerator idGenerator;
+ private final IdpUserMetaService userMetaService;
+ private final IdpGroupMetaService groupMetaService;
+ private final PasswordHasher passwordHasher;
+
+ public IdpManager() {
+ this(
+ GravitinoEnv.getInstance().config(),
+ GravitinoEnv.getInstance().idGenerator(),
+ IdpUserMetaService.getInstance(),
+ IdpGroupMetaService.getInstance(),
+ PasswordHasherFactory.create());
+ }
+
+ IdpManager(
+ Config config,
+ IdGenerator idGenerator,
+ IdpUserMetaService userMetaService,
+ IdpGroupMetaService groupMetaService,
+ PasswordHasher passwordHasher) {
+ this.config = config;
+ this.idGenerator = idGenerator;
+ this.userMetaService = userMetaService;
+ this.groupMetaService = groupMetaService;
+ this.passwordHasher = passwordHasher;
+ }
+
+ @Override
+ public IdpUserDTO createUser(String userName, String password) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ validatePassword(password);
+ if (userMetaService().findUser(userName).isPresent()) {
+ throw new UserAlreadyExistsException("Built-in IdP user %s already
exists", userName);
+ }
+
+ userMetaService()
+ .createUser(
+ IdpUserPO.builder()
+ .withUserId(nextId())
+ .withUserName(userName)
+ .withPasswordHash(passwordHasher.hash(password))
+ .withCurrentVersion(INITIAL_VERSION)
+ .withLastVersion(INITIAL_VERSION)
+ .withDeletedAt(0L)
+ .build());
+ return getUser(userName);
+ }
+
+ @Override
+ public IdpUserDTO getUser(String userName) {
+ ensureServiceAdmin();
+ validateUserName(userName);
+ IdpUserPO userPO =
+ userMetaService()
+ .findUser(userName)
+ .orElseThrow(
+ () -> new NoSuchUserException("Built-in IdP user %s does not
exist", userName));
Review Comment:
`getUser()` does not call `ensureServiceAdmin()`, so any authenticated (or
even anonymous, depending on auth setup) caller can read built-in IdP users and
their group memberships. This seems inconsistent with the service-admin
restriction used for other IdP operations and can expose identity/membership
data. Consider applying the same admin check to read APIs, or explicitly
document/authorize why reads are allowed for non-admins.
##########
docs/open-api/idp.yaml:
##########
@@ -0,0 +1,514 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+---
+
+paths:
+
+ /idp/users:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP user
+ operationId: addIdpUser
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpUserRequest"
+ examples:
+ CreateIdpUserRequest:
+ $ref: "#/components/examples/CreateIdpUserRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP user already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ UserAlreadyExistsException:
+ $ref: "#/components/examples/UserAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/users/{user}:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/user"
+
+ get:
+ tags:
+ - authentication
+ summary: Get built-in IdP user
+ operationId: getIdpUser
+ responses:
+ "200":
+ description: Returns the built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ put:
+ tags:
+ - authentication
+ summary: Reset built-in IdP user password
+ operationId: resetIdpUserPassword
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/ResetIdpUserPasswordRequest"
+ examples:
+ ResetIdpUserPasswordRequest:
+ $ref: "#/components/examples/ResetIdpUserPasswordRequest"
+ responses:
+ "200":
+ description: Returns the updated built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ delete:
+ tags:
+ - authentication
+ summary: Remove built-in IdP user
+ operationId: removeIdpUser
+ responses:
+ "200":
+ $ref: "./openapi.yaml#/components/responses/RemoveResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/groups:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP group
+ operationId: addIdpGroup
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpGroupRequest"
+ examples:
+ CreateIdpGroupRequest:
+ $ref: "#/components/examples/CreateIdpGroupRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP group
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpGroupResponse"
+ examples:
+ IdpGroupResponse:
+ $ref: "#/components/examples/IdpGroupResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP group already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ GroupAlreadyExistsException:
+ $ref: "#/components/examples/GroupAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/groups/{group}:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/group"
+
+ get:
+ tags:
+ - authentication
+ summary: Get built-in IdP group
+ operationId: getIdpGroup
+ responses:
+ "200":
+ description: Returns the built-in IdP group
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpGroupResponse"
+ examples:
+ IdpGroupResponse:
+ $ref: "#/components/examples/IdpGroupResponse"
+ "404":
+ description: Not Found - The specified built-in IdP group does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchGroupException:
+ $ref: "#/components/examples/NoSuchGroupException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ delete:
+ tags:
+ - authentication
+ summary: Remove built-in IdP group
+ operationId: removeIdpGroup
+ parameters:
+ - name: force
+ in: query
+ required: false
+ schema:
+ type: boolean
+ default: false
+ description: Whether to force removal of the built-in IdP group
+ responses:
+ "200":
+ $ref: "./openapi.yaml#/components/responses/RemoveResponse"
+ "404":
+ description: Not Found - The specified built-in IdP group does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchGroupException:
+ $ref: "#/components/examples/NoSuchGroupException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
Review Comment:
OpenAPI for `DELETE /idp/groups/{group}` documents a `404` response, but the
implementation returns `200` with `removed=false` when the group doesn’t exist.
It can also return `405 Method Not Allowed` when `force=false` and the group
still has users (mapped from `UnsupportedOperationException`), and `403` for
non-service-admin callers. Please update the spec (or adjust the API behavior)
to match the actual responses.
##########
docs/open-api/idp.yaml:
##########
@@ -0,0 +1,514 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+---
+
+paths:
+
+ /idp/users:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP user
+ operationId: addIdpUser
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpUserRequest"
+ examples:
+ CreateIdpUserRequest:
+ $ref: "#/components/examples/CreateIdpUserRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP user already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ UserAlreadyExistsException:
+ $ref: "#/components/examples/UserAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/users/{user}:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/user"
+
+ get:
+ tags:
+ - authentication
+ summary: Get built-in IdP user
+ operationId: getIdpUser
+ responses:
+ "200":
+ description: Returns the built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ put:
+ tags:
+ - authentication
+ summary: Reset built-in IdP user password
+ operationId: resetIdpUserPassword
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/ResetIdpUserPasswordRequest"
+ examples:
+ ResetIdpUserPasswordRequest:
+ $ref: "#/components/examples/ResetIdpUserPasswordRequest"
+ responses:
+ "200":
+ description: Returns the updated built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ delete:
+ tags:
+ - authentication
+ summary: Remove built-in IdP user
+ operationId: removeIdpUser
+ responses:
+ "200":
+ $ref: "./openapi.yaml#/components/responses/RemoveResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/groups:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP group
+ operationId: addIdpGroup
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpGroupRequest"
+ examples:
+ CreateIdpGroupRequest:
+ $ref: "#/components/examples/CreateIdpGroupRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP group
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpGroupResponse"
+ examples:
+ IdpGroupResponse:
+ $ref: "#/components/examples/IdpGroupResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP group already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ GroupAlreadyExistsException:
+ $ref: "#/components/examples/GroupAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/groups/{group}:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/group"
+
+ get:
+ tags:
+ - authentication
+ summary: Get built-in IdP group
+ operationId: getIdpGroup
+ responses:
+ "200":
+ description: Returns the built-in IdP group
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpGroupResponse"
+ examples:
+ IdpGroupResponse:
+ $ref: "#/components/examples/IdpGroupResponse"
+ "404":
+ description: Not Found - The specified built-in IdP group does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchGroupException:
+ $ref: "#/components/examples/NoSuchGroupException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ delete:
+ tags:
+ - authentication
+ summary: Remove built-in IdP group
+ operationId: removeIdpGroup
+ parameters:
+ - name: force
+ in: query
+ required: false
+ schema:
+ type: boolean
+ default: false
+ description: Whether to force removal of the built-in IdP group
+ responses:
+ "200":
+ $ref: "./openapi.yaml#/components/responses/RemoveResponse"
+ "404":
+ description: Not Found - The specified built-in IdP group does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchGroupException:
+ $ref: "#/components/examples/NoSuchGroupException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/groups/{group}/add:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/group"
+
+ put:
+ tags:
+ - authentication
+ summary: Add users to built-in IdP group
+ operationId: addUsersToIdpGroup
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/UpdateIdpGroupUsersRequest"
+ examples:
+ UpdateIdpGroupUsersRequest:
+ $ref: "#/components/examples/UpdateIdpGroupUsersRequest"
+ responses:
+ "200":
+ description: Returns the updated built-in IdP group
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpGroupResponse"
+ examples:
+ IdpGroupResponse:
+ $ref: "#/components/examples/IdpGroupResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "404":
+ description: Not Found - The specified built-in IdP group or user
does not exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchGroupException:
+ $ref: "#/components/examples/NoSuchGroupException"
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
Review Comment:
OpenAPI for `PUT /idp/groups/{group}/add` (and similarly `/remove`) is
missing a `403` response, but mutations are service-admin-only and can return
403 via `ForbiddenException`. Please document the 403 case for these endpoints.
##########
docs/open-api/idp.yaml:
##########
@@ -0,0 +1,514 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+---
+
+paths:
+
+ /idp/users:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP user
+ operationId: addIdpUser
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpUserRequest"
+ examples:
+ CreateIdpUserRequest:
+ $ref: "#/components/examples/CreateIdpUserRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP user already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ UserAlreadyExistsException:
+ $ref: "#/components/examples/UserAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
Review Comment:
OpenAPI for `POST /idp/users` is missing a `403` response, but the
implementation enforces service-admin-only mutations (and `ExceptionHandlers`
maps `ForbiddenException` to 403). Please document the 403 case so generated
clients and users understand the authorization requirement.
##########
core/src/main/java/org/apache/gravitino/authorization/IdpManagerFactory.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.authorization;
+
+import java.util.ServiceLoader;
+
+/** Factory for loading built-in IdP manager implementations from the runtime
classpath. */
+public final class IdpManagerFactory {
+
+ private IdpManagerFactory() {}
+
+ /** Create the built-in IdP manager implementation. */
+ public static IdpManager create() {
+ return loadService(IdpManager.class);
+ }
+
+ private static <T> T loadService(Class<T> serviceClass) {
+ for (T service : ServiceLoader.load(serviceClass)) {
+ return service;
+ }
+
+ throw new IllegalStateException(
+ String.format("No %s implementation found",
serviceClass.getSimpleName()));
Review Comment:
`IdpManagerFactory.loadService(...)` returns the first `ServiceLoader` match
without checking for multiple providers. If more than one IdP plugin is
present, the chosen implementation becomes order-dependent and
non-deterministic. Consider enforcing exactly one provider (throw on 0 or >1)
similar to other ServiceLoader-based factories in the repo.
##########
docs/open-api/idp.yaml:
##########
@@ -0,0 +1,514 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+---
+
+paths:
+
+ /idp/users:
+ post:
+ tags:
+ - authentication
+ summary: Add built-in IdP user
+ operationId: addIdpUser
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/CreateIdpUserRequest"
+ examples:
+ CreateIdpUserRequest:
+ $ref: "#/components/examples/CreateIdpUserRequest"
+ responses:
+ "200":
+ description: Returns the added built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "409":
+ description: Conflict - The target built-in IdP user already exists
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ UserAlreadyExistsException:
+ $ref: "#/components/examples/UserAlreadyExistsException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ /idp/users/{user}:
+ parameters:
+ - $ref: "./openapi.yaml#/components/parameters/user"
+
+ get:
+ tags:
+ - authentication
+ summary: Get built-in IdP user
+ operationId: getIdpUser
+ responses:
+ "200":
+ description: Returns the built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ put:
+ tags:
+ - authentication
+ summary: Reset built-in IdP user password
+ operationId: resetIdpUserPassword
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/ResetIdpUserPasswordRequest"
+ examples:
+ ResetIdpUserPasswordRequest:
+ $ref: "#/components/examples/ResetIdpUserPasswordRequest"
+ responses:
+ "200":
+ description: Returns the updated built-in IdP user
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "#/components/schemas/IdpUserResponse"
+ examples:
+ IdpUserResponse:
+ $ref: "#/components/examples/IdpUserResponse"
+ "400":
+ $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
+ $ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
+
+ delete:
+ tags:
+ - authentication
+ summary: Remove built-in IdP user
+ operationId: removeIdpUser
+ responses:
+ "200":
+ $ref: "./openapi.yaml#/components/responses/RemoveResponse"
+ "404":
+ description: Not Found - The specified built-in IdP user does not
exist
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ examples:
+ NoSuchUserException:
+ $ref: "#/components/examples/NoSuchUserException"
+ "5xx":
Review Comment:
OpenAPI for `DELETE /idp/users/{user}` documents a `404` Not Found response,
but the REST implementation returns `200` with `RemoveResponse.removed=false`
when the user doesn’t exist (it doesn’t throw `NoSuchUserException`). Either
adjust the API implementation to return 404 on missing users, or update the
spec to reflect the actual 200/removed=false behavior. Also consider
documenting possible `403` for non-service-admin callers.
--
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]
