> On Dec. 1, 2015, 9:57 a.m., Jonathan Hurley wrote: > > I have a concern about the way that we're coupling the security logic here. > > The AMC class is a mess - it really is just a massive steaming pot of > > "anything goes" - there's no real separation of concerns and no real > > contract. It feels like we're just catching a few places where front-end > > logic eventually gets funneled. What about trying to catch this stuff at a > > lower level? Maybe in the DAOs? > > > > What about catching it at a higher level? Could there be a branch new > > request processor that inspects the incoming request, picks it apart to see > > the different things it's trying to do and throws auth exceptions there? > > This way we wouldn't be trying to find all of the spots sprinkeled > > throughout the code that we need to intercept. > > Nate Cole wrote: > I like the service-level (endpoint) approach. > > DAOs feel like it would get complicated fast, especially when performing > joins across tables - trying to decide if the user is authenticated to update > rows of one table, but not another (if that's a valid use case). > > Robert Levas wrote: > I agree and think this logic should be at a high(er) level. However > doing so may hinder performance since we need to essentially parse the > request multiple times - at least once to determine authorzation and at least > once to perform the operation. It will also duplicate logic needed to > detemine what the user is trying to do. > > Essentually protecting this type of API is very difficult. We really > need a higher-level API for Ambari rather than exposing a web-based SQL > front-end. > > Regarding the AMC code, I suppose we can move the relevant resource > provider code back to the resource provider, but I was trying to limit the > amount of code changes. If you look at the methods being altered, they are > the ones directly related to request handling. The lower-level methods are > untouched. > > Robert Levas wrote: > @jurley, would it be better if the request-specific methods were moved > from the AMC and into the proper resource provider? For example, moving > `org.apache.ambari.server.controller.AmbariManagementControllerImpl#createCluster` > to > `org.apache.ambari.server.controller.internal.ClusterResourceProvider#createCluster` > > Jonathan Hurley wrote: > I think it would be better if we did that, yes. It would at least try to > keep with the consistency of the implementation approach here.
Moving the relevant methods from the AMC is too large of a task for this effort. It would require many test cases to be updated along with a few other non-test-related classes. I think a new JIRA should be created for this. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40793/#review108503 ----------------------------------------------------------- On Dec. 1, 2015, 9:34 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40793/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2015, 9:34 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, Myroslav > Papirkovskyy, Nate Cole, and Sumit Mohanty. > > > Bugs: AMBARI-14072 > https://issues.apache.org/jira/browse/AMBARI-14072 > > > Repository: ambari > > > Description > ------- > > Enforce granular role-based access control for cluster functions: > > | Cluster User | Service Operator | Service > Administrator | Cluster Operator | Cluster Administrator | Administrator > ---------------------------|--------------|------------------|-----------------------|------------------|-----------------------|--------------- > View status information |(+) |(+) |(+) > |(+) |(+) |(+) > View configuration |(+) |(+) |(+) > |(+) |(+) |(+) > View stack version details |(+) |(+) |(+) > |(+) |(+) |(+) > Enable/disable Kerberos | | | > | |(+) |(+) > Upgrade/downgrade stack | | | > | |(+) |(+) > Create new clusters | | | > | | |(+) > Rename clusters | | | > | | |(+) > > Entry points affected: > - PUT /api/v1/clusters/:cluster_name > - POST /api/v1/clusters/:cluster_name > > # Note: Read-only requests (GET) are not protected so that the front end is > not broken. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > 4954a96 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > b446121 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > c0dc342 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java > 84c13b9 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java > 7f88286 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelperTest.java > baa394c > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > e2ec5e0 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 3bf6cad > > ambari-server/src/test/java/org/apache/ambari/server/controller/BackgroundCustomCommandExecutionTest.java > 30be261 > > ambari-server/src/test/java/org/apache/ambari/server/controller/RefreshYarnCapacitySchedulerReleaseConfigTest.java > e93a479 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java > 84de604 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/JMXHostProviderTest.java > 2c6905d > > ambari-server/src/test/java/org/apache/ambari/server/security/TestAuthenticationFactory.java > 634d840 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilterTest.java > d4b7d5a > > ambari-server/src/test/java/org/apache/ambari/server/state/ConfigHelperTest.java > bdb5156 > > ambari-server/src/test/java/org/apache/ambari/server/state/UpgradeHelperTest.java > fa6598c > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalogTest.java > 319b9fe > > Diff: https://reviews.apache.org/r/40793/diff/ > > > Testing > ------- > > Manually tested > > # Local test results: > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 58:54.570s > [INFO] Finished at: Mon Nov 30 07:53:59 EST 2015 > [INFO] Final Memory: 67M/1340M > [INFO] > ------------------------------------------------------------------------ > > #Jenkins test results: PENDING > > > Thanks, > > Robert Levas > >
