> 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. > > Jonathan Hurley wrote: > 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.
Unless I am missing something, if we took the approach you are suggesting, we would need to parse the request and predicate for each call before we allow the call to execute. I think this is a good idea, however a request may contain multiple requests and we will need to have intimate knowlege of what the request might be so we could analyze it. If this was a one-off thing, I might be ok with that, but since it is likely for all end points, it just seemed to make sense to do it inline since the logic to parse the requests and predicates already works. > 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? > > Jonathan Hurley wrote: > 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 :) ) I was going to do the whole API in one patch, but then the patch would be HUGE! So I am breaking it up into peices. - Robert ----------------------------------------------------------- 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 > >
