fix permission check for creating comment while mail #22524

a1012112796 · 2023-01-19T08:09:53Z

only creating comment on locked issue request write permission,
for others, read permission is enough.

related to #22056

only creating comment on locked issue request write permission, for others, read permission is enough. Signed-off-by: a1012112796 <1012112796@qq.com>

wolfogre · 2023-01-19T08:25:43Z

services/mailer/incoming/incoming_handler.go

@@ -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 {


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.

But with write permission, it could post comment even if issue.IsLocked from UI experience?

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 ...

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

Suggested change

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

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

this way?

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 {

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…

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

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

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...

a1012112796 · 2023-01-19T09:01:39Z

@KN4CK3R wonder why get a error when shutdown (tested by outlook)

KN4CK3R · 2023-01-19T09:08:38Z

for others, read permission is enough

Is that correct? That does not sound good.

@KN4CK3R wonder why get a error when shutdown (tested by outlook)

Hm, I have never seen this message.

a1012112796 · 2023-01-19T10:28:01Z

for others, read permission is enough

Is that correct? That does not sound good.

that's same with web ui and api, looks github has same logic also(tested by: a1012112796/test_repo#2 (comment)).

KN4CK3R · 2023-01-19T15:58:46Z

If the "write check" does not do what I think it was doing, we need another check to see if the user has access to the repo. Otherwise an user could reply to an older mail belonging to a now private repo.

delvh · 2023-01-19T16:04:19Z

↑ That's exactly what I mean, it produces a lot of problems.

a1012112796 · 2023-01-21T07:20:04Z

read permission check has do it, isn't it? test result:

we need another check to see if the user has access to the repo.

Co-authored-by: delvh <dev.lh@web.de>

lunny · 2023-01-28T09:27:58Z

make LG-TM work

* giteaofficial/main: (42 commits) Link issue and pull requests status change in UI notifications directly to their event in the timelined view. (go-gitea#22627) fix permission check for creating comment while mail (go-gitea#22524) Fix error on account activation with wrong passwd (go-gitea#22609) Fixes accessibility of empty repository commit status (go-gitea#22632) Use `--index-url` in PyPi description (go-gitea#22620) Show migration validation error (go-gitea#22619) Allow issue templates to not render title (go-gitea#22589) Fix `delete_repo` in template (go-gitea#22606) set org visibility class to basic in header (go-gitea#22605) Add API endpoint to get latest release (go-gitea#21267) Add ARIA support for Fomantic UI checkboxes (go-gitea#22599) Webhooks: for issue close/reopen action, add commit ID that caused it (go-gitea#22583) Add templates to customize text when creating and migrating repositories Prevent duplicate labels when importing more than 99 (go-gitea#22591) Remove address from DCO (go-gitea#22595) Allow setting `redirect_to` cookie on OAuth login (go-gitea#22594) Project links should use parent link methods (go-gitea#22587) link update in README files (go-gitea#22582) Frontport 1.18.2 and 1.18.3 Changelogs (go-gitea#22580) Fix incorrect Redis URL snippets in the example app.ini (go-gitea#22573) ...

fix permission check for creating comment while mail

33affdc

only creating comment on locked issue request write permission, for others, read permission is enough. Signed-off-by: a1012112796 <1012112796@qq.com>

a1012112796 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 19, 2023

wolfogre reviewed Jan 19, 2023

View reviewed changes

GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 19, 2023

Update services/mailer/incoming/incoming_handler.go

a18311c

Co-authored-by: delvh <dev.lh@web.de>

wolfogre approved these changes Jan 27, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 27, 2023

lunny approved these changes Jan 27, 2023

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 27, 2023

lunny and others added 2 commits January 27, 2023 19:44

Merge branch 'main' into zzc/dev/fix_incomail_perm

ea43d38

Merge branch 'main' into zzc/dev/fix_incomail_perm

a1e50e3

lunny merged commit 48f5d51 into go-gitea:main Jan 28, 2023

a1012112796 deleted the zzc/dev/fix_incomail_perm branch January 29, 2023 02:53

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix permission check for creating comment while mail #22524

fix permission check for creating comment while mail #22524

a1012112796 commented Jan 19, 2023

wolfogre Jan 19, 2023 •

edited

Loading

lunny Jan 19, 2023

a1012112796 Jan 19, 2023 •

edited

Loading

wolfogre Jan 19, 2023

a1012112796 Jan 19, 2023 •

edited

Loading

delvh Jan 19, 2023

delvh Jan 19, 2023

lunny Jan 19, 2023

delvh Jan 19, 2023

a1012112796 Jan 21, 2023

a1012112796 commented Jan 19, 2023

KN4CK3R commented Jan 19, 2023

a1012112796 commented Jan 19, 2023 •

edited

Loading

KN4CK3R commented Jan 19, 2023

delvh commented Jan 19, 2023

a1012112796 commented Jan 21, 2023 •

edited

Loading

lunny commented Jan 28, 2023

	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 !perm.CanWriteIssuesOrPulls(issue.IsPull) && issue.IsLocked && !doer.IsAdmin {
	if issue.IsLocked && !perm.CanWriteIssuesOrPulls(issue.IsPull) && !doer.IsAdmin {

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

fix permission check for creating comment while mail #22524

fix permission check for creating comment while mail #22524

Conversation

a1012112796 commented Jan 19, 2023

wolfogre Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

lunny Jan 19, 2023

Choose a reason for hiding this comment

a1012112796 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

wolfogre Jan 19, 2023

Choose a reason for hiding this comment

a1012112796 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

delvh Jan 19, 2023

Choose a reason for hiding this comment

delvh Jan 19, 2023

Choose a reason for hiding this comment

lunny Jan 19, 2023

Choose a reason for hiding this comment

delvh Jan 19, 2023

Choose a reason for hiding this comment

a1012112796 Jan 21, 2023

Choose a reason for hiding this comment

a1012112796 commented Jan 19, 2023

KN4CK3R commented Jan 19, 2023

a1012112796 commented Jan 19, 2023 • edited Loading

KN4CK3R commented Jan 19, 2023

delvh commented Jan 19, 2023

a1012112796 commented Jan 21, 2023 • edited Loading

lunny commented Jan 28, 2023

wolfogre Jan 19, 2023 •

edited

Loading

a1012112796 Jan 19, 2023 •

edited

Loading

a1012112796 Jan 19, 2023 •

edited

Loading

a1012112796 commented Jan 19, 2023 •

edited

Loading

a1012112796 commented Jan 21, 2023 •

edited

Loading