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.

Sign up for GitHub

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

Enable Uploading/Removing Attachments When Editing an Issue/Comment #8426

Merged
merged 19 commits into from
Oct 15, 2019
Merged

Enable Uploading/Removing Attachments When Editing an Issue/Comment #8426

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
36a83bd
Enable adding and edting attachments for issues and comments
blueworrybear Oct 7, 2019
687c880
fix lint error
blueworrybear Oct 7, 2019
95f2393
fix golint error
blueworrybear Oct 8, 2019
1ec28ce
fix golint error
blueworrybear Oct 8, 2019
8007fc5
fix golint error
blueworrybear Oct 8, 2019
7c54450
fix golint error
blueworrybear Oct 8, 2019
99756ba
Merge remote-tracking branch 'upstream/master' into attachment-edit-f…
blueworrybear Oct 8, 2019
f53dbb4
fix uuid undefined error
blueworrybear Oct 8, 2019
dd561e7
fix image size
blueworrybear Oct 8, 2019
640b958
update #8426 rewrite route
blueworrybear Oct 8, 2019
229b0e2
update #8426 fix coding style error
blueworrybear Oct 8, 2019
96cad21
fix lint error
blueworrybear Oct 8, 2019
94cfed7
refactoring codes for updating attachments
blueworrybear Oct 9, 2019
2a9c18e
Remove template string
blueworrybear Oct 9, 2019
427d9c4
fix filenameDict undefined problem
blueworrybear Oct 12, 2019
ce3bdf0
fix remove wrong file and add file delete permission checking
blueworrybear Oct 12, 2019
4ad1190
Merge remote-tracking branch 'upstream/master' into attachment-edit-f…
blueworrybear Oct 12, 2019
382d484
Merge branch 'master' into attachment-edit-final
lunny Oct 15, 2019
f8d15e6
Merge branch 'master' into attachment-edit-final
lafriks Oct 15, 2019
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
20 changes: 20 additions & 0 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,26 @@ func AddDeletePRBranchComment(doer *User, repo *Repository, issueID int64, branc
return sess.Commit()
}

// UpdateAttachments update attachments by UUIDs for the issue
func (issue *Issue) UpdateAttachments(uuids []string) (err error) {
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
return err
}
attachments, err := getAttachmentsByUUIDs(sess, uuids)
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %v", uuids, err)
}
for i := 0; i < len(attachments); i++ {
attachments[i].IssueID = issue.ID
if err := updateAttachment(sess, attachments[i]); err != nil {
return fmt.Errorf("update attachment [id: %d]: %v", attachments[i].ID, err)
}
}
return sess.Commit()
}

// ChangeContent changes issue content, as the given user.
func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
oldContent := issue.Content
Expand Down
21 changes: 21 additions & 0 deletions models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,27 @@ func (c *Comment) LoadAttachments() error {
return nil
}

// UpdateAttachments update attachments by UUIDs for the comment
func (c *Comment) UpdateAttachments(uuids []string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's worth having a common function and use it from Comment and Issue to avoid duplicating code? This function is not too short but also not too long, so it sits in-between.

sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return err
}
attachments, err := getAttachmentsByUUIDs(sess, uuids)
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %v", uuids, err)
}
for i := 0; i < len(attachments); i++ {
attachments[i].IssueID = c.IssueID
attachments[i].CommentID = c.ID
if err := updateAttachment(sess, attachments[i]); err != nil {
return fmt.Errorf("update attachment [id: %d]: %v", attachments[i].ID, err)
}
}
return sess.Commit()
}

