feat(kvstore): grant mr:review and issue:write to full tokens in the schema 4 upgrade

commit: 7e0e489acb1f3da7533a4e2363fedef07d741466
author: brice · 3d ago · 2026-10-19 12:47 UTC
parent: fa37646
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +51 -12

M internal/adapters/kvstore/issues_test.go +31 -2


@@ -4,6 +4,7 @@ package kvstore_test
4	4
5	5		import (
6	6		"context"
	7	+	"slices"
7	8		"sort"
8	9		"strconv"
9	10		"sync"

@@ -266,8 +267,9 @@ func TestRepoSettingsKeepTheIssuesMode(t *testing.T) {
266	267		assert.ErrorIs(t, s.Update(ctx, got), domain.ErrInvalid)
267	268		}
268	269
269		-	// TestSchema4UpgradesTheTokensWithAllScopes: the tokens that held every scope
270		-	// before issue:write existed keep their owner rights; no other token changes.
	270	+	// TestSchema4UpgradesTheTokensWithAllScopes: the tokens that held every
	271	+	// original scope keep their owner rights, gaining mr:review and issue:write
	272	+	// (each only if missing); no other token changes.
271	273		func TestSchema4UpgradesTheTokensWithAllScopes(t *testing.T) {
272	274		ctx := context.Background()
273	275		dir := t.TempDir()

@@ -279,6 +281,8 @@ func TestSchema4UpgradesTheTokensWithAllScopes(t *testing.T) {
279	281		}
280	282		tokens := map[string]*storagepb.Token{
281	283		"full": {Id: "full", Scopes: all9},
	284	+	"full10": {Id: "full10", Scopes: append(slices.Clone(all9), "mr:review")},
	285	+	"reposmr": {Id: "reposmr", Scopes: append(slices.Clone(all9), "mr:review"), Repos: []string{"proj"}},
282	286		"missing": {Id: "missing", Scopes: all9[:8]},
283	287		"repos": {Id: "repos", Scopes: all9, Repos: []string{"proj"}},
284	288		"prefix": {Id: "prefix", Scopes: all9, BranchPrefixes: []string{"agent/"}},

@@ -302,10 +306,22 @@ func TestSchema4UpgradesTheTokensWithAllScopes(t *testing.T) {
302	306
303	307		full := get("full")
304	308		assert.Contains(t, full.Scopes, domain.ScopeIssueWrite)
	309	+	assert.Contains(t, full.Scopes, domain.ScopeMRReview)
	310	+	assert.Len(t, full.Scopes, 11)
	311	+	full10 := get("full10")
	312	+	assert.Len(t, full10.Scopes, 11)
	313	+	assert.Equal(t, 1, countScope(full10.Scopes, domain.ScopeMRReview))
	314	+	assert.Contains(t, full10.Scopes, domain.ScopeIssueWrite)
	315	+	assert.True(t, domain.TokenPrincipal(full10).HasOwnerRights())
	316	+	assert.Len(t, get("reposmr").Scopes, 10)
	317	+	assert.NotContains(t, get("reposmr").Scopes, domain.ScopeIssueWrite)
305	318		assert.True(t, domain.TokenPrincipal(full).HasOwnerRights())
306	319		assert.True(t, domain.TokenPrincipal(get("otherfull")).HasOwnerRights(), "the order of the scopes does not matter")
307	320
308	321		assert.NotContains(t, get("missing").Scopes, domain.ScopeIssueWrite)
	322	+	assert.NotContains(t, get("missing").Scopes, domain.ScopeMRReview)
	323	+	assert.NotContains(t, get("repos").Scopes, domain.ScopeMRReview)
	324	+	assert.NotContains(t, get("prefix").Scopes, domain.ScopeMRReview)
309	325		assert.False(t, domain.TokenPrincipal(get("missing")).HasOwnerRights())
310	326		assert.NotContains(t, get("repos").Scopes, domain.ScopeIssueWrite)
311	327		assert.NotContains(t, get("prefix").Scopes, domain.ScopeIssueWrite)

@@ -325,6 +341,9 @@ func TestSchema4UpgradesTheTokensWithAllScopes(t *testing.T) {
325	341		tok, err := s2.Tokens().Get(ctx, "full")
326	342		require.NoError(t, err)
327	343		assert.Equal(t, full.Scopes, tok.Scopes)
	344	+	tok10, err := s2.Tokens().Get(ctx, "full10")
	345	+	require.NoError(t, err)
	346	+	assert.Equal(t, full10.Scopes, tok10.Scopes)
328	347		require.NoError(t, s2.Close(ctx))
329	348		}
330	349

@@ -530,3 +549,13 @@ func TestStoredPendingIssueStateIsCorrupt(t *testing.T) {
530	549		_, err = is.List(ctx, n, ports.ListIssuesParams{State: domain.IssueStateOpen})
531	550		assert.ErrorContains(t, err, "pending")
532	551		}
	552	+
	553	+	func countScope(scopes []domain.Scope, want domain.Scope) int {
	554	+	n := 0
	555	+	for _, s := range scopes {
	556	+	if s == want {
	557	+	n++
	558	+	}
	559	+	}
	560	+	return n
	561	+	}

 import (
 	"context"
+	"slices"
 	"sort"
 	"strconv"
 	"sync"
 	assert.ErrorIs(t, s.Update(ctx, got), domain.ErrInvalid)
 }
-// TestSchema4UpgradesTheTokensWithAllScopes: the tokens that held every scope
+// TestSchema4UpgradesTheTokensWithAllScopes: the tokens that held every
-// before issue:write existed keep their owner rights; no other token changes.
+// original scope keep their owner rights, gaining mr:review and issue:write
+// (each only if missing); no other token changes.
 func TestSchema4UpgradesTheTokensWithAllScopes(t *testing.T) {
 	ctx := context.Background()
 	dir := t.TempDir()
 	}
 	tokens := map[string]*storagepb.Token{
 		"full":      {Id: "full", Scopes: all9},
+		"full10":    {Id: "full10", Scopes: append(slices.Clone(all9), "mr:review")},
+		"reposmr":   {Id: "reposmr", Scopes: append(slices.Clone(all9), "mr:review"), Repos: []string{"proj"}},
 		"missing":   {Id: "missing", Scopes: all9[:8]},
 		"repos":     {Id: "repos", Scopes: all9, Repos: []string{"proj"}},
 		"prefix":    {Id: "prefix", Scopes: all9, BranchPrefixes: []string{"agent/"}},
 	full := get("full")
 	assert.Contains(t, full.Scopes, domain.ScopeIssueWrite)
+	assert.Contains(t, full.Scopes, domain.ScopeMRReview)
+	assert.Len(t, full.Scopes, 11)
+	full10 := get("full10")
+	assert.Len(t, full10.Scopes, 11)
+	assert.Equal(t, 1, countScope(full10.Scopes, domain.ScopeMRReview))
+	assert.Contains(t, full10.Scopes, domain.ScopeIssueWrite)
+	assert.True(t, domain.TokenPrincipal(full10).HasOwnerRights())
+	assert.Len(t, get("reposmr").Scopes, 10)
+	assert.NotContains(t, get("reposmr").Scopes, domain.ScopeIssueWrite)
 	assert.True(t, domain.TokenPrincipal(full).HasOwnerRights())
 	assert.True(t, domain.TokenPrincipal(get("otherfull")).HasOwnerRights(), "the order of the scopes does not matter")
 	assert.NotContains(t, get("missing").Scopes, domain.ScopeIssueWrite)
+	assert.NotContains(t, get("missing").Scopes, domain.ScopeMRReview)
+	assert.NotContains(t, get("repos").Scopes, domain.ScopeMRReview)
+	assert.NotContains(t, get("prefix").Scopes, domain.ScopeMRReview)
 	assert.False(t, domain.TokenPrincipal(get("missing")).HasOwnerRights())
 	assert.NotContains(t, get("repos").Scopes, domain.ScopeIssueWrite)
 	assert.NotContains(t, get("prefix").Scopes, domain.ScopeIssueWrite)
 	tok, err := s2.Tokens().Get(ctx, "full")
 	require.NoError(t, err)
 	assert.Equal(t, full.Scopes, tok.Scopes)
+	tok10, err := s2.Tokens().Get(ctx, "full10")
+	require.NoError(t, err)
+	assert.Equal(t, full10.Scopes, tok10.Scopes)
 	require.NoError(t, s2.Close(ctx))
 }
 	_, err = is.List(ctx, n, ports.ListIssuesParams{State: domain.IssueStateOpen})
 	assert.ErrorContains(t, err, "pending")
 }
+func countScope(scopes []domain.Scope, want domain.Scope) int {
+	n := 0
+	for _, s := range scopes {
+		if s == want {
+			n++
+		}
+	}
+	return n
+}

M internal/adapters/kvstore/store.go +20 -10


@@ -36,9 +36,10 @@ import (
36	36		// scope, so it must refuse the data. It also adds the pending issues of the
37	37		// public form (ispend/<repo>/<id>, removed with their repository), the bans of
38	38		// their submitters (isban/<hex hash>) and the form's server key
39		-	// (meta/issuekey). The upgrade also adds issue:write to the
40		-	// stored tokens that held all nine earlier scopes with no repository list and
41		-	// no branch prefix, so that they keep the owner rights (see upgradeTokens).
	39	+	// (meta/issuekey). The upgrade also adds mr:review and issue:write (each only
	40	+	// if missing) to the stored tokens that held all nine original scopes with no
	41	+	// repository list and no branch prefix, so that they keep the owner rights (see
	42	+	// upgradeTokens).
42	43		const SchemaVersion = 4
43	44
44	45		const schemaKey = "meta/schema"

@@ -171,7 +172,7 @@ func mustMarshal(m proto.Message) []byte {
171	172		return b
172	173		}
173	174
174		-	// preIssueScopes are the nine scopes that existed before schema 4.
	175	+	// preIssueScopes are the nine original scopes, before mr:review and issue:write.
175	176		var preIssueScopes = []domain.Scope{
176	177		domain.ScopeRepoRead, domain.ScopeRepoWrite, domain.ScopeRepoAdmin,
177	178		domain.ScopeMRWrite, domain.ScopeMRMerge,

@@ -181,9 +182,10 @@ var preIssueScopes = []domain.Scope{
181	182
182	183		// upgradeTokens adds to b the schema 4 upgrade of the tokens: a token that
183	184		// holds all of preIssueScopes, limited to no repository and no branch prefix,
184		-	// had the owner rights, which AllScopes now needs issue:write for, so it gets
185		-	// the scope (this is no escalation). Every other token is left as it is. The
186		-	// batch requires each token to be at the version it was read at.
	185	+	// had the owner rights, which AllScopes now needs mr:review and issue:write
	186	+	// for, so it gets each of them if missing (this is no escalation). Every other
	187	+	// token is left as it is. The batch requires each token to be at the version it
	188	+	// was read at.
187	189		//
188	190		//hexcheck:ignore suspicious-business-logic-in-adapter one-time storage upgrade of the stored tokens: a storage concern, no policy
189	191		func (s Store) upgradeTokens(ctx context.Context, b kv.Batch) error {

@@ -192,8 +194,7 @@ func (s Store) upgradeTokens(ctx context.Context, b kv.Batch) error {
192	194		if err := proto.Unmarshal(item.Value, pb); err != nil {
193	195		return fmt.Errorf("upgrade token %s: %w", key, err)
194	196		}
195		-	if len(pb.GetRepos()) > 0 \|\| len(pb.GetBranchPrefixes()) > 0 \|\|
196		-	slices.Contains(pb.GetScopes(), domain.ScopeIssueWrite.String()) {
	197	+	if len(pb.GetRepos()) > 0 \|\| len(pb.GetBranchPrefixes()) > 0 {
197	198		continue
198	199		}
199	200		full := true

@@ -206,7 +207,16 @@ func (s Store) upgradeTokens(ctx context.Context, b kv.Batch) error {
206	207		if !full {
207	208		continue
208	209		}
209		-	pb.Scopes = append(pb.Scopes, domain.ScopeIssueWrite.String())
	210	+	added := false
	211	+	for _, sc := range []domain.Scope{domain.ScopeMRReview, domain.ScopeIssueWrite} {
	212	+	if !slices.Contains(pb.GetScopes(), sc.String()) {
	213	+	pb.Scopes = append(pb.Scopes, sc.String())
	214	+	added = true
	215	+	}
	216	+	}
	217	+	if !added {
	218	+	continue
	219	+	}
210	220		b.RequireVersion(slices.Clone(key), item.Version)
211	221		b.Put(slices.Clone(key), mustMarshal(pb))
212	222		}

 // scope, so it must refuse the data. It also adds the pending issues of the
 // public form (ispend/<repo>/<id>, removed with their repository), the bans of
 // their submitters (isban/<hex hash>) and the form's server key
-// (meta/issuekey). The upgrade also adds issue:write to the
+// (meta/issuekey). The upgrade also adds mr:review and issue:write (each only
-// stored tokens that held all nine earlier scopes with no repository list and
+// if missing) to the stored tokens that held all nine original scopes with no
-// no branch prefix, so that they keep the owner rights (see upgradeTokens).
+// repository list and no branch prefix, so that they keep the owner rights (see
+// upgradeTokens).
 const SchemaVersion = 4
 const schemaKey = "meta/schema"
 	return b
 }
-// preIssueScopes are the nine scopes that existed before schema 4.
+// preIssueScopes are the nine original scopes, before mr:review and issue:write.
 var preIssueScopes = []domain.Scope{
 	domain.ScopeRepoRead, domain.ScopeRepoWrite, domain.ScopeRepoAdmin,
 	domain.ScopeMRWrite, domain.ScopeMRMerge,
 // upgradeTokens adds to b the schema 4 upgrade of the tokens: a token that
 // holds all of preIssueScopes, limited to no repository and no branch prefix,
-// had the owner rights, which AllScopes now needs issue:write for, so it gets
+// had the owner rights, which AllScopes now needs mr:review and issue:write
-// the scope (this is no escalation). Every other token is left as it is. The
+// for, so it gets each of them if missing (this is no escalation). Every other
-// batch requires each token to be at the version it was read at.
+// token is left as it is. The batch requires each token to be at the version it
+// was read at.
 //
 //hexcheck:ignore suspicious-business-logic-in-adapter one-time storage upgrade of the stored tokens: a storage concern, no policy
 func (s *Store) upgradeTokens(ctx context.Context, b *kv.Batch) error {
 		if err := proto.Unmarshal(item.Value, pb); err != nil {
 			return fmt.Errorf("upgrade token %s: %w", key, err)
 		}
-		if len(pb.GetRepos()) > 0 || len(pb.GetBranchPrefixes()) > 0 ||
+		if len(pb.GetRepos()) > 0 || len(pb.GetBranchPrefixes()) > 0 {
-			slices.Contains(pb.GetScopes(), domain.ScopeIssueWrite.String()) {
 			continue
 		}
 		full := true
 		if !full {
 			continue
 		}
-		pb.Scopes = append(pb.Scopes, domain.ScopeIssueWrite.String())
+		added := false
+		for _, sc := range []domain.Scope{domain.ScopeMRReview, domain.ScopeIssueWrite} {
+			if !slices.Contains(pb.GetScopes(), sc.String()) {
+				pb.Scopes = append(pb.Scopes, sc.String())
+				added = true
+			}
+		}
+		if !added {
+			continue
+		}
 		b.RequireVersion(slices.Clone(key), item.Version)
 		b.Put(slices.Clone(key), mustMarshal(pb))
 	}