Fix stale profile fields during session user synchronization

[sridharkalaibala]

With session-user synchronization enabled, deleting an optional profile field from the account file leaves its old value in the session. Flex refresh(true) restores missing properties; the regular-account branch merges the stored data into the old user.

Refresh the account data without restoring removed fields, while preserving the session's exact authenticated/authorized properties (including absence and false values). Restore the username/default state normally supplied by the Flex user constructor. For regular accounts, replace the old property data before applying the stored snapshot, and release serialized cached file contents used by older Grav versions before loading the account.

Refs getgrav/grav-plugin-form#586. Rebased on develop after #340 and its profile-save error-handling correction merged. The legacy replacement retains hashed_password, secret, and twofa_secret, which account serialization intentionally omits; a subsequent profile save must not lose those credentials. Session-state capture uses jsonSerialize() to avoid Flex avatar parsing.

Reproduction: serialize a logged-in session user with a nonempty about_specialist; remove that field and change fullname in its account YAML; restore the session user in a fresh PHP process and invoke onSessionStart() with synchronization enabled. Upstream retains the old description; this change refreshes the name and removes the description while retaining the same user object and its login state.

Local validation used actual Grav account backends and Login's session-start handler:

• Grav 2.1.2 and the minimum supported Grav 1.7.41, both on PHP 8.3.6 with their locked production dependencies.
• 32 targeted scenario checks pass: Flex and regular accounts on both versions, each covering clear/nested removal, unchanged account, sync disabled, default state, pending two-factor login, unauthenticated state, absent authentication flags, and disabled-account invalidation requests. Permission decisions and object identity are checked too.
• Original Login e72633f reproduces the stale-field failure; simply changing Flex refresh to false without preserving session state loses authentication. The first implementation also exposed serialized file-content caching on Grav 1.7.41 regular accounts, addressed here.
• PHP syntax and git diff --check pass.
• Review regression: the pre-correction regular-account implementation reproduced credential loss both in memory and in the YAML written by the actual processUserProfile() handler. With the correction, all three secret fields survive synchronization and a successful profile save on both account backends in both tested Grav versions (four passing checks). The saved password hash still verifies the synthetic password and two-factor authentication remains enabled.

The broader probes still report account-deletion invalidation failures, also observed without this change on current upstream. This PR does not change the existing initial exists() guard or claim to fix deleted-account handling. These are CLI probes with synthetic files and an observer for invalidation requests, not browser-cookie or Admin-UI tests. PHP 7.3 was not executed.

Compatibility: session synchronization remains disabled by default. The existing SESSION_USER_SYNC_HELP documentation explicitly warns that enabling it may break plugins which change user data without saving the account. This change follows that documented stored-data contract while preserving the authentication properties defined by Grav. An additional synthetic plugin-property assertion verifies that an unsaved property is removed on changed-account synchronization and retained with synchronization disabled, on both backends and both versions. This is not a claim to have tested every third-party plugin. No account files are saved by the synchronization change. Prepared with AI assistance and reviewed against the stated before/after checks.

[rhukster]

Thanks — this is a good catch and the diff is more careful than most. I verified the stale-field bug on both account backends and confirmed the authentication handling is sound: the authenticated/authorized capture reads raw element values on both UserObject and the regular Data user, so absence, false and a pending-2FA session all round-trip exactly as they do today. Running the full matrix — flex and regular accounts, field removal, permission revocation, disabled, missing state, deleted account, all three auth states — the auth outcomes are identical to upstream in every case.

Worth calling out that your change also closes something upstream gets wrong. FlexObject::refresh() rebuilds through objectConstruct(), which skips UserObject::__construct — the only place that strips authenticated/authorized from incoming data. So on the Flex path today, an account file containing those keys has them applied straight to the session user, and defProperty() won't overwrite them. Your undef()-then-restore removes the file-supplied values first, which is the right behaviour. Narrow, but real.

Your note about deleted-account invalidation is accurate too: refresh() returns early when the storage metadata has no checksum, so neither upstream nor this change invalidates a deleted Flex account. Pre-existing and out of scope here.

One thing needs fixing before this can go in. In the legacy branch, $stored->jsonSerialize() deliberately hides hashed_password, secret and twofa_secret — both UserObject and DataUser\User unset them explicitly. Upstream's update() is a merge, so those survive on the old object. Wiping every key first means they're gone, and accounts.type: regular is the default, so this is the common path.

It's fail-closed for login and for 2FA verification, but it reaches disk: the frontend profile form operates on the session user and calls save(), which writes items wholesale. So one profile save after a sync rewrites the account YAML with no password hash and no 2FA secret. That locks the account out permanently — and since the login-time gate tests twofa_enabled && twofa_secret, the challenge is then silently skipped.

The fix is to skip those three keys in the wipe loop:

