-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#5116][#5106][#4616][#5135] improve(auth-ranger): The owner of catalog/metalake should have all the privileges of schemas/tables #5113
base: main
Are you sure you want to change the base?
Conversation
dcafa1d
to
b2cb8f2
Compare
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHivePlugin.java
Outdated
Show resolved
Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java
Show resolved
Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java
Show resolved
Hide resolved
./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.**" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove parameter -PskipTests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because access-control-integration-test.yml
needs to test all of the Authorization mode.
@Override | ||
public void close() throws IOException {} | ||
|
||
public boolean validAuthorizationOperation(List<SecurableObject> securableObjects) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package access level is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
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(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); | |
}); | |
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(); | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
privilege -> { | ||
if (!allowPrivilegesRule().contains(privilege.name())) { | ||
LOG.error( | ||
"Authorization to ignore privilege({}) on metadata object({})!", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are going to ignore privilege, why do you use the error level log and return false here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input privilege is not in the list of allowed privileges, so it needs to be ignored, and the error log output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, I suggest the information 'xxxx privileges are not allowed/appliable for securable object xxxx'
/** | ||
* 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I wonder what the return value means for onRoleCreated
? what does the true
and false
means here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tells the caller that the execution succeeded or failed
}) | ||
.collect(Collectors.toList()); | ||
|
||
if (matchPolicyItems.size() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isEmpty()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
DROP("drop"), | ||
ALTER("alter"), | ||
INDEX("index"), | ||
LOCK("lock"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of lock
privilege?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hive privilege.
* you need. | ||
*/ | ||
@Unstable | ||
public interface RangerSecurableObject extends MetadataObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess RangerSecurableObject
is the corresponding representing object of SecurableObject
in the Rander service, is that right? please add more to clarify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
hi @yuqi1129 Please help me review this PR again, Thanks. |
What changes were proposed in this pull request?
The owner of catalog/metalake should have all the privileges of schemas/tables.
Why are the changes needed?
Fix:
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
Add ITs.