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

Jump to bottom

fix permission check for creating comment while mail #22524

Merged
merged 4 commits into from
Jan 28, 2023
+7 −1
Merged

fix permission check for creating comment while mail #22524

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
33affdc
fix permission check for creating comment while mail
a1012112796 Jan 19, 2023
a18311c
Update services/mailer/incoming/incoming_handler.go
a1012112796 Jan 21, 2023
ea43d38
Merge branch 'main' into zzc/dev/fix_incomail_perm
lunny Jan 27, 2023
a1e50e3
Merge branch 'main' into zzc/dev/fix_incomail_perm
a1012112796 Jan 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion services/mailer/incoming/incoming_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,16 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
return err
}

if !perm.CanWriteIssuesOrPulls(issue.IsPull) || issue.IsLocked && !doer.IsAdmin {
if !perm.CanWriteIssuesOrPulls(issue.IsPull) && issue.IsLocked && !doer.IsAdmin {
Copy link
Member

@wolfogre wolfogre Jan 19, 2023 •
edited

Choose a reason for hiding this comment

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

I think the old code tried to do something like:

  • Return nil if the doer cannot write
  • Even if the doer can write, return nil if the issue is locked and the doer is not admin.

So maybe we could just change it to:

if !perm.CanReadIssuesOrPulls(issue.IsPull) || issue.IsLocked && !doer.IsAdmin {

Edit:

So the old code is 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 with write permission, it could post comment even if issue.IsLocked from UI experience?

Copy link
Member Author

Choose a reason for hiding this comment

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

but in the api, user with write permission can comment on locked issue. ref:

gitea/routers/api/v1/repo/issue_comment.go

Lines 360 to 363 in 9f919cf

if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
ctx.Error(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked")))
return
}

if use your logic, they can't. I wonder ...
then use || and && but not use "(" is really hard to be understand for coder ...

Copy link
Member

Choose a reason for hiding this comment

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

I see, could you please change it to if issue.IsLocked && ... too? It does make it easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if !perm.CanWriteIssuesOrPulls(issue.IsPull) && issue.IsLocked && !doer.IsAdmin {
if issue.IsLocked && !perm.CanWriteIssuesOrPulls(issue.IsPull) && !doer.IsAdmin {

this way?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !perm.CanWriteIssuesOrPulls(issue.IsPull) && issue.IsLocked && !doer.IsAdmin {
// Locked issues require write permissions
if issue.IsLocked && !perm.CanWriteIssuesOrPulls(issue.IsPull) && !doer.IsAdmin {

Copy link
Member

Choose a reason for hiding this comment

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

Wait, is Read really enough to Write an issue comment?
I can see a lot of problems waiting for us, i.e. when we add private issues…

Copy link
Member

Choose a reason for hiding this comment

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

Yes. If you have read permission of issue, you can create issue and post comments.

Copy link
Member

Choose a reason for hiding this comment

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

As I said: That will certainly make implementing private issues (safely) a pain.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said: That will certainly make implementing private issues (safely) a pain.

I think has been diffcult now ... , when implementing private issues. it's better to use a unique servcie fuction to create commnt for both web ui, api and incoming mail...

log.Debug("can't write issue or pull")
return nil
}

if !perm.CanReadIssuesOrPulls(issue.IsPull) {
log.Debug("can't read issue or pull")
return nil
}

switch r := ref.(type) {
case *issues_model.Issue:
attachmentIDs := make([]string, 0, len(content.Attachments))
Expand Down