binder: Add SecurityPolicies#anyOf(), similar to allOf() #9147
Conversation
…etails to the allOf() javadoc to match.
| @@ -191,6 +194,12 @@ private static boolean checkPackageSignature( | |||
| * Creates a {@link SecurityPolicy} that allows access if and only if *all* of the specified | |||
| * {@code securityPolicies} allow access. | |||
| * | |||
| * <p>Policies will be checked in the order that they are passed. If a policy denies access, | |||
There was a problem hiding this comment.
I thought about adding this to the contract too but wasn't sure since it would preclude ever (optimistically) checking the policies in parallel. Is there a specific need for or benefit of this guarantee that you have in mind?
There was a problem hiding this comment.
No. And I'm happy not committing to a particular behavior, but in that case I would prefer to be explicit that we intentionally make no guarantees. Something like "subsequent policies may be checked in any order and on any thread" so that we are clear you should not depend on that behavior.
There was a problem hiding this comment.
Got it thanks. Thinking about this more with http://www.hyrumslaw.com in mind, I doubt anyone would ever be brave enough to make such a change even if we added an explicit non-guarantee to this javadoc. So maybe your proposal to guarantee the order is for the best since it is the simplest. Should be a separate PR though?
There was a problem hiding this comment.
Removed all allOf() changes from this PR.
| policy = | ||
| SecurityPolicies.anyOf( | ||
| SecurityPolicies.internalOnly(), | ||
| new SecurityPolicy() { |
There was a problem hiding this comment.
I don't feel strongly but if we have two copies of this fake perhaps making it a class named RecordingSecurityPolicy or similar would be more self-documenting.
There was a problem hiding this comment.
Added a helper class to the test.
| toReturn = toReturn.augmentDescription(append.getDescription()); | ||
| if (append.getCause() != null) { | ||
| if (toReturn.getCause() != null) { | ||
| toReturn.getCause().addSuppressed(append.getCause()); |
There was a problem hiding this comment.
Do you want test coverage for this cause accumulation logic?
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
|
FYI I needed to adjust a changed import statement, so this requires another kokoro run. |
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
|
@markb74 @jdcormie, feel free to merge stuff like this. Note that you'll probably need to add the "kokoro:run" label to have the rest of the CIs run. The "kokoro:force-run" is only needed if the run failed and you want it to re-run. (Although you can also click on the results and go to the fusion results which lets you rerun one job instead of all of them; it has been years since I used kokoro:force-run.) The main criteria before merge is that two maintainers (have write access) are involved (so that means one authors + one reviews, or two reviews). Since that was already the case here, the second reviewer would have been free to merge (squash). |
binder: Add SecurityPolicies#anyOf(), similar to allOf(). Adds some details to the allOf() javadoc and test assertions to match.