diff --git a/knotserver/backfill.go b/knotserver/backfill.go index 1c61b5e0..df3eb8cc 100644 --- a/knotserver/backfill.go +++ b/knotserver/backfill.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "maps" "slices" "github.com/bluesky-social/indigo/atproto/syntax" @@ -86,39 +87,41 @@ func BackfillCollaborators( return nil } - repoDids, err := d.ListRepoDids() + byRepo, err := e.GetCollaboratorsByRepo(rbac.ThisServer) if err != nil { - return fmt.Errorf("list repos: %w", err) + return fmt.Errorf("list collaborators: %w", err) } var rows []db.Collaborator - for _, repoDid := range repoDids { + var skipped int + for _, repoDid := range slices.Sorted(maps.Keys(byRepo)) { + candidates := byRepo[repoDid] + ownerDid, _, err := d.GetRepoKeyOwner(repoDid) if err != nil { - l.Warn("skipping repo during collaborator backfill", "repoDid", repoDid, "err", err) + l.Warn("skipping collaborators for unresolvable repo", "repoDid", repoDid, "collaborators", len(candidates), "err", err) + skipped += len(candidates) continue } repo, err := syntax.ParseDID(repoDid) if err != nil { - l.Warn("skipping repo with invalid DID", "repoDid", repoDid, "err", err) + l.Warn("skipping collaborators for repo with invalid DID", "repoDid", repoDid, "collaborators", len(candidates), "err", err) + skipped += len(candidates) continue } owner, err := syntax.ParseDID(ownerDid) if err != nil { - l.Warn("skipping repo with invalid owner DID", "repoDid", repoDid, "owner", ownerDid, "err", err) + l.Warn("skipping collaborators for repo with invalid owner DID", "repoDid", repoDid, "owner", ownerDid, "collaborators", len(candidates), "err", err) + skipped += len(candidates) continue } - collaborators, err := e.GetUserByRoleInRepo("repo:collaborator", rbac.ThisServer, repoDid) - if err != nil { - return fmt.Errorf("list collaborators for %s: %w", repoDid, err) - } - - for _, candidate := range collaborators { + for _, candidate := range candidates { subject, err := syntax.ParseDID(candidate) if err != nil { l.Warn("skipping collaborator with invalid DID", "repoDid", repoDid, "candidate", candidate, "err", err) + skipped++ continue } rows = append(rows, db.Collaborator{ @@ -133,6 +136,6 @@ func BackfillCollaborators( return fmt.Errorf("apply backfill: %w", err) } - l.Info("backfilled collaborators from casbin", "count", len(rows), "repos", len(repoDids), "marked", markApplied) + l.Info("backfilled collaborators from casbin", "count", len(rows), "repos", len(byRepo), "skipped", skipped, "marked", markApplied) return nil } diff --git a/rbac/rbac.go b/rbac/rbac.go index e763246f..5908b0ca 100644 --- a/rbac/rbac.go +++ b/rbac/rbac.go @@ -263,29 +263,27 @@ func (e *Enforcer) GetSpindleUsersByRole(role, domain string) ([]string, error) return e.GetUserByRole(role, intoSpindle(domain)) } -func (e *Enforcer) GetUserByRoleInRepo(role, domain, repo string) ([]string, error) { - policies, err := e.E.GetImplicitUsersForResourceByDomain(repo, domain) +func (e *Enforcer) GetCollaboratorsByRepo(domain string) (map[string][]string, error) { + policies, err := e.E.GetFilteredNamedPolicy("p", 3, "repo:collaborator") if err != nil { return nil, err } - var users []string + byRepo := make(map[string][]string) for _, p := range policies { - user := p[0] - if !strings.HasPrefix(user, "did:") { + subject, dom, repo := p[0], p[1], p[2] + if dom != domain || !strings.HasPrefix(subject, "did:") { continue } - ok, err := e.E.Enforce(user, domain, repo, role) - if err != nil { - return nil, err - } - if ok { - users = append(users, user) - } + byRepo[repo] = append(byRepo[repo], subject) + } + + for repo, users := range byRepo { + slices.Sort(users) + byRepo[repo] = slices.Compact(users) } - slices.Sort(users) - return slices.Compact(users), nil + return byRepo, nil } func (e *Enforcer) IsKnotOwner(user, domain string) (bool, error) { diff --git a/rbac/rbac_test.go b/rbac/rbac_test.go index ea0bd82d..b2fc9283 100644 --- a/rbac/rbac_test.go +++ b/rbac/rbac_test.go @@ -151,17 +151,19 @@ func TestCollaboratorPermissions(t *testing.T) { assert.ElementsMatch(t, []string{}, perms) } -func TestGetByRole(t *testing.T) { +func TestGetCollaboratorsByRepo(t *testing.T) { e := setup(t) knot := "example.com" repo := "did:plc:foo/my-repo" + otherRepo := "did:plc:foo/other-repo" owner := "did:plc:foo" collaborator1 := "did:plc:bar" collaborator2 := "did:plc:baz" _ = e.AddKnot(knot) _ = e.AddRepo(owner, knot, repo) + _ = e.AddRepo(owner, knot, otherRepo) err := e.AddCollaborator(collaborator1, knot, repo) assert.NoError(t, err) @@ -169,13 +171,14 @@ func TestGetByRole(t *testing.T) { err = e.AddCollaborator(collaborator2, knot, repo) assert.NoError(t, err) - collaborators, err := e.GetUserByRoleInRepo("repo:collaborator", knot, repo) + byRepo, err := e.GetCollaboratorsByRepo(knot) assert.NoError(t, err) assert.ElementsMatch(t, []string{ "did:plc:bar", // collaborator1 "did:plc:baz", // collaborator2 - }, collaborators) - assert.NotContains(t, collaborators, owner, "owner does not hold repo:collaborator and must not be listed") + }, byRepo[repo]) + assert.NotContains(t, byRepo[repo], owner, "owner does not hold repo:collaborator and must not be listed") + assert.Empty(t, byRepo[otherRepo], "a repo without collaborators must not appear") } func TestGetPermissionsInRepo(t *testing.T) {