added HelloWorld testing annotation (will remove later) + AutomationClientPermissionCheck #1079
Conversation
| public class AutomationClientDelegatingPermissionCheck implements PermissionCheck{ | ||
| private static final Logger logger = LoggerFactory.getLogger( | ||
| AutomationClientDelegatingPermissionCheck.class); | ||
|
|
||
| private PermissionCheck delegate; |
There was a problem hiding this comment.
So for this one I don't think we're going to need a delegate, this can just be a "terminal" permission check. I guess I would say that a permission check either has permission logic of its own (which this one does) or relying on a delegate's logic, but not both. The pass-through and always-allow are in the second category since they don't have any (non-trivial) permission check logic of their own.
| int hasPermissionSuccessMetricInt = hasPermission ? 1 : 0; | ||
| String successMetricName = MetricRegistry.name(AutomationClientDelegatingPermissionCheck.class, "isAllowed", "success", "histogram"); | ||
| metricRegistry.histogram(successMetricName).update(hasPermissionSuccessMetricInt); | ||
|
|
||
| int hasPermissionFailureMetricInt = hasPermission ? 0 : 1; | ||
| String failureMetricName = MetricRegistry.name(AutomationClientDelegatingPermissionCheck.class, "isAllowed", "failure", "histogram"); | ||
| metricRegistry.histogram(failureMetricName).update(hasPermissionFailureMetricInt); |
There was a problem hiding this comment.
Just to shorten things up a bit, you could move all the metrics stuff into an emitMetrics() helper method. That's actually one of the really nice things about the built-in metrics annotations is that they really hide all of this boilerplate and allow the method to just focus on the business logic.
| @Override | ||
| public void checkAllowedOrThrow(KeywhizPrincipal source, String action, Object target) { | ||
| String successMetricName = MetricRegistry.name(AutomationClientDelegatingPermissionCheck.class, "checkAllowedOrThrow", "success", "histogram"); | ||
| String exceptionMetricName = MetricRegistry.name(AutomationClientDelegatingPermissionCheck.class, "checkAllowedOrThrow", "exception", "histogram"); |
There was a problem hiding this comment.
If we gave up on having custom metric names for each entry point ("isAllowed" vs. "checkAllowedOrThrow"), could we just use the default implementation of this method (which just delegates to isAllowed?)
There was a problem hiding this comment.
what if we move all the custom metrics emission into a method defined in PermissionCheck interface directly.
There was a problem hiding this comment.
would have to add MetricRegistry as a parameter...could be difficult
violetd12
changed the title
No description provided.