Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

Merged
merged 29 commits into from
Feb 20, 2025
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6b0f602
fix: Return index.html when object storage used with an empty page name
david-mackessy Feb 10, 2025
96c2a8b
update tests
david-mackessy Feb 10, 2025
6a4e772
Merge branch 'master' into obj-storage-index-html
david-mackessy Feb 10, 2025
d6c79f8
fix: Handle multiple resource variations with tests
david-mackessy Feb 10, 2025
7fbf6c0
fix: Clean up var names and test resource
david-mackessy Feb 10, 2025
dfe8c7b
format
david-mackessy Feb 10, 2025
7b31fc5
Merge branch 'obj-storage-index-html' into DHIS2-18185
david-mackessy Feb 12, 2025
927e83a
fix: Unify serving app resources, fixing multiple issues [DHIS2-18185]
david-mackessy Feb 13, 2025
0e382db
fix: Update test app zip
david-mackessy Feb 13, 2025
87165d3
Merge branch 'master' into DHIS2-18185
david-mackessy Feb 13, 2025
e1cc4a7
fix: Clean up
david-mackessy Feb 13, 2025
5dccafd
fix: Remove tests that won't work in pipeline, file system
david-mackessy Feb 13, 2025
4dd3726
fix: Clean up
david-mackessy Feb 13, 2025
bbbc4df
fix: Move resource result types into 1 file
david-mackessy Feb 14, 2025
4eeeabf
fix: Add redirect result in app override filter
david-mackessy Feb 14, 2025
f48a3f1
fix: Update debug logs
david-mackessy Feb 14, 2025
a5e08cc
fix: Update log format
david-mackessy Feb 14, 2025
5fb46f3
fix: Base app dir with no slash is redirect
david-mackessy Feb 18, 2025
adf4240
Merge branch 'master' into DHIS2-18185
david-mackessy Feb 18, 2025
27e4b7d
fix: Move utils method to existing class
david-mackessy Feb 18, 2025
95a4901
fix: App base calls without slash are redirected
david-mackessy Feb 19, 2025
eadfae0
fix: App base calls without slash are redirected to correct path
david-mackessy Feb 19, 2025
a65309a
fix: Allow dhis-web resource path '/' through for rule application
david-mackessy Feb 19, 2025
729cf7d
Merge branch 'master' into DHIS2-18185
david-mackessy Feb 19, 2025
5c646d7
fix: Remove redundant code and extract method for better reading
david-mackessy Feb 19, 2025
b7f4dd5
fix: Remove permits and use string replace all
david-mackessy Feb 19, 2025
b06a5c8
Merge branch 'master' into DHIS2-18185
david-mackessy Feb 19, 2025
4abb75e
fix: Address PR comments, rename method
david-mackessy Feb 19, 2025
a71a196
Merge branch 'master' into DHIS2-18185
david-mackessy Feb 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import org.hisp.dhis.appmanager.resource.ResourceResult;
import org.springframework.core.io.Resource;

/**
Expand Down Expand Up @@ -198,9 +199,9 @@ public interface AppManager {
*
* @param app the app to look up files for
* @param pageName the page requested
* @return the Resource representing the file, or null if no file was found
* @return the {@link ResourceResult}
*/
Resource getAppResource(App app, String pageName) throws IOException;
ResourceResult getAppResource(App app, String pageName) throws IOException;

/**
* Sets the app status to DELETION_IN_PROGRESS.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
import java.io.File;
import java.io.IOException;
import java.util.Map;
import org.hisp.dhis.appmanager.resource.ResourceResult;
import org.hisp.dhis.cache.Cache;
import org.springframework.core.io.Resource;
import org.springframework.scheduling.annotation.Async;

/**
Expand Down Expand Up @@ -78,5 +78,5 @@ public interface AppStorageService {
* @param pageName the name of the page to look up
* @return The resource representing the page, or null if not found
*/
Resource getAppResource(App app, String pageName) throws IOException;
ResourceResult getAppResource(App app, String pageName) throws IOException;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.hisp.dhis.appmanager.resource;

import javax.annotation.Nonnull;

