Skip to content

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 30, 2019

fix #9543

@6543 6543 changed the title dont reqToken on GetReactions [API] dont reqToken on GetReactions Dec 30, 2019
@codecov-io
Copy link

codecov-io commented Dec 30, 2019

Codecov Report

Merging #9548 into master will increase coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9548      +/-   ##
==========================================
+ Coverage   42.15%   42.16%   +0.01%     
==========================================
  Files         577      577              
  Lines       75982    75982              
==========================================
+ Hits        32027    32039      +12     
+ Misses      39979    39969      -10     
+ Partials     3976     3974       -2
Impacted Files Coverage Δ
routers/api/v1/repo/issue_reaction.go 75.27% <0%> (ø) ⬆️
routers/api/v1/api.go 73.92% <100%> (ø) ⬆️
models/pull.go 46.54% <0%> (+0.48%) ⬆️
services/pull/check.go 55.63% <0%> (+2.11%) ⬆️
services/pull/temp_repo.go 34.18% <0%> (+2.56%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3c5b4b...33bf7dc. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 30, 2019
@zeripath
Copy link
Contributor

I think you need to take a look at:

if !ctx.Repo.CanRead(models.UnitTypeIssues) && !ctx.User.IsAdmin {

Looking at:

}, mustEnableIssuesOrPulls)

I don't think you need the check in issue_reaction.go - and if you're anonymous then I suspect that ctx.User.IsAdmin might cause a nil pointer.

@6543 6543 changed the title [API] dont reqToken on GetReactions [API] dont reqToken on GetReactions (fix #9543) Dec 30, 2019
@6543
Copy link
Member Author

6543 commented Dec 30, 2019

@zeripath done

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Dec 30, 2019
@lafriks lafriks added this to the 1.11.0 milestone Dec 30, 2019
@6543
Copy link
Member Author

6543 commented Dec 30, 2019

@lafriks does this https://github.com/go-gitea/gitea/pull/9548/files#diff-029cb1fefb1ae53a0ef72d3fd38a7f7dR16 resolfe your permission issue ?

-> I created a new file to make it reusable ...

@6543
Copy link
Member Author

6543 commented Jan 1, 2020

@zeripath !ctx.Repo.CanRead(models.UnitTypeIssues) && !ctx.User.IsAdmin works without login -> if issue is public, it works if not 404 is returned

@lunny
Copy link
Member

lunny commented Jan 2, 2020

I think we should keep !ctx.Repo.CanRead(models.UnitTypeIssues) and remove !ctx.User.IsAdmin, because the previous has included the next. And ctx.User may be nil.

@6543
Copy link
Member Author

6543 commented Jan 2, 2020

@lunny is this what you mean? fdf7965

@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 2, 2020
@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 2, 2020
@6543
Copy link
Member Author

6543 commented Jan 2, 2020

ready to merge 🚀

@lafriks lafriks merged commit 134e3fd into go-gitea:master Jan 2, 2020
@6543 6543 deleted the fix-9543 branch January 2, 2020 21:32
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. type/enhancement An improvement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow GET comments reactions without token via API

6 participants