New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all allOf() changes from this PR.
policy = | ||
SecurityPolicies.anyOf( | ||
SecurityPolicies.internalOnly(), | ||
new SecurityPolicy() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want test coverage for this cause accumulation logic?
FYI I needed to adjust a changed import statement, so this requires another 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.