----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52369/#review151223 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/orm/entities/GroupEntity.java (lines 64 - 70) <https://reviews.apache.org/r/52369/#comment219499> Is both `ldapGroup` and `groupType` necessary... wouldn't only `groupType` be needed? ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariPamAuthenticationProvider.java (line 47) <https://reviews.apache.org/r/52369/#comment219500> Missing JavaDoc ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariPamAuthenticationProvider.java (line 53) <https://reviews.apache.org/r/52369/#comment219501> Formatting? ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariPamAuthenticationProvider.java (line 58) <https://reviews.apache.org/r/52369/#comment219502> This should be `private static` ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariPamAuthenticationProvider.java (line 60) <https://reviews.apache.org/r/52369/#comment219503> This should be `private` or possibly `private final`. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Group.java (line 28) <https://reviews.apache.org/r/52369/#comment219504> This seems unnecessary with `groupType` ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 414) <https://reviews.apache.org/r/52369/#comment219512> Why not change `createGroup` to take a group type? ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 503) <https://reviews.apache.org/r/52369/#comment219518> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 522) <https://reviews.apache.org/r/52369/#comment219517> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 545) <https://reviews.apache.org/r/52369/#comment219516> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 568) <https://reviews.apache.org/r/52369/#comment219513> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 592) <https://reviews.apache.org/r/52369/#comment219514> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (line 616) <https://reviews.apache.org/r/52369/#comment219515> `grantAdminPrivilegeToGroup`, `grantClusterAdminPrivilegeToGroup`, `grantClusterOperatorPrivilegeToGroup`, `grantServiceAdminPrivilegeToGroup`, `grantServiceOperatorPrivilegeToGroup`, `grantClusterUserPrivilegeToGroup` should be consolidated to reuse code. Also the different roles may be sort of hard coded today, but in the future the roles (and their names) will be managed by the user. For example Cluser Operator may be deleted and the use may create a new roles instead. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java (lines 955 - 960) <https://reviews.apache.org/r/52369/#comment219519> roles may be set today, but in the future these may be customizabled by the user. ambari-server/src/main/python/ambari_server/setupSecurity.py (lines 837 - 842) <https://reviews.apache.org/r/52369/#comment219521> Roles will be configurable in the future, this will be problematic. ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql (line 296) <https://reviews.apache.org/r/52369/#comment219522> group_type does not need to be that large. ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql (line 287) <https://reviews.apache.org/r/52369/#comment219525> group_type does not need to be that large. ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql (line 296) <https://reviews.apache.org/r/52369/#comment219526> group_type does not need to be that large. ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql (line 300) <https://reviews.apache.org/r/52369/#comment219527> group_type does not need to be that large. Missing unit test for `AmbariPamAuthenticationProvider`. - Robert Levas On Oct. 3, 2016, 2:43 p.m., Vishal Ghugare wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52369/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2016, 2:43 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Di Li, and Robert Levas. > > > Bugs: AMBARI-12263 > https://issues.apache.org/jira/browse/AMBARI-12263 > > > Repository: ambari > > > Description > ------- > > Hello Robert, > > How are you doing? > > We have been working on PAM support into Ambari and have something ready for > review. Can you please take a look at the patch and documentation and provide > your feedback. > > Please let me know if you have any questions. > > Note: I have added you as a reviewer as i see some authentication related > commits under your name. > > Thanks, > -Vishal > > > Diffs > ----- > > ambari-server/pom.xml d507b82 > ambari-server/sbin/ambari-server 762ae19 > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > 2e850ef > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 1fc9dbf > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 5e498f0 > > ambari-server/src/main/java/org/apache/ambari/server/controller/GroupResponse.java > ef28f61 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/GroupResourceProvider.java > e1aa5ac > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UserPrivilegeResourceProvider.java > bdd73a6 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ResourceDAO.java > e4ed9c6 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/GroupEntity.java > 00e233e > > ambari-server/src/main/java/org/apache/ambari/server/security/ClientSecurityType.java > 26d4da7 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariPamAuthenticationProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Group.java > b20df8d > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/GroupType.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/PamAuthenticationException.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/UserType.java > aa9f3e0 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > e547f05 > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog250.java > 185bd58 > ambari-server/src/main/python/ambari-server.py bb6bc0e > ambari-server/src/main/python/ambari_server/setupActions.py 697bc1d > ambari-server/src/main/python/ambari_server/setupSecurity.py 119a7d8 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 1d55515 > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 49f3e2f > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 7aa52ef > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql 0c95471 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 631b5c4 > ambari-server/src/main/resources/properties.json eb27878 > ambari-server/src/main/resources/webapp/WEB-INF/spring-security.xml 500c0bf > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog250Test.java > 7b6c3ad > > Diff: https://reviews.apache.org/r/52369/diff/ > > > Testing > ------- > > No test cases added at this point. > > > File Attachments > ---------------- > > AMBARI-12263_trunk.patch > > https://reviews.apache.org/media/uploaded/files/2016/09/30/80254a19-7d51-46f0-80f9-07e664b814ec__AMBARI-12263_trunk.patch > > > Thanks, > > Vishal Ghugare > >
