erp5_base: do not clone logins when cloning persons

bt5/erp5_base/SkinTemplateItem/portal_skins/erp5_base/Person_afterClone.py

 """Hook called when a person object is closed.
 
-We want to reset reference, which is the user login in ERP5Security.
+We want to reset reference, which is the user login in the old ERP5Security.
+We don't want neither to clone the Logins of the user.
 One exception is when a person object is installed from business template.
 """
 context.setUserId(None)
+context.manage_delObjects(ids=[
+  document.getId() for document in context.objectValues(
+    portal_type=context.getPortalObject().getPortalLoginTypeList()
+  )
+])
+
 context.Person_initUserId()
 if not context.REQUEST.get('is_business_template_installation', 0):
   context.setReference(None)

[Vincent Pelletier]

Why ?

After cloning, the login is supposed to be left in "draft" state, and as a result it does not prevent cloning.

More generally, cloning is cloning: we should preserve as much as possible for cloning to be meaningful. If one wants an empty instance, they should create a new one rather than clone an existing one.

[Nicolas Wavrant]

Because of the set of Permissions on Logins, the only way to set a login and validate it on a person is to use the workflow transition from "user_account_workflow" which makes use of the correct proxy roles (otherwise people with access to logins can't set password, and user who can set passwords cannot validate it). In that case, cloning logins that won't be used neither deleted doesn't make sense.

Also, reusing a login is a bad idea. With the "old" ERP5Security, this script was already reseting the reference to not have duplicated logins that will need to be changed absolutely by the user who clones.

Finally, this constaint on logins (https://lab.nexedi.com/nexedi/erp5/blob/master/bt5/erp5_base/PropertySheetTemplateItem/portal_property_sheets/LoginConstraint/check_duplicate_constraint.xml) make the Person document inconsistent. Indeed, the check applies also to draft logins (that's good), but after cloning a person, when the user wants to validate, the draft login is inconsistent (the login is already used by the original person), so the person document is inconsistent too, so it is impossible to validate.

[Vincent Pelletier]

In that case, cloning logins that won't be used neither deleted doesn't make sense.

Then is it a good idea to clone to begin with ?

Also, reusing a login is a bad idea.

Imprecise.

Having multiple active logins with a common value is beyond the point of "bad idea", it is plain bad.

But having multiple stand-by logins with the same value is not an issue at all, it is by design.

With the "old" ERP5Security, this script was already reseting the reference to not have duplicated logins

"Old" ERP5Security has no way to disable a login, so it had no choice but to get in the way of clone. This is not true with "new" ERP5Security.

Also, "old" ERP5Security used this value as user id (where duplication is equaly bad as login duplication), and as customer-chosen reference (where duplication is not as critical, but can lead to confusion).

that will need to be changed absolutely by the user who clones

Nothing is "absolutely" needed on cloned ERP5 Login: it works straight out of clone without endangering site consistency.

Indeed, the check applies also to draft logins (that's good)

No, that's bad ! Draft must be ignored. To me, this is exactly the wrong way to use constraints: constraints should be executable anytime and not find false problems. Here, we want to check values from a draft document because we expect the constraint to only be run during "validate" transition. This is bogus.

[Julien Muchembled]

In that case, cloning logins that won't be used neither deleted doesn't make sense.

Then is it a good idea to clone to begin with ?

Yes, it is. It saves time to not fill many fields that are identical between Persons. Actually, I do this on a prod instance, not to recreate the same assignments every time.

It is a common pattern,anywhere, to clone everything and just have to change the few things that differ, rather than rebuilding something from scratch. Some devs abuse by not refactoring after copy-paste but that another problem.

On this particular commit, I don't have opinion. Just a remark: if everyone is going to delete (or change completely) login subobjects after clone, then we can consider making it a default behaviour.

[Vincent Pelletier]

Yes, it is. It saves time to not fill many fields that are identical between Persons. Actually, I do this on a prod instance, not to recreate the same assignments every time.

I agree, although I feel this use-case is different from the one being targetted by this change: setting up assignments requires a level of permissions significant to the user who is cloning, so such user would be capable of editing the cloned ERP5 Login as well, and inability to do so seems to be the rationale for deleting that document (plus the constraint issue, which to me fully resides on the constraint side).

Personally, when (and it's rare these days) I have to create a user, I clone a similar user and edit the login, then validate ERP5 Login, Assignment and Person. I think it is faster than having to create a new ERP5 Login. But again, this is from the point of view of a Manager user.

[Nicolas Wavrant]

Having a draft ERP5 Login for me only makes sense if normal users can update it and validate it. But to change a password the permission "Set Own Password" is required (https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/mixin/encrypted_password.py#L75), and this permission is only attributed to "Manager". So as long as the security is defined like so, cloned logins will stay draft for ever, and keep being cloned from person to person, creating a snowball effect. Normal users consider it as garbage, and we shouldn't consider the usability of ERP5 from a Manager point of view only.

I'm not against changing the required permission to change a password, but this needs to be discussed more deeply.

I agree that the constraint shouldn't raise for draft logins, but this should be checked in the validate workflow transition.

[Vincent Pelletier]

"Set Own Password" [...] is only attributed to "Manager".

I believe we have customers who are able to create user accounts in their respective ERP5 instances, and they are not Managers and I believe this also happens in cases where user cannot choose their own password. Do they use a proxy-roled script to pick a password ?

I agree wiht the other points you make.

I agree that the constraint shouldn't raise for draft logins, but this should be checked in the validate workflow transition.

Yes, checking must happen, just not by constraint.

[Jérome Perrin]

I have not confirmed, but this probably breaks storing ERP5 Login in business templates.

[Jérome Perrin]

I have not confirmed, but this probably breaks storing ERP5 Login in business templates.

I was not really suggesting to repair this.