Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#5661] feat(auth): Add JDBC authorization plugin interface #5904

Merged
merged 10 commits into from
Dec 24, 2024
Merged

[#5661] feat(auth): Add JDBC authorization plugin interface #5904

merged 10 commits into from
Dec 24, 2024

Conversation

jerqi
Copy link
Contributor

@jerqi jerqi commented Dec 18, 2024

What changes were proposed in this pull request?

Add JDBC authorization plugin interface

Why are the changes needed?

Fix: #5661

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add a UT

@jerqi jerqi self-assigned this Dec 18, 2024
@jerqi jerqi marked this pull request as draft December 18, 2024 08:27
@jerqi jerqi force-pushed the ISSUE-5661-2 branch 3 times, most recently from f8b1c08 to a144b64 Compare December 19, 2024 03:15
@jerqi jerqi marked this pull request as ready for review December 19, 2024 03:28
@jerqi jerqi force-pushed the ISSUE-5661-2 branch 2 times, most recently from c178a6d to 8601049 Compare December 19, 2024 03:38
@jerqi jerqi requested a review from xunliu December 19, 2024 06:57
@jerqi jerqi requested a review from xunliu December 19, 2024 09:29
@jerqi jerqi requested a review from xunliu December 19, 2024 12:42
tengqm
tengqm reviewed Dec 23, 2024
View reviewed changes
Copy link
Contributor

@tengqm tengqm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is too big to review effectively. Can we split it into a few smaller ones?

...tion-jdbc/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationObject.java Outdated
List<AuthorizationPrivilege> privileges;

JdbcAuthorizationObject(String database, String table, List<AuthorizationPrivilege> privileges) {
Preconditions.checkNotNull(database, "Jdbc authorization object database can't null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Preconditions.checkNotNull(database, "Jdbc authorization object database can't null");
Preconditions.checkNotNull(database, "JDBC authorization object database cannot be null");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Changed.

...tion-jdbc/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java
public void close() throws IOException {
if (dataSource != null) {
try {
dataSource.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this call be followed by a dataSource = null? Not sure about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's more safe.

...tion-jdbc/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java Outdated
revokeObjectPrivileges(role, removeObject);
grantObjectPrivileges(role, addObject);
} else {
throw new IllegalArgumentException(String.format("Don't support RoleChange %s", change));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new IllegalArgumentException(String.format("Don't support RoleChange %s", change));
throw new IllegalArgumentException(String.format("RoleChange is not supported - %s", change));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

...tion-jdbc/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java Outdated Show resolved Hide resolved
.../authorization-jdbc/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcPrivilege.java Outdated
ALL("ALL PRIVILEGES"),
CREATE("CREATE"),
DROP("DROP"),
USAGE("USAGE");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there is a better way to consolidate this enum type with the class constants defined earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:

public enum Type {
  SELECT(new JdbcPrivilege("SELECT"), "SELECT"),
  ...
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's weird. Because it's cyclic dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to xun's suggestion, I changed this class to enumeration.

@jerqi
Copy link
Contributor Author

jerqi commented Dec 23, 2024

This PR is too big to review effectively. Can we split it into a few smaller ones?

It's hard to split otherwise we can't get whole picture of this feature.

@jerqi jerqi requested a review from xunliu December 24, 2024 06:50
@jerqi jerqi requested a review from tengqm December 24, 2024 08:10
xunliu
xunliu approved these changes Dec 24, 2024
View reviewed changes
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xunliu xunliu merged commit a4c8630 into apache:main Dec 24, 2024
26 checks passed
@jerqi
Copy link
Contributor Author

jerqi commented Dec 24, 2024

@tengqm Do you have any other suggestion? I can raise a follow up pull request to fix them.

@tengqm
Copy link
Contributor

tengqm commented Dec 24, 2024

@tengqm Do you have any other suggestion? I can raise a follow up pull request to fix them.

Thanks for asking. None of the issues I raised were meant to be blockers.

@jerqi jerqi deleted the ISSUE-5661-2 branch December 24, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xunliu xunliu xunliu approved these changes

@tengqm tengqm Awaiting requested review from tengqm

Assignees

@jerqi jerqi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Add the SQL-Based authorization plugin interface
3 participants
@jerqi @tengqm @xunliu