N°9723 - IsActionAllowed should work with non instantiated users objects

[bdalsass]

Base information

Question	Answer
Related to a SourceForge thread / Another PR / A GitHub Issue / Combodo ticket?	https://support.combodo.com/pages/UI.php?operation=details&class=Bug&id=9723&c[menu]=SearchBugs
Type of change?	Bug fix

Symptom (bug) / Objective (enhancement)

Infinite loop when user try to reset his password

Reproduction procedure (bug)

reset password

Checklist before requesting a review

• I have performed a self-review of my code
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't
• Is the PR clear and detailed enough so anyone can understand without digging in the code?

[Copilot]

Pull request overview

Fixes password-reset recursion by loading profiles from the database when only unrelated user fields have changed.

Changes:

• Detects changes specifically to profile_list.
• Uses in-memory profiles only when that list was modified.

---

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

[greptile-apps]

Greptile Summary

This PR fixes an infinite loop during password reset by narrowing the condition in ListProfiles from "user object has no in-memory changes at all" to "the profile_list attribute specifically is not among the changes". When a user object had any dirty attribute (e.g. a password field), the old broad check forced execution into the else branch, which called $oUser->Get('profile_list'), triggering permission checks that recursively re-entered ListProfiles.

• The condition in ListProfiles is changed from count($oUser->ListChanges()) === 0 to !array_key_exists('profile_list', $oUser->ListChanges()), so users with non-profile changes now safely use the DBObjectSearch / AllowAllData path instead of the in-memory Get path.
• The sibling method GetProfileList (line 914) and GetUserActionGrant (lines 722, 756) retain the old count($oUser->ListChanges()) === 0 pattern; those are used for caching decisions only, so no correctness issue is introduced, but cache hits are still skipped for any modified user object even when profile_list is untouched."

Confidence Score: 4/5

The fix is minimal, well-scoped, and directly addresses the reported infinite loop without introducing new logic branches or touching unrelated code paths.

The change correctly identifies that the original broad guard was causing re-entrant permission checks for any dirty user object. The narrower attribute-specific check is the right fix. The only remaining concern is that GetProfileList keeps the old broad guard for caching, meaning per-request profile lookups will hit the DB more than necessary for users with non-profile changes, but this does not affect correctness.

The GetProfileList method at the bottom of the same file uses the same old broad guard for cache population — aligning it with the new check would prevent redundant DB queries on the same code path this PR fixes.

Important Files Changed

Filename	Overview
addons/userrights/userrightsprofile.class.inc.php	Fixes infinite loop in ListProfiles by narrowing the condition from 'user has no changes at all' to 'profile_list attribute is not in changes', allowing the safe DB path when only non-profile attributes are modified.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[IsActionAllowed called\nfor modified User object] --> B[GetUserActionGrant]
    B --> C[GetProfileList]
    C --> D[UserRights::ListProfiles]

    subgraph OLD ["BEFORE fix (infinite loop)"]
        D1{"count(ListChanges()) === 0?"}
        D1 -- "false: user has ANY change" --> E1["oUser->Get('profile_list')\npermission check triggered"]
        E1 --> A
    end

    subgraph NEW ["AFTER fix (resolved)"]
        D2{"array_key_exists('profile_list', ListChanges())?"}
        D2 -- "false: profile_list not changed" --> E2["DBObjectSearch with AllowAllData\n(no permission check, no recursion)"]
        D2 -- "true: profile_list modified" --> E3["oUser->Get('profile_list')\n(use in-memory data)"]
    end

    D --> D1
    D --> D2

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[IsActionAllowed called\nfor modified User object] --> B[GetUserActionGrant]
    B --> C[GetProfileList]
    C --> D[UserRights::ListProfiles]

    subgraph OLD ["BEFORE fix (infinite loop)"]
        D1{"count(ListChanges()) === 0?"}
        D1 -- "false: user has ANY change" --> E1["oUser->Get('profile_list')\npermission check triggered"]
        E1 --> A
    end

    subgraph NEW ["AFTER fix (resolved)"]
        D2{"array_key_exists('profile_list', ListChanges())?"}
        D2 -- "false: profile_list not changed" --> E2["DBObjectSearch with AllowAllData\n(no permission check, no recursion)"]
        D2 -- "true: profile_list modified" --> E3["oUser->Get('profile_list')\n(use in-memory data)"]
    end

    D --> D1
    D --> D2

Loading

Comments Outside Diff (1)

1. addons/userrights/userrightsprofile.class.inc.php, line 912-923 (link)

   GetProfileList caching still bypassed on any change

   GetProfileList uses the same broad count($oUser->ListChanges()) === 0 guard to decide whether to populate aUsersProfilesList. For a user with any non-profile_list change (e.g. password reset, the exact scenario this PR fixes), GetProfileList will skip the cache and call UserRights::ListProfiles on every invocation. Because ListProfiles now correctly takes the DB path in this case, there's no correctness problem — but each call to GetUserActionGrant for the same user/class/action will re-query the database rather than hitting aUsersProfilesList. Aligning this guard with the same array_key_exists('profile_list', $oUser->ListChanges()) check used in ListProfiles would make caching consistent and avoid unnecessary repeated queries during a single request.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

_{Reviews (1): Last reviewed commit: "N°9723 - IsActionAllowed should work wit..." | Re-trigger Greptile}