diff --git a/core/user/service.go b/core/user/service.go index 3a97d7adf..94297e7e1 100644 --- a/core/user/service.go +++ b/core/user/service.go @@ -173,8 +173,11 @@ func (s Service) Sudo(ctx context.Context, id string, relationName string) error return err } } else { - // skip - return nil + // A non-email id that resolves to no user cannot be granted anything. + // Fail loudly instead of returning success: a silent skip reports the + // grant as done while nothing changed, so a reconcile add for a missing + // user id would report success and re-plan the same add forever. + return fmt.Errorf("%w: %q is not an existing user id or a valid email", ErrNotExist, id) } } if err != nil { diff --git a/core/user/service_test.go b/core/user/service_test.go index 0a4e2ff37..e63b61d91 100644 --- a/core/user/service_test.go +++ b/core/user/service_test.go @@ -819,6 +819,22 @@ func TestService_Sudo(t *testing.T) { return user.NewService(repo, relationService, sessionService, auditRecordRepository) }, }, + { + // An id that is neither an existing user nor an email must fail, not be + // skipped: a silent skip reports success while granting nothing, so a + // reconcile add for a missing user id would never converge. + name: "an unknown non-email id fails loudly instead of a silent skip", + wantErr: true, + args: args{ + id: "00000000-0000-0000-0000-000000000001", + relationName: schema.AdminRelationName, + }, + setup: func() *user.Service { + repo, relationService, sessionService, auditRecordRepository := mockService(t) + repo.EXPECT().GetByID(mock.Anything, "00000000-0000-0000-0000-000000000001").Return(user.User{}, user.ErrNotExist) + return user.NewService(repo, relationService, sessionService, auditRecordRepository) + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go index 6020fd5fc..d02a82a66 100644 --- a/internal/reconcile/platformuser.go +++ b/internal/reconcile/platformuser.go @@ -83,12 +83,13 @@ func validateSpec(s PlatformUserSpec) error { return fmt.Errorf("empty ref") } // The ref must be a form the server resolves to exactly one principal, so the - // diff can match it to a live principal. A user is an id or a plain email; a - // service user is an id. A slug or a display-name email would resolve on the - // server but not match here, silently planning a remove of access meant to stay. + // diff can match it to a live principal. A user is a uuid or an email; a + // service user is a uuid. An email ref is matched by its address, so a + // display-name or differently-cased form still matches the address the server + // stored. A slug (neither a uuid nor an email) is rejected. if s.Type == principalTypeUser { - if !isUUID(ref) && !isBareEmail(ref) { - return fmt.Errorf("ref %q must be a user id (uuid) or a plain email address", s.Ref) + if _, isEmail := emailAddress(ref); !isUUID(ref) && !isEmail { + return fmt.Errorf("ref %q must be a user id (uuid) or an email address", s.Ref) } } else if !isUUID(ref) { return fmt.Errorf("ref %q must be a service user id (uuid)", s.Ref) @@ -111,26 +112,51 @@ func isUUID(s string) bool { return err == nil } -// isBareEmail reports whether s is a plain email address with no display name, -// so it matches the address the server stores. "Alice " is rejected. -func isBareEmail(s string) bool { - addr, err := mail.ParseAddress(s) - return err == nil && addr.Name == "" && addr.Address == s +// emailAddress returns the lowercased address part of an email and whether s +// parsed as one. "Alice ", "a@x.com", and "A@X.com" all yield +// "a@x.com", so a display-name or differently-cased ref matches the address the +// server stored. A value that is not an email (a uuid, a slug) does not parse +// and returns ok=false. +func emailAddress(s string) (addr string, ok bool) { + a, err := mail.ParseAddress(strings.TrimSpace(s)) + if err != nil { + return "", false + } + got := strings.ToLower(a.Address) + // A quoted local part unquotes to an address ParseAddress cannot read back + // (`"john smith"@x.com` -> `john smith@x.com`). Treat it as not a usable email, + // so callers fall back to the id and export never emits a ref that fails its + // own re-validation. + if _, err := mail.ParseAddress(got); err != nil { + return "", false + } + return got, true } // canonicalRef normalizes a ref to the form the server stores. A UUID in any -// accepted form becomes the canonical lowercase id; anything else (an email) is -// only trimmed and still matches case-insensitively against the stored address. +// accepted form becomes the canonical lowercase id; an email becomes its +// lowercased address with any display name dropped, which is the form an add +// sends and the form matching compares. Anything else is only trimmed. func canonicalRef(ref string) string { ref = strings.TrimSpace(ref) if u, err := uuid.Parse(ref); err == nil { return u.String() } + // An add for an unmatched email spec sends this ref, so normalize it to the + // plain address the server resolves, not a display-name form it would treat as + // a new user. + if addr, ok := emailAddress(ref); ok { + return addr + } return ref } // specMatchesPrincipal reports whether a desired spec refers to a current -// principal: users match by id or email, service users by id. +// principal: a service user matches by id, a user by id or by email address. +// Emails compare by their parsed address (lowercased), so a display-name or +// differently-cased ref matches the plain address the server stored. A uuid ref +// does not parse as an email, so it only ever matches by id, never against a +// user whose email happens to be another user's id. func specMatchesPrincipal(s PlatformUserSpec, p platformPrincipal) bool { if s.Type != p.Type { return false @@ -138,7 +164,12 @@ func specMatchesPrincipal(s PlatformUserSpec, p platformPrincipal) bool { if s.Ref == p.ID { return true } - return p.Type == principalTypeUser && s.Ref != "" && strings.EqualFold(s.Ref, p.Email) + if p.Type != principalTypeUser { + return false + } + refAddr, refOK := emailAddress(s.Ref) + pAddr, pOK := emailAddress(p.Email) + return refOK && pOK && refAddr == pAddr } var platformRelationOrder = []string{schema.AdminRelationName, schema.MemberRelationName} diff --git a/internal/reconcile/platformuser_reconciler.go b/internal/reconcile/platformuser_reconciler.go index 2b181ab05..46a0a0191 100644 --- a/internal/reconcile/platformuser_reconciler.go +++ b/internal/reconcile/platformuser_reconciler.go @@ -80,8 +80,9 @@ func (r *PlatformUserReconciler) Reconcile(ctx context.Context, spec []byte, dry } // Export returns the current platform users as a desired-state spec: one entry -// per (principal, relation), users referenced by email when they have one. -// Entries are sorted so repeated exports produce identical files. +// per (principal, relation), users referenced by email address when they have a +// parseable one, else by id. Entries are sorted so repeated exports produce +// identical files. func (r *PlatformUserReconciler) Export(ctx context.Context) (any, error) { current, err := r.fetchCurrent(ctx) if err != nil { @@ -89,9 +90,16 @@ func (r *PlatformUserReconciler) Export(ctx context.Context) (any, error) { } specs := make([]PlatformUserSpec, 0, len(current)) for _, p := range current { + // Reference a user by its email address when it has a parseable one, else by + // id. The address is normalized (lowercased, display name dropped) so the + // export is canonical and matches the principal on re-reconcile. A stored + // value that is not an email (empty, or another user's id) falls back to the + // id, so it cannot masquerade as that id when the export is reconciled. ref := p.ID - if p.Type == principalTypeUser && p.Email != "" { - ref = p.Email + if p.Type == principalTypeUser { + if addr, ok := emailAddress(p.Email); ok { + ref = addr + } } for _, rel := range platformRelationOrder { if _, ok := p.Relations[rel]; ok { diff --git a/internal/reconcile/platformuser_reconciler_test.go b/internal/reconcile/platformuser_reconciler_test.go index d69caffcc..dff6a33c4 100644 --- a/internal/reconcile/platformuser_reconciler_test.go +++ b/internal/reconcile/platformuser_reconciler_test.go @@ -192,6 +192,75 @@ func TestPlatformUserReconciler_Reconcile(t *testing.T) { } }) + t.Run("a user with a display-name email round-trips via its address", func(t *testing.T) { + // The server can store a display-name email. Export normalizes it to the bare + // address, and re-reconcile matches by address, so the round-trip plans nothing. + api := &fakePlatformUserAPI{ + users: []*frontierv1beta1.User{ + platformUserPBRelations(t, "55555555-5555-5555-5555-555555555555", "Alice ", schema.AdminRelationName), + }, + } + registry := map[string]Reconciler{KindPlatformUser: NewPlatformUserReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPlatformUser) + assert.NoError(t, err) + assert.Contains(t, string(out), "alice@x.com") // the address, normalized + assert.NotContains(t, string(out), "Alice") // display name dropped + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("a user whose email is another user's id exports by id and round-trips", func(t *testing.T) { + // Emails are unvalidated, so a user's email can be another user's id. Export + // must reference such a user by its own id, not emit the id-shaped email as + // the ref (which would grant the other user on re-reconcile). + aID := "aaaaaaaa-1111-1111-1111-111111111111" + bID := "bbbbbbbb-2222-2222-2222-222222222222" + api := &fakePlatformUserAPI{ + users: []*frontierv1beta1.User{ + platformUserPBRelations(t, aID, bID, schema.MemberRelationName), // A's email is B's id + platformUserPBRelations(t, bID, "b@x.com", schema.AdminRelationName), + }, + } + registry := map[string]Reconciler{KindPlatformUser: NewPlatformUserReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPlatformUser) + assert.NoError(t, err) + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("a user with a quoted-local-part email exports by id and round-trips", func(t *testing.T) { + // mail.ParseAddress accepts "john smith"@x.com, but its unquoted address does + // not re-parse, so emitting it would fail the tool's own validation. Export + // must fall back to the id, like the empty and garbage cases. + api := &fakePlatformUserAPI{ + users: []*frontierv1beta1.User{ + platformUserPBRelations(t, "66666666-6666-6666-6666-666666666666", `"john smith"@x.com`, schema.AdminRelationName), + }, + } + registry := map[string]Reconciler{KindPlatformUser: NewPlatformUserReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPlatformUser) + assert.NoError(t, err) + assert.Contains(t, string(out), "66666666-6666-6666-6666-666666666666") // by id + assert.NotContains(t, string(out), "john smith") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + t.Run("export of an empty platform yields a document Run accepts", func(t *testing.T) { registry := map[string]Reconciler{KindPlatformUser: NewPlatformUserReconciler(&fakePlatformUserAPI{}, "")} diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go index 24a66aa8b..f8acfb6c4 100644 --- a/internal/reconcile/platformuser_test.go +++ b/internal/reconcile/platformuser_test.go @@ -164,14 +164,32 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Empty(t, ops) }) - t.Run("rejects a user ref that is neither a uuid nor a plain email", func(t *testing.T) { + t.Run("rejects a user ref that is neither a uuid nor an email", func(t *testing.T) { _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "alice-slug", Relation: admin}}, nil) - assert.ErrorContains(t, err, "must be a user id (uuid) or a plain email address") + assert.ErrorContains(t, err, "must be a user id (uuid) or an email address") }) - t.Run("rejects a display-name email ref", func(t *testing.T) { - _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "Alice ", Relation: admin}}, nil) - assert.ErrorContains(t, err, "must be a user id (uuid) or a plain email address") + t.Run("a display-name email ref matches the stored principal by address", func(t *testing.T) { + // A display-name email is a valid ref; it matches by its address, so it does + // not plan a spurious remove of access it means to keep. + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "Alice ", Relation: admin}}, + []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("an add for a new email ref uses the normalized address, not the ref verbatim", func(t *testing.T) { + // A display-name or differently-cased ref for a user with no current grant + // must add by the plain address, so the server grants the real user rather + // than creating a shadow user whose email is the literal display-name form. + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "Alice ", Relation: admin}}, + nil, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{{Action: opAdd, Type: "user", Ref: "alice@x.com", Relation: admin}}, ops) }) t.Run("rejects a service user ref that is not a uuid", func(t *testing.T) { @@ -206,6 +224,13 @@ func TestSpecRefMatching(t *testing.T) { t.Run("no match on an unrelated ref", func(t *testing.T) { assert.False(t, specMatchesPrincipal(PlatformUserSpec{Type: "user", Ref: "bob@x.com", Relation: admin}, userP)) }) + t.Run("a uuid ref does not match a user whose email is that uuid", func(t *testing.T) { + // A user's email can be any string, including another user's id. A uuid ref + // parses to no address, so it matches only by id, never against that email. + victim := principal("user", "victim-id", "bbbbbbbb-2222-2222-2222-222222222222") + assert.False(t, specMatchesPrincipal( + PlatformUserSpec{Type: "user", Ref: "bbbbbbbb-2222-2222-2222-222222222222", Relation: admin}, victim)) + }) // end-to-end through the diff: a user referenced by id and a service user by id // both make no change when already in the desired relation.