Skip to content

Commit 0200bf6

Browse files
rossigeeClaude Sonnet
andcommitted
Fix review issues: auth on log routes, LoadRepos removal, test coverage
- Add reqToken/reqRepoReader to GET log download endpoints for consistency with the POST streaming endpoint - Remove spurious LoadRepos call in DownloadActionsRunAllJobLogs; jobs are already scoped to the repo by the query and Repo is never read - Refactor reader.Close() in zip loop to use a closure with defer - Update copyright year to 2026 on new services/actions/{cancel,log}.go - Add TestAPIActionsListUserWorkflows and TestAPIActionsListRepoWorkflows as standalone top-level tests (were dropped when breaking up the orchestrator) - Add idempotency assertion to TestAPIActionsApproveWorkflowRun: approving an already-approved run returns 400 Co-Authored-By: Claude Sonnet <claude-sonnet-4-6@anthropic.com>
1 parent cefecc5 commit 0200bf6

5 files changed

Lines changed: 91 additions & 23 deletions

File tree

routers/api/v1/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,11 +1265,11 @@ func Routes() *web.Router {
12651265
m.Post("/approve", reqToken(), reqRepoWriter(unit.TypeActions), repo.ApproveWorkflowRun)
12661266
m.Group("/jobs", func() {
12671267
m.Get("", repo.ListWorkflowRunJobs)
1268-
m.Get("/{job_id}/logs", repo.GetWorkflowJobLogs)
1268+
m.Get("/{job_id}/logs", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowJobLogs)
12691269
m.Post("/{job_id}/rerun", reqToken(), reqRepoWriter(unit.TypeActions), repo.RerunWorkflowJob)
12701270
})
12711271
m.Group("/logs", func() {
1272-
m.Get("", repo.GetWorkflowRunLogs)
1272+
m.Get("", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowRunLogs)
12731273
m.Post("", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowRunLogsStream)
12741274
})
12751275
m.Get("/artifacts", repo.GetArtifactsOfRun)

routers/common/actions.go

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,6 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit
3535
if err != nil {
3636
return fmt.Errorf("GetLatestAttemptJobsByRepoAndRunID: %w", err)
3737
}
38-
if err = runJobs.LoadRepos(ctx); err != nil {
39-
return fmt.Errorf("LoadRepos: %w", err)
40-
}
4138

4239
if len(runJobs) == 0 {
4340
return util.NewNotExistErrorf("no jobs found for run %d", runID)
@@ -77,27 +74,27 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit
7774
continue
7875
}
7976

80-
reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename)
81-
if err != nil {
82-
return fmt.Errorf("OpenLogs for job %d: %w", job.ID, err)
83-
}
84-
8577
// Create file in zip with job name and task ID; sanitize to prevent Zip Slip
8678
safeJobName := strings.NewReplacer("/", "-", `\`, "-", "..", "__").Replace(job.Name)
8779
fileName := fmt.Sprintf("%s-%s-%d.log", safeWorkflowName, safeJobName, task.ID)
88-
zipFile, err := zipWriter.Create(fileName)
89-
if err != nil {
90-
reader.Close()
91-
return fmt.Errorf("Create zip file %s: %w", fileName, err)
92-
}
9380

94-
// Copy log content to zip file
95-
if _, err := io.Copy(zipFile, reader); err != nil {
96-
reader.Close()
97-
return fmt.Errorf("Copy logs for job %d: %w", job.ID, err)
81+
if err := func() error {
82+
reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename)
83+
if err != nil {
84+
return err
85+
}
86+
defer reader.Close()
87+
88+
zipFile, err := zipWriter.Create(fileName)
89+
if err != nil {
90+
return err
91+
}
92+
93+
_, err = io.Copy(zipFile, reader)
94+
return err
95+
}(); err != nil {
96+
return fmt.Errorf("job %d: %w", job.ID, err)
9897
}
99-
100-
reader.Close()
10198
}
10299

103100
return nil

services/actions/cancel.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2025 The Gitea Authors. All rights reserved.
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
22
// SPDX-License-Identifier: MIT
33

44
package actions

services/actions/log.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2025 The Gitea Authors. All rights reserved.
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
22
// SPDX-License-Identifier: MIT
33

44
package actions

tests/integration/api_actions_run_test.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,11 @@ jobs:
358358
assert.Equal(t, actions_model.StatusWaiting, job.Status)
359359
}
360360

361+
// Test approve already approved run (idempotency)
362+
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)).
363+
AddTokenAuth(user2Token)
364+
MakeRequest(t, req, http.StatusBadRequest)
365+
361366
// Test approve non-existent run
362367
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/999999/approve", baseRepo.FullName())).
363368
AddTokenAuth(user2Token)
@@ -436,6 +441,72 @@ func TestAPIActionsRerunWorkflowJob(t *testing.T) {
436441
})
437442
}
438443

444+
func TestAPIActionsListUserWorkflows(t *testing.T) {
445+
defer prepareTestEnvActionsArtifacts(t)()
446+
447+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
448+
session := loginUser(t, user.Name)
449+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadUser)
450+
451+
t.Run("Runs", func(t *testing.T) {
452+
req := NewRequest(t, "GET", "/api/v1/user/actions/runs").AddTokenAuth(token)
453+
resp := MakeRequest(t, req, http.StatusOK)
454+
var runs api.ActionWorkflowRunsResponse
455+
err := json.Unmarshal(resp.Body.Bytes(), &runs)
456+
require.NoError(t, err)
457+
458+
assert.Positive(t, runs.TotalCount)
459+
assert.NotEmpty(t, runs.Entries)
460+
461+
for _, run := range runs.Entries {
462+
assert.NotEmpty(t, run.DisplayTitle, "display_title should be populated")
463+
assert.NotNil(t, run.Repository, "repository should be populated via batch loading")
464+
assert.NotEmpty(t, run.Repository.FullName, "repository full_name should be populated")
465+
assert.NotNil(t, run.TriggerActor, "trigger_actor should be populated via batch loading")
466+
}
467+
})
468+
469+
t.Run("Jobs", func(t *testing.T) {
470+
req := NewRequest(t, "GET", "/api/v1/user/actions/jobs").AddTokenAuth(token)
471+
resp := MakeRequest(t, req, http.StatusOK)
472+
var jobs api.ActionWorkflowJobsResponse
473+
err := json.Unmarshal(resp.Body.Bytes(), &jobs)
474+
require.NoError(t, err)
475+
476+
assert.Positive(t, jobs.TotalCount)
477+
assert.NotEmpty(t, jobs.Entries)
478+
479+
for _, job := range jobs.Entries {
480+
assert.NotEmpty(t, job.Name, "job name should be populated")
481+
assert.NotEmpty(t, job.HTMLURL, "html_url should be populated via batch-loaded repo")
482+
}
483+
})
484+
}
485+
486+
func TestAPIActionsListRepoWorkflows(t *testing.T) {
487+
defer prepareTestEnvActionsArtifacts(t)()
488+
489+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
490+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
491+
session := loginUser(t, user.Name)
492+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
493+
494+
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs", repo.FullName())).AddTokenAuth(token)
495+
resp := MakeRequest(t, req, http.StatusOK)
496+
var runs api.ActionWorkflowRunsResponse
497+
err := json.Unmarshal(resp.Body.Bytes(), &runs)
498+
require.NoError(t, err)
499+
500+
assert.Positive(t, runs.TotalCount)
501+
assert.NotEmpty(t, runs.Entries)
502+
503+
for _, run := range runs.Entries {
504+
assert.NotNil(t, run.Repository, "repository should be populated from ctx.Repo")
505+
assert.Equal(t, repo.FullName(), run.Repository.FullName, "repository full_name should match")
506+
assert.NotNil(t, run.TriggerActor, "trigger_actor should be populated")
507+
}
508+
}
509+
439510
func TestAPIActionsGetWorkflowRunLogs(t *testing.T) {
440511
defer prepareTestEnvActionsArtifacts(t)()
441512

0 commit comments

Comments
 (0)