ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Levas" <rle...@hortonworks.com>
Subject Re: Review Request 40793: Enforce granular role-based access control for cluster functions
Date Wed, 02 Dec 2015 16:40:58 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message