From 9cd9edd3fa03397569a4a3b85fd84ec58d50c8ac Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Mon, 11 Jan 2021 20:10:50 +0530 Subject: [PATCH] Disable uninvited signups via environment variable (#2512) * Disable signup API via environment variable * Allow signup for invited users, even if disabled publicly * Add test for signup when signup is disabled * Run invite flow tests when signup is disabled * Revert status annotation in signup API endpoint * Remove unused tests on invite flow * Change signup disabled error message --- .../server/configurations/CommonConfig.java | 3 + .../server/exceptions/AppsmithError.java | 3 +- .../server/services/UserServiceImpl.java | 12 +- .../src/main/resources/application.properties | 3 + .../server/services/UserServiceTest.java | 24 --- .../UserServiceWithDisabledSignupTest.java | 199 ++++++++++++++++++ 6 files changed, 217 insertions(+), 27 deletions(-) create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java index 3b6a0052d4..08f23d81f2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java @@ -25,6 +25,9 @@ public class CommonConfig { private static final String ELASTIC_THREAD_POOL_NAME = "appsmith-elastic-pool"; + @Value("${signup.disabled}") + private boolean isSignupDisabled; + @Value("${oauth2.allowed-domains}") private String allowedDomainList; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index b63cc551a2..c573a72f4b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -69,7 +69,8 @@ public enum AppsmithError { INVALID_CURL_METHOD(400, 4032, "Invalid method in cURL command: {0}.", AppsmithErrorAction.DEFAULT), OAUTH_NOT_AVAILABLE(500, 5006, "Login with {0} is not supported.", AppsmithErrorAction.LOG_EXTERNALLY), MARKETPLACE_NOT_CONFIGURED(500, 5007, "Marketplace is not configured.", AppsmithErrorAction.DEFAULT), - PAYLOAD_TOO_LARGE(413, 4028, "The request payload is too large. Max allowed size for request payload is {0} KB", AppsmithErrorAction.DEFAULT) + PAYLOAD_TOO_LARGE(413, 4028, "The request payload is too large. Max allowed size for request payload is {0} KB", AppsmithErrorAction.DEFAULT), + SIGNUP_DISABLED(403, 4033, "Signup is restricted on this instance of Appsmith. Please contact the administrator to get an invite.", AppsmithErrorAction.DEFAULT), ; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java index c19fed9b19..236dc11e01 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java @@ -4,6 +4,7 @@ import com.appsmith.external.models.Policy; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.acl.AppsmithRole; import com.appsmith.server.acl.RoleGraph; +import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.constants.FieldName; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.InviteUser; @@ -69,6 +70,7 @@ public class UserServiceImpl extends BaseService i private final UserOrganizationService userOrganizationService; private final RoleGraph roleGraph; private final ConfigService configService; + private final CommonConfig commonConfig; private static final String WELCOME_USER_EMAIL_TEMPLATE = "email/welcomeUserTemplate.html"; private static final String FORGOT_PASSWORD_EMAIL_TEMPLATE = "email/forgotPasswordTemplate.html"; @@ -96,7 +98,8 @@ public class UserServiceImpl extends BaseService i OrganizationRepository organizationRepository, UserOrganizationService userOrganizationService, RoleGraph roleGraph, - ConfigService configService) { + ConfigService configService, + CommonConfig commonConfig) { super(scheduler, validator, mongoConverter, reactiveMongoTemplate, repository, analyticsService); this.organizationService = organizationService; this.analyticsService = analyticsService; @@ -110,6 +113,7 @@ public class UserServiceImpl extends BaseService i this.userOrganizationService = userOrganizationService; this.roleGraph = roleGraph; this.configService = configService; + this.commonConfig = commonConfig; } @Override @@ -515,7 +519,11 @@ public class UserServiceImpl extends BaseService i } return Mono.error(new AppsmithException(AppsmithError.USER_ALREADY_EXISTS_SIGNUP, user.getUsername())); }) - .switchIfEmpty(userCreate(user)) + .switchIfEmpty( + commonConfig.isSignupDisabled() + ? Mono.error(new AppsmithException(AppsmithError.SIGNUP_DISABLED)) + : userCreate(user) + ) .flatMap(savedUser -> sendWelcomeEmail(savedUser, finalOriginHeader)); } diff --git a/app/server/appsmith-server/src/main/resources/application.properties b/app/server/appsmith-server/src/main/resources/application.properties index 32fcba615a..4844a5f66f 100644 --- a/app/server/appsmith-server/src/main/resources/application.properties +++ b/app/server/appsmith-server/src/main/resources/application.properties @@ -77,3 +77,6 @@ management.metrics.web.server.request.ignore-trailing-slash=true management.metrics.web.server.request.autotime.percentiles=0.5, 0.9, 0.95, 0.99 management.metrics.web.server.request.autotime.percentiles-histogram=true management.metrics.distribution.sla.[http.server.requests]=1s + +# Support disabling signup with an environment variable +signup.disabled = ${APPSMITH_SIGNUP_DISABLED:false} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java index dfdb5c18f7..e178b1f845 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java @@ -263,30 +263,6 @@ public class UserServiceTest { .verifyComplete(); } - @Test - @WithMockAppsmithUser - public void confirmInviteTokenFlow() { - User newUser = new User(); - newUser.setEmail("newEmail@newEmail.com"); - newUser.setIsEnabled(false); - newUser.setInviteToken("inviteToken"); - - userRepository.save(newUser).block(); - - newUser.setPassword("newPassword"); - - Mono afterConfirmationUserMono = userService.confirmInviteUser(newUser, "http://localhost:8080") - .then(userRepository.findByEmail("newEmail@newEmail.com")); - - StepVerifier.create(afterConfirmationUserMono) - .assertNext(user -> { - assertThat(user).isNotNull(); - assertThat(user.getIsEnabled()).isTrue(); - }) - .verifyComplete(); - - } - @Test @WithMockAppsmithUser public void signUpViaFormLoginIfAlreadyInvited() { diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java new file mode 100644 index 0000000000..87dc97cbb3 --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java @@ -0,0 +1,199 @@ +package com.appsmith.server.services; + +import com.appsmith.external.models.Policy; +import com.appsmith.server.acl.AppsmithRole; +import com.appsmith.server.configurations.CommonConfig; +import com.appsmith.server.configurations.WithMockAppsmithUser; +import com.appsmith.server.domains.Application; +import com.appsmith.server.domains.InviteUser; +import com.appsmith.server.domains.LoginSource; +import com.appsmith.server.domains.Organization; +import com.appsmith.server.domains.User; +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.repositories.UserRepository; +import com.appsmith.server.solutions.UserSignup; +import lombok.extern.slf4j.Slf4j; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.security.crypto.password.PasswordEncoder; +import org.springframework.security.test.context.support.WithUserDetails; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringRunner; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.util.HashSet; +import java.util.Set; + +import static com.appsmith.server.acl.AclPermission.MANAGE_APPLICATIONS; +import static com.appsmith.server.acl.AclPermission.READ_APPLICATIONS; +import static org.assertj.core.api.Assertions.assertThat; + +@Slf4j +@RunWith(SpringRunner.class) +@SpringBootTest(properties = { "signup.disabled = true" }) +@DirtiesContext +public class UserServiceWithDisabledSignupTest { + + @Autowired + UserService userService; + + @Autowired + OrganizationService organizationService; + + @Autowired + ApplicationService applicationService; + + @Autowired + UserRepository userRepository; + + @Autowired + PasswordEncoder passwordEncoder; + + @Autowired + CommonConfig commonConfig; + + Mono userMono; + + Mono organizationMono; + + @Autowired + UserSignup userSignup; + + @Before + public void setup() { + userMono = userService.findByEmail("usertest@usertest.com"); + organizationMono = organizationService.getBySlug("spring-test-organization"); + } + + @Test + @WithMockAppsmithUser + public void createNewUserValidWhenDisabled() { + User newUser = new User(); + newUser.setEmail("new-user-email@email.com"); + newUser.setPassword("new-user-test-password"); + + Mono userMono = userService.create(newUser); + + StepVerifier.create(userMono) + .expectErrorMatches(error -> { + assertThat(error).isInstanceOf(AppsmithException.class); + assertThat(((AppsmithException) error).getError()).isEqualTo(AppsmithError.SIGNUP_DISABLED); + return true; + }) + .verify(); + } + + @Test + @DirtiesContext + @WithUserDetails(value = "api_user") + public void inviteUserToApplicationValidAsAdmin() { + InviteUser inviteUser = new InviteUser(); + inviteUser.setEmail("inviteUserToApplication@test.com"); + inviteUser.setRole(AppsmithRole.APPLICATION_ADMIN); + + Mono applicationMono = applicationService.findByName("LayoutServiceTest TestApplications", MANAGE_APPLICATIONS) + .switchIfEmpty(Mono.error(new Exception("No such app"))); + + Mono userMono = applicationMono.flatMap(application -> userService + .inviteUserToApplication(inviteUser, "http://localhost:8080", application.getId())).cache(); + + Mono updatedApplication = userMono.then(applicationService.findByName("LayoutServiceTest TestApplications", MANAGE_APPLICATIONS)); + + StepVerifier.create(Mono.zip(updatedApplication, userMono)) + .assertNext(tuple -> { + Application application = tuple.getT1(); + User user = tuple.getT2(); + + Policy manageAppPolicy = Policy.builder() + .permission(MANAGE_APPLICATIONS.getValue()) + .users(Set.of("api_user", user.getUsername())) + .groups(new HashSet<>()) + .build(); + + Policy readAppPolicy = Policy.builder() + .permission(READ_APPLICATIONS.getValue()) + .users(Set.of("api_user", user.getUsername())) + .groups(new HashSet<>()) + .build(); + + + assertThat(application).isNotNull(); + assertThat(application.getPolicies()).isNotEmpty(); + assertThat(application.getPolicies()).containsAll(Set.of(manageAppPolicy, readAppPolicy)); + + assertThat(user).isNotNull(); + }) + .verifyComplete(); + } + + @Test + @WithUserDetails(value = "api_user") + public void inviteUserToApplicationValidAsViewer() { + InviteUser inviteUser = new InviteUser(); + inviteUser.setEmail("inviteUserToApplication@test.com"); + inviteUser.setRole(AppsmithRole.APPLICATION_VIEWER); + + Mono applicationMono = applicationService.findByName("LayoutServiceTest TestApplications", READ_APPLICATIONS) + .switchIfEmpty(Mono.error(new Exception("No such app"))); + + Mono userMono = applicationMono.flatMap(application -> userService + .inviteUserToApplication(inviteUser, "http://localhost:8080", application.getId())).cache(); + + Mono updatedApplication = userMono.then(applicationService.findByName("LayoutServiceTest TestApplications", READ_APPLICATIONS)); + + + StepVerifier.create(Mono.zip(updatedApplication, userMono)) + .assertNext(tuple -> { + Application application = tuple.getT1(); + User user = tuple.getT2(); + + Policy readAppPolicy = Policy.builder() + .permission(READ_APPLICATIONS.getValue()) + .users(Set.of("api_user", user.getUsername())) + .groups(new HashSet<>()) + .build(); + + Policy manageAppPolicy = Policy.builder() + .permission(MANAGE_APPLICATIONS.getValue()) + .users(Set.of("api_user")) + .build(); + + assertThat(application).isNotNull(); + assertThat(application.getPolicies()).isNotEmpty(); + assertThat(application.getPolicies()).containsAll(Set.of(manageAppPolicy, readAppPolicy)); + + assertThat(user).isNotNull(); + }) + .verifyComplete(); + } + + @Test + @WithMockAppsmithUser + public void signUpViaFormLoginIfAlreadyInvited() { + User newUser = new User(); + newUser.setEmail("alreadyInvited@alreadyInvited.com"); + newUser.setIsEnabled(false); + + userRepository.save(newUser).block(); + + User signupUser = new User(); + signupUser.setEmail(newUser.getEmail()); + signupUser.setPassword("password"); + signupUser.setSource(LoginSource.FORM); + + Mono userMono = userService.create(signupUser); + + StepVerifier.create(userMono) + .assertNext(user -> { + assertThat(user.getEmail().equals(newUser.getEmail())); + assertThat(user.getSource().equals(LoginSource.FORM)); + assertThat(user.getIsEnabled()).isTrue(); + }) + .verifyComplete(); + } +}