Enforce Flow Model permission at runtime - #62
Conversation
Confidence Score: 4/5Safe to merge; the change is narrow and well-tested, though background-job user context deserves a quick sanity check. The guard is correct and the tests are solid, but flow/flow/doctype/flow_agent/flow_agent.py — confirm Reviews (1): Last reviewed commit: "feat: enforce Flow Model read permission..." | Re-trigger Greptile |
| if not frappe.has_permission("Flow Model", "read", model_name): | ||
| frappe.throw( | ||
| _("You are not permitted to use Flow Model {0}.").format(model_name), | ||
| frappe.PermissionError, | ||
| title=_("Model Not Permitted"), | ||
| ) |
There was a problem hiding this comment.
Permission check bypassed in background jobs —
frappe.has_permission reads frappe.session.user, so if any caller of assemble() runs as Administrator in a queue worker (common in Frappe background jobs), the check always passes and the User Permission restriction is silently skipped. Verify that start_run and any enqueue call preserves or re-sets the originating user before this gate can be trusted end-to-end.
There was a problem hiding this comment.
Routing a run through the scheduler requires creating/editing a Flow Trigger, which is System Manager-only. A restricted (non-admin) user can't do it. If they could, they'd be an admin and the restriction is moot anyway.
There was a problem hiding this comment.
That's a fair point — if Flow Trigger creation/editing is gated to System Manager, then the only path to a scheduler-triggered background job (where session user might be Administrator) is already admin-only, making the restriction self-reinforcing.
The remaining question is whether start_run called directly (via HTTP/API by a restricted user) ever enqueues a background job that calls assemble() inside the worker. If assemble() is always called synchronously in the web worker before any enqueueing, frappe.session.user is correct and the check is sound. If it's called inside an enqueued job, the session user would be whatever the worker runs as.
Do you want me to look at start_run to confirm?
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Closes #59
Model selection was never permission-checked at runtime —
assemble()only checkedenabled, so any user could pass any model name tostart_runregardless of User Permission restrictions. Admins can now restrict which Flow Models a user can run via standard Frappe User Permissions, and this is enforced where every session/run resolves its model.