Modified VisibilityFilter to handle multiple Authorizations objects#6402
Modified VisibilityFilter to handle multiple Authorizations objects#6402dlmarion wants to merge 5 commits into
Conversation
Accumulo Access provides an AccessEvaluator that accepts a Collection of Sets of Strings (effectively Collection<Authorizations>). The existing user VisibilityFilter only accepts a single Authorizations object. This new visibility filter accepts a set of Authorizations in a single filter.
dlmarion
changed the title
| io.addNamedOption(AUTHS, | ||
| "the serialized set of authorizations to filter against (default: empty" | ||
| + " string, accepts only entries visible by all)"); |
There was a problem hiding this comment.
Can't remove this option. The configuration parameter is the main API for this class. Removing or renaming it breaks it.
There was a problem hiding this comment.
Ok, I can add it back. I didn't consider the case where someone has this configured on a table prior to a 4.0 upgrade.
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
The concatenated set of serialized Authorizations looks good to me. I think the description needs work, though.
| io.addNamedOption(AUTHS, "The set of Authorizations to use when filtering (default: empty" | ||
| + " string, accepts only entries visible by all). The value can" | ||
| + " either be of the older form (\"auth1,auth2\") which only supports" | ||
| + " a single Authorizations object, or the newer form which uses " + Authorizations.HEADER | ||
| + " concatenated with Base64 encoded comma-separated authorization tokens." | ||
| + "The latter case supports one or more Authorizations (\"" + Authorizations.HEADER | ||
| + "Base64(auth1,auth2)\") or (\"" + Authorizations.HEADER + "Base64(auth1,auth2)" | ||
| + Authorizations.HEADER + "auth2,auth3\")"); |
There was a problem hiding this comment.
We haven't serialized using the old format since before 1.5.0. I kinda forgot about that.
It's fine if we continue to support that, but the description should instruct users on which serialization format they should be using, which should be VisibilityFilter.setAuthorizations() or if the config is manually set, they should be serialized using Authorizations.serialize().
Previously, this said "the serialized set of authorizations". I think it should still say that. The wording can be something like "concatenated serialized sets of authorizations to filter against". I don't think it should go into detail about the serialization format, but should instead defer to the setAuthorizations method and Authorizations.serialize.
| @Test | ||
| public void testAuthorizationSerialization() { | ||
| String auth = new Authorizations("abc", "def").serialize(); | ||
| System.out.println(auth); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
This test isn't doing any testing. Looks like it was examining the output during dev. Probably can delete it now.
| this.accessEvaluator = BytesAccess.newEvaluator(authObj); | ||
| if (auths == null || auths.isEmpty()) { | ||
| this.accessEvaluator = EMPTY_EVALUATOR; | ||
| } else if (!auths.contains(Authorizations.HEADER)) { |
There was a problem hiding this comment.
This should use startsWith (the header could appear in the old format, inside of quotes, but can only appear at the beginning if it's in the standard/newer serialized format)
| } else if (!auths.contains(Authorizations.HEADER)) { | |
| } else if (!auths.startsWith(Authorizations.HEADER)) { | |
| // the old serialization format does not support multiple auth sets, so treat the whole thing as one set |
2 participants
Accumulo Access provides an AccessEvaluator
that accepts a Collection of Sets of Strings (effectively
Collection<Authorizations>). Modified the existing userVisibilityFilter to accept a set of Authorizations and
use the new AccessEvaluator.