$storedData = $stored->jsonSerialize();
// jsonSerialize() hides the password hash and the 2FA secrets, so they are
// never present in $storedData. They are account secrets, not stale profile
// fields: removing them here strips them from the session user, and a later
// profile save writes that loss back to the account file.
$hidden = ['hashed_password' => true, 'secret' => true, 'twofa_secret' => true];
foreach (array_keys($user->toArray()) as $field) {
    if (!isset($hidden[$field])) {
        $user->undef($field);
    }
}
$user->update($storedData);

Two smaller notes. The file()->free() call is correct and needed on 1.7.x, where CompiledFile::__sleep() still serializes raw/content and __wakeup() re-registers the instance, so load() gets the frozen session copy back. On 2.x core already fixed that, so it's redundant there but harmless. And since toArray() on a Flex user runs the avatar media-field parse on every request, jsonSerialize() would be a cheaper capture for $sessionState.

#340 is merged, so please rebase on develop — the two don't conflict textually, but #340 makes that profile save more reliable, which is exactly the path that would write the credential loss to disk.

[sridharkalaibala]

Rebased on develop (including #340 and the follow-up error-handling fix) and pushed 2a58949. The legacy wipe now skips hashed_password, secret and twofa_secret, and session-state capture uses jsonSerialize() as suggested.

I reproduced the credential loss with the pre-correction implementation: an actual successful processUserProfile() call after synchronization wrote the synthetic regular account without its credentials. With the fix, all three fields survive in memory and on disk after that same profile-save handler on Flex and regular accounts, on Grav 2.1.2 and 1.7.41. The saved hash still verifies the fixture password and two-factor authentication remains enabled.

The existing 32 targeted session checks still pass; the four pre-existing deleted-account checks still fail as documented. PHP syntax and diff checks pass. These are real account/Form/Login handlers in CLI fixtures, not a full browser session. Thanks for catching the missing credential case in my original validation.

[rhukster]

To put it more plainly than I did above: as it stands this would be a bad commit, and it would break login accounts on real sites.

The legacy branch is the default account backend. This wipes every key off the session user and then reapplies $stored->jsonSerialize(), which deliberately omits hashed_password, secret and twofa_secret. The session user comes back without them. The frontend profile form operates on that same user object and saves it wholesale, so the first time anyone saves their profile after a sync, their account file is rewritten with no password hash and no 2FA secret.

That user can no longer log in, and no amount of retrying fixes it because the hash is gone from disk. Once an admin resets the password to get them back in, 2FA is silently off for that account, because the login gate checks twofa_enabled && twofa_secret and the secret is no longer there. On a site with session sync enabled that is every user who touches their profile, not an edge case.

None of that is a criticism of the rest of it. The auth-flag handling is careful and it genuinely fixes something upstream gets wrong. It is one loop that needs to skip three keys, and the patch for it is in my previous comment.

Holding this until that is in.

[sridharkalaibala]

Agreed: the original wipe was unsafe, and those three keys must survive. The correction is already in the current PR head, 2a58949, pushed after your first review. Here is the exact current loop: it skips hashed_password, secret and twofa_secret before updating the remaining data.

I rechecked the GitHub head and its file contents just now, and reran the regular-account sync → actual profile-save regression: all three credentials remain in memory and in the saved YAML. The earlier verification also covers both backends on Grav 2.1.2 and 1.7.41. The PR is rebased on develop after #340, and uses jsonSerialize() for the authentication snapshot as requested. Sorry for the unsafe first version; your concern was valid.

[rhukster]

Verified the current head independently rather than taking the diff on trust. The wipe loop now skips hashed_password, secret and twofa_secret, and I reproduced the whole thing end to end against real account backends: on 3be504e a sync plus a real save() writes the account YAML with no hash and no 2FA secret, the password stops verifying and the 2FA gate goes quiet. On 2a58949 all three survive in memory and on disk, on both regular and flex, the stale field is gone and fullname refreshes. I ran every auth state through both backends too — absent, false, pending-2FA and fully authorized — and the outcomes are identical to upstream in all ten cases.

The Flex undef()-then-restore also stops an account file containing authenticated: true from handing the session user authentication, which upstream does today. Base is 62ca2db, so both #340 commits are in, and it merges clean.

One thing I'm going to change on top rather than send back. Keeping the session's copies of those three keys means the legacy path can never pick up an out-of-band change: if an admin resets a password or rotates a 2FA secret while the user is logged in, the user's next profile save writes the old values back over it. Flex doesn't do that, because refresh() replaces the elements from the file. Reading the three off $stored instead of off $user fixes both problems at once, and I've checked it keeps everything your version fixes. Same for refresh(true) on the Flex-collection sub-branch at line 192, which should be false for the same reason the branch below it is.

Good work on the auth handling, and thanks for taking the credential correction cleanly.