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.

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

fix permission check for creating comment while mail #22524

Merged
merged 4 commits into from
Jan 28, 2023

Conversation

a1012112796
Copy link
Member

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

related to #22056

/cc @KN4CK3R

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

Signed-off-by: a1012112796 <1012112796@qq.com>
@a1012112796 a1012112796 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 19, 2023
@@ -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

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

@a1012112796 a1012112796 Jan 19, 2023

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:

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

@a1012112796 a1012112796 Jan 19, 2023

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

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

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

@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 19, 2023

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) image

Hm, I have never seen this message.

@a1012112796
Copy link
Member Author

a1012112796 commented Jan 19, 2023

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
Copy link
Member

KN4CK3R commented Jan 19, 2023

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
Copy link
Member

delvh commented Jan 19, 2023

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

@a1012112796
Copy link
Member Author

a1012112796 commented Jan 21, 2023

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

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

Co-authored-by: delvh <dev.lh@web.de>
@GiteaBot 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
@GiteaBot 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
Copy link
Member

lunny commented Jan 28, 2023

make LG-TM work

@lunny lunny merged commit 48f5d51 into go-gitea:main Jan 28, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 28, 2023
* 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)
  ...
@a1012112796 a1012112796 deleted the zzc/dev/fix_incomail_perm branch January 29, 2023 02:53
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants