> 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).
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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40793/#review108503 ----------------------------------------------------------- On Nov. 30, 2015, 5:43 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40793/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2015, 5:43 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 > 0e3e7b8 > > 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 > ca3ca36 > > 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 > >
