Fixed upload logo payload limit and added delete logo API (#1574)

Also changed name of default organization to xyz's apps
This commit is contained in:
Nidhi 2020-11-09 07:52:08 +05:30 committed by GitHub
parent fd12e907af
commit 62e4e28c8e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 175 additions and 100 deletions

View File

@ -0,0 +1,5 @@
package com.appsmith.server.constants;
public class Constraint {
public static final int ORGANIZATION_LOGO_SIZE_KB = 250;
}

View File

@ -53,4 +53,5 @@ public class FieldName {
public static String ANONYMOUS_USER = "anonymousUser";
public static String USERNAMES = "usernames";
public static String ACTION = "action";
public static String ASSET = "asset";
}

View File

@ -9,16 +9,17 @@ import com.appsmith.server.services.UserOrganizationService;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.codec.multipart.Part;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RequestPart;
import org.springframework.web.bind.annotation.RestController;
import reactor.core.publisher.Mono;
import java.util.List;
@ -37,6 +38,7 @@ public class OrganizationController extends BaseController<OrganizationService,
/**
* This function would be used to fetch all possible user roles at organization level.
*
* @return
*/
@GetMapping("/roles")
@ -67,4 +69,10 @@ public class OrganizationController extends BaseController<OrganizationService,
.map(url -> new ResponseDTO<>(HttpStatus.OK.value(), url, null));
}
@DeleteMapping("/{organizationId}/logo")
public Mono<ResponseDTO<Organization>> deleteLogo(@PathVariable String organizationId) {
return service.deleteLogo(organizationId)
.map(organization -> new ResponseDTO<>(HttpStatus.OK.value(), organization, null));
}
}

View File

@ -10,7 +10,6 @@ enum AppsmithErrorAction {
}
@Getter
public enum AppsmithError {
INVALID_PARAMETER(400, 4000, "Please enter a valid parameter {0}.", AppsmithErrorAction.DEFAULT),
PLUGIN_NOT_INSTALLED(400, 4001, "Plugin {0} not installed", AppsmithErrorAction.DEFAULT),
PLUGIN_ID_NOT_GIVEN(400, 4002, "Missing plugin id. Please enter one.", AppsmithErrorAction.DEFAULT),
@ -69,6 +68,7 @@ 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)
;

View File

