nexedi / erp5

The various planning box forms allows user to edit the planning box in a context user cannot edit, so it is more or less the same thing, but implemented in a different way. If I understand correctly it relies on using form_report in place of form_view

https://lab.nexedi.com/search?utf8=%E2%9C%93&search=Base_editUnrestricted&group_id=&project_id=1&search_code=true

Off-topic comment for ae489bc0 :

• When editing an existing guard, modify it instead of always recreate one. no very useful here, but that's good practice: this is nicer for the ZODB and it's easier to browse the history. BT should do the same when upgrading.

I agree for 'BT should do the same', but at the same time I also like having a copy in portal_trash, that can survive even after ZODB pack.

The problems I have with with portal trash is:

• the view is broken: ZMI on portal_trash level is unusable (too any trashes), ERP5 view on a specific trash is unusable (listbox with unstable sort, so pagination is useless). So one has to first use ERP5 UI, then switch to ZMI to actually do anything useful.

• the user who installs the bt becomes owner of everything (should be possible to fix)

• everything becomes changed/created at BT install time (may be impossible to fix)

• the history is lost anyway (may be impossible to fix)

@jm : Maybe we should suggest Jim to create a way to have persistent transparent references to historical revisions ? Ex: trash entries would just point at the actual object, just with a connection stuck in the past. Of course, non-current revision pruning becomes much more complex as we have to keep track of which revisions are being referenced... But I think this way of thinking could be very useful, including as a way to introduce lightweight time-travel (getting a consistent view of several documents at some non-packed point in the past).

Added 11 commits:

• 340e20ec...66483d11 - 8 commits from branch master
• 81de15db - Review guards on PythonScript/ExternalMethod and expose a method to check them
• dd7af1b9 - Add support for TALES guard on PythonScript/ExternalMethod
• 66b86156 - Check action guard instead of hardcoding required permission for object_view forms

@jerome comment on 340e20ec:

If we introduce a guard on Base_edit, this guard will cause an Unauthorized error in the case where user submits the form but no longer have Modify portal content permission and this part will no longer be executed to handle that case gracefully.

I don't understand the purpose of checking 'Modify portal content' permission inside Base_edit. Isn't it done automatically when calling the edit method ? IOW, "this part" looks useless.

But you're right that the guard would break Base_edit when called with silent_mode=1. Maybe this can be fixed by renaming Base_edit, a create a new Base_edit that is just a wrapper around the old one, with 'Modify portal content' guard.

I rebased the branch and amended the first commit, due to a regression when checking roles/permissions. The fix also fixes an already existing bug:

Fix role/permission checking for ExternalMethod called by a PythonScript with proxy roles.

@jerome, you also mention Base_editUnrestricted but if we remove the explicit permission check inside Base_edit (as suggested in a previous comment), what would be the difference between the 2 scripts (apart from the guard) ?

I don't understand the purpose of checking 'Modify portal content' permission inside Base_edit. Isn't it done automatically when calling the edit method ? IOW, "this part" looks useless.

It is mostly to show user a nice portal_status_message instead of a scary error page.

In theory it should not happen often, only when user use go back button after validating and loosing modify permission, or when another user change state of document while user is edition. I just grepped the logs of a production instance and found one case happened today.

Hi, what is Base_editUnrestricted (its existence sounds scarry, I would like to remove it)

I would like to remind some rules:

• we should never pack an ERP5 system (if we have to pack it, it is a bug or it is the result of not improving what should be improved at the core)
• if some part of ERP5 is not nice enough, it should be improved rather than removed (ex. portal trash)

About portal trashes and other tools that still use ZMI, our direction should be to migrate them progressively to ERP5 only UI and ERP5ish tools.

It is mostly to show user a nice portal_status_message instead of a scary error page.

I thought the same thing about guards. It currently raises Forbidden with a generic message. I was tempted to change some project scripts in which I implemented something similar to guards at the beginning (plus a check_security boolean parameter), but with much nicer messages. It looks like Guards would still be limited for such use.

Hi, what is Base_editUnrestricted (its existence sounds scarry, I would like to remove it)

Base_editUnrestricted is used for planning boxs, when user does not have Modify portal content permission on the context. It used for exemple in Project_viewPlanning , where user can edit tasks related to a project even without modify permission on the project, as long as user have permissions to modify the tasks.

The difference with Base_edit is just that it does not check that user has permission on the context and it does not call edit on context, it just uses "editor" from fields (listbox and planningbox have "editors" to edit other documents than context). In case of planningbox editor, edit will be used and this will raise Unauthorized when trying to modify properties for which users do not have "write permission".

So the problem with this script is just its name.

And yes, Base_edit and Base_editUnrestricted are very similar and can probably merged, also the form_report (see 9ad43681 ) case of always showing edit button can probably handled with the same approach than the one discussed here.

I would like to know a real use case of editing document without 'Modify portal content'. Can you tell us your case @jm?

In a project, we have Sale Orders where most properties should be only editable in Draft state (to give an idea: 69 fields in SaleOrder_view). This part is controlled by workflow permissions as usual.

