From 989d93f1090fcdefa13a2b0fd0419a084afefbdc Mon Sep 17 00:00:00 2001 From: Suwon Chae Date: Wed, 29 Jan 2014 15:56:49 +0900 Subject: [PATCH 1/2] attachment: fix unintentionally attaching files when a user posting something --- app/Global.java | 13 +++++++++ app/controllers/AbstractPostingApp.java | 24 +++++++++++++++-- app/controllers/AttachmentApp.java | 9 ++----- app/controllers/BoardApp.java | 3 +-- app/controllers/IssueApp.java | 8 +++--- app/models/Attachment.java | 25 +++++++++++++++++ public/javascripts/common/yobi.Attachments.js | 27 ++++++++++++++++++- 7 files changed, 93 insertions(+), 16 deletions(-) diff --git a/app/Global.java b/app/Global.java index 1a4381267..6567718f6 100644 --- a/app/Global.java +++ b/app/Global.java @@ -28,6 +28,7 @@ import java.nio.file.Paths; import java.security.SecureRandom; import java.util.Date; +import java.util.List; import com.typesafe.config.Config; import com.typesafe.config.ConfigFactory; @@ -37,6 +38,7 @@ import controllers.UserApp; import controllers.routes; +import models.enumeration.ResourceType; import org.apache.commons.lang3.StringUtils; import org.apache.http.impl.cookie.DateUtils; import play.Application; @@ -110,6 +112,17 @@ public void onStart(Application app) { } NotificationEvent.scheduleDeleteOldNotifications(); + cleanupTemporaryUploadFiles(); + } + + /** + * Remove all of temporary files uploaded by users + */ + private void cleanupTemporaryUploadFiles() { + List attachmentList = Attachment.find.where().eq("containerType", ResourceType.USER).findList(); + for (Attachment attachment : attachmentList) { + attachment.delete(); + } } private boolean notificationEnabled() { diff --git a/app/controllers/AbstractPostingApp.java b/app/controllers/AbstractPostingApp.java index f406a6a7e..e59cb4b26 100644 --- a/app/controllers/AbstractPostingApp.java +++ b/app/controllers/AbstractPostingApp.java @@ -5,10 +5,12 @@ import models.enumeration.Operation; import models.resource.Resource; +import org.apache.commons.lang3.StringUtils; import play.data.Form; import play.db.ebean.Model; import play.mvc.Call; import play.mvc.Controller; +import play.mvc.Http; import play.mvc.Result; import utils.*; @@ -65,7 +67,7 @@ public static Result newComment(final Comment comment, Form c comment.save(); // Attach all of the files in the current user's temporary storage. - Attachment.moveAll(UserApp.currentUser().asResource(), comment.asResource()); + attachUploadFilesToPost(comment.asResource()); String urlToView = toView + "#comment-" + comment.id; NotificationEvent.afterNewComment(comment, urlToView); @@ -141,9 +143,27 @@ protected static Result editPosting(AbstractPosting original, AbstractPosting po posting.updateProperties(); // Attach the files in the current user's temporary storage. - Attachment.moveAll(UserApp.currentUser().asResource(), original.asResource()); + attachUploadFilesToPost(original.asResource()); return redirect(redirectTo); } + /** + * 특정 리소스(게시글이나 댓글)에 사용자가 이전 폼에서 업로드한 임시파일을 첨부시킨다 + * + * when: 이슈등록/수정, 게시판에 글 등록/수정, 댓글쓰기 등에서 업로드한 파일을 해당 리소스에 연결할 때 + * + * {code AttachmentApp.TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES}에 지정된 이름의 input tag에 + * 업로드한 임시파일의 file id 값이 ,(comma) separator로 구분되어 들어가 있다. + * + * @param resource 이슈글,게시판글,댓글 + */ + protected static void attachUploadFilesToPost(Resource resource) { + Http.MultipartFormData body = request().body().asMultipartFormData(); + final String temporaryUploadFiles = body.asFormUrlEncoded().get(AttachmentApp.TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES)[0]; + if(StringUtils.isNotBlank(temporaryUploadFiles)){ + Attachment.moveOnlySelected(UserApp.currentUser().asResource(), resource, + temporaryUploadFiles.split(",")); + } + } } diff --git a/app/controllers/AttachmentApp.java b/app/controllers/AttachmentApp.java index d42dd8e52..501e045a8 100644 --- a/app/controllers/AttachmentApp.java +++ b/app/controllers/AttachmentApp.java @@ -27,6 +27,8 @@ public class AttachmentApp extends Controller { + public static final String TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES = "temporaryUploadFiles"; + /** * 사용자 첨부파일로 업로드한다 * @@ -256,13 +258,6 @@ public static Result getFileList() { Map>> files = new HashMap<>(); - // Get files from the user's area. - List> userFiles = new ArrayList<>(); - for (Attachment attach : Attachment.findByContainer(UserApp.currentUser().asResource())) { - userFiles.add(extractFileMetaDataFromAttachementAsMap(attach)); - } - files.put("tempFiles", userFiles); - // Get attached files only if the user has permission to read it. Map query = request().queryString(); String containerType = HttpUtil.getFirstValueFromQuery(query, "containerType"); diff --git a/app/controllers/BoardApp.java b/app/controllers/BoardApp.java index 745306d1b..b169f9f98 100644 --- a/app/controllers/BoardApp.java +++ b/app/controllers/BoardApp.java @@ -141,8 +141,7 @@ public static Result newPost(String userName, String projectName) { post.save(); - // Attach all of the files in the current user's temporary storage. - Attachment.moveAll(UserApp.currentUser().asResource(), post.asResource()); + attachUploadFilesToPost(post.asResource()); NotificationEvent.afterNewPost(post); diff --git a/app/controllers/IssueApp.java b/app/controllers/IssueApp.java index 40fb70bd4..4fb77cf76 100644 --- a/app/controllers/IssueApp.java +++ b/app/controllers/IssueApp.java @@ -11,7 +11,8 @@ import models.enumeration.Operation; import models.enumeration.ResourceType; import models.enumeration.State; -import org.apache.commons.lang.StringUtils; +import models.resource.Resource; +import org.apache.commons.lang3.StringUtils; import org.apache.tika.Tika; import org.codehaus.jackson.node.ObjectNode; import play.data.Form; @@ -527,8 +528,7 @@ public static Result newIssue(String ownerName, String projectName) { newIssue.save(); - // Attach all of the files in the current user's temporary storage. - Attachment.moveAll(UserApp.currentUser().asResource(), newIssue.asResource()); + attachUploadFilesToPost(newIssue.asResource()); NotificationEvent.afterNewIssue(newIssue); @@ -599,7 +599,7 @@ public static Result nextState(String ownerName, String projectName, Long number * @param number 이슈 번호 * @return * @throws IOException - * @see {@link AbstractPostingApp#editPosting(models.AbstractPosting, models.AbstractPosting, play.data.Form} + * @see {@link AbstractPostingApp#editPosting} */ @With(NullProjectCheckAction.class) public static Result editIssue(String ownerName, String projectName, Long number) { diff --git a/app/models/Attachment.java b/app/models/Attachment.java index 517190c5c..8001205bb 100644 --- a/app/models/Attachment.java +++ b/app/models/Attachment.java @@ -7,6 +7,7 @@ import java.security.DigestInputStream; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.Arrays; import java.util.Formatter; import java.util.List; @@ -33,6 +34,7 @@ public class Attachment extends Model implements ResourceConvertible { private static final long serialVersionUID = 7856282252495067924L; public static final Finder find = new Finder<>(Long.class, Attachment.class); + public static final int NOTHING_TO_ATTACH = 0; private static String uploadDirectory = "uploads"; @Id public Long id; @@ -138,6 +140,29 @@ public static int moveAll(Resource from, Resource to) { return attachments.size(); } + /** + * {@code from}에 첨부된 파일중 파일 아이디가{@code selectedFileIds}에 해당하는 첨부 파일을 {@code to}로 옮긴다. + * + * when: 업로드 직후 일시적으로 사용자에게 첨부되었던 첨부 파일들을, 특정 리소스(이슈, 게시물 등)으로 옮기려 할 때 + * + * @param from 첨부 파일이 원래 있었던 리소스 + * @param to 첨부 파일이 새로 옮겨질 리소스 + * @return + */ + public static int moveOnlySelected(Resource from, Resource to, String[] selectedFileIds) { + if(selectedFileIds.length == 0){ + return NOTHING_TO_ATTACH; + } + List attachments = Attachment.find.where().idIn(Arrays.asList(selectedFileIds)).findList(); + for (Attachment attachment : attachments) { + if(attachment.containerId.equals(from.getId()) + && attachment.containerType == from.getType()){ + attachment.moveTo(to); + } + } + return attachments.size(); + } + /** * 이 첨부 파일을 {@code to}로 옮긴다. * diff --git a/public/javascripts/common/yobi.Attachments.js b/public/javascripts/common/yobi.Attachments.js index 47bdeacba..c09b80f0d 100755 --- a/public/javascripts/common/yobi.Attachments.js +++ b/public/javascripts/common/yobi.Attachments.js @@ -64,6 +64,14 @@ yobi.Attachments = function(htOptions) { * @param {Hash Table} htOptions 초기화 옵션 */ function _initElement(htOptions){ + + // parentForm + htElements.welToAttach = htOptions.targetFormId || $(htOptions.elContainer); + var sTagName = htOptions.sTagNameForTemporaryUploadFiles || "temporaryUploadFiles"; + htElements.welTemporaryUploadFileList = $(''); + htElements.welToAttach.prepend(htElements.welTemporaryUploadFileList); + aTemporaryFileIds = []; + // welContainer htElements.welContainer = $(htOptions.elContainer); htElements.welContainer.data("isYobiAttachment", true); @@ -269,6 +277,21 @@ yobi.Attachments = function(htOptions) { }, 500); } + function _addUploadFileIdToListAndForm(sFileId) { + if( aTemporaryFileIds.indexOf(sFileId) === -1) { + aTemporaryFileIds.push(sFileId); + htElements.welTemporaryUploadFileList.val(aTemporaryFileIds.join(",")); + } + } + + function _removeDeletedFileIdFromListAndForm(sFileId) { + var nIndex = aTemporaryFileIds.indexOf(sFileId.toString()); + if( nIndex !== -1){ + aTemporaryFileIds.splice(nIndex, 1); + htElements.welTemporaryUploadFileList.val(aTemporaryFileIds.join(",")); + } + } + /** * 첨부 파일 전송에 성공시 이벤트 핸들러 * On success to submit temporary form created in onChangeFile() @@ -282,6 +305,7 @@ yobi.Attachments = function(htOptions) { var oRes = htData.oRes; var nSubmitId = htData.nSubmitId; + _addUploadFileIdToListAndForm(htData.oRes.id); // Validate server response if(!(oRes instanceof Object) || !oRes.name || !oRes.url){ return _onErrorUpload(nSubmitId, oRes); @@ -384,9 +408,10 @@ yobi.Attachments = function(htOptions) { function _deleteAttachedFile(welItem){ var sURL = welItem.attr("data-href"); - yobi.Files.deleteFile({ + yobi.Files.deleteFile({ "sURL" : sURL, "fOnLoad": function(){ + _removeDeletedFileIdFromListAndForm(welItem.data("id")) _clearLinkInTextarea(welItem); welItem.remove(); From 396857eeb58500ba2107bd6c91e60ecb10397c9d Mon Sep 17 00:00:00 2001 From: Suwon Chae Date: Thu, 6 Feb 2014 18:56:51 +0900 Subject: [PATCH 2/2] attachment: addtional fix - change method to remove user upload temporary files Remove only unused temporary files. It can be change sever keep-up time of temporary files wit application.temporaryfiles.keep-up.time at application.conf. Default value is 24*60*60 seconds. - show alert message temporary file missing at attachment due to keep-up time - add testcase --- app/Global.java | 58 ++++++++++++++++++++++--- app/controllers/AbstractPostingApp.java | 25 ++++++++--- app/controllers/AttachmentApp.java | 4 ++ app/models/Attachment.java | 7 ++- app/utils/JodaDateUtil.java | 4 ++ conf/application.conf.default | 3 ++ conf/evolutions/default/61.sql | 7 +++ conf/messages | 1 + conf/messages.ko | 1 + test/models/AttachmentTest.java | 25 +++++++++++ 10 files changed, 122 insertions(+), 13 deletions(-) create mode 100644 conf/evolutions/default/61.sql diff --git a/app/Global.java b/app/Global.java index 6567718f6..60242d080 100644 --- a/app/Global.java +++ b/app/Global.java @@ -29,9 +29,11 @@ import java.security.SecureRandom; import java.util.Date; import java.util.List; +import java.util.concurrent.TimeUnit; import com.typesafe.config.Config; import com.typesafe.config.ConfigFactory; +import controllers.AttachmentApp; import models.*; import com.avaje.ebean.Ebean; @@ -47,14 +49,17 @@ import play.Play; import play.api.mvc.Handler; import play.data.Form; +import play.libs.Akka; import play.mvc.Action; import play.mvc.Http; import play.mvc.Http.RequestHeader; import play.mvc.Result; import play.mvc.Results; +import scala.concurrent.duration.Duration; import utils.AccessLogger; import utils.ErrorViews; +import utils.JodaDateUtil; import utils.YamlUtil; import views.html.welcome.secret; @@ -112,17 +117,58 @@ public void onStart(Application app) { } NotificationEvent.scheduleDeleteOldNotifications(); - cleanupTemporaryUploadFiles(); + cleanupTemporaryUploadFilesWithSchedule(); } /** * Remove all of temporary files uploaded by users */ - private void cleanupTemporaryUploadFiles() { - List attachmentList = Attachment.find.where().eq("containerType", ResourceType.USER).findList(); - for (Attachment attachment : attachmentList) { - attachment.delete(); - } + private void cleanupTemporaryUploadFilesWithSchedule() { + Akka.system().scheduler().schedule( + Duration.create(0, TimeUnit.SECONDS), + Duration.create(AttachmentApp.TEMPORARYFILES_KEEPUP_TIME_MILLIS, TimeUnit.MILLISECONDS), + new Runnable() { + @Override + public void run() { + try { + String result = removeUserTemporaryFiles(); + play.Logger.info("User uploaded temporary files are cleaned up..." + result); + } catch (Exception e) { + play.Logger.warn("Failed!! User uploaded temporary files clean-up action failed!", e); + } + } + + /** + * 사용자 임시 첨부 파일을 삭제 + * + * 사용자에 의해 업로드 된지 {@code application.temporaryfiles.keep-up.time}초 이상 경과한 + * 임시파일들은 서버에서 삭제한다. + * + * @return 전체 대상 파일 중 지운 파일 ex> (10 of 10) + */ + private String removeUserTemporaryFiles() { + List attachmentList = Attachment.find.where() + .eq("containerType", ResourceType.USER) + .ge("createdDate", JodaDateUtil.beforeByMillis(AttachmentApp.TEMPORARYFILES_KEEPUP_TIME_MILLIS)) + .findList(); + int deletedFileCount = 0; + for (Attachment attachment : attachmentList) { + attachment.delete(); + deletedFileCount++; + } + if( attachmentList.size() != deletedFileCount) { + play.Logger.error( + String.format("Failed to delete user temporary files.\nExpected: %d Actual: %d", + attachmentList.size(), deletedFileCount) + ); + } + return "(" + attachmentList.size() + "of" + deletedFileCount + ")"; + } + }, + Akka.system().dispatcher() + ); + + } private boolean notificationEnabled() { diff --git a/app/controllers/AbstractPostingApp.java b/app/controllers/AbstractPostingApp.java index e59cb4b26..fb68eb3db 100644 --- a/app/controllers/AbstractPostingApp.java +++ b/app/controllers/AbstractPostingApp.java @@ -8,6 +8,7 @@ import org.apache.commons.lang3.StringUtils; import play.data.Form; import play.db.ebean.Model; +import play.i18n.Messages; import play.mvc.Call; import play.mvc.Controller; import play.mvc.Http; @@ -159,11 +160,25 @@ protected static Result editPosting(AbstractPosting original, AbstractPosting po * @param resource 이슈글,게시판글,댓글 */ protected static void attachUploadFilesToPost(Resource resource) { - Http.MultipartFormData body = request().body().asMultipartFormData(); - final String temporaryUploadFiles = body.asFormUrlEncoded().get(AttachmentApp.TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES)[0]; - if(StringUtils.isNotBlank(temporaryUploadFiles)){ - Attachment.moveOnlySelected(UserApp.currentUser().asResource(), resource, - temporaryUploadFiles.split(",")); + final String[] temporaryUploadFiles = getTemporaryFileListFromHiddenForm(); + if(StringUtils.isNotBlank(temporaryUploadFiles[0])){ + int attachedFileCount = Attachment.moveOnlySelected(UserApp.currentUser().asResource(), resource, + temporaryUploadFiles); + if( attachedFileCount != temporaryUploadFiles.length){ + flash("failed", Messages.get("post.popup.fileAttach.hasMissing", + temporaryUploadFiles.length - attachedFileCount, getTemporaryFilesServerKeepUpTimeOfMinuntes())); + } } } + + private static long getTemporaryFilesServerKeepUpTimeOfMinuntes() { + return AttachmentApp.TEMPORARYFILES_KEEPUP_TIME_MILLIS/(60*1000l); + } + + private static String[] getTemporaryFileListFromHiddenForm() { + Http.MultipartFormData body = request().body().asMultipartFormData(); + final String CSV_DELEMETER = ","; + return body.asFormUrlEncoded() + .get(AttachmentApp.TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES)[0].split(CSV_DELEMETER); + } } diff --git a/app/controllers/AttachmentApp.java b/app/controllers/AttachmentApp.java index 501e045a8..8a40b08a8 100644 --- a/app/controllers/AttachmentApp.java +++ b/app/controllers/AttachmentApp.java @@ -18,6 +18,7 @@ import org.codehaus.jackson.JsonNode; import org.h2.util.StringUtils; +import play.Configuration; import play.Logger; import play.mvc.Controller; import play.mvc.Http.MultipartFormData.FilePart; @@ -28,6 +29,9 @@ public class AttachmentApp extends Controller { public static final String TAG_NAME_FOR_TEMPORARY_UPLOAD_FILES = "temporaryUploadFiles"; + //사용자 임시 첨부 파일의 서버내 보관시간. 임시파일은 글 작성시 파일은 업로드 한 파일 중 글 작성이 완료 되지 않은 상태의 파일을 말한다. + public static final long TEMPORARYFILES_KEEPUP_TIME_MILLIS = Configuration.root() + .getMilliseconds("application.temporaryfiles.keep-up.time", 24 * 60 * 60 * 1000L); /** * 사용자 첨부파일로 업로드한다 diff --git a/app/models/Attachment.java b/app/models/Attachment.java index 8001205bb..6d6bca187 100644 --- a/app/models/Attachment.java +++ b/app/models/Attachment.java @@ -8,6 +8,7 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.Arrays; +import java.util.Date; import java.util.Formatter; import java.util.List; @@ -29,6 +30,7 @@ import play.db.ebean.Model; import scalax.file.NotDirectoryException; import utils.FileUtil; +import utils.JodaDateUtil; @Entity public class Attachment extends Model implements ResourceConvertible { @@ -49,11 +51,11 @@ public class Attachment extends Model implements ResourceConvertible { public ResourceType containerType; public String mimeType; - public Long size; - public String containerId; + private Date createdDate; + /** * 주어진 {@code attach}와 내용이 같은 첨부 파일을 찾는다. * @@ -247,6 +249,7 @@ public boolean store(File file, String name, Resource container) throws IOExcept // metadata - containerType, containerId, size and hash - in Database. this.containerType = container.getType(); this.containerId = container.getId(); + this.createdDate = JodaDateUtil.now(); if (name == null) { this.name = file.getName(); diff --git a/app/utils/JodaDateUtil.java b/app/utils/JodaDateUtil.java index 83e6f5cdc..773418b48 100644 --- a/app/utils/JodaDateUtil.java +++ b/app/utils/JodaDateUtil.java @@ -41,6 +41,10 @@ public static Date before(int days){ return new DateTime(today()).minusDays(days).toDate(); } + public static Date beforeByMillis(long millis){ + return new DateTime(today()).minus(millis).toDate(); + } + public static String momentFromNow(Long time) { return momentFromNow(time, Constants.DEFAULT_LANGUAGE); } diff --git a/conf/application.conf.default b/conf/application.conf.default index 5288a652d..470dce9aa 100644 --- a/conf/application.conf.default +++ b/conf/application.conf.default @@ -103,6 +103,9 @@ smtp.archive.size = 5 #if you want to use sign-up confirm, uncomment below #signup.require.confirm = true +# User uploaded temporary files cleanup schedule (sec, default 24hour: 24*60*60 = 86400) +# application.temporaryfiles.keep-up.time = 86400 + # Notification # ~~~~~~~~~~~~ # Check mails to send every this seconds. diff --git a/conf/evolutions/default/61.sql b/conf/evolutions/default/61.sql new file mode 100644 index 000000000..27177d112 --- /dev/null +++ b/conf/evolutions/default/61.sql @@ -0,0 +1,7 @@ +# --- !Ups + +ALTER TABLE attachment ADD COLUMN created_date DATE; + +# --- !Downs + +ALTER TABLE attachment DROP COLUMN IF EXISTS created_date; diff --git a/conf/messages b/conf/messages index 779fe1c95..85f239224 100755 --- a/conf/messages +++ b/conf/messages @@ -329,6 +329,7 @@ post.notice = notice post.notice.label = Set this post to notice post.popup.fileAttach.contents = Please, select file to attach post.popup.fileAttach.title = Select file +post.popup.fileAttach.hasMissing = There are {0} missing attachment files. It may caused by server keep time of temporary upload file ({1} min). Please upload again. post.unwatch.start = Notifications of this post has muted post.update.error = Errors on Input Value post.watch.start = You will receive notifications of this post diff --git a/conf/messages.ko b/conf/messages.ko index 2b2824d72..1f5dfbd79 100755 --- a/conf/messages.ko +++ b/conf/messages.ko @@ -330,6 +330,7 @@ post.notice = 공지 post.notice.label = 이 글을 공지사항으로 설정합니다 post.popup.fileAttach.contents = 첨부할 파일을 선택해주세요. post.popup.fileAttach.title = 첨부파일 선택 +post.popup.fileAttach.hasMissing = 첨부되지 못한 파일이 {0}개 있습니다. (첨부 파일 업로드 후 일정기간({1}분)을 초과 할때까지 글 작성을 완료하지 않았을 경우 발생할 수 있습니다. 다시 파일을 첨부해 주세요.) post.unwatch.start = 이제 이 글에 관한 알림을 받지 않습니다 post.update.error = 입력값 오류 post.watch.start = 이제 이 글에 관한 알림을 받습니다 diff --git a/test/models/AttachmentTest.java b/test/models/AttachmentTest.java index 5dd1c45cd..1a7514f7c 100644 --- a/test/models/AttachmentTest.java +++ b/test/models/AttachmentTest.java @@ -40,6 +40,31 @@ public void testSaveInUserTemporaryArea() throws IOException, NoSuchAlgorithmExc assertThat(new String(b, 0, length)).isEqualTo(new String("Hello")); } + @Test + public void testMoveOnlySelected() throws Exception { + // Given + File foo = createFileWithContents("foo.txt", "Hello".getBytes()); + File bar = createFileWithContents("bar.html", "

Bye

".getBytes()); + Issue issue = Issue.finder.byId(1L); + User user = User.findByLoginId("doortts"); + + Attachment thisOne = new Attachment(); + Attachment notThis = new Attachment(); + thisOne.store(foo, "foo.txt", user.asResource()); + notThis.store(bar, "bar.html", user.asResource()); + String[] selectedFileIds = {thisOne.id + ""}; + + // When + Attachment.moveOnlySelected(user.asResource(), issue.asResource(), selectedFileIds); + + // Then + List attachedFiles = Attachment.findByContainer(issue.asResource()); + List unattachedFiles = Attachment.findByContainer(user.asResource()); + + assertThat(attachedFiles.size()).isEqualTo(1); + assertThat(unattachedFiles.size()).isEqualTo(1); + } + public void testAttachFiles() throws IOException, NoSuchAlgorithmException { // Given File foo = createFileWithContents("foo.txt", "Hello".getBytes());