From f469ee98def20f09d494173d955e3d937bb00ea1 Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Wed, 16 Sep 2026 14:21:02 +0200 Subject: [PATCH] Improve stamping of fields in the config --- client/internal/profilemanager/migration.go | 3 +- .../internal/profilemanager/migration_test.go | 23 ++++ client/internal/profilemanager/service.go | 47 +++++--- .../internal/profilemanager/service_test.go | 101 ++++++++++++++++++ 4 files changed, 156 insertions(+), 18 deletions(-) diff --git a/client/internal/profilemanager/migration.go b/client/internal/profilemanager/migration.go index 0cc1855da..a53d5aff3 100644 --- a/client/internal/profilemanager/migration.go +++ b/client/internal/profilemanager/migration.go @@ -204,7 +204,8 @@ func (s *ServiceManager) stampActiveUserDir(profiles []Profile, active *ActivePr continue } if err := stampPrincipal(p.Path, principal); err != nil { - return fmt.Errorf("stamp %s: %w", p.ID, err) + log.Warnf("leaving %s unowned, its owner could not be recorded: %v", p.Path, err) + continue } log.Infof("recorded %s as the owner of %s, the directory it sits in is that account's", principal, p.Path) } diff --git a/client/internal/profilemanager/migration_test.go b/client/internal/profilemanager/migration_test.go index 002b36437..1a9c53385 100644 --- a/client/internal/profilemanager/migration_test.go +++ b/client/internal/profilemanager/migration_test.go @@ -266,3 +266,26 @@ func TestMigrate_LeavesAProfileThatAlreadyNamesAnOwnerAlone(t *testing.T) { "only the profiles with no owner of their own are attributed") }) } + +func TestMigrate_SkipsAProfileItCannotStamp(t *testing.T) { + username, principal := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + good := writeLegacyProfile(t, configDir, dir, "work", nil) + broken := filepath.Join(configDir, dir, "broken.json") + require.NoError(t, os.WriteFile(broken, []byte("null"), 0600)) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + + assert.Equal(t, []string{principal}, readOwners(t, good), + "one profile that cannot be stamped does not hold up the rest") + assert.DirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "and the marker still lands, since the claim path retries the one left behind") + + data, err := os.ReadFile(broken) + require.NoError(t, err) + assert.Equal(t, "null", string(data), "the profile it could not stamp is untouched") + }) +} diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index b1c66d12a..dc365c325 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -63,7 +63,12 @@ type ownerMeta struct { Owners []string } -// ownersFieldName is the key on disk. +// Config JSON keys on disk. +const ( + ownersFieldName = "Owners" + nameFieldName = "Name" +) + func (e *ErrAmbiguousHandle) Error() string { switch e.Kind { case AmbiguityKindIDPrefix: @@ -900,37 +905,45 @@ func StampOwner(path string, owner ipcauth.Identity) error { // resolves an account name rather than a caller, and a name the kernel never // vouched for must not become an Identity on the way. func stampPrincipal(path, principal string) error { - return updateProfileConfig(path, func(cfg *Config) { - cfg.Owners = []string{principal} - }) + return setProfileField(path, ownersFieldName, []string{principal}) } // writeProfileName sets a profile's display name. Renaming does it on request, // migration does it to move a name out of a filename that is about to change. func writeProfileName(path, name string) error { - return updateProfileConfig(path, func(cfg *Config) { - cfg.Name = name - }) + return setProfileField(path, nameFieldName, name) } -// updateProfileConfig reads a profile, applies mutate and writes it back. -// -// The whole config makes the round trip, which is what every writer here does, -// so a field this version does not model is dropped. That only happens after a -// downgrade, and a downgrade already drops the owners it cannot read. -func updateProfileConfig(path string, mutate func(*Config)) error { +// setProfileField replaces one top-level key of a profile's JSON and leaves the +// rest of the document as it found it. +func setProfileField(path, field string, value any) error { data, err := os.ReadFile(path) if err != nil { return err } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { + doc := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &doc); err != nil { return err } - mutate(&cfg) + if doc == nil { + return fmt.Errorf("profile %s holds no object to set %s on", path, field) + } - if err := util.WriteJsonWithRestrictedPermission(context.Background(), path, cfg); err != nil { + raw, err := json.Marshal(value) + if err != nil { + return fmt.Errorf("encode %s of %s: %w", field, path, err) + } + + // Decoding matches keys case-insensitively + for k := range doc { + if k != field && strings.EqualFold(k, field) { + delete(doc, k) + } + } + doc[field] = raw + + if err := util.WriteJsonWithRestrictedPermission(context.Background(), path, doc); err != nil { return fmt.Errorf("write profile %s: %w", path, err) } return nil diff --git a/client/internal/profilemanager/service_test.go b/client/internal/profilemanager/service_test.go index 936ec5a39..b32a7b532 100644 --- a/client/internal/profilemanager/service_test.go +++ b/client/internal/profilemanager/service_test.go @@ -659,3 +659,104 @@ func TestActiveProfilePath_RefusesToGuessBetweenNamesakes(t *testing.T) { "the recorded directory is what tells the namesakes apart") }) } + +func TestListProfiles_ClaimKeepsFieldsThisVersionDoesNotModel(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // What a client newer than this one leaves behind: a key Config has no + // field for, next to one it does. + newer := map[string]any{"Enabled": true, "Hosts": []any{"a", "b"}} + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "MTU": 1280, + "SomethingNewer": newer, + }) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + _, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path), "the claim still lands") + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + assert.Equal(t, newer, doc["SomethingNewer"], + "a listing must not drop the settings of a client that models more than this one") + assert.Equal(t, float64(1280), doc["MTU"], "and leaves the ones it does model alone") + assert.NotContains(t, doc, "PrivateKey", + "nor write out the rest of Config just because it has fields for it") + }) +} + +func TestSetProfileField_ReplacesAKeySpelledInAnotherCase(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "owners": []any{"uid:1"}, + }) + + require.NoError(t, stampPrincipal(path, "uid:4242")) + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + assert.NotContains(t, doc, "owners", + "two spellings of one field would leave the reader to pick") + assert.Equal(t, []any{"uid:4242"}, doc["Owners"]) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) + }) +} + +func TestSetProfileField_RefusesADocumentThatIsNotAnObject(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := filepath.Join(configDir, "alice", "work.json") + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) + require.NoError(t, os.WriteFile(path, []byte("null"), 0600)) + + require.Error(t, stampPrincipal(path, "uid:4242"), + "a profile that is not an object is not one an owner can be set on") + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "null", string(data), "and it is left as it was found") + }) +} + +func TestSetProfileField_KeepsKeysItWasNotAskedToWrite(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // Keys Config has no field for, in every shape a newer client could + // leave one behind. + unknown := map[string]any{ + "AString": "keep me", + "ANumber": float64(7), + "ABool": true, + "ANull": nil, + "AList": []any{"a", float64(2), false}, + "AnObject": map[string]any{"Nested": map[string]any{"Deep": []any{float64(1)}}}, + } + fields := map[string]any{"MTU": 1280} + for k, v := range unknown { + fields[k] = v + } + path := writeLegacyProfile(t, configDir, "alice", "work", fields) + + require.NoError(t, setProfileField(path, ownersFieldName, []string{"uid:4242"})) + require.NoError(t, setProfileField(path, nameFieldName, "Work")) + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + for k, want := range unknown { + assert.Equal(t, want, doc[k], + "%s is not a key this version models, so it is not this version's to drop", k) + } + assert.Equal(t, float64(1280), doc["MTU"], "a key it does model is left where it was too") + assert.Equal(t, []any{"uid:4242"}, doc["Owners"], "and the fields it was asked for are written") + assert.Equal(t, "Work", doc["Name"]) + assert.Len(t, doc, len(unknown)+3, "with nothing else added") + }) +}