You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/04/16 11:30:29 UTC
[GitHub] [shardingsphere] totalo opened a new pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
totalo opened a new pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115
Fixes #10101 .
Changes proposed in this pull request:
- Implement PostgreSQLPrivilegeHandler and add unit test
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero merged pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r615536039
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -46,25 +45,56 @@
* PostgreSQL privilege handler.
*/
public final class PostgreSQLPrivilegeHandler implements StoragePrivilegeHandler {
-
+
+ private static final String CREATE_USER_SQL = "CREATE USER %s WITH PASSWORD '%s'";
+
+ private static final String GRANT_ALL_SQL = "GRANT ALL ON ALL TABLES IN SCHEMA public TO %s";
+
private static final String ROLES_SQL = "select * from pg_roles WHERE rolname IN (%s)";
-
- private static final String TABLE_PRIVILEGE_SQL =
+
+ private static final String TABLE_PRIVILEGE_SQL =
"SELECT grantor, grantee, table_catalog, table_name, privilege_type, is_grantable from information_schema.table_privileges WHERE grantee IN (%s)";
-
+
@Override
public Collection<ShardingSphereUser> diff(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
- return Collections.emptyList();
+ Collection<Grantee> grantees = new LinkedList<>();
+ try (Connection connection = dataSource.getConnection()) {
+ Statement statement = connection.createStatement();
+ try (ResultSet resultSet = statement.executeQuery(getRolePrivilegesSQL(users))) {
+ while (resultSet.next()) {
+ grantees.add(new Grantee(resultSet.getString("rolname"), ""));
+ }
+ }
+ }
+ return users.stream().filter(each -> !grantees.contains(each.getGrantee())).collect(Collectors.toList());
}
-
+
@Override
public void create(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
+ try (Connection connection = dataSource.getConnection(); Statement statement = connection.createStatement()) {
+ for (ShardingSphereUser each : users) {
+ statement.execute(getCreateUsersSQL(each));
+ }
+ }
+ }
+
+ private String getCreateUsersSQL(final ShardingSphereUser users) {
+ return String.format(CREATE_USER_SQL, users.getGrantee(), users.getPassword());
Review comment:
Yes, in postgresql, the host configuration does not seem to be tailored to the user. You can only bind the corresponding host by modifying `postgresql.conf` and `pg_hda.conf`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r616272125
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -130,19 +160,19 @@ private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges>
}
}
}
-
+
private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges> userPrivilegeMap, final ResultSet resultSet) throws SQLException {
Optional<ShardingSphereUser> user = findShardingSphereUser(userPrivilegeMap, resultSet);
if (user.isPresent()) {
userPrivilegeMap.get(user.get()).getAdministrativePrivileges().getPrivileges().addAll(loadRolePrivileges(resultSet));
}
}
-
+
Review comment:
Okay, thank you, I'm happy to do this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#issuecomment-823910690
Hey, Surely.
I will update here once some issues are ready for you.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#issuecomment-823765520
> @totalo I can see your being-careful from this PR, great.
hhhh , I am happy to do these things. BTW, I want to participate in contributing more things. Can you give me some more directions?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r615537078
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -130,19 +160,19 @@ private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges>
}
}
}
-
+
private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges> userPrivilegeMap, final ResultSet resultSet) throws SQLException {
Optional<ShardingSphereUser> user = findShardingSphereUser(userPrivilegeMap, resultSet);
if (user.isPresent()) {
userPrivilegeMap.get(user.get()).getAdministrativePrivileges().getPrivileges().addAll(loadRolePrivileges(resultSet));
}
}
-
+
Review comment:
This should be a different specification. I used idea’s automatic formatting. In his specification, blank lines do not contain any characters, but in our project, blank lines seem to contain a Tab placeholder. Which one should we choose? Which way do you think is better?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r615510335
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -46,25 +45,56 @@
* PostgreSQL privilege handler.
*/
public final class PostgreSQLPrivilegeHandler implements StoragePrivilegeHandler {
-
+
+ private static final String CREATE_USER_SQL = "CREATE USER %s WITH PASSWORD '%s'";
+
+ private static final String GRANT_ALL_SQL = "GRANT ALL ON ALL TABLES IN SCHEMA public TO %s";
+
private static final String ROLES_SQL = "select * from pg_roles WHERE rolname IN (%s)";
-
- private static final String TABLE_PRIVILEGE_SQL =
+
+ private static final String TABLE_PRIVILEGE_SQL =
"SELECT grantor, grantee, table_catalog, table_name, privilege_type, is_grantable from information_schema.table_privileges WHERE grantee IN (%s)";
-
+
@Override
public Collection<ShardingSphereUser> diff(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
- return Collections.emptyList();
+ Collection<Grantee> grantees = new LinkedList<>();
+ try (Connection connection = dataSource.getConnection()) {
+ Statement statement = connection.createStatement();
+ try (ResultSet resultSet = statement.executeQuery(getRolePrivilegesSQL(users))) {
+ while (resultSet.next()) {
+ grantees.add(new Grantee(resultSet.getString("rolname"), ""));
+ }
+ }
+ }
+ return users.stream().filter(each -> !grantees.contains(each.getGrantee())).collect(Collectors.toList());
}
-
+
@Override
public void create(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
+ try (Connection connection = dataSource.getConnection(); Statement statement = connection.createStatement()) {
+ for (ShardingSphereUser each : users) {
+ statement.execute(getCreateUsersSQL(each));
+ }
+ }
+ }
+
+ private String getCreateUsersSQL(final ShardingSphereUser users) {
+ return String.format(CREATE_USER_SQL, users.getGrantee(), users.getPassword());
Review comment:
Hi, `user.getGrantee()` will returen `user@host` and `host` is always empty string, is that what you expect?
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -130,19 +160,19 @@ private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges>
}
}
}
-
+
private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges> userPrivilegeMap, final ResultSet resultSet) throws SQLException {
Optional<ShardingSphereUser> user = findShardingSphereUser(userPrivilegeMap, resultSet);
if (user.isPresent()) {
userPrivilegeMap.get(user.get()).getAdministrativePrivileges().getPrivileges().addAll(loadRolePrivileges(resultSet));
}
}
-
+
Review comment:
Please give a look at these blank space changes.
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -46,25 +45,56 @@
* PostgreSQL privilege handler.
*/
public final class PostgreSQLPrivilegeHandler implements StoragePrivilegeHandler {
-
+
+ private static final String CREATE_USER_SQL = "CREATE USER %s WITH PASSWORD '%s'";
+
+ private static final String GRANT_ALL_SQL = "GRANT ALL ON ALL TABLES IN SCHEMA public TO %s";
+
private static final String ROLES_SQL = "select * from pg_roles WHERE rolname IN (%s)";
-
- private static final String TABLE_PRIVILEGE_SQL =
+
+ private static final String TABLE_PRIVILEGE_SQL =
"SELECT grantor, grantee, table_catalog, table_name, privilege_type, is_grantable from information_schema.table_privileges WHERE grantee IN (%s)";
-
+
@Override
public Collection<ShardingSphereUser> diff(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
- return Collections.emptyList();
+ Collection<Grantee> grantees = new LinkedList<>();
+ try (Connection connection = dataSource.getConnection()) {
+ Statement statement = connection.createStatement();
+ try (ResultSet resultSet = statement.executeQuery(getRolePrivilegesSQL(users))) {
+ while (resultSet.next()) {
+ grantees.add(new Grantee(resultSet.getString("rolname"), ""));
Review comment:
Does that mean `username` equals `rolename`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r616253396
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -46,25 +45,56 @@
* PostgreSQL privilege handler.
*/
public final class PostgreSQLPrivilegeHandler implements StoragePrivilegeHandler {
-
+
+ private static final String CREATE_USER_SQL = "CREATE USER %s WITH PASSWORD '%s'";
+
+ private static final String GRANT_ALL_SQL = "GRANT ALL ON ALL TABLES IN SCHEMA public TO %s";
+
private static final String ROLES_SQL = "select * from pg_roles WHERE rolname IN (%s)";
-
- private static final String TABLE_PRIVILEGE_SQL =
+
+ private static final String TABLE_PRIVILEGE_SQL =
"SELECT grantor, grantee, table_catalog, table_name, privilege_type, is_grantable from information_schema.table_privileges WHERE grantee IN (%s)";
-
+
@Override
public Collection<ShardingSphereUser> diff(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
- return Collections.emptyList();
+ Collection<Grantee> grantees = new LinkedList<>();
+ try (Connection connection = dataSource.getConnection()) {
+ Statement statement = connection.createStatement();
+ try (ResultSet resultSet = statement.executeQuery(getRolePrivilegesSQL(users))) {
+ while (resultSet.next()) {
+ grantees.add(new Grantee(resultSet.getString("rolname"), ""));
+ }
+ }
+ }
+ return users.stream().filter(each -> !grantees.contains(each.getGrantee())).collect(Collectors.toList());
}
-
+
@Override
public void create(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
+ try (Connection connection = dataSource.getConnection(); Statement statement = connection.createStatement()) {
+ for (ShardingSphereUser each : users) {
+ statement.execute(getCreateUsersSQL(each));
+ }
+ }
+ }
+
+ private String getCreateUsersSQL(final ShardingSphereUser users) {
+ return String.format(CREATE_USER_SQL, users.getGrantee(), users.getPassword());
Review comment:
Thanks for you clarification.
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -130,19 +160,19 @@ private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges>
}
}
}
-
+
private void fillRolePrivileges(final Map<ShardingSphereUser, NativePrivileges> userPrivilegeMap, final ResultSet resultSet) throws SQLException {
Optional<ShardingSphereUser> user = findShardingSphereUser(userPrivilegeMap, resultSet);
if (user.isPresent()) {
userPrivilegeMap.get(user.get()).getAdministrativePrivileges().getPrivileges().addAll(loadRolePrivileges(resultSet));
}
}
-
+
Review comment:
Hi, it is suggested to indent each line.
BTW, if you like, [IDEA Settings](https://shardingsphere.apache.org/community/cn/contribute/code-conduct/) is convenient to import into IDEA.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#issuecomment-823922542
> Hey, Surely.
> I will update here once some issues are ready for you.
Thank you very much.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] totalo commented on a change in pull request #10115: Implement PostgreSQLPrivilegeHandler and add unit test
Posted by GitBox <gi...@apache.org>.
totalo commented on a change in pull request #10115:
URL: https://github.com/apache/shardingsphere/pull/10115#discussion_r615537883
##########
File path: shardingsphere-features/shardingsphere-authority/shardingsphere-authority-common/src/main/java/org/apache/shardingsphere/authority/provider/natived/builder/dialect/PostgreSQLPrivilegeHandler.java
##########
@@ -46,25 +45,56 @@
* PostgreSQL privilege handler.
*/
public final class PostgreSQLPrivilegeHandler implements StoragePrivilegeHandler {
-
+
+ private static final String CREATE_USER_SQL = "CREATE USER %s WITH PASSWORD '%s'";
+
+ private static final String GRANT_ALL_SQL = "GRANT ALL ON ALL TABLES IN SCHEMA public TO %s";
+
private static final String ROLES_SQL = "select * from pg_roles WHERE rolname IN (%s)";
-
- private static final String TABLE_PRIVILEGE_SQL =
+
+ private static final String TABLE_PRIVILEGE_SQL =
"SELECT grantor, grantee, table_catalog, table_name, privilege_type, is_grantable from information_schema.table_privileges WHERE grantee IN (%s)";
-
+
@Override
public Collection<ShardingSphereUser> diff(final Collection<ShardingSphereUser> users, final DataSource dataSource) throws SQLException {
- return Collections.emptyList();
+ Collection<Grantee> grantees = new LinkedList<>();
+ try (Connection connection = dataSource.getConnection()) {
+ Statement statement = connection.createStatement();
+ try (ResultSet resultSet = statement.executeQuery(getRolePrivilegesSQL(users))) {
+ while (resultSet.next()) {
+ grantees.add(new Grantee(resultSet.getString("rolname"), ""));
Review comment:
There is no concept of distinguishing users and roles in PostgreSQL, so `rolename` is equivalent to `username` here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org