But some specific properties has to be editable as long as the order is not completed. For example, a property that control whether some transition for related Sale Packing List should be done automatically or not (this one is controlled by 2 workflow actions to enable/disable). That can also be notification messages that are sent at different steps of the order lifetime. Or simply the order title or internal comments.

So far, we have an action opening a dialog that compiles all those specific properties. Currently, it contains 8 fields.

Now, we've requested to add a new tab for an extended notification mechanism. It would contain 3 fields. I don't want to again waste time by creating a dialog for this.

Thanks. I understood. I had a similar case in my project. At that time, I created a dedicated portal type(let's say "user changement document") to let user edit and once user finishhed to edit this document, then ERP5 automatically copy values from this "user changement document" to a read-only document that user wanted to modify. This way, end user only need to edit editable documents. So I did not need any custom Base_edit.

@xiaowu.zhang should be able to show you this implementation as an example if needed.

It is mostly to show user a nice portal_status_message instead of a scary error page.

Romain told me that an even better solution would to act as if a field does not validate, so that the user does not lose what is submitted. This would also apply to dialogs.

Added 1 commit:

• d2a6d69e - Check action guard instead of hardcoding required permission for object_view forms

Amended last commit for ERP5Document_getHateoas.

The first commit only contains bugfixes so I won't wait to push it to master.

Romain told me that an even better solution would to act as if a field does not validate, so that the user does not lose what is submitted. This would also apply to dialogs.

For this, we could inspect the callable on execution, see if it expects a _guard_result boolean argument, and fill it automatically with the result of the guard check, instead of raising. This way, the script can customize how to fail.

Can't we just let the exception propagate here ?

At least, we want to catch Unauthorized if the security is managed by AccessControl (case of a method) rather than a Guard. Apart from that, yes, it looks better not to mask errors.

Added 29 commits:

• d2a6d69e...65d893ea - 26 commits from branch master
• e73183d4 - Special _guard_result parameter on Guard'ed callables for custom behaviour if unauthorized
• 97903feb - Add support for TALES guard on PythonScript/ExternalMethod
• 5d82e02b - Check action guard instead of hardcoding required permission for object_view forms

After some clean up to Guards and External Methods, that is now pushed to master, I rebased this work and implemented what I wrote above about a special _guard_result boolean argument.

This MR does not change anymore the behaviour of Base_edit: any improvement and refactoring discussed about this script and its friends remains possible but it's off-topic.

For me, it's ready to be merged, in that it's enough for my project.

I missed the case of scripts that call Base_edit and are used as form actions. An example is DCWorkflow_edit. I'd have to duplicate the Guard in such scripts :( Maybe extend Guards with delegation.

Added 7 commits:

• 5d82e02b...1bf56d7d - 4 commits from branch master
• a778f71a - Special _guard_result parameter on Guard'ed callables for custom behaviour if unauthorized
• 381dfe20 - Add support for TALES guard on PythonScript/ExternalMethod
• 416fe866 - Check action guard instead of hardcoding required permission for object_view forms

Added 6 commits:

• 88d772fd - 1 commit from branch master
• e9a5a618 - Special _guard_result parameter on Guard'ed callables for custom behaviour if unauthorized
• 58ee703c - Add support for TALES guard on PythonScript/ExternalMethod
• 3e2d4e02 - ERP5Type: new Guard class for PythonScript/ExternalMethod
• e467d76c - Check action guard instead of hardcoding required permission for object_view forms
• 986ca810 - wip

I tried to continue a little and pushed all I've done so far, but I may give up the idea of using guards for this. I start to think they'll never replace elegantly what I've already done a few times in some project scripts.

Instead of checking guards, the script would be called with a special parameter (e.g. _check_security) set to 1. If the script does not take such parameter (inspection is easy), it would instead check Modify portal content permission, at least for the moment, to avoid having to review all existing scripts. From the above commits, I'd only keep a small part of e467d76c81ed7361a87421fbec9c8e1dfe0f1db1.

I must stop working on this as soon as possible and this other solution is very quick to implement.

In fact, I'm even tempted to remove Guards from PythonScripts/ExternalMethods, and in this direction at least I would not extend them by adding TALES support. Currently, they're mostly used for:

• init scripts, which are anyway a bad idea to begin with (interaction workflows would be better)
• and *_afterClone scripts (no opinion about them yet)

There remain:

Manager                         erp5_administration/Base_cleanupRolesAndUsersTable
Owner                           erp5_banking_cash/MoneyDeposit_statSourceCredit
Owner                           erp5_base/Delivery_generateReference
Owner                           erp5_mrp/ProductionDelivery_generateReference
View                            erp5_simulation/Delivery_getSolverProcess
Modify portal content           erp5_trade/Order_applyTradeCondition
Assignor;Auditor;Manager;Owner  erp5_web_ung_theme/WebPage_shareDocument

With a good API to check security, that would be enough to write 2 lines of Python at the beginning of the few scripts listed above.

Added 1 commit:

• fb953b8d - For object_view forms, test action by calling it with a special parameter instea…

Added 4 commits:

• fb953b8d...8c90e61c - 3 commits from branch master
• 23d8edc5 - For object_view forms, test action by calling it with a special parameter instea…