@ -20,7 +20,7 @@ public interface OrganizationService extends CrudService<Organization, String> {
Mono<String> getNextUniqueSlug(String initialSlug);
Mono<Organization> createPersonal(Organization organization, User user);
Mono<Organization> createDefault(Organization organization, User user);
Mono<Organization> create(Organization organization, User user);
@ -37,4 +37,6 @@ public interface OrganizationService extends CrudService<Organization, String> {
Mono<List<UserRole>> getOrganizationMembers(String orgId);
Mono<Organization> uploadLogo(String organizationId, Part filePart);
Mono<Organization> deleteLogo(String organizationId);
}

View File

@ -3,6 +3,7 @@ package com.appsmith.server.services;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.acl.AppsmithRole;
import com.appsmith.server.acl.RoleGraph;
import com.appsmith.server.constants.Constraint;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.Asset;
import com.appsmith.server.domains.Organization;
@ -108,16 +109,17 @@ public class OrganizationServiceImpl extends BaseService<OrganizationRepository,
}
/**
* Creates the given organization as a personal organization for the given user. That is, the organization's name
* is changed to "[username]'s Personal Organization" and then created. The current value of the organization name
* Creates the given organization as a default organization for the given user. That is, the organization's name
* is changed to "[username]'s apps" and then created. The current value of the organization name
* is discarded.
*
* @param organization Organization object to be created.
* @param user User to whom this organization will belong to, as a personal organization.
* @param user User to whom this organization will belong to, as a default organization.
* @return Publishes the saved organization.
*/
@Override
public Mono<Organization> createPersonal(final Organization organization, User user) {
organization.setName(user.computeFirstName() + "'s Personal Organization");
public Mono<Organization> createDefault(final Organization organization, User user) {
organization.setName(user.computeFirstName() + "'s apps");
return create(organization, user);
}
@ -130,7 +132,7 @@ public class OrganizationServiceImpl extends BaseService<OrganizationRepository,
* 5. Assigns the default groups to the user creating the organization
*
* @param organization Organization object to be created.
* @param user User to whom this organization will belong to.
* @param user User to whom this organization will belong to.
* @return Publishes the saved organization.
*/
@Override
@ -300,7 +302,7 @@ public class OrganizationServiceImpl extends BaseService<OrganizationRepository,
Set<AppsmithRole> appsmithRoles = roleGraph.generateHierarchicalRoles(roleName);
Map<String, String> appsmithRolesMap = appsmithRoles
Map<String, String> appsmithRolesMap = appsmithRoles
.stream()
.collect(toMap(AppsmithRole::getName, AppsmithRole::getDescription));
@ -321,17 +323,27 @@ public class OrganizationServiceImpl extends BaseService<OrganizationRepository,
@Override
public Mono<Organization> uploadLogo(String organizationId, Part filePart) {
return Mono
.zip(
repository
.findById(organizationId, MANAGE_ORGANIZATIONS)
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.ORGANIZATION, organizationId))),
filePart.content().single()
)
return repository
.findById(organizationId, MANAGE_ORGANIZATIONS)
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.ORGANIZATION, organizationId)))
.flatMap(organization -> {
if (filePart != null && filePart.headers().getContentType() != null) {
// Default implementation for the BufferFactory used breaks down the FilePart into chunks of 4KB
// To limit file size to 250KB, we only allow 63 (250/4 = 62.5) such chunks to be derived from the incoming FilePart
return filePart.content().count().flatMap(count -> {
if (count > (int) Math.ceil(Constraint.ORGANIZATION_LOGO_SIZE_KB / 4.0)) {
return Mono.error(new AppsmithException(AppsmithError.PAYLOAD_TOO_LARGE, Constraint.ORGANIZATION_LOGO_SIZE_KB));
} else {
return Mono.zip(Mono.just(organization), DataBufferUtils.join(filePart.content()));
}
});
} else {
return Mono.error(new AppsmithException(AppsmithError.VALIDATION_FAILURE, "Please upload a valid image."));
}
})
.flatMap(tuple -> {
final Organization organization = tuple.getT1();
final DataBuffer dataBuffer = tuple.getT2();
final String prevAssetId = organization.getLogoAssetId();
byte[] data = new byte[dataBuffer.readableByteCount()];
@ -342,13 +354,40 @@ public class OrganizationServiceImpl extends BaseService<OrganizationRepository,
.save(new Asset(filePart.headers().getContentType(), data))
.flatMap(asset -> {
organization.setLogoAssetId(asset.getId());
return repository.save(organization);
Mono<Organization> savedOrganization = repository.save(organization);
Mono<Asset> createdAsset = analyticsService.sendCreateEvent(asset);
return savedOrganization.zipWith(createdAsset);
})
.flatMap(savedOrganization ->
prevAssetId != null
? assetRepository.deleteById(prevAssetId).thenReturn(savedOrganization)
: Mono.just(savedOrganization)
);
.flatMap(savedTuple -> {
Organization savedOrganization = savedTuple.getT1();
if (prevAssetId != null) {
return assetRepository.findById(prevAssetId)
.flatMap(asset -> assetRepository.delete(asset).thenReturn(asset))
.flatMap(analyticsService::sendDeleteEvent)
.thenReturn(savedOrganization);
} else {
return Mono.just(savedOrganization);
}
});
});
}
@Override
public Mono<Organization> deleteLogo(String organizationId) {
return repository
.findById(organizationId, MANAGE_ORGANIZATIONS)
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.ORGANIZATION, organizationId)))
.flatMap(organization -> {
final String prevAssetId = organization.getLogoAssetId();
if(prevAssetId == null) {
return Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.ASSET, prevAssetId));
}
organization.setLogoAssetId(null);
return assetRepository.findById(prevAssetId)
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.ASSET, prevAssetId)))
.flatMap(asset -> assetRepository.delete(asset).thenReturn(asset))
.flatMap(analyticsService::sendDeleteEvent)
.then(repository.save(organization));
});
}

View File

