fix: harden Agent-level access control (allow_guest/allowed_users/allowed_roles) - #597
Merged
Merged
Conversation
… v15 (#591) frappe.tests.IntegrationTestCase and UnitTestCase are v16/develop-only (renamed from FrappeTestCase in frappe PR #27992) and were never backported to version-15. 40+ huf test files import them, crashing bench run-tests --app huf at collection time on any v15 bench. Wrapped in try/except ImportError to match the surrounding init-time side effects in this file, so a future frappe release that removes FrappeTestCase entirely fails soft instead of breaking app import. Co-authored-by: Safwan Erooth <talktoartai@gmail.com>
…owed_roles) Implements the Phase 1/2 fixes from Tracks/AgentPermissionsAudit/AGENT_PERMISSIONS_AUDIT.md. Adds a single shared helper (huf/ai/agent_access.py) for "can user X run/access Agent Y", replacing duplicated and divergent logic in Agent.has_permission() and agent_integration._is_user_allowed(). Closes real bypasses: - Gateway-routed agent runs (Slack/Discord/Teams/Telegram/webhook) and the ElevenLabs voice webhook previously ran under a shared service identity, bypassing allow_guest/allowed_users/allowed_roles entirely. Both now require allow_guest, since no external-sender-to-HUF-user mapping exists. A backward- compat migration (huf/patches/v1/preserve_gateway_agent_access.py) preserves existing gateway integrations by backfilling allow_guest on already-bound agents; ElevenLabs needs the same done manually per-agent (see doc, PR #584 touches this same file and needs reconciling). - Seven agent_chat.py endpoints (get_history, create_conversation, add_message, uploads, model override) had no agent-access check at all. get_history and add_message additionally now require conversation ownership (or System Manager), not just agent-access, since being allowed to run an unrestricted agent must not let another user read or write into someone else's conversation with it. - conversation_fork.py forking never checked the source agent's restrictions. Also: Agent.validate() warns (non-blocking) on dangling/disabled allowlist references; allow_guest's frontend type fixed from number to boolean with all call sites updated; Permissions tab copy rewritten to state the "both lists empty = everyone" rule and the guest/external-channel semantics; unbounded user/role picker fetch capped from 1000 to 200 pending a real search endpoint; new unit tests for the shared helper (no live bench required). Not done in this pass: live-bench test execution (required before merge), F13/F14/F17/3.3/3.4/2.2 Phase 3 items, un-skipping test_seed_fk.py's quarantined class. See AGENT_PERMISSIONS_AUDIT.md section 8 for full status.
…e-bench testing Live-bench verification (bench --site ... run-tests --app huf) surfaced a real regression: agent_integration.py, agent_chat.py, chat_api.py, and conversation_fork.py called assert_agent_access(agent_doc) relying on its internal frappe.session.user resolution. That resolution uses huf.ai.agent_access's own frappe import, which is NOT the same object a test patches when it does @patch("huf.ai.agent_integration.frappe") (or similar) -- so tests mocking the caller module's frappe reference silently stopped exercising the guest/session-user branch, breaking test_get_agent_run_status_denies_guest_for_private_agent. Fixed by passing user=frappe.session.user explicitly at every call site, using each module's own (possibly-mocked-in-tests) frappe reference, rather than relying on assert_agent_access's implicit fallback. Also updated three conversation_fork tests and one gateway_service test whose mocks pre-dated the new access checks: test_conversation_fork.py's agent_doc fixtures now set .owner to match the mocked session user (these tests are "user forks their own conversation" scenarios, so the owner bypass is the correct path), and test_gateway_service.py's frappe.get_doc mock sequence now accounts for the new Agent lookup the F1 fix added. Full suite run on a live bench: 790 tests, 0 failures, 54 errors -- all 54 are the pre-existing, documented frappe.flags.currently_saving test-isolation flake (RegressionCI backlog), confirmed by tracing every error's traceback: none touch agent_access.py, agent_chat.py, gateway_service.py, elevenlabs_convai_api.py, conversation_fork.py, agent_integration.py, or chat_api.py logic. Also manually verified via bench console: owner/System Manager/allowlisted-user access, non-allowlisted-user and Guest denial, allow_guest=1 public access, empty-allowlist-allows-authenticated-users (but not Guest) semantics, and Agent.has_permission() now correctly denying a non-allowlisted user read access.
…F17, 3.3, 2.3) F13/3.2 - Document the intentional config-edit vs execution-access permission split (agent_config_api.py module docstring), including the read/write asymmetry: get_agent_section (read) is allowlist-gated via check_agent_access, same as execution; only update_agent_section (write) requires the agent.edit capability. Added tests pinning both directions. F14/3.1 - Moved six access/capability fields (conversation_data_api_permission, allow_code_execution, execution_profile, execution_shared_dir_limit_mb, allow_ssh, ssh_connections) from AdvancedTab.tsx into three new Cards on PermissionsTab.tsx, organized alongside the existing Access Control card. Unrelated sibling fields (enable_conversation_data, inject_conversation_data, max_context_chars) stay in AdvancedTab. Fixed a hardcoded #advanced hash in the "New" button return-navigation for execution profiles/SSH connections so it now follows showTab instead of always bouncing back to Advanced. F17/3.5 - Surface the document owner's standing access exception explicitly: added "owner" to the Agent config API's general section (read-only field, already protected in READ_ONLY_FIELDS -- confirmed live that attempting to write it via update_agent_section is rejected) and a permanent disclosure line on the Permissions tab naming the actual owner when known. 3.3 - Fixed the unbounded/unscoped user+role picker (consulted separately on the design): added searchUsers/searchRoles/fetchUsersByName/fetchRolesByName to agentApi.ts (service-layer, matching repo convention), moved the fetch out of AgentFormPage's eager mount effect into PermissionsTab itself (lazy -- only fetches when the tab is actually opened, since Radix Tabs unmounts inactive content), added debounced (250ms) server-side search-as-you-type for the Allowed Users picker via new optional searchValue/onSearchChange props on MultiSelectCombobox (strictly additive, the two other call sites of this shared component are unaffected), and seed currently-selected users/roles into the option set so an admin editing a restricted agent never sees their existing picks vanish or get silently dropped. 2.3 - Mirrored the settled Guest semantic (allow_guest gates alone, allowlists never apply to Guest) into the Agent DocType's own field labels/descriptions in agent.json, not just the frontend copy shipped in the prior commit. Verified: yarn typecheck passes cleanly across all touched frontend files. Live bench: full backend suite re-run, 0 failures (53 pre-existing/unrelated currently_saving flake errors, same baseline as before). Caught and fixed two real issues during live verification: F13's new tests patched the wrong mock target (has_capability is a function-local import, not a module attribute -- fixed to patch huf.permissions.has_capability where it's actually defined), and the test's second assertion incorrectly assumed get_agent_section was capability-gated when it's actually allowlist-gated like execution (only update_agent_section requires agent.edit) -- fixed both the test and the docstring to state this asymmetry precisely. Manually confirmed via bench console that "owner" flows through the general section and is genuinely rejected on write.
… fixes onto agent_access.py PR #597's assert_agent_access/check_agent_access unify allow_guest/allowed_users/ allowed_roles enforcement, but branched before #411 landed three narrower fixes that #597 doesn't otherwise cover: - Agent-name enumeration oracle: run_agent_sync/run_agent_stream/run_agent_sync_chat loaded the Agent doc before checking allow_guest, so a Guest caller could tell "no such agent" (DoesNotExistError) from "exists but guests aren't allowed" (PermissionError) apart by exception type. Now both collapse to one generic PermissionError for Guest callers. Implemented inline per call site (not via a new agent_access.py helper) because agent_access.py has its own `frappe` import distinct from each caller module's — a shared helper doing frappe.db.get_value there would bypass the @patch("huf.ai.agent_integration.frappe")-style mocks these call sites' existing tests rely on (the same class of bug #597's own 7646fe7 fixed for assert_agent_access). - agent.use capability (RBAC) was never consulted on these three entry points; now enforced for all non-Guest callers. - Guest callers could pass provider/model overrides that were resolved (and thus billed against site API keys) before the guest check ran; now nulled for Guest before resolution. - ConversationManager.get_or_create_conversation trusted a caller-supplied conversation_id with no ownership check, letting any authenticated user (or Guest on a guest agent) read/append to another user's conversation. Now requires conversation.agent match plus owner/session_id/chat.view_all, and a missing id and an inaccessible id fail identically (no existence oracle). Adds ai/tests/test_conversation_manager_access.py (pure-mock unit tests, no live bench required) covering the new ownership branch, mirroring test_agent_access.py's style since no test previously existed for this method. Supersedes #411 (SEC-gap1-fix), which will be closed in favor of this branch.
- Updated get_permission_query_conditions to return is_system = 0 for users with these capabilities, ensuring agents appear in the list view. - Updated has_permission(read) to allow document access for users with these capabilities, properly separating configuration access from execution access.
- Added a document event hook for the native Frappe User doctype. - Bound the on_update event to sync_from_frappe_user to ensure that manual role changes in the Frappe backend trigger a sync back to HUF.
- Updated create_huf_roles() to define standard weights for default system roles (Huf Admin=100, Huf Manager=80, Huf User=50, Huf Viewer=10). - Modified the idempotent role creation logic to ensure that existing Huf Roles in the database are properly patched with their new weights during migrations.
- Added sync_from_frappe_user() to automatically update a user's Huf User Role document when their native Frappe User roles change. - Implemented loop-protection (frappe.flags.syncing_huf_roles) to safely prevent infinite recursion during the two-way sync. - Removed hardcoded role ranking and replaced it with a dynamic database query that resolves multi-role conflicts by selecting the Huf Role with the highest role_weight.
…ld for integration credential doctype
- Fixed a critical, silent data-loss bug where modifying Code Execution, SSH Execution, or Conversation Data API Permissions from the Permissions tab would not save. - Moved these fields from the advanced section to the permissions section in both the backend AGENT_SECTIONS router and frontend tabConfig to match the UI layout. - Fixed an issue where Conversation Data Access would disappear on a hard refresh of the Permissions tab. - Added enable_conversation_data to the Permissions section payloads so the React form safely tracks its default state and conditionally renders dependent UI components without requiring a visit to the Advanced tab.
Sanjusha-tridz
marked this pull request as ready for review
August 14, 2026 18:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the Phase 1/2 fixes from the Agent Permissions audit, plus resolutions to all 9 open questions from that audit (dates/enumerability/caching verified in code, product intent confirmed for
allow_guest).Adds a single shared helper
huf/ai/agent_access.py(check_agent_access/assert_agent_access) — the one place that now decides "can user X run/access Agent Y" — replacing duplicated, divergent logic previously inAgent.has_permission()andagent_integration._is_user_allowed().Bypasses closed
gateway.execution_user/ignore_permissions=True), completely bypassingallow_guest/allowed_users/allowed_roles. Both now requireallow_guest, since no mechanism maps an external sender to a specific HUF user. A backward-compat migration (huf/patches/v1/preserve_gateway_agent_access.py) backfillsallow_guest=1on agents already bound to a live Gateway Binding so existing integrations don't silently break — ElevenLabs-bound agents need the same flag set manually (no auto-migration there, see coordination note below).agent_chat.pyendpoints (get_history,create_conversation,set_conversation_model_override,add_message, three upload endpoints) had no agent-access check at all.get_history/add_messageadditionally now require conversation ownership (or System Manager) — being allowed to run an unrestricted agent must not let another user read or write into someone else's conversation with it.conversation_fork.pyforking never checked the source agent's restrictions (confirmed_can_forkalone was already safe on ownership grounds — this is defense-in-depth).Other changes
Agent.validate()warns (non-blocking) on dangling/disabled allowlist references — deliberately non-blocking so pre-existing dangling data doesn't make unrelated edits unsaveable.allow_guestfrontend type fixed fromnumbertoboolean; every=== 1/=== 0comparison site updated.limit: 1000user/role picker fetch capped to200pending a real search endpoint (still exposes N emails to anyone who can open the Agent form — flagged as a follow-up, not fully fixed).huf/ai/tests/test_agent_access.py, mocked, no live bench required).✅ Live-bench verification (done)
Provisioned a disposable bench off this branch (
bench --site <site> run-tests --app huf) and:test_agent_access.py(9 cases): pass.frappe.flags.currently_savingtest-isolation flake (RegressionCI backlog), confirmed by inspecting every traceback: none touch this PR's files.test_get_agent_run_status_denies_guest_for_private_agentfailed becauseassert_agent_access(agent_doc)'s implicitfrappe.session.userresolution usesagent_access.py's ownfrappeimport, which isn't the object a test patches via@patch("huf.ai.agent_integration.frappe"). Fixed by passinguser=frappe.session.userexplicitly at every call site (using each module's own frappe reference) — see the second commit. Also updated 3conversation_forktest fixtures and 1gateway_servicetest mock sequence that pre-dated the new checks.bench consoleagainst real persistedAgentdocuments: owner/System Manager/allowlisted-user allowed, non-allowlisted-user and Guest denied on a restricted agent,allow_guest=1public access works, empty-allowlist-allows-authenticated-but-not-Guest semantics hold, andAgent.has_permission('read')now correctly denies a non-allowlisted user.Still recommend a reviewer's own pass before merge — this was a disposable single-developer bench, not CI, and doesn't cover the frontend copy/type changes or a real gateway/webhook integration end-to-end.
Phase 3 items (F13, F14, F17, 3.3, 2.3) — also included in this PR
agent_config_api.py's module docstring, including a real asymmetry the docs didn't previously state:get_agent_section(read) is allowlist-gated (check_agent_access), same as execution — onlyupdate_agent_section(write) requires theagent.editcapability. Added two tests pinning both directions.conversation_data_api_permission,allow_code_execution,execution_profile,execution_shared_dir_limit_mb,allow_ssh,ssh_connections) fromAdvancedTab.tsxinto three new Cards onPermissionsTab.tsx. Sibling non-security fields (enable_conversation_data,inject_conversation_data,max_context_chars) stay in Advanced. Also fixed a hardcoded#advancedhash in the "New Execution Profile"/"New SSH Connection" return-navigation that would have bounced users back to the wrong tab now that those buttons live on Permissions.owneradded to the Agent config API's read-onlygeneralsection, and a permanent disclosure line on the Permissions tab names the actual owner when known.searchUsers/searchRoles/fetchUsersByName/fetchRolesByNameto the service layer, moved the fetch to lazy-on-tab-open (was eager on every page mount), added debounced server-side search-as-you-type for the Allowed Users picker via new optional (strictly backward-compatible) props on the sharedMultiSelectCombobox, and seeded currently-selected users/roles so existing picks never silently vanish.agent.json(previously only the frontend copy was updated).Live-bench verification of Phase 3:
yarn typecheckpasses cleanly across all touched frontend files. Full backend suite re-run: 0 failures, same pre-existing/unrelated error baseline. This pass caught two real bugs before push: F13's tests initially patched the wrong mock target forhas_capability(it's a function-local import, not a module attribute) and incorrectly assumedget_agent_sectionwas capability-gated when it's actually allowlist-gated — both fixed, plus the docstring corrected to state the read/write asymmetry precisely. Manually confirmed viabench consolethatowneris genuinely rejected on write throughupdate_agent_section.huf/ai/providers/elevenlabs_convai_api.py, including the exactignore_permissions=Trueline this PR adds a check in front of. Whoever merges second needs to reconcile.Agent.has_permission()now requiresallow_guest=1for Guest instead of the old "empty allowlists = anyone including Guest" bug — any Guest-facing surface relying on the old behavior will start getting denied.Full findings, severity ratings, OQ resolutions, and what's explicitly not done in this pass are in
AGENT_PERMISSIONS_AUDIT.md§7-8 (linked above).Test plan
bench --site <site> run-tests --app huf --module huf.ai.tests.test_agent_access— 9/9 passbench --site <site> run-tests --app huf— 0 failures, 54 pre-existing/unrelated errorspreserve_gateway_agent_accessbackfillsallow_guest=1correctly for agents already bound to an enabled Gateway Bindingallowed_users-only) agent is unreachable via gateway-equivalent (Guest) access when the calling identity isn't in the allowlistget_history/add_messagedeny a non-owner on someone else's conversation with an unrestricted agent — logic verified viacheck_agent_access/ownership-check code review and the endpoint-level implementation, but not exercised end-to-end over the actual whitelisted API in this pass