> On Nov. 23, 2015, 3:47 p.m., Jonathan Hurley wrote: > > What about using AOP for this kind of stuff? Instead of trying to find and > > sprinkle the code with a bunch of tightly coupled calls, you could easily > > intercept multiple join point matches. Kind of prevents problems with > > placing the checks in resource providers vs impls. I did notice that there > > were some checks added to AMCImpl - just seems like it's going to be hard > > to know what's covered and what isn't. > > Robert Levas wrote: > I guess we could create our own annotations, but it seems liked more work > than my current approach. In many caes, we need to look at the request to > determine if the user can perform the operation. For example, some fields can > only be updated based on role... or you can view/edit resources that you > _own_ but cannot have access or know about other resources of the same type - > for example, I shouldn't be able to _know_ whether a user with some username > exists. > > Robert Levas wrote: > I think if the API was RPC-based, it would be a different story and we > would be able to annotate the interfaces rather than need to perform logic on > the request data before determing authorization.
You still have access to all of the parameters being passed into the join points; it's not really annotation-based, but advice-based. It's just a thought. Typically when you have cross cutting concerns like logging and security you'd use AOP to decouple your code. It just feels very brute-force-ish to add it directly into each method. There's no single piece of advice that's being applied to multiple places. With that said, I have no real issues with the patch; just thought that this would be a great opportunity to decouple security from the providers. > On Nov. 23, 2015, 3:47 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java, > > lines 61-67 > > <https://reviews.apache.org/r/40606/diff/1/?file=1137525#file1137525line61> > > > > I think we're missing one for /alert_targets ... that's outside the > > scope of a cluster and might be missed. > > Robert Levas wrote: > I am not sure I follow this. The current patch is for the user and > privilege resources. Alerts will be handled later. > > Maybe this is a current security flaw that will be fixed ones the rest of > the RBAC patches are created/applied? I think you're right - it's a current flaw that needs to be covered later. I'll drop it for now since it's not part of the scope (or you could just add it :) ) > On Nov. 23, 2015, 3:47 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > lines 2784-2787 > > <https://reviews.apache.org/r/40606/diff/1/?file=1137516#file1137516line2784> > > > > Why the impl and not a resource provider since that's the entry point > > to this stuff? > > Robert Levas wrote: > In this case, the implementation of the resource provider _lives_ in the > AmbariManagementControlerImpl. Other resource providers keep their > implementation _local_. That's weird... OK, I'll drop it. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40606/#review107642 ----------------------------------------------------------- On Nov. 23, 2015, 2:53 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40606/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2015, 2:53 p.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, Robert Nettleton, and > Sumit Mohanty. > > > Bugs: AMBARI-13977 > https://issues.apache.org/jira/browse/AMBARI-13977 > > > Repository: ambari > > > Description > ------- > > Enforce granular role-based access control for user functions: > > | Cluster | Service | Service | Cluster > | Cluster | > | User | Operator > | Administrator | Operator | Administrator | Administrator > ------------------------------|---------|----------|---------------|----------|---------------|-------------- > Create new clusters | | | | > | | (+) > Manage users | | | | > | | (+) > Assign permissions/roles | | | | > | | (+) > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > ea7603f > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 443c715 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractResourceProvider.java > 3464c19 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ActiveWidgetLayoutResourceProvider.java > 52b0d56 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AmbariPrivilegeResourceProvider.java > 3670775 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterPrivilegeResourceProvider.java > bbcd4a1 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PrivilegeResourceProvider.java > 88e9906 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UserAuthorizationResourceProvider.java > 15aa0ec > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UserPrivilegeResourceProvider.java > a8a9909 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UserResourceProvider.java > b993450 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java > 81794d8 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AuthorizationHelper.java > 198e209 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > 1d9e53d > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 385e3f7 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ActiveWidgetLayoutResourceProviderTest.java > e74520e > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AmbariPrivilegeResourceProviderTest.java > 68f1467 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterPrivilegeResourceProviderTest.java > 1412470 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UserAuthorizationResourceProviderTest.java > e71c219 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UserPrivilegeResourceProviderTest.java > e65786b > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UserResourceProviderTest.java > 94f6fd7 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ViewPrivilegeResourceProviderTest.java > 8400efd > > ambari-server/src/test/java/org/apache/ambari/server/security/TestAuthenticationFactory.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilterTest.java > 2efab89 > > Diff: https://reviews.apache.org/r/40606/diff/ > > > Testing > ------- > > Manually tested > > # Local test results: > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 57:31.344s > [INFO] Finished at: Mon Nov 23 14:52:50 EST 2015 > [INFO] Final Memory: 67M/1255M > [INFO] > ------------------------------------------------------------------------ > > # Jenkins test results: PENDING > > > Thanks, > > Robert Levas > >