// LoadAssigneeUser if comment.Type is CommentTypeAssignees, then load assignees
func (c *Comment) LoadAssigneeUser() error {
var err error
Expand Down
10 changes: 10 additions & 0 deletions modules/util/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ func ExistsInSlice(target string, slice []string) bool {
return i < len(slice)
}

// IsStringInSlice sequential searches if string exists in slice.
func IsStringInSlice(target string, slice []string) bool {
for i := 0; i < len(slice); i++ {
if slice[i] == target {
return true
}
}
return false
}

// IsEqualSlice returns true if slices are equal.
func IsEqualSlice(target []string, source []string) bool {
if len(target) != len(source) {
Expand Down
117 changes: 102 additions & 15 deletions public/js/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,71 @@ function initRepository() {
issuesTribute.attach($textarea.get());
emojiTribute.attach($textarea.get());

const $dropzone = $editContentZone.find('.dropzone');
$dropzone.data("saved", false);
const $files = $editContentZone.find('.comment-files');
if ($dropzone.length > 0) {
const filenameDict = {};
$dropzone.dropzone({
url: $dropzone.data('upload-url'),
headers: {"X-Csrf-Token": csrf},
maxFiles: $dropzone.data('max-file'),
maxFilesize: $dropzone.data('max-size'),
acceptedFiles: ($dropzone.data('accepts') === '*/*') ? null : $dropzone.data('accepts'),
addRemoveLinks: true,
dictDefaultMessage: $dropzone.data('default-message'),
dictInvalidFileType: $dropzone.data('invalid-input-type'),
dictFileTooBig: $dropzone.data('file-too-big'),
dictRemoveFile: $dropzone.data('remove-file'),
init: function () {
this.on("success", function (file, data) {
filenameDict[file.name] = {
"uuid": data.uuid,
"submitted": false
}
const input = $('<input id="' + data.uuid + '" name="files" type="hidden">').val(data.uuid);
$files.append(input);
});
this.on("removedfile", function (file) {
if (file.name in filenameDict) {
$('#' + filenameDict[file.name].uuid).remove();
}
if ($dropzone.data('remove-url') && $dropzone.data('csrf') && !filenameDict[file.name].submitted) {
$.post($dropzone.data('remove-url'), {
file: file.uuid,
_csrf: $dropzone.data('csrf')
});
}
});
this.on("submit", function () {
$.each(filenameDict, function(name){
filenameDict[name].submitted = true;
});
});
this.on("reload", function (){
$.getJSON($editContentZone.data('attachment-url'), function(data){
const drop = $dropzone.get(0).dropzone;
drop.removeAllFiles(true);
$files.empty();
$.each(data, function(){
drop.emit("addedfile", this);
drop.emit("thumbnail", this, `${$dropzone.data('upload-url')}/${this.uuid}`);
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
drop.emit("complete", this);
drop.files.push(this);
filenameDict[this.name] = {
"submitted": true,
"uuid": this.uuid
}
$dropzone.find(`img[src='${$dropzone.data('upload-url')}/${this.uuid}']`).css("max-width", "100%");
const input = $('<input id="' + this.uuid + '" name="files" type="hidden">').val(this.uuid);
$files.append(input);
});
});
});
},
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
});
$dropzone.get(0).dropzone.emit("reload");
}
// Give new write/preview data-tab name to distinguish from others
const $editContentForm = $editContentZone.find('.ui.comment.form');
const $tabMenu = $editContentForm.find('.tabular.menu');
Expand All @@ -845,27 +910,49 @@ function initRepository() {
$editContentZone.find('.cancel.button').click(function () {
$renderContent.show();
$editContentZone.hide();
$dropzone.get(0).dropzone.emit("reload");
});
$editContentZone.find('.save.button').click(function () {
$renderContent.show();
$editContentZone.hide();

const $attachments = $files.find("[name=files]").map(function(){
return $(this).val();
}).get();
$.post($editContentZone.data('update-url'), {
"_csrf": csrf,
"content": $textarea.val(),
"context": $editContentZone.data('context')
},
function (data) {
if (data.length == 0) {
$renderContent.html($('#no-content').html());
} else {
$renderContent.html(data.content);
emojify.run($renderContent[0]);
$('pre code', $renderContent[0]).each(function () {
hljs.highlightBlock(this);
});
"_csrf": csrf,
"content": $textarea.val(),
"context": $editContentZone.data('context'),
"files": $attachments
},
function (data) {
if (data.length == 0) {
$renderContent.html($('#no-content').html());
} else {
$renderContent.html(data.content);
emojify.run($renderContent[0]);
$('pre code', $renderContent[0]).each(function () {
hljs.highlightBlock(this);
});
}
const $content = $segment.parent();
if(!$content.find(".ui.small.images").length){
if(data.attachments != ""){
$content.append(`
<div class="ui bottom attached segment">
<div class="ui small images">
</div>
</div>
`);
$content.find(".ui.small.images").html(data.attachments);
}
});
}else if(data.attachments == ""){
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
$content.find(".ui.small.images").parent().remove();
}else{
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
$content.find(".ui.small.images").html(data.attachments);
}
$dropzone.get(0).dropzone.emit("submit");
$dropzone.get(0).dropzone.emit("reload");
});
});
} else {
$textarea = $segment.find('textarea');
Expand Down
19 changes: 19 additions & 0 deletions routers/repo/attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,22 @@ func UploadAttachment(ctx *context.Context) {
"uuid": attach.UUID,
})
}

// DeleteAttachment response for deleting issue's attachment
func DeleteAttachment(ctx *context.Context) {
file := ctx.Query("file")
fmt.Printf("file %s", file)
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
attach, err := models.GetAttachmentByUUID(file)
if err != nil {
ctx.Error(400, err.Error())
return
}
err = models.DeleteAttachment(attach, true)
if err != nil {
ctx.Error(500, fmt.Sprintf("DeleteAttachment: %v", err))
return
}
ctx.JSON(200, map[string]string{
"uuid": attach.UUID,
})
}
104 changes: 102 additions & 2 deletions routers/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
)

const (
tplAttachment base.TplName = "repo/issue/view_content/attachments"

tplIssues base.TplName = "repo/issue/list"
tplIssueNew base.TplName = "repo/issue/new"
tplIssueView base.TplName = "repo/issue/view"
Expand Down Expand Up @@ -1074,8 +1076,38 @@ func UpdateIssueContent(ctx *context.Context) {
return
}

files := ctx.QueryStrings("files[]")
for i := 0; i < len(issue.Attachments); i++ {
if !util.IsStringInSlice(issue.Attachments[i].UUID, files) {
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
if err := models.DeleteAttachment(issue.Attachments[i], true); err != nil {
ctx.ServerError("DeleteAttachment", err)
return
}
}
}
if len(files) > 0 {
if err := issue.UpdateAttachments(files); err != nil {
ctx.ServerError("UpdateAttachments", err)
return
}
}
var err error
issue.Attachments, err = models.GetAttachmentsByIssueID(issue.ID)
if err != nil {
ctx.ServerError("GetAttachmentsByIssueID", err)
return
}
attachHTML, err := ctx.HTMLString(string(tplAttachment), map[string]interface{}{
"ctx": ctx.Data,
"Attachments": issue.Attachments,
})
if err != nil {
ctx.ServerError("UpdateIssueContent.HTMLString", err)
return
}
ctx.JSON(200, map[string]interface{}{
"content": string(markdown.Render([]byte(issue.Content), ctx.Query("context"), ctx.Repo.Repository.ComposeMetas())),
"content": string(markdown.Render([]byte(issue.Content), ctx.Query("context"), ctx.Repo.Repository.ComposeMetas())),
"attachments": attachHTML,
})
}

Expand Down Expand Up @@ -1325,6 +1357,13 @@ func UpdateCommentContent(ctx *context.Context) {
return
}

if comment.Type == models.CommentTypeComment {
if err := comment.LoadAttachments(); err != nil {
ctx.ServerError("LoadAttachments", err)
return
}
}

if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
ctx.Error(403)
return
Expand All @@ -1346,10 +1385,41 @@ func UpdateCommentContent(ctx *context.Context) {
return
}

files := ctx.QueryStrings("files[]")
for i := 0; i < len(comment.Attachments); i++ {
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
if !util.IsStringInSlice(comment.Attachments[i].UUID, files) {
blueworrybear marked this conversation as resolved.
Show resolved Hide resolved
if err := models.DeleteAttachment(comment.Attachments[i], true); err != nil {
ctx.ServerError("DeleteAttachment", err)
return
}
}
}
if len(files) > 0 {
if err := comment.UpdateAttachments(files); err != nil {
ctx.ServerError("UpdateAttachments", err)
return
}
}

comment.Attachments, err = models.GetAttachmentsByCommentID(comment.ID)
if err != nil {
ctx.ServerError("GetAttachmentsByCommentID", err)
return
}
attachHTML, err := ctx.HTMLString(string(tplAttachment), map[string]interface{}{
"ctx": ctx.Data,
"Attachments": comment.Attachments,
})
if err != nil {
ctx.ServerError("UpdateCommentContent.HTMLString", err)
return
}

notification.NotifyUpdateComment(ctx.User, comment, oldContent)

ctx.JSON(200, map[string]interface{}{
"content": string(markdown.Render([]byte(comment.Content), ctx.Query("context"), ctx.Repo.Repository.ComposeMetas())),
"content": string(markdown.Render([]byte(comment.Content), ctx.Query("context"), ctx.Repo.Repository.ComposeMetas())),
"attachments": attachHTML,
})
}

