runTransitionOnElementValue applies access checks

Created on 5 September 2022, about 2 years ago
Updated 15 February 2023, over 1 year ago

Please excuse a little rant - this is the second patch I've submitted that's dealing with the same basic problem.

Access checks shouldn't be done in runTransitionOnElementValue for multiple reasons:

1) The access checks here are hard coded and force a person implementing a programmatic change to a workflow state to either copy and modify the code or patch it.

2) Access checks should be done as part of route access checking anyway. This allows menu items that shouldn't be accessible to a user to be hidden and access prevented, rather than requiring additional code to say "Sorry. We showed you this menu item but you can't actually use it." It also makes the code better structured and more maintainable.

Submitting a patch that removes the access check here, thanks.

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇦🇺Australia Nigel Cunningham Geelong

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • (1) Fully agree with needing this ability, was not envisioned as part of writing this. See below proposed approach.

    Re: (2) - access checks are not just routing. They also need to be done on the element level, and the transition option level, for webforms generally and for this module to provide a good UI for selecting available transitions. I know in the other issue you raised (now resolved) it could be done in routing, but here the user can have full access to the route, but not necessarily access to every workflow element, and for each element they do have access to, not necessarily access to every transition. It needs to be (able to be) checked at some point separately from routing.

    Currently this check in runTransitionOnElementValue prevents someone programmatically running a transition via this helper function that the user does not have access to, as you say. We can easily make that an optional access check, to provide an easy way to safely run the transition, or 'unsafely' run it programmatically if needed.

    So I think the simplest compromise right now is to add an optional parameter to the function, $access_check, which is by analogy to how entity queries can disable the access check. Here, it will default to checking access, to avoid unintended consequences, but allow overriding.

    If at some point we identify every possible case where runTransitionOnElementValue is used and ensure there is an access check of some sort in front of it, I think it's still good DX to have an optional access check in this function for doing things programatically, so we could just change the default for $access_check to false.

    Anyway. I've added $access_check in the latest dev for now. Happy to discuss as I'm sure I've missed something, development of this module is in fits and starts due to my workload and my planned approaches may have drifted off a bit.

  • Status changed to Needs review over 1 year ago
  • Status changed to Fixed about 2 months ago
  • $access_check provided as optional parameter. Did not apply merge request for reasons stated, and this approach covers off the first use case.

    2) Access checks should be done as part of route access checking anyway. This allows menu items that shouldn't be accessible to a user to be hidden and access prevented, rather than requiring additional code to say "Sorry. We showed you this menu item but you can't actually use it." It also makes the code better structured and more maintainable.

    As mentioned we need to be able to access check at different points, not just at the routing level. Without this approach, it'll end up less maintainable as we'd need to repeat the access check logic across functions I think.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024