Fix users with code unit access permission only can not access contributors page #32299
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/XS
github-actions
bot
added
modifies/go
pull-request-size
bot
added
size/S
yp05327
changed the title
yp05327
changed the title
routers/web/web.go
Outdated
| @@ -1446,17 +1446,17 @@ func registerRoutes(m *web.Router) { | |||
| m.Group("/contributors", func() { | |||
| m.Get("", repo.Contributors) | |||
| m.Get("/data", repo.ContributorsData) | |||
| }) | |||
| }, reqRepoCodeReader) | |||
There was a problem hiding this comment.
That's just questionable, why it needs to check this permission? Does the handler expose any "real code"?
There was a problem hiding this comment.
The original logic in the template needs code permission or it will display nothing.
So, I think it should have same permission checks in router, or when you removed them in the menu in UI, but you can still access these pages by just editing the URL.
And for the original logic, maybe it depends on the definition of Code: does author info include in it?
These information come from commits, and commits are Code.
There was a problem hiding this comment.
Actually I have no interest to touch the legacy logic at the moment because it is quite a mess (I did do some quick fixes just because these "bugs" affected end users and are potentially related to my PRs).
The real problem is that there is NO clear definition for the "permission" system, people just patched the code again and again, and there are more serious abuses like "issue reader / pull reader permissions are mixed (/{type:issues|pulls})".
Maybe you could define these permissions clearly first before code work. The definition should be reasonable to most end users and daily usage, but not by what it literally looks.
There was a problem hiding this comment.
IMO, anything contain the information from commits (no matter from original repo or forks) or themselves should be controlled by code unit permission, as codes are managed by git, and git uses commits to record the status/changes of these codes.
I'm not sure whether it is acceptable to others. What's your opinion?
There was a problem hiding this comment.
Why they need such strict rules?
Permissions should be as loose as possible to satisfy every users for various use cases, as long as the permission design doesn't cause harm or leak sensitive data.
There was a problem hiding this comment.
In my mind:
The purpose of a permission system is to "prevent from users doing harm to a repo".
codepermission means the user could access the full code. Disabling it means that prevent the user from accessing the full code. The purpose should be "protect the full code from leaking"pr readmeans that the user could discuss and review the code change (not the full code) , approve or reject.activitymeans that the user could see the statistics of the repo (commits number and authors do not really leak code)
So, that's why I would like to keep the commit/files tabs on the "pr" page, and would like to make users could read the statistics of the repo as much as possible (useful and no harm)
There was a problem hiding this comment.
Give me some time to think about it.
As we added contributor page in activity, the permission check isn't update correctly.
We should show activity unit when user only have code read permission, for the contributor page, but it is impossible now.
Case 1: code unit no access, pr unit has access
Before: (Empty page)
After:
Case 2: code unit has access, pr unit has no access
Before: You can not see
ActivitytabAfter: (redirect to contributors page)