@ -458,10 +458,10 @@ public class UserServiceImpl extends BaseService<UserRepository, User, String> i
final String templateOrganizationId = tuple.getT2();
if (!StringUtils.hasText(templateOrganizationId)) {
// Since template organization is not configured, we create an empty personal organization.
// Since template organization is not configured, we create an empty default organization.
final User savedUser = tuple.getT1();
log.debug("Creating blank personal organization for user '{}'.", savedUser.getEmail());
return organizationService.createPersonal(new Organization(), savedUser);
log.debug("Creating blank default organization for user '{}'.", savedUser.getEmail());
return organizationService.createDefault(new Organization(), savedUser);
}
return Mono.empty();
@ -472,8 +472,8 @@ public class UserServiceImpl extends BaseService<UserRepository, User, String> i
/**
* This function creates a new user in the system. Primarily used by new users signing up for the first time on the
* platform. This flow also ensures that a personal workspace name is created for the user. The new user is then
* given admin permissions to the personal workspace.
* platform. This flow also ensures that a default organization name is created for the user. The new user is then
* given admin permissions to the default organization.
* <p>
* For new user invite flow, please {@link UserService#inviteUser(InviteUsersDTO, String)}
*
@ -694,7 +694,7 @@ public class UserServiceImpl extends BaseService<UserRepository, User, String> i
// user was invited.
newUser.setInviteToken(UUID.randomUUID().toString());
// Call user service's userCreate function so that the personal organization, etc are also created along with assigning basic permissions.
// Call user service's userCreate function so that the default organization, etc are also created along with assigning basic permissions.
return userCreate(newUser)
.flatMap(createdUser -> {
log.debug("Going to send email for invite user to {}", createdUser.getEmail());

View File

@ -126,7 +126,7 @@ public class ExamplesOrganizationCloner {
organization.getUserRoles().clear();
}
organization.setSlug(null);
return organizationService.createPersonal(organization, user);
return organizationService.createDefault(organization, user);
})
.flatMap(newOrganization -> {
User userUpdate = new User();

View File

@ -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.constants.Constraint;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.Asset;
@ -18,17 +19,18 @@ import com.appsmith.server.repositories.AssetRepository;
import com.appsmith.server.repositories.DatasourceRepository;
import com.appsmith.server.repositories.OrganizationRepository;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.NotImplementedException;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.buffer.DataBuffer;
import org.springframework.core.io.buffer.DataBufferUtils;
import org.springframework.core.io.buffer.DefaultDataBufferFactory;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.codec.multipart.Part;
import org.springframework.http.codec.multipart.FilePart;
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.security.test.context.support.WithUserDetails;
import org.springframework.test.annotation.DirtiesContext;
@ -36,12 +38,10 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.test.StepVerifier;
import reactor.util.annotation.NonNull;
import reactor.util.function.Tuple2;
import reactor.util.function.Tuple3;
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
@ -954,28 +954,70 @@ public class OrganizationServiceTest {
@Test
@WithUserDetails(value = "api_user")
public void uploadOrganizationLogo() throws IOException {
final InputStream imageResourceStream = getClass().getClassLoader()
.getResourceAsStream("test_assets/OrganizationServiceTest/my_organization_logo.png");
assertThat(imageResourceStream).isNotNull();
public void uploadOrganizationLogo_nullFilePart() throws IOException {
Mono<Organization> createOrganization = organizationService.create(organization).cache();
final Mono<Organization> resultMono = createOrganization
.flatMap(organization -> organizationService.uploadLogo(organization.getId(), null));
final byte[] bytes = imageResourceStream.readAllBytes();
final InMemoryFilePart filePart = new InMemoryFilePart(bytes, MediaType.IMAGE_PNG);
StepVerifier.create(resultMono)
.expectErrorMatches(throwable -> throwable instanceof AppsmithException &&
throwable.getMessage().equals(AppsmithError.VALIDATION_FAILURE.getMessage("Please upload a valid image.")))
.verify();
}
final String organizationId = organizationRepository
.findByName("Spring Test Organization")
.blockOptional(Duration.ofSeconds(3))
.map(Organization::getId)
.orElse(null);
@Test
@WithUserDetails(value = "api_user")
public void uploadOrganizationLogo_largeFilePart() throws IOException {
FilePart filepart = Mockito.mock(FilePart.class, Mockito.RETURNS_DEEP_STUBS);
Flux<DataBuffer> dataBufferFlux = DataBufferUtils
.read(new ClassPathResource("test_assets/OrganizationServiceTest/my_organization_logo_large.png"), new DefaultDataBufferFactory(), 4096);
assertThat(dataBufferFlux.count().block()).isGreaterThan((int) Math.ceil(Constraint.ORGANIZATION_LOGO_SIZE_KB/4.0));
assertThat(organizationId).isNotNull();
Mockito.when(filepart.content()).thenReturn(dataBufferFlux);
Mockito.when(filepart.headers().getContentType()).thenReturn(MediaType.IMAGE_PNG);
final Mono<Tuple2<Organization, Asset>> resultMono = organizationService
.uploadLogo(organizationId, filePart)
.flatMap(organizationWithLogo -> Mono.zip(
Mono.just(organizationWithLogo),
assetRepository.findById(organizationWithLogo.getLogoAssetId())
));
// The pre-requisite of creating an organization has been blocked for code readability
// The duration sets an upper limit for this test to run
String orgId = organizationService.create(organization).blockOptional(Duration.ofSeconds(3)).map(Organization::getId).orElse(null);
assertThat(orgId).isNotNull();
final Mono<Organization> resultMono = organizationService.uploadLogo(orgId, filepart);
StepVerifier.create(resultMono)
.expectErrorMatches(throwable -> throwable instanceof AppsmithException &&
throwable.getMessage().equals(AppsmithError.PAYLOAD_TOO_LARGE.getMessage(Constraint.ORGANIZATION_LOGO_SIZE_KB)))
.verify();
}
@Test
@WithUserDetails(value = "api_user")
public void testDeleteLogo_invalidOrganization() {
Mono<Organization> deleteLogo = organizationService.deleteLogo("");
StepVerifier.create(deleteLogo)
.expectErrorMatches(throwable -> throwable instanceof AppsmithException &&
throwable.getMessage().equals(AppsmithError.NO_RESOURCE_FOUND.getMessage(FieldName.ORGANIZATION, "")))
.verify();
}
@Test
@WithUserDetails(value = "api_user")
public void testUpdateAndDeleteLogo_validLogo() throws IOException {
FilePart filepart = Mockito.mock(FilePart.class, Mockito.RETURNS_DEEP_STUBS);
Flux<DataBuffer> dataBufferFlux = DataBufferUtils
.read(new ClassPathResource("test_assets/OrganizationServiceTest/my_organization_logo.png"), new DefaultDataBufferFactory(), 4096).cache();
assertThat(dataBufferFlux.count().block()).isLessThanOrEqualTo((int) Math.ceil(Constraint.ORGANIZATION_LOGO_SIZE_KB/4.0));
Mockito.when(filepart.content()).thenReturn(dataBufferFlux);
Mockito.when(filepart.headers().getContentType()).thenReturn(MediaType.IMAGE_PNG);
Mono<Organization> createOrganization = organizationService.create(organization).cache();
final Mono<Tuple2<Organization, Asset>> resultMono = createOrganization
.flatMap(organization -> organizationService.uploadLogo(organization.getId(), filepart)
.flatMap(organizationWithLogo -> Mono.zip(
Mono.just(organizationWithLogo),
assetRepository.findById(organizationWithLogo.getLogoAssetId()))
));
StepVerifier.create(resultMono)
.assertNext(tuple -> {
@ -985,41 +1027,19 @@ public class OrganizationServiceTest {
final Asset asset = tuple.getT2();
assertThat(asset).isNotNull();
assertThat(asset.getData()).isEqualTo(bytes);
DataBuffer buffer = DataBufferUtils.join(dataBufferFlux).block(Duration.ofSeconds(3));
byte[] res = new byte[buffer.readableByteCount()];
buffer.read(res);
assertThat(asset.getData()).isEqualTo(res);
})
.verifyComplete();
Mono<Organization> deleteLogo = createOrganization.flatMap(organization -> organizationService.deleteLogo(organization.getId()));
StepVerifier.create(deleteLogo)
.assertNext(x -> {
assertThat(x.getLogoAssetId()).isNull();
log.debug("Deleted logo for org: {}", x.getId());
})
.verifyComplete();
}
private static class InMemoryFilePart implements Part {
private final DataBuffer buffer;
private final HttpHeaders headers;
public InMemoryFilePart(byte[] bytes, MediaType contentType) {
this.buffer = new DefaultDataBufferFactory().wrap(bytes);
headers = new HttpHeaders();
headers.setContentType(contentType);
}
@Override
@NonNull
public String name() {
throw new NotImplementedException(
"This is a FilePart made for testing. The name() method is not implemented.");
}
@Override
@NonNull
public HttpHeaders headers() {
return headers;
}
@Override
@NonNull
public Flux<DataBuffer> content() {
return Flux.just(buffer);
}
}
}

View File

@ -171,8 +171,8 @@ public class UserServiceTest {
assertThat(user.getName()).isEqualTo("new-user-email@email.com");
assertThat(user.getPolicies()).isNotEmpty();
assertThat(user.getPolicies()).containsAll(Set.of(manageUserPolicy, manageUserOrgPolicy, readUserPolicy, readUserOrgPolicy));
// Since there is a template organization, the user won't have an empty personal organization. They
// will get a clone of the personal organization when they first login. So, we expect it to be
// Since there is a template organization, the user won't have an empty default organization. They
// will get a clone of the default organization when they first login. So, we expect it to be
// empty here.
assertThat(user.getOrganizationIds()).isNullOrEmpty();
})

View File

@ -160,7 +160,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).isEmpty();
@ -209,7 +209,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).hasSize(1);
@ -268,7 +268,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).hasSize(2);
@ -323,7 +323,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).isEmpty();
@ -374,7 +374,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.datasources).hasSize(2);
@ -458,7 +458,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).hasSize(2);
@ -610,7 +610,7 @@ public class ExamplesOrganizationClonerTests {
.assertNext(data -> {
assertThat(data.organization).isNotNull();
assertThat(data.organization.getId()).isNotNull();
assertThat(data.organization.getName()).isEqualTo("api_user's Personal Organization");
assertThat(data.organization.getName()).isEqualTo("api_user's apps");
assertThat(data.organization.getPolicies()).isNotEmpty();
assertThat(data.applications).hasSize(2);

Binary file not shown.

After

Width:  |  Height:  |  Size: 311 KiB