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
Cluster-state based Security role mapper #107410
Cluster-state based Security role mapper #107410
Conversation
Hi @albertzaharovits, I've created a changelog YAML for you. |
Pinging @elastic/es-security (Team:Security) |
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.
Thanks for your patience on this review. Nice work, I left a couple comments but nothing major - no need for another round of review once they're addressed to your satisfaction.
|
||
@BeforeClass | ||
public static void beforeTests() { | ||
anonymousRole = randomBoolean(); |
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.
Any reason not to just run both every time? Keep the way the tests are parameterized, just run them both ways instead of making it random.
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.
The xpack.security.authc.anonymous.roles
is a static setting, so it's cumbersome to vary from test to test. It's also not super relevant for the scope of the tests.
import static org.elasticsearch.xpack.security.authc.support.mapper.ExpressionRoleMappingTests.randomRoleMapping; | ||
import static org.hamcrest.Matchers.is; | ||
|
||
public class RoleMappingMetadataTests extends ESTestCase { |
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.
Why not AbstractWireSerializingTestCase
?
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.
Good point, pushed: 4135aeb
verify(userRoleMapper1, times(1)).clearRealmCacheOnChange(eq(realm)); | ||
verify(userRoleMapper1, times(1)).clearRealmCacheOnChange(eq(realm)); |
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 think you meant:
verify(userRoleMapper1, times(1)).clearRealmCacheOnChange(eq(realm)); | |
verify(userRoleMapper1, times(1)).clearRealmCacheOnChange(eq(realm)); | |
verify(userRoleMapper1, times(1)).clearRealmCacheOnChange(eq(realm)); | |
verify(userRoleMapper2, times(1)).clearRealmCacheOnChange(eq(realm)); |
?
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.
Good catch! Thanks!
This implements a new
UserRoleMapper
that sources the role mapping rules from the cluster state.The role mapping rules are stored under a new custom cluster state that is persisted (both disk and snapshots).
The role mapper refreshes realm caches when role mapping changes are published.
The role mapper is disabled by default, and it can only be enabled from code, by other plugins.
When enabled, the cluster state role mappings rules, if any, are additive to the rules from
the index-based native role mapping store and the file-based DN one.