> 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.

@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`


- 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
> 
>

Reply via email to