Expand Down Expand Up @@ -1603,3 +1673,33 @@ func filterXRefComments(ctx *context.Context, issue *models.Issue) error {
}
return nil
}

// GetIssueAttachments returns attachments for the issue
func GetIssueAttachments(ctx *context.Context) {
issue := GetActionIssue(ctx)
var attachments = make([]*api.Attachment, len(issue.Attachments))
for i := 0; i < len(issue.Attachments); i++ {
attachments[i] = issue.Attachments[i].APIFormat()
}
ctx.JSON(200, attachments)
}

// GetCommentAttachments returns attachments for the comment
func GetCommentAttachments(ctx *context.Context) {
comment, err := models.GetCommentByID(ctx.ParamsInt64(":id"))
if err != nil {
ctx.NotFoundOrServerError("GetCommentByID", models.IsErrCommentNotExist, err)
return
}
var attachments = make([]*api.Attachment, 0)
if comment.Type == models.CommentTypeComment {
if err := comment.LoadAttachments(); err != nil {
ctx.ServerError("LoadAttachments", err)
return
}
for i := 0; i < len(comment.Attachments); i++ {
attachments = append(attachments, comment.Attachments[i].APIFormat())
}
}
ctx.JSON(200, attachments)
}
7 changes: 5 additions & 2 deletions routers/routes/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,8 +513,9 @@ func RegisterRoutes(m *macaron.Macaron) {
})
}, ignSignIn)

