Skip to content

Commit 5833ca6

Browse files
refactor: remove duplicate permission checks and use middleware
- Register API routes for org/repo actions permissions - Use reqOrgOwnership and reqAdmin middleware for auth - Remove manual usage of IsOwnedBy/IsAdmin in handlers to avoid duplication
1 parent 96c6b2d commit 5833ca6

File tree

3 files changed

+36
-58
lines changed

3 files changed

+36
-58
lines changed

routers/api/v1/api.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,6 +1270,11 @@ func Routes() *web.Router {
12701270
})
12711271
}, reqToken(), reqAdmin())
12721272
m.Group("/actions", func() {
1273+
m.Group("/permissions", func() {
1274+
m.Get("", reqAdmin(), repo.GetActionsPermissions)
1275+
m.Put("", reqAdmin(), repo.UpdateActionsPermissions)
1276+
}, reqToken())
1277+
12731278
m.Get("/tasks", repo.ListActionTasks)
12741279
m.Group("/runs", func() {
12751280
m.Group("/{run}", func() {
@@ -1618,6 +1623,18 @@ func Routes() *web.Router {
16181623
m.Post("/orgs", tokenRequiresScopes(auth_model.AccessTokenScopeCategoryOrganization), reqToken(), bind(api.CreateOrgOption{}), org.Create)
16191624
m.Get("/orgs", org.GetAll, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryOrganization))
16201625
m.Group("/orgs/{org}", func() {
1626+
m.Group("/settings/actions", func() {
1627+
m.Group("/permissions", func() {
1628+
m.Get("", reqOrgOwnership(), org.GetActionsPermissions)
1629+
m.Put("", reqOrgOwnership(), org.UpdateActionsPermissions)
1630+
})
1631+
m.Group("/cross-repo-access", func() {
1632+
m.Get("", reqOrgOwnership(), org.ListCrossRepoAccess)
1633+
m.Post("", reqOrgOwnership(), org.AddCrossRepoAccess)
1634+
m.Delete("/{id}", reqOrgOwnership(), org.DeleteCrossRepoAccess)
1635+
})
1636+
}, reqToken())
1637+
16211638
m.Combo("").Get(org.Get).
16221639
Patch(reqToken(), reqOrgOwnership(), bind(api.EditOrgOption{}), org.Edit).
16231640
Delete(reqToken(), reqOrgOwnership(), org.Delete)

routers/api/v1/org/org_actions_permissions.go

Lines changed: 13 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,10 @@ func GetActionsPermissions(ctx *context.APIContext) {
3434
// Organization settings are more sensitive than repo settings because they
3535
// affect ALL repositories in the org. We should be extra careful here.
3636
// Only org owners should be able to modify these settings.
37-
isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
38-
if err != nil {
39-
ctx.APIErrorInternal(err)
40-
return
41-
} else if !isOwner {
42-
ctx.APIError(http.StatusForbidden, "You must be an organization owner")
43-
return
44-
}
37+
// Organization settings are more sensitive than repo settings because they
38+
// affect ALL repositories in the org. We should be extra careful here.
39+
// Only org owners should be able to modify these settings.
40+
// This is enforced by the reqOrgOwnership middleware.
4541

4642
perms, err := actions_model.GetOrgActionPermissions(ctx, ctx.Org.Organization.ID)
4743
if err != nil {
@@ -90,14 +86,10 @@ func UpdateActionsPermissions(ctx *context.APIContext) {
9086
// "403":
9187
// "$ref": "#/responses/forbidden"
9288

93-
isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
94-
if err != nil {
95-
ctx.APIError(http.StatusInternalServerError, err)
96-
return
97-
} else if !isOwner {
98-
ctx.APIError(http.StatusForbidden, "Organization owner access required")
99-
return
100-
}
89+
// Organization settings are more sensitive than repo settings because they
90+
// affect ALL repositories in the org. We should be extra careful here.
91+
// Only org owners should be able to modify these settings.
92+
// This is enforced by the reqOrgOwnership middleware.
10193

10294
form := web.GetForm(ctx).(*api.OrgActionsPermissions)
10395

@@ -160,20 +152,13 @@ func ListCrossRepoAccess(ctx *context.APIContext) {
160152
// "200":
161153
// "$ref": "#/responses/CrossRepoAccessList"
162154

163-
isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
164-
if err != nil {
165-
ctx.APIErrorInternal(err)
166-
return
167-
}
168-
if !isOwner {
169-
ctx.APIError(http.StatusForbidden, "Organization owner access required")
170-
return
171-
}
172-
173155
// This is a critical security feature - cross-repo access allows one repo's
174156
// Actions to access another repo's code/resources. We need to be very careful
175157
// about how we implement this. See the discussion:
176158
// https://github.com/go-gitea/gitea/issues/24635
159+
// Permission check handled by reqOrgOwnership middleware
160+
161+
177162

178163
rules, err := actions_model.ListCrossRepoAccessRules(ctx, ctx.Org.Organization.ID)
179164
if err != nil {
@@ -214,15 +199,7 @@ func AddCrossRepoAccess(ctx *context.APIContext) {
214199
// "403":
215200
// "$ref": "#/responses/forbidden"
216201

217-
isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
218-
if err != nil {
219-
ctx.APIErrorInternal(err)
220-
return
221-
}
222-
if !isOwner {
223-
ctx.APIError(http.StatusForbidden, "Organization owner access required")
224-
return
225-
}
202+
// Permission check handled by reqOrgOwnership middleware
226203

227204
form := web.GetForm(ctx).(*api.CrossRepoAccessRule)
228205

@@ -274,16 +251,7 @@ func DeleteCrossRepoAccess(ctx *context.APIContext) {
274251
// "403":
275252
// "$ref": "#/responses/forbidden"
276253

277-
isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
278-
if err != nil {
279-
ctx.APIErrorInternal(err)
280-
return
281-
}
282-
if !isOwner {
283-
ctx.APIError(http.StatusForbidden, "Organization owner access required")
284-
return
285-
}
286-
254+
// Permission check handled by reqOrgOwnership middleware
287255
ruleID := ctx.PathParamInt64("id")
288256

289257
// Security check: Verify the rule belongs to this org before deleting

routers/api/v1/repo/repo_actions_permissions.go

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,8 @@ func GetActionsPermissions(ctx *context.APIContext) {
4040
// NOTE: Only repo admins should be able to view/modify permission settings
4141
// This is important for security - we don't want regular contributors
4242
// to be able to grant themselves elevated permissions via Actions
43-
if !ctx.Repo.IsAdmin() {
44-
ctx.APIError(http.StatusForbidden, "You must be a repository admin to access this")
45-
return
46-
}
43+
// Only repo admins and owners should be able to view/modify permission settings
44+
// This is enforced by the reqAdmin middleware.
4745

4846
perms, err := actions_model.GetRepoActionPermissions(ctx, ctx.Repo.Repository.ID)
4947
if err != nil {
@@ -98,11 +96,8 @@ func UpdateActionsPermissions(ctx *context.APIContext) {
9896
// "$ref": "#/responses/forbidden"
9997
// "422":
10098
// "$ref": "#/responses/validationError"
101-
102-
if !ctx.Repo.IsAdmin() {
103-
ctx.APIError(http.StatusForbidden, "You must be a repository admin to modify this")
104-
return
105-
}
99+
// Only repo admins and owners should be able to modify these settings.
100+
// This is enforced by the reqAdmin middleware.
106101

107102
form := web.GetForm(ctx).(*api.ActionsPermissions)
108103

@@ -166,10 +161,8 @@ func ResetActionsPermissions(ctx *context.APIContext) {
166161
// "403":
167162
// "$ref": "#/responses/forbidden"
168163

169-
if !ctx.Repo.IsAdmin() {
170-
ctx.APIError(http.StatusForbidden, "You must be a repository admin")
171-
return
172-
}
164+
// Only repo admins and owners should be able to modify these settings.
165+
// This is enforced by the reqAdmin middleware.
173166

174167
// Create default restricted permissions
175168
// This is a "safe reset" - puts the repo back to secure defaults

0 commit comments

Comments
 (0)