fix(reconcile): close the PlatformUser Rule 2 and Rule 5 gaps

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 {

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) {

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,34 +112,64 @@ 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 <a@x.com>" 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>", "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
 	}
 	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}

internal/reconcile/platformuser_reconciler.go

@@ -80,18 +80,26 @@ 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 {
 		return nil, err
 	}
 	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 {

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 <alice@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), "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{}, "")}
 

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 <alice@x.com>", 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 <alice@x.com>", 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 <Alice@X.com>", 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.