refactor: move identities code out of state package

[benhoyt]

This moves the identities state handling out of the main state package and into its own overlord/identities package, to avoid cluttering the state package with more and more domain concerns. We use State.Get and State.Set to store the identities data under an "identities" key. We also keep a local copy of the in-memory identities map to avoid having to call Get every time it's read (which unmarshals the data from JSON every time).

In the state JSON file, currently the "identities" dict lives at the top level, whereas after this it lives under the "data" field (Get/Set data). Hence the small amount of code in patch2 to unmarshal LegacyIdentities from the top level and move it into data.

Most of the lines of code in this PR are trivial changes due to the refactor, for example state.AdminAccess -> identities.AdminAccess.

In a follow-up PR I'd like to refactor the Identity types. We currently have Identity, apiIdentity (without sensitive data, for the API), and marshalledIdentity (for state). This is a bit messy. We should probably move the API-related marshalling and validation into the daemon (api_identities.go).

The automated tests cover most cases, I believe. I've also done manual testing to ensure the identities feature is working, and the migration/upgrade works as expected.

Fixes #710.

[benhoyt]

These types and code are directly copied from state/state.go.

[Copilot]

Pull request overview

This PR refactors the identity management code from the state package into a dedicated overlord/identities package to improve separation of concerns. The identities data is now stored using State.Get/Set under an "identities" key rather than as a direct field on the State struct. A new identities.Manager handles all identity operations and maintains a local in-memory cache to avoid repeated JSON unmarshaling.

Key changes:

• Created new overlord/identities package with Manager type
• Moved identity types and methods from state package to new package
• Added legacy identities migration support for backward compatibility with old state files

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internals/overlord/state/state.go	Removed identities field and marshalling code; added legacy identities migration
internals/overlord/state/state_test.go	Removed "identities" from state fields list
internals/overlord/identities/identities.go	New file containing Identity types and Manager implementation
internals/overlord/identities/state.go	New file with marshalling/unmarshalling helpers for state persistence
internals/overlord/identities/identities_test.go	Migrated tests to use identities.Manager
internals/overlord/overlord.go	Added identitiesMgr initialization and accessor method
internals/overlord/pairingstate/manager.go	Updated to accept and use identities.Manager
internals/overlord/pairingstate/manager_test.go	Updated tests for new Manager signature
internals/overlord/pairingstate/package_test.go	Added identitiesMgr field to test suite
internals/daemon/daemon.go	Updated to use identities.Manager and new Access type
internals/daemon/daemon_test.go	Updated tests to use identities package types
internals/daemon/access.go	Updated to use identities.Access type
internals/daemon/access_test.go	Updated tests to use identities.Access constants
internals/daemon/api_identities.go	Updated to use identities.Manager methods
internals/daemon/api_identities_test.go	Updated tests and added Basic identity test case
internals/daemon/api_notices.go	Updated to use identities.Access type
internals/daemon/api_notices_test.go	Updated test helper and tests to use identities.Access

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[james-garner-canonical]

s.data.has("identities") is a second source of truth for identitiesKey in identities.go. I guess maybe this is fine since it's only for the data migration. To avoid a circular dependency, I guess the only way to dry this would be defining identitiesKey in a third place, which may not be worth it.

[benhoyt]

Yeah, I don't think it's worth it. Plus, this code path is tested in identities_test.go.

[james-garner-canonical]

Testing my understanding: this works because we no longer call unmarshalIdentities on LegacyIdentities here, so s.data["identities"] is in the state that the identity manager expects to load it in.

[dimaqq]

Fly-by review, because I would not be surprised to find myself merging these changes into the FIPS branch soon.

[dmitry-lyfar]

@benhoyt LGTM. It's a bit pity that we cannot use the overlord's patch system for that kind of one-shot migration and will have to live with LegacyIdentities in marshalledState (we'd have to leave something like Identities json.RawMessage as part of the State for a patch to work I suppose). After some thinking, your approach looks more reasonable.

Maybe one of the hidden advantages of this PR would also be an ability to migrate your marshaled identities if needed similarly to how snapd patches do it for the snap setup struct.

[benhoyt]

Thanks @dmitry-lyfar -- I had forgotten about the patch package (it's not yet used in Pebble). I'll give the Identities json.RawMessage approach some thought. It would probably be cleaner for it to be an official "patch".

[dmitry-lyfar]

I think that now, when the patch system is used, it looks better with the legacy handling logic separated.