SAK-29606 Allow manual archive to create ZIPs.

When you manually create a site archive it would just create a folder. Now we give the user the option of creating a ZIP. This also means it shows up in the archives to download. Also fixed here: - an unclosed input stream. - not returning when an error occurs downloading the file and rely on the container to not send back the actual data. - better path normalisation to ensure people only download files our of the archive. - Assume archive folder is relative to sakai.home unless an absolute path is given.
axxter99 · Feb 5, 2016 · d16cfb1 · d16cfb1
1 parent fb06071
commit d16cfb1
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 29 deletions.
diff --git a/archive/archive-tool/tool/src/bundle/admin.properties b/archive/archive-tool/tool/src/bundle/admin.properties
@@ -13,6 +13,7 @@ archive.vm.archive1 		= Archive
 archive.vm.file     		= file
 archive.vm.from     		= from site
 archive.vm.import   		= Import
+archive.vm.zip      		= Zip
 archive.vm.site     		= to site
 archive.button.batch		= Batch Import/Export
 archive.button.single		= Import/Export

diff --git a/archive/archive-tool/tool/src/java/org/sakaiproject/archive/tool/ArchiveAction.java b/archive/archive-tool/tool/src/java/org/sakaiproject/archive/tool/ArchiveAction.java
@@ -57,6 +57,8 @@
 import javax.servlet.http.HttpServletResponse;
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.*;
@@ -221,7 +223,10 @@ public String buildDownloadContext(VelocityPortlet portlet, Context context, Run
 
 		//get list of existing archives
 		Collection<File> files = Collections.<File>emptySet();
-		File archiveBaseDir = new File(serverConfigurationService.getString("archive.storage.path", "sakai/archive"));
+		Path sakaiHome = Paths.get(serverConfigurationService.getSakaiHomePath());
+		// Either relative to sakai.home or absolute
+		Path archivePath = sakaiHome.resolve(serverConfigurationService.getString("archive.storage.path", "archive"));
+		File archiveBaseDir = archivePath.toFile();
 
 		if (archiveBaseDir.exists() && archiveBaseDir.isDirectory()) {
 			files = FileUtils.listFiles(archiveBaseDir, new SuffixFileFilter(".zip"), null);
@@ -291,8 +296,7 @@ public String buildDownloadContext(VelocityPortlet portlet, Context context, Run
 	* doArchive called when "eventSubmit_doArchive" is in the request parameters
 	* to run the archive.
 	*/
-	public void doArchive(RunData data, Context context)
-	{
+	public void doArchive(RunData data, Context context) throws IOException {
 		SessionState state = ((JetspeedRunData)data).getPortletSessionState(((JetspeedRunData)data).getJs_peid());
 
 		if (!securityService.isSuperUser())
@@ -302,9 +306,15 @@ public void doArchive(RunData data, Context context)
 		}
 
 		String id = data.getParameters().getString("archive-id");
+		boolean zip = data.getParameters().getBoolean("zip-id");
 		if (StringUtils.isNotBlank(id))
 		{
-			String msg = archiveService.archive(id.trim());
+			String msg;
+			if(zip) {
+				msg = archiveService.archiveAndZip(id.trim());
+			} else {
+				msg = archiveService.archive(id.trim());
+			}
 			addAlert(state, rb.getFormattedMessage("archive", new Object[]{id}) + " \n " + msg);
 		}
 		else

diff --git a/archive/archive-tool/tool/src/java/org/sakaiproject/archive/tool/DownloadServlet.java b/archive/archive-tool/tool/src/java/org/sakaiproject/archive/tool/DownloadServlet.java
@@ -5,6 +5,8 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
@@ -62,32 +64,30 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
 		if (!securityService.isSuperUser()){
 			log.error("Must be super user to download archives");
 			response.sendError(HttpServletResponse.SC_FORBIDDEN, "Must be super user to download archives");
+			return;
 		}
 
-		String archiveName = (String)request.getParameter("archive");
-		String archiveBaseDir = serverConfigurationService.getString("archive.storage.path", "sakai/archive");
-
-		//add in some basic protection
-		if(StringUtils.contains(archiveName, "..")) {
-			log.error("Archive param must be a valid site archive");
-			response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Archive param must be a valid site archive. Param was: " + archiveName);
-		}
+		String archiveName = request.getParameter("archive");
+		Path sakaiHome = Paths.get(serverConfigurationService.getSakaiHomePath());
+		Path archives = sakaiHome.resolve(serverConfigurationService.getString("archive.storage.path", "archive"));
 
 		response.setContentType("application/zip");
         response.setHeader("Content-Disposition","attachment;filename=" +archiveName);
 
-		//need to ensure user cant spoof this to get other files. Check for .. in name?
-		//does this even matter? Surely we can trust an admin?
-		String archivePath = archiveBaseDir + File.separator + archiveName;
-		File archiveFile = new File(archivePath);
-
+		Path archivePath = archives.resolve(archiveName).normalize();
+		if (!archivePath.startsWith(archives)) {
+			log.error(String.format("The archive file (%s) is not inside the archives folder (%s)",
+			archivePath.toString(), archives.toString()));
+			response.sendError(HttpServletResponse.SC_BAD_REQUEST,
+					"Archive param must be a valid site archive. Param was: " + archiveName);
+			return;
+		}
 		OutputStream out = response.getOutputStream();
-
-        InputStream in = FileUtils.openInputStream(archiveFile);
-        IOUtils.copyLarge(in, out);
-        out.flush();
-        out.close();
-
-
+
+		try (InputStream in = FileUtils.openInputStream(archivePath.toFile())) {
+			IOUtils.copyLarge(in, out);
+			out.flush();
+			out.close();
+		}
 	}
 }
diff --git a/archive/archive-tool/tool/src/webapp/vm/archive/chef_archive.vm b/archive/archive-tool/tool/src/webapp/vm/archive/chef_archive.vm
@@ -17,6 +17,13 @@
 				$tlang.getString("archive.vm.from")
 			</label>	
 			<input type="text" size="80" name="archive-id" id="archive-id" />
+
+		</p>
+		<p class="shorttext">
+		    <label for="zip-id">
+		        $tlang.getString("archive.vm.zip")
+		    </label>
+			<input type="checkbox" name="zip-id" value="true" id="zip-id" />
 		</p>
 		<p class="act"><input type="submit" name="eventSubmit_doArchive" value="$tlang.getString("archive.vm.archive1")" class="indnt1" /></p>		
 

diff --git a/common/archive-api/src/main/java/org/sakaiproject/archive/api/ArchiveService.java b/common/archive-api/src/main/java/org/sakaiproject/archive/api/ArchiveService.java
@@ -90,8 +90,8 @@ public interface ArchiveService
 	/**
 	 * Archive a site then compress it to a zip. 
 	 * @param siteId - id of site to be archived
-	 * @return true if archive and zip successful, false if not.
+	 * @return A log of messages from creating the archive
 	 * @throws IOException 
 	 */
-	public boolean archiveAndZip(String siteId) throws IOException;
+	public String archiveAndZip(String siteId) throws IOException;
 }
diff --git a/common/archive-impl/impl2/src/java/org/sakaiproject/archive/impl/ArchiveService2Impl.java b/common/archive-impl/impl2/src/java/org/sakaiproject/archive/impl/ArchiveService2Impl.java
@@ -170,9 +170,14 @@ public String mergeFromZip(String zipFilePath, String siteId, String creatorId)
 	}
 
 	@Override
-	public boolean archiveAndZip(String siteId) throws IOException {
-		m_siteArchiver.archive(siteId, m_storagePath, FROM_SAKAI_2_8);
-		return m_siteZipper.zipArchive(siteId, m_storagePath);
+	public String archiveAndZip(String siteId) throws IOException {
+		String log = m_siteArchiver.archive(siteId, m_storagePath, FROM_SAKAI_2_8);
+		if (m_siteZipper.zipArchive(siteId, m_storagePath) ){
+			log = log + "Zipfile success.\n";
+		} else {
+			log = log + "Zipfile failed\n";
+		}
+		return log;
 	}
 
 } // ArchiveService2Impl