m.Group("", func() {
m.Post("/attachments", repo.UploadAttachment)
m.Group("/attachments", func() {
m.Post("", repo.UploadAttachment)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we get repo attachments for update and delete, but issue/comment attachments for get? I understand that there's a special case for issues/comments under creation, but attachments can also be modified on edit.

By the way, this is a great opportunity to validate user permissions when updating/deleting an attachment associated to an existing issue/comment that they don't have access to. The criteria should be the same as for edit/delete the issue/comment.

Perhaps this is a matter for another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right checking for deleting attachment should definitely be done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guillep2k
I use [issue/comment]/attachments because I could get attachments information without inject information into view_content.tmpl. Otherwise I have to inject attachments name, uuid and size during rendering view_content.tmpl for initializing dropzone when first time edit button click.

However, as you said it's okay to modify attachments on edit. Do you think I should modify my codes? If there is anything I misunderstand, please let me know 😃

@lafriks
I think I could try to enable user permissions checking, but I think it's better to open another pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guillep2k
I use [issue/comment]/attachments because I could get attachments information without inject information into view_content.tmpl. Otherwise I have to inject attachments name, uuid and size during rendering view_content.tmpl for initializing dropzone when first time edit button click.

However, as you said it's okay to modify attachments on edit. Do you think I should modify my codes? If there is anything I misunderstand, please let me know 😃

@lafriks
I think I could try to enable user permissions checking, but I think it's better to open another pull request.

I think that changing the API is not required per se, but it would be needed when access permissions were to be checked, so it can wait for the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guillep2k
Ok, I'll finish refactoring UpdateCommentContent and UpdateIssueContent. Then moving on checking access permissions. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hummm... I see the problem. I underestimate the effort to check attachment permission. No wonder Gitea does not actually delete file on server when removing it from dropzone.

So I guess the possible solution may be either:

  1. Not to delete attachment on server actually during editing for short term. Since UpdateIssueContent and UpdateCommentContent will check login user, there is less security concern IMO.
  2. Keep working on [WIP] Refactor attachement security #7956 to check attachment's uploader before deleting.

Correct me if I'm wrong. 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the same problem present in update as well? I think that only repo access is checked when you know the attachment GUID.

Copy link
Contributor Author

@blueworrybear blueworrybear Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me recap:

  1. My /attachments/delete will cause security issue because I only check whether user is login with:
m.Group("/attachments", func() {
  m.Post("", repo.UploadAttachment)
  m.Post("/delete", repo.DeleteAttachment)
}, reqSignIn)

The way to fix it is either:

  • Remove /attachments/delete route, attachments will not be deleted on server just as usual when creating new issue/comment
  • At least check Uploader ID in repo.DeleteAttachment such as 
    func DeleteAttachment(ctx *context.Context) {
  file := ctx.Query("file")
  attach, err := models.GetAttachmentByUUID(file)
  if attach.UploaderID != ctx.User.ID {
    ctx.Error(403)
  }
  ...
}
    But I see there is a comment in: 
    type Attachment struct {
  ID            int64  `xorm:"pk autoincr"`
  UUID          string `xorm:"uuid UNIQUE"`
  IssueID       int64  `xorm:"INDEX"`
  ReleaseID     int64  `xorm:"INDEX"`
  UploaderID    int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
  CommentID     int64
  Name          string
  DownloadCount int64              `xorm:"DEFAULT 0"`
  Size          int64              `xorm:"DEFAULT 0"`
  CreatedUnix   timeutil.TimeStamp `xorm:"created"`
}
    So maybe UploaderID is not available yet?

    After trying, it seems like the UploaderID is not correctly set yet (using sqlite)

  1. Issue.UpdateAttachments may also casue security issue(?)
    It seems to me that I use it only inside repo.UpdateIssueContent and
    user permission will be checked in the begin of repo.UpdateIssueContent. So
    maybe it's okay?
  2. Other attachments' security issue requires finishing [WIP] Refactor attachement security #7956 to solve

Correct me if I'm wrong. Thank you~

Copy link
Member

@guillep2k guillep2k Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct 😉
The same problems delete introduces are already present with update. What can happen is the following:

User bob creates an issue with attachments abcd and efgh. Alice is a little mad with bob (reasons unknown), so she decides to play him a joke. She's skilled in Javascript, so she opens the issue, starts a new comment and POST content into abcd. Now the attachment is modified. 😢. The same can happen with the new delete option, as it copies the previous behavior.

Back to your options: I think we should decide whether solving this is a matter of this PR or another. Considering that update was already affected, perhaps we can treat this as a different problem. I forgot about @sapk PR. It's reasonable to expect this to be resolved there.

@lafriks What do you think? @sapk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After trying, it seems like the UploaderID is not correctly set yet (using sqlite)

I found I'm wrong, UploaderID is correctly set (using sqlite). It turns out it's my codes in index.js is wrong.

$.post($dropzone.data('remove-url'), {
    file: file.uuid,
    _csrf: $dropzone.data('csrf')
});

I used file.uuid for POST request, but it will not be set. filenameDict[file.name].uuid should be used instead.

So I think checking UploaderID before delete attachment could be a short-term workaround.
I'll update my codes later. 😓

m.Post("/delete", repo.DeleteAttachment)
sapk marked this conversation as resolved.
Show resolved Hide resolved
}, reqSignIn)

m.Group("/:username", func() {
Expand Down Expand Up @@ -710,6 +711,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeIssueReaction)
m.Post("/lock", reqRepoIssueWriter, bindIgnErr(auth.IssueLockForm{}), repo.LockIssue)
m.Post("/unlock", reqRepoIssueWriter, repo.UnlockIssue)
m.Get("/attachments", repo.GetIssueAttachments)
}, context.RepoMustNotBeArchived())

m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel)
Expand All @@ -721,6 +723,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("", repo.UpdateCommentContent)
m.Post("/delete", repo.DeleteComment)
m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeCommentReaction)
m.Get("/attachments", repo.GetCommentAttachments)
}, context.RepoMustNotBeArchived())
m.Group("/labels", func() {
m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel)
Expand Down
Loading