/**
* Should be used when a HTTP redirect is required
*
* @param path the redirect path to use
*/
public record Redirect(@Nonnull String path) implements ResourceResult {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package org.hisp.dhis.appmanager.resource;

import javax.annotation.Nonnull;
import org.springframework.core.io.Resource;

/**
* Should only be used with a Resource that exists
*
* @param resource the resource
*/
public record ResourceFound(@Nonnull Resource resource) implements ResourceResult {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.hisp.dhis.appmanager.resource;

import javax.annotation.Nonnull;

/**
* Should be used when a Resource does not exist.
*
* @param path the path at which the resource was attempted to be retrieved from
*/
public record ResourceNotFound(@Nonnull String path) implements ResourceResult {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.hisp.dhis.appmanager.resource;

/**
* Models the potential results when trying to retrieve a Resource. <br>
* Can be one of:
*
* <ul>
* <li>ResourceFound
* <li>ResourceNotFound
* <li>Redirect
* </ul>
*
* <p>This enables:
*
* <ul>
* <li>clearer understanding of control flow & intent
* <li>easier handling of multiple scenarios without having to deal with nulls or exceptions
* <li>easier extension potential
* </ul>
*/
public sealed interface ResourceResult permits ResourceFound, ResourceNotFound, Redirect {}
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,14 @@
import java.util.UUID;
import java.util.function.Predicate;
import java.util.stream.Stream;
import javax.annotation.Nonnull;
import javax.annotation.PostConstruct;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.StringUtils;
import org.hisp.dhis.apphub.AppHubService;
import org.hisp.dhis.appmanager.resource.ResourceResult;
import org.hisp.dhis.cache.Cache;
import org.hisp.dhis.cache.CacheBuilderProvider;
import org.hisp.dhis.datastore.DatastoreNamespace;
Expand Down Expand Up @@ -402,19 +404,29 @@ public boolean isAccessible(App app) {
}

@Override
public Resource getAppResource(App app, String pageName) throws IOException {
public ResourceResult getAppResource(App app, String pageName) throws IOException {
return getAppStorageServiceByApp(app).getAppResource(app, pageName);
}

/**
* We need to handle scenarios when the Resource is a File (knowing the content length) or when
* it's URL (not knowing the content length and having to make a call, e.g. remote web link in AWS
* S3/MinIO) - otherwise content length can be set to 0 which causes issues at the front-end,
* returning an empty body. If it's a URL resource, an underlying HEAD request is made to get the
* content length.
*
* @param resource resource to check content length
* @return the content length or -1 (unknown size) if exception caught
*/
@Override
public int getUriContentLength(Resource resource) {
public int getUriContentLength(@Nonnull Resource resource) {
try {
URLConnection urlConnection = resource.getURL().openConnection();
return urlConnection.getContentLength();
if (resource.isFile()) {
return (int) resource.contentLength();
} else {
URLConnection urlConnection = resource.getURL().openConnection();
return urlConnection.getContentLength();
}
} catch (IOException e) {
log.error("Error trying to retrieve content length of Resource: {}", e.getMessage());
e.printStackTrace();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URI;
import java.util.ArrayList;
import java.util.HashMap;
Expand All @@ -47,21 +48,23 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipException;
import java.util.zip.ZipFile;
import javax.annotation.Nonnull;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.hisp.dhis.appmanager.resource.Redirect;
import org.hisp.dhis.appmanager.resource.ResourceFound;
import org.hisp.dhis.appmanager.resource.ResourceNotFound;
import org.hisp.dhis.appmanager.resource.ResourceResult;
import org.hisp.dhis.cache.Cache;
import org.hisp.dhis.datastore.DatastoreNamespace;
import org.hisp.dhis.external.location.LocationManager;
import org.hisp.dhis.external.location.LocationManagerException;
import org.hisp.dhis.fileresource.FileResourceContentStore;
import org.hisp.dhis.jclouds.JCloudsStore;
import org.hisp.dhis.util.StringUtils;
import org.hisp.dhis.util.ZipFileUtils;
import org.jclouds.blobstore.BlobRequestSigner;
import org.jclouds.blobstore.LocalBlobRequestSigner;
import org.jclouds.blobstore.domain.Blob;
import org.jclouds.blobstore.domain.StorageMetadata;
import org.jclouds.blobstore.internal.RequestSigningUnsupported;
import org.jclouds.blobstore.options.ListContainerOptions;
import org.jclouds.http.HttpRequest;
import org.joda.time.Minutes;
import org.springframework.core.io.FileSystemResource;
import org.springframework.core.io.Resource;
Expand All @@ -83,6 +86,7 @@ public class JCloudsAppStorageService implements AppStorageService {
private final LocationManager locationManager;

private final ObjectMapper jsonMapper;
private final FileResourceContentStore fileResourceContentStore;

private void discoverInstalledApps(Consumer<App> handler) {
ObjectMapper mapper = new ObjectMapper();
Expand Down Expand Up @@ -276,13 +280,10 @@ public App installApp(File file, String filename, Cache<App> appCache) {
String namespace = app.getActivities().getDhis().getNamespace();

log.info(
String.format(
"New app '%s' installed"
+ "\n\tInstall path: %s"
+ (namespace != null && !namespace.isEmpty() ? "\n\tNamespace reserved: %s" : ""),
app.getName(),
dest,
namespace));
"New app {} installed, Install path: {}, Namespace reserved: {}",
app.getName(),
dest,
(namespace != null && !namespace.isEmpty() ? namespace : "no namespace reserved"));

// -----------------------------------------------------------------
// Installation complete.
Expand Down Expand Up @@ -327,65 +328,82 @@ public void deleteApp(App app) {
log.info("Deleted app {}", app.getName());
}

/**
* Returns a {@link ResourceResult}
*
* @param app the app to serve
* @param resource the name of the app resource to serve. This can be a directory, sub-directory
* or a page name
* @return app resource
* @throws IOException ex if any IO issues
*/
@Override
public Resource getAppResource(App app, String pageName) throws IOException {
public ResourceResult getAppResource(App app, @Nonnull String resource) throws IOException {
if (app == null || !app.getAppStorageSource().equals(AppStorageSource.JCLOUDS)) {
log.warn(
"Can't look up resource {}. The specified app was not found in JClouds storage.",
pageName);
return null;
resource);
return new ResourceNotFound(resource);
}

String key = (app.getFolderName() + ("/" + pageName)).replaceAll("//", "/");
URI uri = getSignedGetContentUri(key);

if (uri == null) {
String resolvedFileResource = useIndexHtmlIfWarranted(resource);
String key = (app.getFolderName() + ("/" + resolvedFileResource));
String cleanedKey = StringUtils.replaceAllRecursively(key, "//", "/");
URI uri = fileResourceContentStore.getSignedGetContentUri(cleanedKey);

String filepath = jCloudsStore.getBlobContainer() + "/" + key;
filepath = filepath.replaceAll("//", "/");
File res;

try {
res = locationManager.getFileForReading(filepath);
} catch (LocationManagerException e) {
return null;
}

if (res.isDirectory()) {
String indexPath = pageName.replaceAll("/+$", "") + "/index.html";
log.info("Resource {} ({} is a directory, serving {}", pageName, filepath, indexPath);
return getAppResource(app, indexPath);
} else if (res.exists()) {
return new FileSystemResource(res);
} else {
return null;
}
log.info("Checking if blob exists {}", cleanedKey);
if (jCloudsStore.blobExists(cleanedKey)) {
log.info("Blob found {}", cleanedKey);
return new ResourceFound(getResourceType(uri, cleanedKey));
}

return new UrlResource(uri);
log.info("Checking if blob exists with extra '/' {}", cleanedKey + "/");
if (jCloudsStore.blobExists(cleanedKey + "/")) {
log.info("Blob exists with extra '/' {}", cleanedKey + "/");
return new Redirect(resolvedFileResource + "/");
}
log.info("No resource found for {}", cleanedKey);
return new ResourceNotFound(resource);
}

public URI getSignedGetContentUri(String key) {
BlobRequestSigner signer = jCloudsStore.getBlobRequestSigner();

if (!requestSigningSupported(signer)) {
return null;
private Resource getResourceType(URI uri, @Nonnull String filePath) throws MalformedURLException {
if (uri == null) {
String cleanedFilepath = jCloudsStore.getBlobContainer() + "/" + filePath;
return new FileSystemResource(
locationManager.getFileForReading(
StringUtils.replaceAllRecursively(cleanedFilepath, "//", "/")));
} else {
return new UrlResource(uri);
}
}

HttpRequest httpRequest;

try {
httpRequest =
signer.signGetBlob(jCloudsStore.getBlobContainer(), key, FIVE_MINUTES_IN_SECONDS);
} catch (UnsupportedOperationException uoe) {
return null;
/**
* The server is expected to handle multiple resource path variations, which have different rules.
* <br>
*
* <p>Examples: <br>
* <li>'' ->'index.html`
* <li>'index.html' ->'index.html`
* <li>'subDir/index.html' ->'subDir/index.html`
* <li>'subDir/' ->'subDir/index.html`
* <li>'subDir' ->'subDir`
* <li>'static/js/138.af8b0ff6.chunk.js' ->'static/js/138.af8b0ff6.chunk.js`
*
* @param resource app resource to resolve
* @return potentially-updated app resource (file)
*/
private String useIndexHtmlIfWarranted(@Nonnull String resource) {
// this condition is treated as the base app resource
if (resource.isBlank()) {
log.debug("Resource is blank, using 'index.html'");
return "index.html";
}

return httpRequest.getEndpoint();
}

private boolean requestSigningSupported(BlobRequestSigner signer) {
return !(signer instanceof RequestSigningUnsupported)
&& !(signer instanceof LocalBlobRequestSigner);
// this condition is treated as a directory (any level) resource, ending in '/'
if (resource.endsWith("/")) {
log.debug("Resource ends with '/', appending 'index.html' to {}", resource);
return resource + "index.html";
}
// any other resource, no special handling required, return as is
return resource;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.FileUtils;
import org.hisp.dhis.appmanager.resource.ResourceFound;
import org.hisp.dhis.appmanager.resource.ResourceResult;
import org.hisp.dhis.cache.Cache;
import org.hisp.dhis.external.location.LocationManager;
import org.hisp.dhis.external.location.LocationManagerException;
Expand Down Expand Up @@ -184,7 +186,7 @@ private String getAppFolderPath() {
}

@Override
public Resource getAppResource(App app, String pageName) throws IOException {
public ResourceResult getAppResource(App app, String pageName) throws IOException {
List<Resource> locations =
Lists.newArrayList(
resourceLoader.getResource(
Expand All @@ -199,7 +201,7 @@ public Resource getAppResource(App app, String pageName) throws IOException {

// Make sure that file resolves into path app folder
if (file != null && file.toPath().startsWith(getAppFolderPath())) {
return resource;
return new ResourceFound(resource);
}
}
}
Expand Down
Loading
Loading