diff --git a/oidc/class/oidcflow.class.php b/oidc/class/oidcflow.class.php index 6c4927e..2091299 100644 --- a/oidc/class/oidcflow.class.php +++ b/oidc/class/oidcflow.class.php @@ -497,6 +497,7 @@ private static function _resolveUser($provider, array $claims) _('The FOG account for this identity no longer exists') ); } + self::_refreshProfile($provider, $claims, $user); return $user; } @@ -530,8 +531,75 @@ private static function _resolveUser($provider, array $claims) ->set('userId', $namedId) ->save(); + self::_refreshProfile($provider, $claims, $byName); + return $byName; } + /** + * Brings the FOG row back in line with the provider, on every sign-in. + * + * The display name used to be written once, by _provisionUser(), and + * never again: renaming somebody in the directory left FOG showing the + * name they had on the day they first signed in, forever, with nothing + * an admin could do about it short of editing the row by hand. That is + * not what a directory-backed account means -- the provider is supposed + * to be the source of truth for who this is, and a value copied once is + * a value that starts drifting immediately. + * + * The LDAP plugin already does this (ldappluginhook.hook.php sets name, + * display, api and authsource on every login, new row or not), so this + * is the OIDC plugin catching up to the behaviour beside it rather than + * a new idea. + * + * Deliberately NOT refreshed here: + * + * - uName. The username is what oidcIdentity is keyed against for + * accounts that predate their link, and renaming a FOG account out + * from under its history, tasks and audit rows is a migration, not a + * login side effect. A provider-side rename keeps working because the + * binding is the subject, not the name. + * - uAuthSource. Stamping it here would take local password login away + * from an account an admin created, which is the opposite of + * break-glass. _provisionUser() stamps rows this plugin made; those + * are the only ones that get it. + * + * uAllowAPI IS re-asserted, and that is a real behaviour choice: the + * provider's setting wins over a FOG-side edit at the next sign-in. It + * matches LDAP, and it means "can these accounts use the API" is + * answered in one place instead of drifting per user. + * + * Saves only when something actually changed, so an unchanged sign-in + * costs no write and leaves no history row. + * + * @param OIDC $provider the provider + * @param array $claims the verified claims + * @param User $user the account being signed in + * + * @return void + */ + private static function _refreshProfile($provider, array $claims, $user) + { + $display = trim((string)($claims['name'] ?? '')); + if ('' === $display) { + // No name claim at all -- Google omits it on some scopes. Leave + // whatever is there rather than blanking a good value. + $display = (string)$user->get('display'); + } + $api = (string)$provider->get('allowapi'); + + $changed = false; + if ($display !== (string)$user->get('display')) { + $user->set('display', $display); + $changed = true; + } + if ($api !== (string)$user->get('api')) { + $user->set('api', $api); + $changed = true; + } + if ($changed) { + $user->save(); + } + } /** * Creates the FOG account for an identity that has none. * diff --git a/tests/oidc-profile-refresh.test.php b/tests/oidc-profile-refresh.test.php new file mode 100644 index 0000000..b985c85 --- /dev/null +++ b/tests/oidc-profile-refresh.test.php @@ -0,0 +1,166 @@ += 2) { + ok("_refreshProfile() is called from $calls sites (both existing-account paths)"); +} else { + bad("_refreshProfile() is called from $calls site(s); both existing-account paths must refresh"); +} + +/* + * Scope the write assertions to this method's body and nothing else. + * + * A fixed-length window is not good enough: the very next method is + * _provisionUser(), which legitimately writes name and authsource, so a + * window that overruns reports those as violations here. Cut at the start of + * the following method instead. + */ +$at = strpos($squashed, 'privatestaticfunction_refreshProfile('); +$bodySquashed = ''; +if (false !== $at) { + $next = strpos($squashed, 'staticfunction', $at + 40); + $bodySquashed = false === $next + ? substr($squashed, $at) + : substr($squashed, $at, $next - $at); +} +if ('' === $bodySquashed) { + bad('could not isolate _refreshProfile()\'s body'); +} + +if (false !== strpos($bodySquashed, "set('display'")) { + ok('_refreshProfile() writes the display name'); +} else { + bad('_refreshProfile() does not write the display name'); +} + +echo "\n2. it does not write what must not be rewritten\n"; + +foreach ( + [ + 'name' => 'uName -- renaming an account out from under its history is a migration, not a login side effect', + 'authsource' => 'uAuthSource -- stamping it removes local password login from an admin-created account', + ] as $field => $why +) { + if (false === strpos($bodySquashed, "set('" . $field . "'")) { + ok("does not write '$field'"); + } else { + bad("writes '$field': $why"); + } +} + +echo "\n3. a missing claim does not destroy a good value\n"; + +/* + * The guard has to be on emptiness, not on presence: ?? '' yields '' for a + * provider that sends "name": "", which is not the same as absent. + * + * And the FALLBACK VALUE is what actually matters. Asserting only that the + * `if` exists passes a version that keeps the branch and assigns '' inside + * it -- which is exactly the bug, with the guard still visible in the diff. + * Pin the assignment. + */ +if (preg_match('/if\s*\(\s*\'\'\s*===\s*\$display\s*\)\s*\{\s*\$display\s*=\s*\(string\)\$user->get\(\s*\'display\'\s*\)/', $s)) { + ok('an empty name claim falls back to the value already stored'); +} else { + bad('an empty or missing name claim does not fall back to the stored display name'); +} + +// Only write when something moved, so an unchanged sign-in leaves no history +// row. Pinned because the cheap version -- save() unconditionally -- looks +// identical and writes on every login. +if (preg_match('/if\s*\(\s*\$changed\s*\)\s*\{\s*\$user->save\(\)/', $s)) { + ok('saves only when a value actually changed'); +} else { + bad('saves unconditionally -- every sign-in writes a row and a history entry'); +} + +printf("\n%d passed, %d failed\n", $pass, $fail); +exit($fail === 0 ? 0 : 1);