fix(app): let mr:write see a private repository for merge request writes
The agent preset (repo:write, mr:write) has no repo:read, so it could not open a merge request on the private repository it had just pushed. A token holding the required mr scope on the repository now sees it for that action; reads, and unknown repositories or tokens without the scope, are unchanged (not_found). The e2e test uses the real agent preset.
- commit
- f878cf15e203e0d0772f937bc2d09364ff0bb336
- author
- brice · · 2026-10-10 17:46 UTC
- parent
- 50c6964
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +83 -11
M cmd/ermite/cli_mr_e2e_test.go +2 -2
| @@ -80,7 +80,7 @@ func TestMergeRequestsCLIEndToEnd(t *testing.T) { | |||
| 80 | 80 | require.Equal(t, 0, code, stderr) | |
| 81 | 81 | secret := strings.TrimSpace(stdout) | |
| 82 | 82 | agent := strings.TrimSpace(func() string { | |
| 83 | - | out, errOut, code := c.bin(t, adminToken, "token", "create", "agent-7", "--preset", "agent", "--scope", "repo:read") | |
| 83 | + | out, errOut, code := c.bin(t, adminToken, "token", "create", "agent-7", "--preset", "agent") | |
| 84 | 84 | require.Equal(t, 0, code, errOut) | |
| 85 | 85 | return out | |
| 86 | 86 | }()) | |
| @@ -297,7 +297,7 @@ func TestMergeRequestsOfAPrivateRepositoryAreNotFound(t *testing.T) { | |||
| 297 | 297 | c.mustGit(wt, "push", "-q", c.urlAuth(adminToken, "/secret.git"), "main", "feat") | |
| 298 | 298 | c.cli(t, adminToken, "mr", "create", "secret", "--source", "feat", "--title", "t").ok(t) | |
| 299 | 299 | ||
| 300 | - | unscoped := c.token([]domain.Scope{domain.ScopeMRWrite, domain.ScopeMRMerge}) | |
| 300 | + | unscoped := c.token([]domain.Scope{domain.ScopeRepoWrite}) | |
| 301 | 301 | otherRepo := c.token([]domain.Scope{domain.ScopeRepoRead, domain.ScopeMRWrite}, "elsewhere") | |
| 302 | 302 | for who, secret := range map[string]string{"anonymous": "", "no repo:read": unscoped, "token of another repository": otherRepo} { | |
| 303 | 303 | for _, args := range [][]string{ | |
| @@ -80,7 +80,7 @@ func TestMergeRequestsCLIEndToEnd(t *testing.T) { | |||||
| 80 | require.Equal(t, 0, code, stderr) | 80 | require.Equal(t, 0, code, stderr) | ||
| 81 | secret := strings.TrimSpace(stdout) | 81 | secret := strings.TrimSpace(stdout) | ||
| 82 | agent := strings.TrimSpace(func() string { | 82 | agent := strings.TrimSpace(func() string { | ||
| 83 | - | out, errOut, code := c.bin(t, adminToken, "token", "create", "agent-7", "--preset", "agent", "--scope", "repo:read") | 83 | + | out, errOut, code := c.bin(t, adminToken, "token", "create", "agent-7", "--preset", "agent") |
| 84 | require.Equal(t, 0, code, errOut) | 84 | require.Equal(t, 0, code, errOut) | ||
| 85 | return out | 85 | return out | ||
| 86 | }()) | 86 | }()) | ||
| @@ -297,7 +297,7 @@ func TestMergeRequestsOfAPrivateRepositoryAreNotFound(t *testing.T) { | |||||
| 297 | c.mustGit(wt, "push", "-q", c.urlAuth(adminToken, "/secret.git"), "main", "feat") | 297 | c.mustGit(wt, "push", "-q", c.urlAuth(adminToken, "/secret.git"), "main", "feat") | ||
| 298 | c.cli(t, adminToken, "mr", "create", "secret", "--source", "feat", "--title", "t").ok(t) | 298 | c.cli(t, adminToken, "mr", "create", "secret", "--source", "feat", "--title", "t").ok(t) | ||
| 299 | 299 | ||||
| 300 | - | unscoped := c.token([]domain.Scope{domain.ScopeMRWrite, domain.ScopeMRMerge}) | 300 | + | unscoped := c.token([]domain.Scope{domain.ScopeRepoWrite}) |
| 301 | otherRepo := c.token([]domain.Scope{domain.ScopeRepoRead, domain.ScopeMRWrite}, "elsewhere") | 301 | otherRepo := c.token([]domain.Scope{domain.ScopeRepoRead, domain.ScopeMRWrite}, "elsewhere") | ||
| 302 | for who, secret := range map[string]string{"anonymous": "", "no repo:read": unscoped, "token of another repository": otherRepo} { | 302 | for who, secret := range map[string]string{"anonymous": "", "no repo:read": unscoped, "token of another repository": otherRepo} { | ||
| 303 | for _, args := range [][]string{ | 303 | for _, args := range [][]string{ | ||
M cmd/ermite/mr_api_test.go +14 -2
| @@ -26,7 +26,7 @@ type mrRig struct { | |||
| 26 | 26 | wt string | |
| 27 | 27 | head string | |
| 28 | 28 | anon, root, reader, writer *clients | |
| 29 | - | merger, noScope *clients | |
| 29 | + | merger, noScope, agent *clients | |
| 30 | 30 | } | |
| 31 | 31 | ||
| 32 | 32 | func newMRRig(t *testing.T, vis domain.Visibility) *mrRig { | |
| @@ -47,7 +47,9 @@ func newMRRig(t *testing.T, vis domain.Visibility) *mrRig { | |||
| 47 | 47 | r.reader = h.api(h.token(scopes(domain.ScopeRepoRead))) | |
| 48 | 48 | r.writer = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite))) | |
| 49 | 49 | r.merger = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge))) | |
| 50 | - | r.noScope = h.api(h.token(scopes(domain.ScopeMRWrite, domain.ScopeMRMerge))) | |
| 50 | + | // noScope holds neither repo:read nor an mr scope; agent is the agent preset. | |
| 51 | + | r.noScope = h.api(h.token(scopes(domain.ScopeRepoWrite))) | |
| 52 | + | r.agent = h.api(h.token(scopes(domain.ScopeRepoWrite, domain.ScopeMRWrite))) | |
| 51 | 53 | return r | |
| 52 | 54 | } | |
| 53 | 55 | ||
| @@ -125,6 +127,16 @@ func TestMergeRequestAccessOverTheAPI(t *testing.T) { | |||
| 125 | 127 | } | |
| 126 | 128 | } | |
| 127 | 129 | ||
| 130 | + | // GO-001: the agent preset has no repo:read, yet mr:write is enough to write | |
| 131 | + | // merge requests on the private repository; it still does not read them nor merge. | |
| 132 | + | assert.Equal(t, "invalid_argument", codeName(writes["CreateMergeRequest"](r.agent)), "create sees the repository: the branch is what is wrong") | |
| 133 | + | assert.Equal(t, "ok", codeName(writes["AddComment"](r.agent)), "AddComment agent") | |
| 134 | + | assert.Equal(t, "ok", codeName(writes["MarkRead"](r.agent)), "MarkRead agent") | |
| 135 | + | assert.Equal(t, "not_found", codeName(writes["MergeMergeRequest"](r.agent)), "mr:merge is not held: the repository stays hidden") | |
| 136 | + | for name, call := range reads { | |
| 137 | + | assert.Equal(t, "not_found", codeName(call(r.agent)), name+" agent: repo:write does not read") | |
| 138 | + | } | |
| 139 | + | ||
| 128 | 140 | // Public: anonymous reads, and writing asks for credentials. | |
| 129 | 141 | h2 := newMRRig(t, domain.VisibilityPublic) | |
| 130 | 142 | assert.Equal(t, "ok", codeName(reads["ListMergeRequests"](h2.anon)), "public list") | |
| @@ -26,7 +26,7 @@ type mrRig struct { | |||||
| 26 | wt string | 26 | wt string | ||
| 27 | head string | 27 | head string | ||
| 28 | anon, root, reader, writer *clients | 28 | anon, root, reader, writer *clients | ||
| 29 | - | merger, noScope *clients | 29 | + | merger, noScope, agent *clients |
| 30 | } | 30 | } | ||
| 31 | 31 | ||||
| 32 | func newMRRig(t *testing.T, vis domain.Visibility) *mrRig { | 32 | func newMRRig(t *testing.T, vis domain.Visibility) *mrRig { | ||
| @@ -47,7 +47,9 @@ func newMRRig(t *testing.T, vis domain.Visibility) *mrRig { | |||||
| 47 | r.reader = h.api(h.token(scopes(domain.ScopeRepoRead))) | 47 | r.reader = h.api(h.token(scopes(domain.ScopeRepoRead))) | ||
| 48 | r.writer = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite))) | 48 | r.writer = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite))) | ||
| 49 | r.merger = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge))) | 49 | r.merger = h.api(h.token(scopes(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge))) | ||
| 50 | - | r.noScope = h.api(h.token(scopes(domain.ScopeMRWrite, domain.ScopeMRMerge))) | 50 | + | // noScope holds neither repo:read nor an mr scope; agent is the agent preset. |
| 51 | + | r.noScope = h.api(h.token(scopes(domain.ScopeRepoWrite))) | |||
| 52 | + | r.agent = h.api(h.token(scopes(domain.ScopeRepoWrite, domain.ScopeMRWrite))) | |||
| 51 | return r | 53 | return r | ||
| 52 | } | 54 | } | ||
| 53 | 55 | ||||
| @@ -125,6 +127,16 @@ func TestMergeRequestAccessOverTheAPI(t *testing.T) { | |||||
| 125 | } | 127 | } | ||
| 126 | } | 128 | } | ||
| 127 | 129 | ||||
| 130 | + | // GO-001: the agent preset has no repo:read, yet mr:write is enough to write | |||
| 131 | + | // merge requests on the private repository; it still does not read them nor merge. | |||
| 132 | + | assert.Equal(t, "invalid_argument", codeName(writes["CreateMergeRequest"](r.agent)), "create sees the repository: the branch is what is wrong") | |||
| 133 | + | assert.Equal(t, "ok", codeName(writes["AddComment"](r.agent)), "AddComment agent") | |||
| 134 | + | assert.Equal(t, "ok", codeName(writes["MarkRead"](r.agent)), "MarkRead agent") | |||
| 135 | + | assert.Equal(t, "not_found", codeName(writes["MergeMergeRequest"](r.agent)), "mr:merge is not held: the repository stays hidden") | |||
| 136 | + | for name, call := range reads { | |||
| 137 | + | assert.Equal(t, "not_found", codeName(call(r.agent)), name+" agent: repo:write does not read") | |||
| 138 | + | } | |||
| 139 | + | ||||
| 128 | // Public: anonymous reads, and writing asks for credentials. | 140 | // Public: anonymous reads, and writing asks for credentials. | ||
| 129 | h2 := newMRRig(t, domain.VisibilityPublic) | 141 | h2 := newMRRig(t, domain.VisibilityPublic) | ||
| 130 | assert.Equal(t, "ok", codeName(reads["ListMergeRequests"](h2.anon)), "public list") | 142 | assert.Equal(t, "ok", codeName(reads["ListMergeRequests"](h2.anon)), "public list") | ||
M internal/app/mergerequests.go +16 -4
| @@ -31,7 +31,8 @@ const mrTokenPrefix = "mp1:" | |||
| 31 | 31 | // close and merge a branch. Reading needs the access of reading the | |
| 32 | 32 | // repository: a repository the principal may not read, a missing one and an | |
| 33 | 33 | // unknown number all answer domain.ErrNotFound. Writing needs mr:write, and | |
| 34 | - | // merging mr:merge, on the repository (see scopeAccess). | |
| 34 | + | // merging mr:merge, on the repository (see scopeAccess), and that scope also | |
| 35 | + | // makes a private repository visible for the action (see writable). | |
| 35 | 36 | type MergeRequests struct { | |
| 36 | 37 | meta ports.RepoMetaStore | |
| 37 | 38 | store ports.MergeRequestStore | |
| @@ -90,7 +91,9 @@ func (m *MergeRequests) lockRepo(ctx context.Context, name domain.RepoName) (unl | |||
| 90 | 91 | } | |
| 91 | 92 | } | |
| 92 | 93 | ||
| 93 | - | // readable looks the repository up and applies the read rule. | |
| 94 | + | // readable looks the repository up and applies the read rule: reading merge | |
| 95 | + | // requests needs the access of reading the repository (canRead). repo:write | |
| 96 | + | // alone does not read them, as it does not fetch a private repository either. | |
| 94 | 97 | func (m *MergeRequests) readable(ctx context.Context, p domain.Principal, name domain.RepoName) (domain.Repo, error) { | |
| 95 | 98 | repo, err := lookupRepo(ctx, m.meta, name) | |
| 96 | 99 | if err != nil { | |
| @@ -102,12 +105,21 @@ func (m *MergeRequests) readable(ctx context.Context, p domain.Principal, name d | |||
| 102 | 105 | return repo, nil | |
| 103 | 106 | } | |
| 104 | 107 | ||
| 105 | - | // writable is readable plus the scope. | |
| 108 | + | // writable looks the repository up for an action that needs a merge request | |
| 109 | + | // scope. A principal whose token holds that scope on the repository sees the | |
| 110 | + | // repository for this action even without repo:read, so that the agent preset | |
| 111 | + | // (repo:write, mr:write) can open a request on the private repository it just | |
| 112 | + | // pushed. A principal with neither repo:read nor the scope, and an unknown | |
| 113 | + | // repository, still get domain.ErrNotFound; one that can read the repository | |
| 114 | + | // but lacks the scope gets the error of scopeAccess. | |
| 106 | 115 | func (m *MergeRequests) writable(ctx context.Context, p domain.Principal, name domain.RepoName, scope domain.Scope) (domain.Repo, error) { | |
| 107 | - | repo, err := m.readable(ctx, p, name) | |
| 116 | + | repo, err := lookupRepo(ctx, m.meta, name) | |
| 108 | 117 | if err != nil { | |
| 109 | 118 | return domain.Repo{}, err | |
| 110 | 119 | } | |
| 120 | + | if !canRead(p, repo) && !p.Allows(scope, name) { | |
| 121 | + | return domain.Repo{}, domain.ErrNotFound | |
| 122 | + | } | |
| 111 | 123 | if err := scopeAccess(p, repo, scope); err != nil { | |
| 112 | 124 | return domain.Repo{}, err | |
| 113 | 125 | } | |
| @@ -31,7 +31,8 @@ const mrTokenPrefix = "mp1:" | |||||
| 31 | // close and merge a branch. Reading needs the access of reading the | 31 | // close and merge a branch. Reading needs the access of reading the | ||
| 32 | // repository: a repository the principal may not read, a missing one and an | 32 | // repository: a repository the principal may not read, a missing one and an | ||
| 33 | // unknown number all answer domain.ErrNotFound. Writing needs mr:write, and | 33 | // unknown number all answer domain.ErrNotFound. Writing needs mr:write, and | ||
| 34 | - | // merging mr:merge, on the repository (see scopeAccess). | 34 | + | // merging mr:merge, on the repository (see scopeAccess), and that scope also |
| 35 | + | // makes a private repository visible for the action (see writable). | |||
| 35 | type MergeRequests struct { | 36 | type MergeRequests struct { | ||
| 36 | meta ports.RepoMetaStore | 37 | meta ports.RepoMetaStore | ||
| 37 | store ports.MergeRequestStore | 38 | store ports.MergeRequestStore | ||
| @@ -90,7 +91,9 @@ func (m *MergeRequests) lockRepo(ctx context.Context, name domain.RepoName) (unl | |||||
| 90 | } | 91 | } | ||
| 91 | } | 92 | } | ||
| 92 | 93 | ||||
| 93 | - | // readable looks the repository up and applies the read rule. | 94 | + | // readable looks the repository up and applies the read rule: reading merge |
| 95 | + | // requests needs the access of reading the repository (canRead). repo:write | |||
| 96 | + | // alone does not read them, as it does not fetch a private repository either. | |||
| 94 | func (m *MergeRequests) readable(ctx context.Context, p domain.Principal, name domain.RepoName) (domain.Repo, error) { | 97 | func (m *MergeRequests) readable(ctx context.Context, p domain.Principal, name domain.RepoName) (domain.Repo, error) { | ||
| 95 | repo, err := lookupRepo(ctx, m.meta, name) | 98 | repo, err := lookupRepo(ctx, m.meta, name) | ||
| 96 | if err != nil { | 99 | if err != nil { | ||
| @@ -102,12 +105,21 @@ func (m *MergeRequests) readable(ctx context.Context, p domain.Principal, name d | |||||
| 102 | return repo, nil | 105 | return repo, nil | ||
| 103 | } | 106 | } | ||
| 104 | 107 | ||||
| 105 | - | // writable is readable plus the scope. | 108 | + | // writable looks the repository up for an action that needs a merge request |
| 109 | + | // scope. A principal whose token holds that scope on the repository sees the | |||
| 110 | + | // repository for this action even without repo:read, so that the agent preset | |||
| 111 | + | // (repo:write, mr:write) can open a request on the private repository it just | |||
| 112 | + | // pushed. A principal with neither repo:read nor the scope, and an unknown | |||
| 113 | + | // repository, still get domain.ErrNotFound; one that can read the repository | |||
| 114 | + | // but lacks the scope gets the error of scopeAccess. | |||
| 106 | func (m *MergeRequests) writable(ctx context.Context, p domain.Principal, name domain.RepoName, scope domain.Scope) (domain.Repo, error) { | 115 | func (m *MergeRequests) writable(ctx context.Context, p domain.Principal, name domain.RepoName, scope domain.Scope) (domain.Repo, error) { | ||
| 107 | - | repo, err := m.readable(ctx, p, name) | 116 | + | repo, err := lookupRepo(ctx, m.meta, name) |
| 108 | if err != nil { | 117 | if err != nil { | ||
| 109 | return domain.Repo{}, err | 118 | return domain.Repo{}, err | ||
| 110 | } | 119 | } | ||
| 120 | + | if !canRead(p, repo) && !p.Allows(scope, name) { | |||
| 121 | + | return domain.Repo{}, domain.ErrNotFound | |||
| 122 | + | } | |||
| 111 | if err := scopeAccess(p, repo, scope); err != nil { | 123 | if err := scopeAccess(p, repo, scope); err != nil { | ||
| 112 | return domain.Repo{}, err | 124 | return domain.Repo{}, err | ||
| 113 | } | 125 | } | ||
M internal/app/mergerequests_integration_test.go +51 -3
| @@ -194,6 +194,9 @@ func writer(scopes ...domain.Scope) domain.Principal { | |||
| 194 | 194 | } | |
| 195 | 195 | ||
| 196 | 196 | var ( | |
| 197 | + | // agent is the agent preset: it pushes and writes merge requests, and has | |
| 198 | + | // no repo:read. | |
| 199 | + | agent = writer(domain.ScopeRepoWrite, domain.ScopeMRWrite) | |
| 197 | 200 | reader = writer(domain.ScopeRepoRead) | |
| 198 | 201 | mrW = writer(domain.ScopeRepoRead, domain.ScopeMRWrite) | |
| 199 | 202 | mrM = writer(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge) | |
| @@ -318,7 +321,7 @@ func TestMergeRequestAccess(t *testing.T) { | |||
| 318 | 321 | }, | |
| 319 | 322 | } | |
| 320 | 323 | for what, call := range reads { | |
| 321 | - | for who, p := range map[string]domain.Principal{"anonymous": domain.Anonymous(), "no repo:read": noScope, "token of another repo": otherRepo} { | |
| 324 | + | for who, p := range map[string]domain.Principal{"anonymous": domain.Anonymous(), "no repo:read": noScope, "agent, no repo:read": agent, "token of another repo": otherRepo} { | |
| 322 | 325 | assert.ErrorIs(t, call(p), domain.ErrNotFound, "%s by %s on a private repository", what, who) | |
| 323 | 326 | } | |
| 324 | 327 | assert.NoError(t, call(reader), what+" with repo:read") | |
| @@ -361,14 +364,20 @@ func TestMergeRequestAccess(t *testing.T) { | |||
| 361 | 364 | scope = domain.ScopeMRMerge | |
| 362 | 365 | } | |
| 363 | 366 | assert.ErrorIs(t, call(domain.Anonymous()), domain.ErrNotFound, "%s: anonymous cannot see the private repository", what) | |
| 364 | - | assert.ErrorIs(t, call(noScope), domain.ErrNotFound, "%s: no repo:read", what) | |
| 367 | + | assert.ErrorIs(t, call(noScope), domain.ErrNotFound, "%s: no repo:read and not the scope", what) | |
| 365 | 368 | assert.ErrorIs(t, call(otherRepo), domain.ErrNotFound, "%s: another repository's token", what) | |
| 366 | 369 | assert.ErrorIs(t, call(reader), domain.ErrForbidden, "%s: reading is not writing", what) | |
| 367 | 370 | if scope == domain.ScopeMRMerge { | |
| 368 | 371 | assert.ErrorIs(t, call(mrW), domain.ErrForbidden, "mr:write does not merge") | |
| 372 | + | assert.ErrorIs(t, call(agent), domain.ErrNotFound, "no repo:read and not mr:merge: the repository stays hidden") | |
| 369 | 373 | continue | |
| 370 | 374 | } | |
| 371 | - | // With the scope the call gets past access (and fails on its subject). | |
| 375 | + | // With the scope the call gets past access (and fails on its subject), | |
| 376 | + | // with or without repo:read (GO-001; TestAgentPresetWorksOnItsPrivateRepository | |
| 377 | + | // shows the calls that succeed, as some of these fail with not found on | |
| 378 | + | // their subject). | |
| 379 | + | assert.NotErrorIs(t, call(agent), domain.ErrForbidden, what+": the agent preset") | |
| 380 | + | assert.NotErrorIs(t, call(agent), domain.ErrUnauthenticated, what+": the agent preset") | |
| 372 | 381 | assert.NotErrorIs(t, call(mrW), domain.ErrForbidden, what) | |
| 373 | 382 | assert.NotErrorIs(t, call(mrW), domain.ErrUnauthenticated, what) | |
| 374 | 383 | } | |
| @@ -1176,3 +1185,42 @@ func TestSigningKeyInfo(t *testing.T) { | |||
| 1176 | 1185 | assert.Equal(t, "ermite@git.example.org "+info.Key.AuthorizedKey, info.AllowedSigners()) | |
| 1177 | 1186 | assert.Equal(t, domain.Identity{Name: "ermite", Email: "ermite@git.example.org"}, info.Committer) | |
| 1178 | 1187 | } | |
| 1188 | + | ||
| 1189 | + | // GO-001: the agent preset (repo:write, mr:write) has no repo:read, yet it | |
| 1190 | + | // opens, comments on, resolves, marks read and closes merge requests on the | |
| 1191 | + | // private repository it pushed to. Reading them still needs repo:read. | |
| 1192 | + | func TestAgentPresetWorksOnItsPrivateRepository(t *testing.T) { | |
| 1193 | + | r := newMRRig(t) // proj is private | |
| 1194 | + | r.seed() | |
| 1195 | + | head := r.feature("feat") | |
| 1196 | + | ||
| 1197 | + | mr, err := r.mrs.Create(r.ctx, agent, r.repo, app.CreateMergeRequestParams{Title: "From the agent", Source: "feat"}) | |
| 1198 | + | require.NoError(t, err) | |
| 1199 | + | assert.Equal(t, head, mr.HeadID) | |
| 1200 | + | assert.Equal(t, "agent-1", mr.Author) | |
| 1201 | + | ||
| 1202 | + | c, err := r.mrs.AddComment(r.ctx, agent, r.repo, mr.Number, app.AddCommentParams{Text: "note"}) | |
| 1203 | + | require.NoError(t, err) | |
| 1204 | + | _, err = r.mrs.ResolveComment(r.ctx, agent, r.repo, mr.Number, c.ID, true) | |
| 1205 | + | require.NoError(t, err) | |
| 1206 | + | _, err = r.mrs.MarkRead(r.ctx, agent, r.repo, mr.Number, domain.ObjectID{}) | |
| 1207 | + | require.NoError(t, err) | |
| 1208 | + | ||
| 1209 | + | // repo:write alone does not read merge requests. | |
| 1210 | + | _, err = r.mrs.List(r.ctx, agent, r.repo, app.ListMergeRequestsParams{}) | |
| 1211 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 1212 | + | _, err = r.mrs.Get(r.ctx, agent, r.repo, mr.Number, false) | |
| 1213 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 1214 | + | ||
| 1215 | + | closed, err := r.mrs.Close(r.ctx, agent, r.repo, mr.Number) | |
| 1216 | + | require.NoError(t, err) | |
| 1217 | + | assert.Equal(t, domain.MRStateClosed, closed.State) | |
| 1218 | + | ||
| 1219 | + | // An unknown repository is not found whatever the scopes. | |
| 1220 | + | _, err = r.mrs.Create(r.ctx, agent, name(t, "ghost"), app.CreateMergeRequestParams{Title: "t", Source: "feat"}) | |
| 1221 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 1222 | + | // A token limited to other repositories does not get the scope here. | |
| 1223 | + | elsewhere := domain.TokenPrincipal(domain.Token{ID: "e", Scopes: []domain.Scope{domain.ScopeRepoWrite, domain.ScopeMRWrite}, Repos: []domain.RepoName{name(t, "else")}}) | |
| 1224 | + | _, err = r.mrs.Create(r.ctx, elsewhere, r.repo, app.CreateMergeRequestParams{Title: "t", Source: "feat"}) | |
| 1225 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 1226 | + | } | |
| @@ -194,6 +194,9 @@ func writer(scopes ...domain.Scope) domain.Principal { | |||||
| 194 | } | 194 | } | ||
| 195 | 195 | ||||
| 196 | var ( | 196 | var ( | ||
| 197 | + | // agent is the agent preset: it pushes and writes merge requests, and has | |||
| 198 | + | // no repo:read. | |||
| 199 | + | agent = writer(domain.ScopeRepoWrite, domain.ScopeMRWrite) | |||
| 197 | reader = writer(domain.ScopeRepoRead) | 200 | reader = writer(domain.ScopeRepoRead) | ||
| 198 | mrW = writer(domain.ScopeRepoRead, domain.ScopeMRWrite) | 201 | mrW = writer(domain.ScopeRepoRead, domain.ScopeMRWrite) | ||
| 199 | mrM = writer(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge) | 202 | mrM = writer(domain.ScopeRepoRead, domain.ScopeMRWrite, domain.ScopeMRMerge) | ||
| @@ -318,7 +321,7 @@ func TestMergeRequestAccess(t *testing.T) { | |||||
| 318 | }, | 321 | }, | ||
| 319 | } | 322 | } | ||
| 320 | for what, call := range reads { | 323 | for what, call := range reads { | ||
| 321 | - | for who, p := range map[string]domain.Principal{"anonymous": domain.Anonymous(), "no repo:read": noScope, "token of another repo": otherRepo} { | 324 | + | for who, p := range map[string]domain.Principal{"anonymous": domain.Anonymous(), "no repo:read": noScope, "agent, no repo:read": agent, "token of another repo": otherRepo} { |
| 322 | assert.ErrorIs(t, call(p), domain.ErrNotFound, "%s by %s on a private repository", what, who) | 325 | assert.ErrorIs(t, call(p), domain.ErrNotFound, "%s by %s on a private repository", what, who) | ||
| 323 | } | 326 | } | ||
| 324 | assert.NoError(t, call(reader), what+" with repo:read") | 327 | assert.NoError(t, call(reader), what+" with repo:read") | ||
| @@ -361,14 +364,20 @@ func TestMergeRequestAccess(t *testing.T) { | |||||
| 361 | scope = domain.ScopeMRMerge | 364 | scope = domain.ScopeMRMerge | ||
| 362 | } | 365 | } | ||
| 363 | assert.ErrorIs(t, call(domain.Anonymous()), domain.ErrNotFound, "%s: anonymous cannot see the private repository", what) | 366 | assert.ErrorIs(t, call(domain.Anonymous()), domain.ErrNotFound, "%s: anonymous cannot see the private repository", what) | ||
| 364 | - | assert.ErrorIs(t, call(noScope), domain.ErrNotFound, "%s: no repo:read", what) | 367 | + | assert.ErrorIs(t, call(noScope), domain.ErrNotFound, "%s: no repo:read and not the scope", what) |
| 365 | assert.ErrorIs(t, call(otherRepo), domain.ErrNotFound, "%s: another repository's token", what) | 368 | assert.ErrorIs(t, call(otherRepo), domain.ErrNotFound, "%s: another repository's token", what) | ||
| 366 | assert.ErrorIs(t, call(reader), domain.ErrForbidden, "%s: reading is not writing", what) | 369 | assert.ErrorIs(t, call(reader), domain.ErrForbidden, "%s: reading is not writing", what) | ||
| 367 | if scope == domain.ScopeMRMerge { | 370 | if scope == domain.ScopeMRMerge { | ||
| 368 | assert.ErrorIs(t, call(mrW), domain.ErrForbidden, "mr:write does not merge") | 371 | assert.ErrorIs(t, call(mrW), domain.ErrForbidden, "mr:write does not merge") | ||
| 372 | + | assert.ErrorIs(t, call(agent), domain.ErrNotFound, "no repo:read and not mr:merge: the repository stays hidden") | |||
| 369 | continue | 373 | continue | ||
| 370 | } | 374 | } | ||
| 371 | - | // With the scope the call gets past access (and fails on its subject). | 375 | + | // With the scope the call gets past access (and fails on its subject), |
| 376 | + | // with or without repo:read (GO-001; TestAgentPresetWorksOnItsPrivateRepository | |||
| 377 | + | // shows the calls that succeed, as some of these fail with not found on | |||
| 378 | + | // their subject). | |||
| 379 | + | assert.NotErrorIs(t, call(agent), domain.ErrForbidden, what+": the agent preset") | |||
| 380 | + | assert.NotErrorIs(t, call(agent), domain.ErrUnauthenticated, what+": the agent preset") | |||
| 372 | assert.NotErrorIs(t, call(mrW), domain.ErrForbidden, what) | 381 | assert.NotErrorIs(t, call(mrW), domain.ErrForbidden, what) | ||
| 373 | assert.NotErrorIs(t, call(mrW), domain.ErrUnauthenticated, what) | 382 | assert.NotErrorIs(t, call(mrW), domain.ErrUnauthenticated, what) | ||
| 374 | } | 383 | } | ||
| @@ -1176,3 +1185,42 @@ func TestSigningKeyInfo(t *testing.T) { | |||||
| 1176 | assert.Equal(t, "ermite@git.example.org "+info.Key.AuthorizedKey, info.AllowedSigners()) | 1185 | assert.Equal(t, "ermite@git.example.org "+info.Key.AuthorizedKey, info.AllowedSigners()) | ||
| 1177 | assert.Equal(t, domain.Identity{Name: "ermite", Email: "ermite@git.example.org"}, info.Committer) | 1186 | assert.Equal(t, domain.Identity{Name: "ermite", Email: "ermite@git.example.org"}, info.Committer) | ||
| 1178 | } | 1187 | } | ||
| 1188 | + | ||||
| 1189 | + | // GO-001: the agent preset (repo:write, mr:write) has no repo:read, yet it | |||
| 1190 | + | // opens, comments on, resolves, marks read and closes merge requests on the | |||
| 1191 | + | // private repository it pushed to. Reading them still needs repo:read. | |||
| 1192 | + | func TestAgentPresetWorksOnItsPrivateRepository(t *testing.T) { | |||
| 1193 | + | r := newMRRig(t) // proj is private | |||
| 1194 | + | r.seed() | |||
| 1195 | + | head := r.feature("feat") | |||
| 1196 | + | ||||
| 1197 | + | mr, err := r.mrs.Create(r.ctx, agent, r.repo, app.CreateMergeRequestParams{Title: "From the agent", Source: "feat"}) | |||
| 1198 | + | require.NoError(t, err) | |||
| 1199 | + | assert.Equal(t, head, mr.HeadID) | |||
| 1200 | + | assert.Equal(t, "agent-1", mr.Author) | |||
| 1201 | + | ||||
| 1202 | + | c, err := r.mrs.AddComment(r.ctx, agent, r.repo, mr.Number, app.AddCommentParams{Text: "note"}) | |||
| 1203 | + | require.NoError(t, err) | |||
| 1204 | + | _, err = r.mrs.ResolveComment(r.ctx, agent, r.repo, mr.Number, c.ID, true) | |||
| 1205 | + | require.NoError(t, err) | |||
| 1206 | + | _, err = r.mrs.MarkRead(r.ctx, agent, r.repo, mr.Number, domain.ObjectID{}) | |||
| 1207 | + | require.NoError(t, err) | |||
| 1208 | + | ||||
| 1209 | + | // repo:write alone does not read merge requests. | |||
| 1210 | + | _, err = r.mrs.List(r.ctx, agent, r.repo, app.ListMergeRequestsParams{}) | |||
| 1211 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 1212 | + | _, err = r.mrs.Get(r.ctx, agent, r.repo, mr.Number, false) | |||
| 1213 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 1214 | + | ||||
| 1215 | + | closed, err := r.mrs.Close(r.ctx, agent, r.repo, mr.Number) | |||
| 1216 | + | require.NoError(t, err) | |||
| 1217 | + | assert.Equal(t, domain.MRStateClosed, closed.State) | |||
| 1218 | + | ||||
| 1219 | + | // An unknown repository is not found whatever the scopes. | |||
| 1220 | + | _, err = r.mrs.Create(r.ctx, agent, name(t, "ghost"), app.CreateMergeRequestParams{Title: "t", Source: "feat"}) | |||
| 1221 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 1222 | + | // A token limited to other repositories does not get the scope here. | |||
| 1223 | + | elsewhere := domain.TokenPrincipal(domain.Token{ID: "e", Scopes: []domain.Scope{domain.ScopeRepoWrite, domain.ScopeMRWrite}, Repos: []domain.RepoName{name(t, "else")}}) | |||
| 1224 | + | _, err = r.mrs.Create(r.ctx, elsewhere, r.repo, app.CreateMergeRequestParams{Title: "t", Source: "feat"}) | |||
| 1225 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 1226 | + | } | |||