basic security extension ignore permissions that use unknown ResourceType or Action#10896
Conversation
clintropolis
changed the title
| } | ||
| catch (JsonProcessingException e) { | ||
| // ignore unparseable, it might be resource types we don't know about | ||
| log.warn(e, "Failed to deserialize authorizer role, ignoring"); |
There was a problem hiding this comment.
Should we print the string representation of the JsonNode here to aid in debugging?
| } | ||
|
|
||
| @Test | ||
| public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProcessingException |
There was a problem hiding this comment.
nit: can this be split into a few different tests if not too much trouble?
There was a problem hiding this 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.
There was a problem hiding this 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.
…Type or Action (apache#10896) * suppress unknown ResourceType and Action for basic-security authorizer stuff * fix pom * print failed role, test logs
3 participants
Description
This PR makes the
druid-basic-securityextension a bit more relaxed across versions by ignoring any permissions which have unknownResourceTypeorActionvalues (they use enums).A subtle backwards incompatibility was introduced in #10812, if permissions using
Resourceof typeResourceType.VIEWare added (or created automatically when creating default admin user in a new install), 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.This PR has: