You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/02/17 13:28:03 UTC
[GitHub] [druid] clintropolis opened a new pull request #10896: basic security extension ignore permissions that use unknown ResourceType and Action
clintropolis opened a new pull request #10896:
URL: https://github.com/apache/druid/pull/10896
### Description
This PR makes the `druid-basic-security` extension a bit more relaxed across versions by ignoring any permissions which have unknown `ResourceType` or `Action` values (they use enums).
A subtle backwards incompatibility was introduced in #10812, if permissions using the `VIEW` `ResourceType` are added, rolling back to a previous version of Druid results in explosions because the enum in the older version does not contain this value. (The same backwards incompatibility would occur with the changes in #10571).
Rather than rework these types to deserialize into strings and then filtering out permissions with unknown resource types or action values from the list (or, i guess null checking everywhere), this PR instead introduces a custom deserializer for the list of permissions stored in a `BasicAuthorizerRole`, chomping json processing exceptions so it can ignore any permissions which fail to deserialize. I'm open to other approaches to this too if anyone has opinions here.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r578749327
##########
File path: extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/BasicAuthUtilsTest.java
##########
@@ -36,4 +51,112 @@ public void testHashPassword()
Assert.assertEquals(BasicAuthUtils.SALT_LENGTH, salt.length);
Assert.assertEquals(BasicAuthUtils.KEY_LENGTH / 8, hash.length);
}
+
+ @Test
+ public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProcessingException
Review comment:
can this be split into a few different tests if not too much trouble?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r578749327
##########
File path: extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/BasicAuthUtilsTest.java
##########
@@ -36,4 +51,112 @@ public void testHashPassword()
Assert.assertEquals(BasicAuthUtils.SALT_LENGTH, salt.length);
Assert.assertEquals(BasicAuthUtils.KEY_LENGTH / 8, hash.length);
}
+
+ @Test
+ public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProcessingException
Review comment:
nit: can this be split into a few different tests if not too much trouble?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r578744980
##########
File path: extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/entity/BasicAuthorizerRole.java
##########
@@ -78,4 +90,35 @@ public int hashCode()
result = 31 * result + (getPermissions() != null ? getPermissions().hashCode() : 0);
return result;
}
+
+
+ static class PermissionsDeserializer extends JsonDeserializer<List<BasicAuthorizerPermission>>
+ {
+ @Override
+ public List<BasicAuthorizerPermission> deserialize(
+ JsonParser jsonParser,
+ DeserializationContext deserializationContext
+ ) throws IOException
+ {
+ List<BasicAuthorizerPermission> permissions = new ArrayList<>();
+ // sanity check
+ ObjectCodec codec = jsonParser.getCodec();
+ JsonNode hopefullyAnArray = codec.readTree(jsonParser);
+ if (!hopefullyAnArray.isArray()) {
+ throw new RE("Failed to deserialize authorizer role list");
+ }
+
+ for (JsonNode node : hopefullyAnArray) {
+ try {
+ permissions.add(codec.treeToValue(node, BasicAuthorizerPermission.class));
+ }
+ catch (JsonProcessingException e) {
+ // ignore unparseable, it might be resource types we don't know about
+ log.warn(e, "Failed to deserialize authorizer role, ignoring");
Review comment:
Should we print the string representation of the JsonNode here to aid in debugging?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r581261777
##########
File path: extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/BasicAuthUtilsTest.java
##########
@@ -36,4 +51,112 @@ public void testHashPassword()
Assert.assertEquals(BasicAuthUtils.SALT_LENGTH, salt.length);
Assert.assertEquals(BasicAuthUtils.KEY_LENGTH / 8, hash.length);
}
+
+ @Test
+ public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProcessingException
Review comment:
this test seems to test 3 cases: 1 good Permission, one bad resourceType permission and one bad resourceAciont permission. Thought it made sens to split into 3 tests accordingly. Its ok to leave as is too, just a nit, no biggie.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r580912940
##########
File path: extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/entity/BasicAuthorizerRole.java
##########
@@ -78,4 +90,35 @@ public int hashCode()
result = 31 * result + (getPermissions() != null ? getPermissions().hashCode() : 0);
return result;
}
+
+
+ static class PermissionsDeserializer extends JsonDeserializer<List<BasicAuthorizerPermission>>
+ {
+ @Override
+ public List<BasicAuthorizerPermission> deserialize(
+ JsonParser jsonParser,
+ DeserializationContext deserializationContext
+ ) throws IOException
+ {
+ List<BasicAuthorizerPermission> permissions = new ArrayList<>();
+ // sanity check
+ ObjectCodec codec = jsonParser.getCodec();
+ JsonNode hopefullyAnArray = codec.readTree(jsonParser);
+ if (!hopefullyAnArray.isArray()) {
+ throw new RE("Failed to deserialize authorizer role list");
+ }
+
+ for (JsonNode node : hopefullyAnArray) {
+ try {
+ permissions.add(codec.treeToValue(node, BasicAuthorizerPermission.class));
+ }
+ catch (JsonProcessingException e) {
+ // ignore unparseable, it might be resource types we don't know about
+ log.warn(e, "Failed to deserialize authorizer role, ignoring");
Review comment:
:+1:, added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10896:
URL: https://github.com/apache/druid/pull/10896#discussion_r580921964
##########
File path: extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/BasicAuthUtilsTest.java
##########
@@ -36,4 +51,112 @@ public void testHashPassword()
Assert.assertEquals(BasicAuthUtils.SALT_LENGTH, salt.length);
Assert.assertEquals(BasicAuthUtils.KEY_LENGTH / 8, hash.length);
}
+
+ @Test
+ public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProcessingException
Review comment:
I didn't end up splitting this one up yet, since I wasn't quite sure on what way made the most sense after thinking about how it doesn't really take a different code path from the `deserializeAuthorizerRoleMap` methods perspective here, (the same exception is thrown and same code path taken regardless of which enum failed to deserialize). I put both enums in here because it made me feel better, but from a pure coverage perspective i don't think splitting at least on different enum failures changes much.
Any suggestion on what split makes sense? There are more general tests that cover happier paths with JSON serde of the types involved here in some of the other test classes, I just did this unhappy path test in this test class so I could call the `deserializeAuthorizerRoleMap` method directly without worrying about the rest of the scaffolding stuff the happy path tests are using.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10896:
URL: https://github.com/apache/druid/pull/10896
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #10896: basic security extension ignore permissions that use unknown ResourceType or Action
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10896:
URL: https://github.com/apache/druid/pull/10896#issuecomment-784568959
thanks for review @jon-wei and @zachjsh :metal:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org