From 486e394cb8727470e6efe564ef04359f4ec6d1b1 Mon Sep 17 00:00:00 2001 From: RyanHolstien Date: Tue, 14 Nov 2023 14:06:33 -0600 Subject: [PATCH] fix(signup): prevent invalid email signup (#9234) --- .../app/auth/NativeAuthenticationConfigs.java | 16 ++++++++++++---- .../controllers/AuthenticationController.java | 8 ++++++++ datahub-frontend/conf/application.conf | 4 ++++ .../authentication/user/NativeUserService.java | 10 +++++++++- .../user/NativeUserServiceTest.java | 15 ++++++++++++++- .../authentication/AuthServiceController.java | 8 ++++++++ .../factory/auth/NativeUserServiceFactory.java | 7 ++++++- 7 files changed, 61 insertions(+), 7 deletions(-) diff --git a/datahub-frontend/app/auth/NativeAuthenticationConfigs.java b/datahub-frontend/app/auth/NativeAuthenticationConfigs.java index db17313d67f9a..3114da92d7d79 100644 --- a/datahub-frontend/app/auth/NativeAuthenticationConfigs.java +++ b/datahub-frontend/app/auth/NativeAuthenticationConfigs.java @@ -6,18 +6,26 @@ public class NativeAuthenticationConfigs { public static final String NATIVE_AUTHENTICATION_ENABLED_CONFIG_PATH = "auth.native.enabled"; + public static final String NATIVE_AUTHENTICATION_ENFORCE_VALID_EMAIL_ENABLED_CONFIG_PATH = "auth.native.signUp.enforceValidEmail"; private Boolean _isEnabled = true; + private Boolean _isEnforceValidEmailEnabled = true; public NativeAuthenticationConfigs(final com.typesafe.config.Config configs) { - if (configs.hasPath(NATIVE_AUTHENTICATION_ENABLED_CONFIG_PATH) - && Boolean.FALSE.equals( - Boolean.parseBoolean(configs.getValue(NATIVE_AUTHENTICATION_ENABLED_CONFIG_PATH).toString()))) { - _isEnabled = false; + if (configs.hasPath(NATIVE_AUTHENTICATION_ENABLED_CONFIG_PATH)) { + _isEnabled = Boolean.parseBoolean(configs.getValue(NATIVE_AUTHENTICATION_ENABLED_CONFIG_PATH).toString()); + } + if (configs.hasPath(NATIVE_AUTHENTICATION_ENFORCE_VALID_EMAIL_ENABLED_CONFIG_PATH)) { + _isEnforceValidEmailEnabled = + Boolean.parseBoolean(configs.getValue(NATIVE_AUTHENTICATION_ENFORCE_VALID_EMAIL_ENABLED_CONFIG_PATH).toString()); } } public boolean isNativeAuthenticationEnabled() { return _isEnabled; } + + public boolean isEnforceValidEmailEnabled() { + return _isEnforceValidEmailEnabled; + } } diff --git a/datahub-frontend/app/controllers/AuthenticationController.java b/datahub-frontend/app/controllers/AuthenticationController.java index 4f89f4f67e149..e28d4ba2ee37e 100644 --- a/datahub-frontend/app/controllers/AuthenticationController.java +++ b/datahub-frontend/app/controllers/AuthenticationController.java @@ -27,6 +27,7 @@ import org.pac4j.play.store.PlaySessionStore; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import play.data.validation.Constraints; import play.libs.Json; import play.mvc.Controller; import play.mvc.Http; @@ -203,6 +204,13 @@ public Result signUp(Http.Request request) { JsonNode invalidCredsJson = Json.newObject().put("message", "Email must not be empty."); return Results.badRequest(invalidCredsJson); } + if (_nativeAuthenticationConfigs.isEnforceValidEmailEnabled()) { + Constraints.EmailValidator emailValidator = new Constraints.EmailValidator(); + if (!emailValidator.isValid(email)) { + JsonNode invalidCredsJson = Json.newObject().put("message", "Email must not be empty."); + return Results.badRequest(invalidCredsJson); + } + } if (StringUtils.isBlank(password)) { JsonNode invalidCredsJson = Json.newObject().put("message", "Password must not be empty."); diff --git a/datahub-frontend/conf/application.conf b/datahub-frontend/conf/application.conf index 1a62c8547e721..0f4ddb7c497e6 100644 --- a/datahub-frontend/conf/application.conf +++ b/datahub-frontend/conf/application.conf @@ -196,6 +196,10 @@ auth.oidc.preferredJwsAlgorithm = ${?AUTH_OIDC_PREFERRED_JWS_ALGORITHM} # Which # auth.jaas.enabled = ${?AUTH_JAAS_ENABLED} auth.native.enabled = ${?AUTH_NATIVE_ENABLED} + +# Enforces the usage of a valid email for user sign up +auth.native.signUp.enforceValidEmail = true +auth.native.signUp.enforceValidEmail = ${?ENFORCE_VALID_EMAIL} # # To disable all authentication to the app, and proxy all users through a master "datahub" account, make sure that, # jaas, native and oidc auth are disabled: diff --git a/metadata-service/auth-impl/src/main/java/com/datahub/authentication/user/NativeUserService.java b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/user/NativeUserService.java index 7f0c16d28f121..bff675ddd9cb2 100644 --- a/metadata-service/auth-impl/src/main/java/com/datahub/authentication/user/NativeUserService.java +++ b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/user/NativeUserService.java @@ -1,7 +1,9 @@ package com.datahub.authentication.user; import com.datahub.authentication.Authentication; +import com.datahub.authentication.AuthenticationConfiguration; import com.linkedin.common.AuditStamp; +import com.linkedin.common.urn.CorpuserUrn; import com.linkedin.common.urn.Urn; import com.linkedin.entity.client.EntityClient; import com.linkedin.events.metadata.ChangeType; @@ -34,6 +36,7 @@ public class NativeUserService { private final EntityService _entityService; private final EntityClient _entityClient; private final SecretService _secretService; + private final AuthenticationConfiguration _authConfig; public void createNativeUser(@Nonnull String userUrnString, @Nonnull String fullName, @Nonnull String email, @Nonnull String title, @Nonnull String password, @Nonnull Authentication authentication) throws Exception { @@ -45,7 +48,12 @@ public void createNativeUser(@Nonnull String userUrnString, @Nonnull String full Objects.requireNonNull(authentication, "authentication must not be null!"); final Urn userUrn = Urn.createFromString(userUrnString); - if (_entityService.exists(userUrn) || userUrn.toString().equals(SYSTEM_ACTOR)) { + if (_entityService.exists(userUrn) + // Should never fail these due to Controller level check, but just in case more usages get put in + || userUrn.toString().equals(SYSTEM_ACTOR) + || userUrn.toString().equals(new CorpuserUrn(_authConfig.getSystemClientId()).toString()) + || userUrn.toString().equals(DATAHUB_ACTOR) + || userUrn.toString().equals(UNKNOWN_ACTOR)) { throw new RuntimeException("This user already exists! Cannot create a new user."); } updateCorpUserInfo(userUrn, fullName, email, title, authentication); diff --git a/metadata-service/auth-impl/src/test/java/com/datahub/authentication/user/NativeUserServiceTest.java b/metadata-service/auth-impl/src/test/java/com/datahub/authentication/user/NativeUserServiceTest.java index b0b10af82155a..0102311ff3b61 100644 --- a/metadata-service/auth-impl/src/test/java/com/datahub/authentication/user/NativeUserServiceTest.java +++ b/metadata-service/auth-impl/src/test/java/com/datahub/authentication/user/NativeUserServiceTest.java @@ -3,6 +3,7 @@ import com.datahub.authentication.Actor; import com.datahub.authentication.ActorType; import com.datahub.authentication.Authentication; +import com.datahub.authentication.AuthenticationConfiguration; import com.linkedin.common.urn.CorpuserUrn; import com.linkedin.common.urn.Urn; import com.linkedin.entity.client.EntityClient; @@ -48,8 +49,10 @@ public void setupTest() throws Exception { _entityService = mock(EntityService.class); _entityClient = mock(EntityClient.class); _secretService = mock(SecretService.class); + AuthenticationConfiguration authenticationConfiguration = new AuthenticationConfiguration(); + authenticationConfiguration.setSystemClientId("someCustomId"); - _nativeUserService = new NativeUserService(_entityService, _entityClient, _secretService); + _nativeUserService = new NativeUserService(_entityService, _entityClient, _secretService, authenticationConfiguration); } @Test @@ -74,6 +77,16 @@ public void testCreateNativeUserUserAlreadyExists() throws Exception { _nativeUserService.createNativeUser(USER_URN_STRING, FULL_NAME, EMAIL, TITLE, PASSWORD, SYSTEM_AUTHENTICATION); } + @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "This user already exists! Cannot create a new user.") + public void testCreateNativeUserUserDatahub() throws Exception { + _nativeUserService.createNativeUser(DATAHUB_ACTOR, FULL_NAME, EMAIL, TITLE, PASSWORD, SYSTEM_AUTHENTICATION); + } + + @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "This user already exists! Cannot create a new user.") + public void testCreateNativeUserUserSystemUser() throws Exception { + _nativeUserService.createNativeUser(SYSTEM_ACTOR, FULL_NAME, EMAIL, TITLE, PASSWORD, SYSTEM_AUTHENTICATION); + } + @Test public void testCreateNativeUserPasses() throws Exception { when(_entityService.exists(any())).thenReturn(false); diff --git a/metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java b/metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java index b5ce99902108a..34354a47b7f04 100644 --- a/metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java +++ b/metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java @@ -12,6 +12,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.linkedin.common.urn.CorpuserUrn; import com.linkedin.common.urn.Urn; import com.linkedin.gms.factory.config.ConfigurationProvider; import java.util.concurrent.CompletableFuture; @@ -28,6 +29,8 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.HttpClientErrorException; +import static com.linkedin.metadata.Constants.*; + @Slf4j @RestController @@ -177,6 +180,11 @@ CompletableFuture> signUp(final HttpEntity httpEn } String userUrnString = userUrn.asText(); + String systemClientUser = new CorpuserUrn(_configProvider.getAuthentication().getSystemClientId()).toString(); + + if (userUrnString.equals(systemClientUser) || userUrnString.equals(DATAHUB_ACTOR) || userUrnString.equals(UNKNOWN_ACTOR)) { + return CompletableFuture.completedFuture(new ResponseEntity<>(HttpStatus.BAD_REQUEST)); + } String fullNameString = fullName.asText(); String emailString = email.asText(); String titleString = title.asText(); diff --git a/metadata-service/factories/src/main/java/com/linkedin/gms/factory/auth/NativeUserServiceFactory.java b/metadata-service/factories/src/main/java/com/linkedin/gms/factory/auth/NativeUserServiceFactory.java index 3df499ea9392e..a0df661852935 100644 --- a/metadata-service/factories/src/main/java/com/linkedin/gms/factory/auth/NativeUserServiceFactory.java +++ b/metadata-service/factories/src/main/java/com/linkedin/gms/factory/auth/NativeUserServiceFactory.java @@ -3,6 +3,7 @@ package com.linkedin.gms.factory.auth; import com.datahub.authentication.user.NativeUserService; +import com.linkedin.gms.factory.config.ConfigurationProvider; import com.linkedin.metadata.client.JavaEntityClient; import com.linkedin.metadata.spring.YamlPropertySourceFactory; import com.linkedin.metadata.entity.EntityService; @@ -31,10 +32,14 @@ public class NativeUserServiceFactory { @Qualifier("dataHubSecretService") private SecretService _secretService; + @Autowired + private ConfigurationProvider _configurationProvider; + @Bean(name = "nativeUserService") @Scope("singleton") @Nonnull protected NativeUserService getInstance() throws Exception { - return new NativeUserService(this._entityService, this._javaEntityClient, this._secretService); + return new NativeUserService(_entityService, _javaEntityClient, _secretService, + _configurationProvider.getAuthentication()); } } \ No newline at end of file