mirror of
https://github.com/kevinveenbirkenbach/computer-playbook.git
synced 2026-11-06 10:59:59 +00:00
kevinveenbirkenbach
086bbaf8a3
495d23dbderaised an admin context with a fresh in-memory bean, which fixed hasSaveAccess() and moved the failure one guard further. User::verify_data() compares $this->id == $current_user->id, and between two unsaved beans that is '' == '', so a deployment with a single administrator takes the last-admin branch for the very account CreateUser is in the middle of creating. The branch reads the legacy $mod_strings, which this request never loads. That is why the two SuiteCRM jobs of CI run 38818071194 looked like two failures: the LDAP variant ended in ErrorException "Trying to access array offset on value of type null" at modules/Users/User.php:1678 and answered HTTP 500 to POST /login, while the SAML variant took the same verify_data() failure, avoided the 500 because $mod_strings happened to be loaded, redirected to index.php?action=Error&module=Users and left no session, so its spec timed out waiting for the SPA instead. Both are the first login of a persona that has no local row yet; administrator passes in both because it already has one. Use the stored system user, which has a real id, so hasSaveAccess() passes, restrictAdminOnlyFields() early-returns, setIsAdmin()'s reloaded bean resolves and '' == '1' skips the last-admin branch. Assign $current_user before calling getSystemUser(), not from its return value. User::retrieve() runs loadPreferences() after filling the bean, and both UserPreference::loadPreferences() and reloadPreferences() dereference $current_user->id unguarded, so retrieving while the caller's anonymous $current_user is still in place raises "Attempt to read property id on null" and answers HTTP 500 one line further on. Assigning first also means the only session preferences key written is the system user's own, never the key of the account signing in. The patch header records both failure modes, because the patch outlives this commit and is applied at image build time with -F0, so whoever reads it next when a hunk stops matching needs to know what it was for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit207cdc70ff)
70 lines
3.1 KiB
Diff
70 lines
3.1 KiB
Diff
SuiteCRM 8.10.2 tightened User::hasSaveAccess() from `return true` to
|
|
`return is_admin($current_user)` for a record without an id, and
|
|
restrictAdminOnlyFields() now forces `status = Inactive` on such a record
|
|
(release commit 6c7ce002a). CreateUser::create() runs while the browser is
|
|
still anonymous, because it is the LDAP and SAML auto-create path
|
|
(AppLdapUserProvider::createUser, AppSamlUserFactory::createUser, both via
|
|
UserHandler::createExternalAuthUser), so every first login of a directory
|
|
account ends in `RuntimeException: Not authorized`, HTTP 500 on POST /login
|
|
and no session after a SAML assertion. v8.10.1 and earlier exempted a new
|
|
record from both guards; upstream has no release and no hotfix carrying a fix.
|
|
|
|
\CreateUser is reachable only from that provisioning stack and from the
|
|
installer, so the admin context is raised here instead of relaxing the guards,
|
|
which keeps them closed for every request-driven save.
|
|
|
|
The raised context is the stored system user, never a fresh in-memory bean
|
|
carrying `is_admin = 1`. User::verify_data() compares `$this->id ==
|
|
$current_user->id`, which is `'' == ''` between two unsaved beans, so a
|
|
deployment holding a single administrator takes the last-admin branch for the
|
|
very account it is creating. That branch reads the legacy `$mod_strings`, which
|
|
this request never loads, so an LDAP login ends in `ErrorException: Trying to
|
|
access array offset on value of type null` at modules/Users/User.php:1678 and
|
|
HTTP 500 on POST /login, while a SAML assertion redirects to
|
|
`index.php?action=Error&module=Users` and leaves no session.
|
|
|
|
`$current_user` is assigned before `getSystemUser()` runs, not from its return
|
|
value. User::retrieve() calls loadPreferences() after it has filled the bean,
|
|
and both UserPreference::loadPreferences() and reloadPreferences() dereference
|
|
`$current_user->id` unguarded, so retrieving while the caller's anonymous
|
|
`$current_user` is still in place would raise `Attempt to read property "id" on
|
|
null` and answer HTTP 500 again. Assigning first makes the global the very bean
|
|
being filled, so those reads see an object throughout and the only session key
|
|
written is the system user's own.
|
|
|
|
Generated with `diff -u` against the v8.10.2 tarball and applied with `-F0`, so
|
|
the build fails instead of letting GNU patch's default fuzz factor land the
|
|
hunk somewhere upstream moved it to.
|
|
|
|
--- a/public/legacy/include/portability/User/CreateUser.php
|
|
+++ b/public/legacy/include/portability/User/CreateUser.php
|
|
@@ -68,6 +68,8 @@
|
|
*/
|
|
public function create(string $name, array $fields): ?SugarBean
|
|
{
|
|
+ global $current_user;
|
|
+
|
|
$user = BeanFactory::newBean('Users');
|
|
$user->user_name = $name;
|
|
|
|
@@ -77,7 +79,18 @@
|
|
$user->$key = $value;
|
|
}
|
|
|
|
- $user->save();
|
|
+ $callerUser = $current_user;
|
|
+
|
|
+ if (!is_admin($current_user)) {
|
|
+ $current_user = BeanFactory::newBean('Users');
|
|
+ $current_user->getSystemUser();
|
|
+ }
|
|
+
|
|
+ try {
|
|
+ $user->save();
|
|
+ } finally {
|
|
+ $current_user = $callerUser;
|
|
+ }
|
|
|
|
if (empty($user)) {
|
|
return null;
|