yuqi1129 commented on code in PR #5113:
URL: https://github.com/apache/gravitino/pull/5113#discussion_r1805955938
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -522,36 +657,35 @@ private boolean
doRemoveSecurableObject(RoleChange.RemoveSecurableObject change)
return true;
}
- /**
- * 1. Find the policy base the metadata object. <br>
- * 2. If the policy exists, then user new securable object's privilege to
update. <br>
- * 3. If the policy does not exist, return false. <br>
- */
- private boolean doUpdateSecurableObject(RoleChange.UpdateSecurableObject
change) {
- RangerPolicy policy =
rangerHelper.findManagedPolicy(change.getSecurableObject());
- if (policy == null) {
- LOG.warn(
- "Cannot find the Ranger policy({}) for the Gravitino securable
object({})!",
- change.getRoleName(),
- change.getSecurableObject().fullName());
- // Don't throw exception or return false, because need support immutable
operation.
- return false;
- }
-
- rangerHelper.removePolicyItem(policy, change.getRoleName(),
change.getSecurableObject());
- rangerHelper.addPolicyItem(policy, change.getRoleName(),
change.getNewSecurableObject());
- try {
- if (policy.getId() == null) {
- rangerClient.createPolicy(policy);
- } else {
- rangerClient.updatePolicy(policy.getId(), policy);
- }
- } catch (RangerServiceException e) {
- throw new RuntimeException(e);
- }
- return true;
- }
-
@Override
public void close() throws IOException {}
+
+ public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
Review Comment:
The package access level is sufficient.
##########
.github/workflows/access-control-integration-test.yml:
##########
@@ -87,9 +87,9 @@ jobs:
- name: Authorization Integration Test (JDK${{ matrix.java-version }})
id: integrationTest
run: |
- ./gradlew -PskipTests -PtestMode=embedded -PjdbcBackend=h2
-PjdkVersion=${{ matrix.java-version }} -PskipDockerTests=false
:authorizations:authorization-ranger:test --tests
"org.apache.gravitino.authorization.ranger.integration.test.**"
- ./gradlew -PskipTests -PtestMode=deploy -PjdbcBackend=mysql
-PjdkVersion=${{ matrix.java-version }} -PskipDockerTests=false
:authorizations:authorization-ranger:test --tests
"org.apache.gravitino.authorization.ranger.integration.test.**"
- ./gradlew -PskipTests -PtestMode=deploy -PjdbcBackend=postgresql
-PjdkVersion=${{ matrix.java-version }} -PskipDockerTests=false
:authorizations:authorization-ranger:test --tests
"org.apache.gravitino.authorization.ranger.integration.test.**"
+ ./gradlew -PtestMode=embedded -PjdbcBackend=h2 -PjdkVersion=${{
matrix.java-version }} -PskipDockerTests=false
:authorizations:authorization-ranger:test --tests
"org.apache.gravitino.authorization.ranger.**"
Review Comment:
Why do we remove parameter `-PskipTests`?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -522,36 +657,35 @@ private boolean
doRemoveSecurableObject(RoleChange.RemoveSecurableObject change)
return true;
}
- /**
- * 1. Find the policy base the metadata object. <br>
- * 2. If the policy exists, then user new securable object's privilege to
update. <br>
- * 3. If the policy does not exist, return false. <br>
- */
- private boolean doUpdateSecurableObject(RoleChange.UpdateSecurableObject
change) {
- RangerPolicy policy =
rangerHelper.findManagedPolicy(change.getSecurableObject());
- if (policy == null) {
- LOG.warn(
- "Cannot find the Ranger policy({}) for the Gravitino securable
object({})!",
- change.getRoleName(),
- change.getSecurableObject().fullName());
- // Don't throw exception or return false, because need support immutable
operation.
- return false;
- }
-
- rangerHelper.removePolicyItem(policy, change.getRoleName(),
change.getSecurableObject());
- rangerHelper.addPolicyItem(policy, change.getRoleName(),
change.getNewSecurableObject());
- try {
- if (policy.getId() == null) {
- rangerClient.createPolicy(policy);
- } else {
- rangerClient.updatePolicy(policy.getId(), policy);
- }
- } catch (RangerServiceException e) {
- throw new RuntimeException(e);
- }
- return true;
- }
-
@Override
public void close() throws IOException {}
+
+ public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
+ return securableObjects.stream()
+ .noneMatch(
+ securableObject -> {
+ AtomicBoolean match = new AtomicBoolean(true);
+ securableObject.privileges().stream()
+ .forEach(
+ privilege -> {
+ if (!allowPrivilegesRule().contains(privilege.name()))
{
+ LOG.error(
+ "Authorization to ignore privilege({}) on
metadata object({})!",
+ privilege.name(),
+ securableObject.fullName());
+ match.set(false);
+ return;
+ }
+
+ if (!privilege.canBindTo(securableObject.type())) {
+ LOG.error(
+ "The privilege({}) is not supported for the
metadata object({})!",
+ privilege.name(),
+ securableObject.fullName());
+ match.set(false);
+ }
+ });
+ return !match.get();
+ });
Review Comment:
```suggestion
return securableObjects.stream()
.allMatch(
securableObject -> {
AtomicBoolean match = new AtomicBoolean(true);
securableObject.privileges().stream()
.forEach(
privilege -> {
if
(!allowPrivilegesRule().contains(privilege.name())) {
LOG.error(
"Authorization to ignore privilege({}) on
metadata object({})!",
privilege.name(),
securableObject.fullName());
match.set(false);
return;
}
if (!privilege.canBindTo(securableObject.type())) {
LOG.error(
"The privilege({}) is not supported for the
metadata object({})!",
privilege.name(),
securableObject.fullName());
match.set(false);
}
});
return match.get();
});
```
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerSecurableObject.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.ranger;
+
+import java.util.List;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.annotation.Unstable;
+
+/**
+ * The Ranger securable object is the entity which access can be granted.
Unless allowed by a grant,
+ * access is denied. <br>
+ * You can use the helper class `RangerSecurableObjects` to create the Ranger
securable object which
+ * you need.
+ */
+@Unstable
+public interface RangerSecurableObject extends MetadataObject {
Review Comment:
I guess `RangerSecurableObject` is the corresponding representing object of
`SecurableObject` in the Rander service, is that right? please add more to
clarify it.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -78,41 +82,34 @@ protected RangerAuthorizationPlugin(Map<String, String>
config) {
// Apache Ranger Password should be minimum 8 characters with min one
alphabet and one numeric.
String password = config.get(AuthorizationPropertiesMeta.RANGER_PASSWORD);
rangerServiceName =
config.get(AuthorizationPropertiesMeta.RANGER_SERVICE_NAME);
- RangerHelper.check(rangerUrl != null, "Ranger admin URL is required");
- RangerHelper.check(authType != null, "Ranger auth type is required");
- RangerHelper.check(rangerAdminName != null, "Ranger username is required");
- RangerHelper.check(password != null, "Ranger password is required");
- RangerHelper.check(rangerServiceName != null, "Ranger service name is
required");
+ Preconditions.checkArgument(rangerUrl != null, "Ranger admin URL is
required");
+ Preconditions.checkArgument(authType != null, "Ranger auth type is
required");
+ Preconditions.checkArgument(rangerAdminName != null, "Ranger username is
required");
+ Preconditions.checkArgument(password != null, "Ranger password is
required");
+ Preconditions.checkArgument(rangerServiceName != null, "Ranger service
name is required");
rangerClient = new RangerClientExtension(rangerUrl, authType,
rangerAdminName, password);
rangerHelper =
new RangerHelper(
rangerClient,
rangerAdminName,
rangerServiceName,
- privilegesMappingRule(),
ownerMappingRule(),
policyResourceDefinesRule());
}
- /**
- * Translate the privilege name to the corresponding privilege name in the
Ranger
- *
- * @param name The privilege name to translate
- * @return The corresponding Ranger privilege name in the underlying
permission system
- */
- public Set<String> translatePrivilege(Privilege.Name name) {
- return rangerHelper.translatePrivilege(name);
- }
-
/**
* Create a new role in the Ranger. <br>
* 1. Create a policy for metadata object. <br>
* 2. Save role name in the Policy items. <br>
*/
@Override
public Boolean onRoleCreated(Role role) throws RuntimeException {
- rangerHelper.createRangerRoleIfNotExists(role.name());
+ if (!validAuthorizationOperation(role.securableObjects())) {
+ return false;
Review Comment:
So I wonder what the return value means for `onRoleCreated`? what does the
`true` and `false` means here?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivileges.java:
##########
@@ -18,25 +18,118 @@
*/
package org.apache.gravitino.authorization.ranger;
+import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import java.util.List;
+import org.apache.gravitino.authorization.Privilege;
public class RangerPrivileges {
+ /** Ranger Hive privileges enumeration. */
+ public enum RangerHivePrivilege implements RangerPrivilege {
+ ALL("all"),
+ SELECT("select"),
+ UPDATE("update"),
+ CREATE("create"),
+ DROP("drop"),
+ ALTER("alter"),
+ INDEX("index"),
+ LOCK("lock"),
Review Comment:
What's the meaning of `lock` privilege?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -522,36 +657,35 @@ private boolean
doRemoveSecurableObject(RoleChange.RemoveSecurableObject change)
return true;
}
- /**
- * 1. Find the policy base the metadata object. <br>
- * 2. If the policy exists, then user new securable object's privilege to
update. <br>
- * 3. If the policy does not exist, return false. <br>
- */
- private boolean doUpdateSecurableObject(RoleChange.UpdateSecurableObject
change) {
- RangerPolicy policy =
rangerHelper.findManagedPolicy(change.getSecurableObject());
- if (policy == null) {
- LOG.warn(
- "Cannot find the Ranger policy({}) for the Gravitino securable
object({})!",
- change.getRoleName(),
- change.getSecurableObject().fullName());
- // Don't throw exception or return false, because need support immutable
operation.
- return false;
- }
-
- rangerHelper.removePolicyItem(policy, change.getRoleName(),
change.getSecurableObject());
- rangerHelper.addPolicyItem(policy, change.getRoleName(),
change.getNewSecurableObject());
- try {
- if (policy.getId() == null) {
- rangerClient.createPolicy(policy);
- } else {
- rangerClient.updatePolicy(policy.getId(), policy);
- }
- } catch (RangerServiceException e) {
- throw new RuntimeException(e);
- }
- return true;
- }
-
@Override
public void close() throws IOException {}
+
+ public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
+ return securableObjects.stream()
+ .noneMatch(
+ securableObject -> {
+ AtomicBoolean match = new AtomicBoolean(true);
+ securableObject.privileges().stream()
+ .forEach(
+ privilege -> {
+ if (!allowPrivilegesRule().contains(privilege.name()))
{
+ LOG.error(
+ "Authorization to ignore privilege({}) on
metadata object({})!",
Review Comment:
Since you are going to **ignore** privilege, why do you use the error level
log and return false here?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -123,56 +107,47 @@ void checkPolicyItemAccess(RangerPolicy.RangerPolicyItem
policyItem)
* We cannot clean the policy items because one Ranger policy maybe contains
multiple Gravitino
* securable objects. <br>
*/
- void addPolicyItem(RangerPolicy policy, String roleName, SecurableObject
securableObject) {
- // First check the privilege if support in the Ranger Hive
- checkPrivileges(securableObject);
-
+ void addPolicyItem(RangerPolicy policy, String roleName,
RangerSecurableObject securableObject) {
// Add the policy items by the securable object's privileges
securableObject
.privileges()
.forEach(
- gravitinoPrivilege -> {
- // Translate the Gravitino privilege to map Ranger privilege
- translatePrivilege(gravitinoPrivilege.name())
- .forEach(
- rangerPrivilege -> {
- // Find the policy item that matches Gravitino
privilege
- List<RangerPolicy.RangerPolicyItem> matchPolicyItems =
- policy.getPolicyItems().stream()
- .filter(
- policyItem -> {
- return policyItem.getAccesses().stream()
- .anyMatch(
- access ->
access.getType().equals(rangerPrivilege));
- })
- .collect(Collectors.toList());
-
- if (matchPolicyItems.size() == 0) {
- // If the policy item does not exist, then create a
new policy item
- RangerPolicy.RangerPolicyItem policyItem =
- new RangerPolicy.RangerPolicyItem();
- RangerPolicy.RangerPolicyItemAccess access =
- new RangerPolicy.RangerPolicyItemAccess();
- access.setType(rangerPrivilege);
- policyItem.getAccesses().add(access);
- policyItem.getRoles().add(roleName);
- if (Privilege.Condition.ALLOW ==
gravitinoPrivilege.condition()) {
- policy.getPolicyItems().add(policyItem);
- } else {
- policy.getDenyPolicyItems().add(policyItem);
+ rangerPrivilege -> {
+ // Find the policy item that matches Gravitino privilege
+ List<RangerPolicy.RangerPolicyItem> matchPolicyItems =
+ policy.getPolicyItems().stream()
+ .filter(
+ policyItem -> {
+ return policyItem.getAccesses().stream()
+ .anyMatch(
+ access ->
access.getType().equals(rangerPrivilege.getName()));
+ })
+ .collect(Collectors.toList());
+
+ if (matchPolicyItems.size() == 0) {
Review Comment:
isEmpty()
--
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]
