From 5d433643a769d0948c8969b2bc78df278210863d Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 6 Feb 2020 21:11:31 +1100 Subject: [PATCH] Don't allow null User.principal Some parts of the User class (e.g. equals/hashCode) assumed that principal could never be null, but the constructor didn't enforce that. This adds a null check into the constructor and fixes a few tests that relied on being able to pass in null usernames. --- .../xpack/core/security/user/User.java | 3 +- .../logfile/LoggingAuditTrailFilterTests.java | 6 ++-- .../ingest/SetSecurityUserProcessorTests.java | 29 ++++++++++++------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java index 1123930bc7375..4d2d1410e89c4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Map; +import java.util.Objects; /** * An authenticated user @@ -49,7 +50,7 @@ public User(String username, String[] roles, String fullName, String email, Map< private User(String username, String[] roles, String fullName, String email, Map metadata, boolean enabled, User authenticatedUser) { - this.username = username; + this.username = Objects.requireNonNull(username); this.roles = roles == null ? Strings.EMPTY_ARRAY : roles; this.metadata = metadata == null ? Map.of() : metadata; this.fullName = fullName; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java index 5cdc4400c3801..f4e0deac3e8dd 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java @@ -113,11 +113,9 @@ public void testPolicyDoesNotMatchNullValuesInEvent() throws Exception { // user field matches assertTrue("Matches the user filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate().test( new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.empty(), Optional.empty(), Optional.empty()))); - final User unfilteredUser; + final User unfilteredUser = mock(User.class); if (randomBoolean()) { - unfilteredUser = new User(null); - } else { - unfilteredUser = new User(new User(null), new User(randomFrom(filteredUsers).principal())); + when(unfilteredUser.authenticatedUser()).thenReturn(new User(randomFrom(filteredUsernames))); } // null user field does NOT match assertFalse("Does not match the user filter predicate because of null username.", diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorTests.java index 3c82da113aeea..d368297159875 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorTests.java @@ -13,6 +13,7 @@ import org.elasticsearch.xpack.core.security.authc.AuthenticationField; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.ingest.SetSecurityUserProcessor.Property; +import org.mockito.Mockito; import java.util.Arrays; import java.util.EnumSet; @@ -42,15 +43,21 @@ public void testProcessor() throws Exception { assertThat(result.get("email"), equalTo("_email")); assertThat(((Map) result.get("metadata")).size(), equalTo(1)); assertThat(((Map) result.get("metadata")).get("key"), equalTo("value")); + } - // test when user holds no data: - threadContext = new ThreadContext(Settings.EMPTY); - user = new User(null, null, null); - threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null)); - ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>()); - processor = new SetSecurityUserProcessor("_tag", threadContext, "_field", EnumSet.allOf(Property.class)); + public void testProcessorWithEmptyUserData() throws Exception { + // test when user returns null for all values (need a mock, because a real user cannot have a null username) + User user = Mockito.mock(User.class); + Authentication authentication = Mockito.mock(Authentication.class); + Mockito.when(authentication.getUser()).thenReturn(user); + + ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, authentication); + + IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>()); + SetSecurityUserProcessor processor = new SetSecurityUserProcessor("_tag", threadContext, "_field", EnumSet.allOf(Property.class)); processor.execute(ingestDocument); - result = ingestDocument.getFieldValue("_field", Map.class); + Map result = ingestDocument.getFieldValue("_field", Map.class); assertThat(result.size(), equalTo(0)); } @@ -80,7 +87,7 @@ public void testUsernameProperties() throws Exception { public void testRolesProperties() throws Exception { ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - User user = new User(null, "role1", "role2"); + User user = new User(randomAlphaOfLengthBetween(4, 12), "role1", "role2"); Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name"); threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null)); @@ -96,7 +103,7 @@ public void testRolesProperties() throws Exception { public void testFullNameProperties() throws Exception { ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - User user = new User(null, null, "_full_name", null, Map.of(), true); + User user = new User(randomAlphaOfLengthBetween(4, 12), null, "_full_name", null, Map.of(), true); Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name"); threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null)); @@ -112,7 +119,7 @@ public void testFullNameProperties() throws Exception { public void testEmailProperties() throws Exception { ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - User user = new User(null, null, null, "_email", Map.of(), true); + User user = new User(randomAlphaOfLengthBetween(4, 12), null, null, "_email", Map.of(), true); Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name"); threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null)); @@ -128,7 +135,7 @@ public void testEmailProperties() throws Exception { public void testMetadataProperties() throws Exception { ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - User user = new User(null, null, null, null, Map.of("key", "value"), true); + User user = new User(randomAlphaOfLengthBetween(4, 12), null, null, null, Map.of("key", "value"), true); Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name"); threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null));