From 9a8bf9dc89fdf8cf29e894a1e7a69a7da6e275ba Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Wed, 13 May 2020 18:00:03 +0000 Subject: [PATCH] In case the user is not signed in, return a 401 so that the user can be redirected to login by the frontend. --- .../server/configurations/SecurityConfig.java | 1 + .../controllers/ApplicationController.java | 5 +- .../com/appsmith/server/domains/User.java | 5 + .../appsmith/server/dtos/UserHomepageDTO.java | 18 ++++ .../server/exceptions/AppsmithError.java | 1 + .../appsmith/server/filters/AclFilter.java | 95 +++++++++---------- .../server/services/ApplicationService.java | 6 +- .../services/ApplicationServiceImpl.java | 25 ++++- .../server/services/UserServiceImpl.java | 3 + .../services/ApplicationServiceTest.java | 13 ++- 10 files changed, 105 insertions(+), 67 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserHomepageDTO.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java index 65ec0ca215..d64f49cafe 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java @@ -143,6 +143,7 @@ public class SecurityConfig { user.setEmail("anonymousUser"); user.setCurrentOrganizationId(""); user.setOrganizationIds(new HashSet<>()); + user.setIsAnonymous(true); return user; } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java index 5efe498846..16c89edca7 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java @@ -2,7 +2,7 @@ package com.appsmith.server.controllers; import com.appsmith.server.constants.Url; import com.appsmith.server.domains.Application; -import com.appsmith.server.dtos.OrganizationApplicationsDTO; +import com.appsmith.server.dtos.UserHomepageDTO; import com.appsmith.server.dtos.ResponseDTO; import com.appsmith.server.services.ApplicationPageService; import com.appsmith.server.services.ApplicationService; @@ -22,7 +22,6 @@ import org.springframework.web.bind.annotation.RestController; import reactor.core.publisher.Mono; import javax.validation.Valid; -import java.util.List; @RestController @RequestMapping(Url.APPLICATION_URL) @@ -65,7 +64,7 @@ public class ApplicationController extends BaseController>> getAllApplicationsMock() { + public Mono> getAllApplicationsForHome() { log.debug("Going to get all applications grouped by organization"); return service.getAllApplications() .map(applications -> new ResponseDTO<>(HttpStatus.OK.value(), applications, null)); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java index 1df1d96ac6..39fd6a6aa5 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/User.java @@ -4,8 +4,10 @@ import com.appsmith.external.models.BaseDomain; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import lombok.Getter; +import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; +import org.springframework.data.annotation.Transient; import org.springframework.data.mongodb.core.mapping.Document; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.userdetails.UserDetails; @@ -53,6 +55,9 @@ public class User extends BaseDomain implements UserDetails { // During evaluation a union of the group permissions and user-specific permissions will take effect. private Set permissions = new HashSet<>(); + @Transient + Boolean isAnonymous = false; + @Override public Collection getAuthorities() { return null; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserHomepageDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserHomepageDTO.java new file mode 100644 index 0000000000..322d23abe3 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UserHomepageDTO.java @@ -0,0 +1,18 @@ +package com.appsmith.server.dtos; + +import com.appsmith.server.domains.User; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import lombok.ToString; + +import java.util.List; + +@Getter +@Setter +@NoArgsConstructor +@ToString +public class UserHomepageDTO { + User user; + List organizationApplications; +} 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 fd089e2a3e..aec9dec075 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 @@ -26,6 +26,7 @@ public enum AppsmithError { PAGE_DOESNT_BELONG_TO_APPLICATION(400, 4018, "Page {0} does not belong to the application {1}"), NO_DSL_FOUND_IN_PAGE(400, 4020, "The page {0} doesn't have a DSL. This is an unexpected state"), UNAUTHORIZED_DOMAIN(401, 4019, "Invalid email domain provided. Please sign in with a valid work email ID"), + USER_NOT_SIGNED_IN(401, 4020, "User is not logged in. Please sign in with the registered email ID or sign up" ), INVALID_PASSWORD_RESET(400, 4020, "Unable to reset the password. Please initiate a request via 'forgot password' link to reset your password"), LOGIN_INTERNAL_ERROR(401, 4021, "Internal error while trying to login"), JSON_PROCESSING_ERROR(400, 4022, "Json processing error with error {0}"), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java index f1f92da013..600ffebf0e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java @@ -1,19 +1,9 @@ package com.appsmith.server.filters; import com.appsmith.server.acl.AclService; -import com.appsmith.server.acl.OpaResponse; -import com.appsmith.server.dtos.ResponseDTO; -import com.appsmith.server.exceptions.AppsmithError; -import com.appsmith.server.exceptions.ErrorDTO; -import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.io.buffer.DataBuffer; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.stereotype.Component; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; @@ -35,48 +25,51 @@ public class AclFilter implements WebFilter { @Override public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { - ServerHttpRequest request = exchange.getRequest(); - HttpMethod httpMethod = request.getMethod(); - String url = request.getPath().value(); - String[] urlParts = url.split("/"); - // This is because all the urls are of the form /api/v1/{resource}. When we split by "/", the resource is always - // the 4th element in the result array - if (urlParts.length < 4) { - log.debug("Got request path {}. Not applying ACL filter", request.getPath()); - return chain.filter(exchange); - } + return chain.filter(exchange); - String resource = urlParts[3]; - - Mono aclResponse = aclService.evaluateAcl(httpMethod, resource, url); - return aclResponse - .map(acl -> { - log.debug("Got ACL response: {}", acl); - return acl; - }) - .flatMap(acl -> { - if (acl != null && acl.isSuccessful()) { - // Acl returned true. Continue with the filter chain - return chain.filter(exchange); - } - - // The Acl response is false. Return unauthorized exception to the client - // We construct the error response JSON here because throwing an exception here doesn't get caught - // in the {@see GlobalExceptionHandler}. - AppsmithError error = AppsmithError.UNAUTHORIZED_ACCESS; - exchange.getResponse().setStatusCode(HttpStatus.resolve(error.getHttpErrorCode())); - exchange.getResponse().getHeaders().setContentType(MediaType.APPLICATION_JSON); - try { - ResponseDTO responseBody = new ResponseDTO<>(error.getHttpErrorCode(), new ErrorDTO(error.getAppErrorCode(), - error.getMessage())); - String responseStr = objectMapper.writeValueAsString(responseBody); - DataBuffer buffer = exchange.getResponse().bufferFactory().allocateBuffer().write(responseStr.getBytes()); - return exchange.getResponse().writeWith(Mono.just(buffer)); - } catch (JsonProcessingException e) { - log.error("Exception caught while serializing JSON in AclFilter. Cause: ", e); - return exchange.getResponse().writeWith(Mono.empty()); - } - }); +// ServerHttpRequest request = exchange.getRequest(); +// HttpMethod httpMethod = request.getMethod(); +// String url = request.getPath().value(); +// String[] urlParts = url.split("/"); +// +// // This is because all the urls are of the form /api/v1/{resource}. When we split by "/", the resource is always +// // the 4th element in the result array +// if (urlParts.length < 4) { +// log.debug("Got request path {}. Not applying ACL filter", request.getPath()); +// return chain.filter(exchange); +// } +// +// String resource = urlParts[3]; +// +// Mono aclResponse = aclService.evaluateAcl(httpMethod, resource, url); +// return aclResponse +// .map(acl -> { +// log.debug("Got ACL response: {}", acl); +// return acl; +// }) +// .flatMap(acl -> { +// if (acl != null && acl.isSuccessful()) { +// // Acl returned true. Continue with the filter chain +// return chain.filter(exchange); +// } +// +// // The Acl response is false. Return unauthorized exception to the client +// // We construct the error response JSON here because throwing an exception here doesn't get caught +// // in the {@see GlobalExceptionHandler}. +// AppsmithError error = AppsmithError.UNAUTHORIZED_ACCESS; +// exchange.getResponse().setStatusCode(HttpStatus.resolve(error.getHttpErrorCode())); +// exchange.getResponse().getHeaders().setContentType(MediaType.APPLICATION_JSON); +// try { +// ResponseDTO responseBody = new ResponseDTO<>(error.getHttpErrorCode(), new ErrorDTO(error.getAppErrorCode(), +// error.getMessage())); +// String responseStr = objectMapper.writeValueAsString(responseBody); +// DataBuffer buffer = exchange.getResponse().bufferFactory().allocateBuffer().write(responseStr.getBytes()); +// return exchange.getResponse().writeWith(Mono.just(buffer)); +// } catch (JsonProcessingException e) { +// log.error("Exception caught while serializing JSON in AclFilter. Cause: ", e); +// return exchange.getResponse().writeWith(Mono.empty()); +// } +// }); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationService.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationService.java index 8c27ed1a6f..d48c67be4e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationService.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationService.java @@ -2,11 +2,9 @@ package com.appsmith.server.services; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.domains.Application; -import com.appsmith.server.dtos.OrganizationApplicationsDTO; +import com.appsmith.server.dtos.UserHomepageDTO; import reactor.core.publisher.Mono; -import java.util.List; - public interface ApplicationService extends CrudService { Mono findById(String id); @@ -23,5 +21,5 @@ public interface ApplicationService extends CrudService { Mono archive(Application application); - Mono> getAllApplications(); + Mono getAllApplications(); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java index 972f1c0c34..78afab538c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java @@ -7,6 +7,8 @@ import com.appsmith.server.domains.Application; import com.appsmith.server.domains.ApplicationPage; import com.appsmith.server.domains.Layout; import com.appsmith.server.domains.Organization; +import com.appsmith.server.domains.User; +import com.appsmith.server.dtos.UserHomepageDTO; import com.appsmith.server.dtos.OrganizationApplicationsDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; @@ -29,6 +31,7 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -167,13 +170,21 @@ public class ApplicationServiceImpl extends BaseService> getAllApplications() { + public Mono getAllApplications() { - return sessionUserService + Mono userMono = sessionUserService .getCurrentUser() - .map(user -> user.getOrganizationIds()) - .flatMap(orgIds -> { + .flatMap(user -> { + if (user.getIsAnonymous()) { + return Mono.error(new AppsmithException(AppsmithError.USER_NOT_SIGNED_IN)); + } + return Mono.just(user); + }) + .cache(); + return userMono + .flatMap(user -> { + Set orgIds = user.getOrganizationIds(); /* * For all the organization ids present in the user object, fetch all the organization objects * and store in a map for fast access; @@ -181,6 +192,9 @@ public class ApplicationServiceImpl extends BaseService> organizationsMapMono = organizationService.findByIdsIn(orgIds, READ_ORGANIZATIONS) .collectMap(Organization::getId, Function.identity()); + UserHomepageDTO userHomepageDTO = new UserHomepageDTO(); + userHomepageDTO.setUser(user); + return repository // Fetch all the applications which belong the organization ids present in the user .findByMultipleOrganizationIds(orgIds, READ_APPLICATIONS) @@ -208,7 +222,8 @@ public class ApplicationServiceImpl extends BaseService i public Mono getUserProfile() { return sessionUserService.getCurrentUser() .flatMap(user -> { + if (user.getIsAnonymous()) { + return Mono.error(new AppsmithException(AppsmithError.USER_NOT_SIGNED_IN)); + } String currentOrganizationId = user.getCurrentOrganizationId(); UserProfileDTO userProfile = new UserProfileDTO(); userProfile.setUser(user); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java index b17759b857..ba01b1b1ee 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java @@ -4,6 +4,7 @@ import com.appsmith.external.models.Policy; import com.appsmith.server.constants.FieldName; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.Page; +import com.appsmith.server.dtos.UserHomepageDTO; import com.appsmith.server.dtos.OrganizationApplicationsDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; @@ -253,14 +254,18 @@ public class ApplicationServiceTest { @Test @WithUserDetails(value = "api_user") public void getAllApplicationsForHome() { - Mono> allApplications = applicationService.getAllApplications(); + Mono allApplications = applicationService.getAllApplications(); StepVerifier .create(allApplications) - .assertNext(organizationApplicationsDTOS -> { - assertThat(organizationApplicationsDTOS).isNotEmpty(); + .assertNext(userHomepageDTO -> { + assertThat(userHomepageDTO).isNotNull(); + //In case of anonymous user, we should have errored out. Assert that the user is not anonymous. + assertThat(userHomepageDTO.getUser().getIsAnonymous()).isFalse(); - OrganizationApplicationsDTO orgAppDto = organizationApplicationsDTOS.get(0); + List organizationApplications = userHomepageDTO.getOrganizationApplications(); + + OrganizationApplicationsDTO orgAppDto = organizationApplications.get(0); assertThat(orgAppDto.getOrganization().getUserPermissions().contains("read:organizations")); Application application = orgAppDto.getApplications().get(0);