Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/23905 )
Change subject: IMPALA-12844 Support setting DBPROPERTIES ...................................................................... Patch Set 7: (1 comment) Thanks for taking care of this Balazs! I left some comments on AlterDbSetDbPropertiesStmt.java about what privilege to require for ALTER DATABASE SET DBPROPERTIES. Let me know if they make sense. http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java: http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java@51 PS7, Line 51: ALL > ALTER privilege looks enough to me. I agree with Csaba on this. We could require the requesting user to just have the ALTER privilege on the target database in our case (v.s. the ALL privilege). Otherwise, a user allowed to execute ALTER DATABASE SET DBPROPERTIES will be too powerful. Moreover, it would be a good idea to align Apache Impala with Apache Hive with respect to the required privilege on the target database. Specifically, when Hive is using Ranger as the authorization provider, for ALTER DATABASE, Hive's Ranger plug-in sets the access type to HiveAccessType.ALTER as seen at https://github.com/apache/ranger/blob/6ddad43/hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java#L1870-L1926. switch (hiveOpType) { case ALTERDATABASE: ... case MSCK: accessType = HiveAccessType.ALTER; break; } In addition, we probably do not have to set the field of 'requireGrantOption' of getDb() to true. For ALTER DATABASE SET DBPROPERTIES, the value of 'requireGrantOption' does not affect how we populate the RangerAccessRequestImpl passed to Impala's Ranger plug-in. More precisely, when we set 'requireGrantOption' to true, in protected boolean authorizeResource(), request.hasGrantOption() would evaluate to true, but we do not pass request.hasGrantOption() to RangerAuthorizationChecker#authorizeAll(), which eventually results in a call to private boolean authorizeResource() that invokes Impala's Ranger plug-in to determine whether the query should be allowed. RangerAccessResult authorized = plugin_.isAccessAllowed(request, auditHandler); Based on the observations above, we could probably simplify the statement at line 52 to the following. analyzer.getDb(dbName_, Privilege.ALTER); On a related note, it would be great if we could add more test cases to verify that the corresponding Ranger audit event could be produced. For instance, we could add the following to testAuditLogSuccess() in https://github.com/apache/impala/blob/6b93f09/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java. authzOk(events -> { assertEquals(1, events.size()); assertEventEquals("@database", "alter", "functional", 1, events.get(0)); assertEquals("alter database functional set dbproperties ('a'='b')", events.get(0).getRequestData()); }, "alter database functional set dbproperties ('a'='b')", onDatabase("functional", TPrivilegeLevel.ALTER)); We could similarly add some negative test cases in testAuditLogFailure() of RangerAuditLogTest.java to show what will be logged when this command fails the authorization due to insufficient privileges/permissions on the target database, e.g., what would happen if the requesting user is only granted the SELECT privilege on the target database, or is not granted any privilege on the target database? -- To view, visit http://gerrit.cloudera.org:8080/23905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I628c7bed4f0c39aed7f5f4ffea52421caf501933 Gerrit-Change-Number: 23905 Gerrit-PatchSet: 7 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 03 Feb 2026 00:03:56 +0000 Gerrit-HasComments: Yes
