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

Fix users with code unit access permission only can not access contributors page #32299

Closed
wants to merge 3 commits into from
Closed

Fix users with code unit access permission only can not access contributors page #32299

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0956e50
fix
yp05327 Oct 21, 2024
7486f27
improve
yp05327 Oct 21, 2024
8da7c4a
remove code read permission for contributors page
yp05327 Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions routers/web/repo/activity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ func Activity(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("repo.activity")
ctx.Data["PageIsActivity"] = true

if !ctx.Repo.CanReadAny(unit.TypePullRequests, unit.TypeIssues, unit.TypeReleases) {
ctx.Redirect(ctx.Repo.RepoLink + "/activity/contributors")
return
}

ctx.Data["PageIsPulse"] = true

ctx.Data["Period"] = ctx.PathParam("period")
Expand Down
8 changes: 4 additions & 4 deletions routers/web/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1446,17 +1446,17 @@ func registerRoutes(m *web.Router) {
m.Group("/contributors", func() {
m.Get("", repo.Contributors)
m.Get("/data", repo.ContributorsData)
})
}, reqRepoCodeReader)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's just questionable, why it needs to check this permission? Does the handler expose any "real code"?

Copy link
Contributor Author

@yp05327 yp05327 Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my mind:

The purpose of a permission system is to "prevent from users doing harm to a repo".

  • code permission 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 read means that the user could discuss and review the code change (not the full code) , approve or reject.
  • activity means 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give me some time to think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is removed.

m.Group("/code-frequency", func() {
m.Get("", repo.CodeFrequency)
m.Get("/data", repo.CodeFrequencyData)
})
}, reqRepoCodeReader)
m.Group("/recent-commits", func() {
m.Get("", repo.RecentCommits)
m.Get("/data", repo.RecentCommitsData)
})
}, reqRepoCodeReader)
},
ignSignIn, context.RepoAssignment, context.RequireRepoReaderOr(unit.TypePullRequests, unit.TypeIssues, unit.TypeReleases),
ignSignIn, context.RepoAssignment, context.RequireRepoReaderOr(unit.TypePullRequests, unit.TypeIssues, unit.TypeReleases, unit.TypeCode),
context.RepoRef(), repo.MustBeNotEmpty,
)
// end "/{username}/{reponame}/activity"
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/header.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
</a>
{{end}}

{{if and (.Permission.CanReadAny ctx.Consts.RepoUnitTypePullRequests ctx.Consts.RepoUnitTypeIssues ctx.Consts.RepoUnitTypeReleases) (not .IsEmptyRepo)}}
{{if and (.Permission.CanReadAny ctx.Consts.RepoUnitTypePullRequests ctx.Consts.RepoUnitTypeIssues ctx.Consts.RepoUnitTypeReleases ctx.Consts.RepoUnitTypeCode) (not .IsEmptyRepo)}}
<a class="{{if .PageIsActivity}}active {{end}}item" href="{{.RepoLink}}/activity">
{{svg "octicon-pulse"}} {{ctx.Locale.Tr "repo.activity"}}
</a>
Expand Down
4 changes: 4 additions & 0 deletions templates/repo/navbar.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
<div class="ui fluid vertical menu">
{{if (or (.Permission.CanRead ctx.Consts.RepoUnitTypeIssues) (.Permission.CanRead ctx.Consts.RepoUnitTypePullRequests))}}
<a class="{{if .PageIsPulse}}active {{end}}item" href="{{.RepoLink}}/activity">
{{ctx.Locale.Tr "repo.activity.navbar.pulse"}}
</a>
{{end}}
{{if .Permission.CanRead ctx.Consts.RepoUnitTypeCode}}
<a class="{{if .PageIsContributors}}active {{end}}item" href="{{.RepoLink}}/activity/contributors">
{{ctx.Locale.Tr "repo.activity.navbar.contributors"}}
</a>
Expand All @@ -11,4 +14,5 @@
<a class="{{if .PageIsRecentCommits}}active{{end}} item" href="{{.RepoLink}}/activity/recent-commits">
{{ctx.Locale.Tr "repo.activity.navbar.recent_commits"}}
</a>
{{end}}
</div>
Loading