Univention Bugzilla – Bug 42080
users/user open() unexpectedly modifies users
Last modified: 2022-11-22 10:25:02 CET
4.1-2 errata211 (Vahr) Die Ausführung des Kommandos helpdesk/send ist fehlgeschlagen: Traceback (most recent call last): File "/usr/lib/pymodules/python2.7/univention/management/console/base.py", line 283, in execute function(self, request) File "/usr/lib/pymodules/python2.7/univention/management/console/modules/decorators.py", line 190, in _response return function(self, request) File "/usr/lib/pymodules/python2.7/ucsschool/lib/schoolldap.py", line 140, in wrapper_func return func(*args, **kwargs) File "/usr/lib/pymodules/python2.7/univention/management/console/modules/helpdesk/__init__.py", line 126, in send user.open() File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line 1422, in open self.__primary_group() File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line 1680, in __primary_group self.lo.modify(self.dn, [('gidNumber', 'None', primaryGroupNumber)]) File "/usr/lib/pymodules/python2.7/univention/admin/uldap.py", line 396, in modify raise univention.admin.uexceptions.permissionDenied permissionDenied
This is caused by Bug #12621 / svn r7705 which modifies the user object in open().
https://billy.knut.univention.de/websvn/diff.php?repname=dev&path=%2Fbranches%2Fucs-2.1%2Fucs%2Fmanagement%2Funivention-directory-manager-modules%2Fmodules%2Funivention%2Fadmin%2Fhandlers%2Fusers%2Fuser.py&rev=7664&peg=7664
Interesting question would be: why and how was the primary group of the UMC user deleted?
(In reply to Sönke Schwardt-Krummrich from comment #3) > Interesting question would be: > why and how was the primary group of the UMC user deleted? Maybe it is not yet replicated? Maybe the user has no permission to read her primary group?
*** Bug 44715 has been marked as a duplicate of this bug. ***
(In reply to Florian Best from comment #4) > Maybe the user has no permission to read her primary group? Yeah, that was the problem with one of our customers today. And I also suspect that this is the problem of the original post (UCS@school uses selective replication). Detailed description: The problem occurs when a non-privileged user (A) attempts to open a user object (B) via users/user.py. In open() the gidNumber of B is read and then a group with the same gidNumber is searched. If such a group does not exist in LDAP or user A does not have sufficient rights in LDAP to search for/find the group, then open() tries to set the default primary group for user B. If user A does not have sufficient rights to change the gidNumber for user B, traceback occurs. As Moritz stated in bug 12621 comment 3, I do not see a reason why users/user.py should modify a user object in open().
In the customer case, a helpdesk user that is only allowed to see a (ONE!) specific group in LDAP, got a traceback when groups/group is opened, due to some extended attributes that tried to show all users in a drop-down. To display the users within the drop-down widget, open() is called on each user object to retrive the displayName.
External feedback: UCS Version: 4.2-3 errata284 (Lesum) Interner Server-Fehler in "schoolusers/query (student)". Traceback (most recent call last): File "/usr/lib/pymodules/python2.7/univention/management/console/base.py", line, in execute function.__func__(self, request, *args, **kwargs) File "/usr/lib/pymodules/python2.7/univention/management/console/modules/decorators.py", line, in _response return function(self, request) File "/usr/lib/pymodules/python2.7/ucsschool/lib/schoolldap.py", line, in wrapper_func return func(*args, **kwargs) File "/usr/lib/pymodules/python2.7/univention/management/console/modules/schoolusers/__init__.py", line, in query } for usr in self._users(ldap_user_read, request.options['school'], group=klass, user_type=request.flavor, pattern=request.options.get('pattern', ''))] File "/usr/lib/pymodules/python2.7/ucsschool/lib/schoolldap.py", line, in _users _users = cls.get_all(ldap_connection, school, LDAP_Filter.forUsers(pattern)) File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line, in get_all udm_obj.open() File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line, in open self.__primary_group() File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line, in __primary_group self.lo.modify(self.dn, [('gidNumber', 'None', primaryGroupNumber)]) File "/usr/lib/pymodules/python2.7/univention/admin/uldap.py", line, in modify raise univention.admin.uexceptions.permissionDenied permissionDenied
This is still a problem on UCS 5.0 with UCS@school. The following traceback appears regularly in our jenkins tests. On a replica in the connector-s4.log. I think this might be a replication problem. The s4 connector, on the replica, seems to open a new user before the update to its primary group "Domain Users" has been synced to the local replica ldap. Since the s4 connector uses the computer account on the replica, it is not allowed to change the global "Domain Users" group. 10.11.2021 00:38:17.988 LDAP (WARNING): Traceback (most recent call last): File "/usr/lib/python3/dist-packages/univention/uldap.py", line 812, in modify_ext_s rtype, rdata, rmsgid, resp_ctrls = self.lo.modify_ext_s(dn, ml, serverctrls=serverctrls) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 1253, in modify_ext_s return self._apply_method_s(SimpleLDAPObject.modify_ext_s,*args,**kwargs) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 1197, in _apply_method_s return func(self,*args,**kwargs) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 602, in modify_ext_s resp_type, resp_data, resp_msgid, resp_ctrls = self.result3(msgid,all=1,timeout=self.timeout) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 749, in result3 resp_ctrl_classes=resp_ctrl_classes File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 756, in result4 ldap_result = self._ldap_call(self._l.result4,msgid,all,timeout,add_ctrls,add_intermediates,add_extop) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 329, in _ldap_call reraise(exc_type, exc_value, exc_traceback) File "/usr/lib/python3/dist-packages/ldap/compat.py", line 44, in reraise raise exc_value File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 313, in _ldap_call result = func(*args,**kwargs) ldap.REFERRAL: {'desc': 'Referral', 'info': 'Referral:\nldap://master208.autotest208.local:7389/cn=Domain%20Users,cn=groups,dc=autotest208,dc=local'} During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/univention/admin/uldap.py", line 803, in modify return self.lo.modify(dn, changes, serverctrls=serverctrls, response=response, rename_callback=rename_callback) File "/usr/lib/python3/dist-packages/univention/uldap.py", line 208, in _decorated return func(self, *args, **kwargs) File "/usr/lib/python3/dist-packages/univention/uldap.py", line 753, in modify self.modify_ext_s(dn, ml, serverctrls=serverctrls, response=response) File "/usr/lib/python3/dist-packages/univention/uldap.py", line 208, in _decorated return func(self, *args, **kwargs) File "/usr/lib/python3/dist-packages/univention/uldap.py", line 817, in modify_ext_s rtype, rdata, rmsgid, resp_ctrls = lo_ref.modify_ext_s(dn, ml, serverctrls=serverctrls) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 1253, in modify_ext_s return self._apply_method_s(SimpleLDAPObject.modify_ext_s,*args,**kwargs) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 1197, in _apply_method_s return func(self,*args,**kwargs) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 602, in modify_ext_s resp_type, resp_data, resp_msgid, resp_ctrls = self.result3(msgid,all=1,timeout=self.timeout) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 749, in result3 resp_ctrl_classes=resp_ctrl_classes File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 756, in result4 ldap_result = self._ldap_call(self._l.result4,msgid,all,timeout,add_ctrls,add_intermediates,add_extop) File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 329, in _ldap_call reraise(exc_type, exc_value, exc_traceback) File "/usr/lib/python3/dist-packages/ldap/compat.py", line 44, in reraise raise exc_value File "/usr/lib/python3/dist-packages/ldap/ldapobject.py", line 313, in _ldap_call result = func(*args,**kwargs) ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access'} During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/univention/s4connector/__init__.py", line 824, in __sync_file_from_ucs if not self.sync_from_ucs(key, mapped_object, pre_mapped_ucs_dn, old_dn, old, new): File "/usr/lib/python3/dist-packages/univention/s4connector/s4/__init__.py", line 2337, in sync_from_ucs post_con_modify_function(self, property_type, object) File "/usr/lib/python3/dist-packages/univention/s4connector/s4/__init__.py", line 95, in disable_user_from_ucs return connector.disable_user_from_ucs(key, object) File "/usr/lib/python3/dist-packages/univention/s4connector/s4/__init__.py", line 1701, in disable_user_from_ucs ucs_admin_object.open() File "/usr/lib/python3/dist-packages/univention/admin/handlers/users/user.py", line 1350, in open self._load_groups(loadGroups) File "/usr/lib/python3/dist-packages/univention/admin/handlers/users/user.py", line 1382, in _load_groups self.__primary_group() File "/usr/lib/python3/dist-packages/univention/admin/handlers/users/user.py", line 1627, in __primary_group grpobj.fast_member_add([self.dn], [new_uid]) File "/usr/lib/python3/dist-packages/univention/admin/handlers/groups/group.py", line 381, in fast_member_add return self.lo.modify(self.dn, ml) File "/usr/lib/python3/dist-packages/univention/admin/uldap.py", line 809, in modify raise univention.admin.uexceptions.permissionDenied() univention.admin.uexceptions.permissionDenied
Workaround: "ucr set directory/manager/user/primarygroup/update=false"
I moved my patch into a merge request. https://git.knut.univention.de/univention/ucs/-/merge_requests/179 Don't know the current state - maybe we should also remove the code which checks the UCR variable.
Created attachment 10994 [details] test script / reproducer Attached is a script which reproduces the behavior and tests if the behavior is delayed until a modify() operation is done. If we don't find a primary group for the gidNumber opening a UDM module should just be possible, while `primaryGroup` is unset then. As `primaryGroup` is required we can set it to the default value before modify() so that it still raises permissionDenied and prevents modification of users if permission or replication problems exists. permissionDenied is of course then not raised for administrator accounts with write access to e.g. the default primary group "Domain Users" and in that case the primary group would still be changed/reset. This way the change is at least not so invasive as an complete API change.
> If we don't find a primary group for the gidNumber opening a UDM module should just be possible, while `primaryGroup` is unset then. Why? The situation is obviously one where data in LDAP is inconsistent. IMHO in such a case a library should not manipulate the data to a "best guess", but deny all further operations. That is the only way to ensure that no customer data is lost! > As `primaryGroup` is required we can set it to the default value before modify() so that it still raises permissionDenied and prevents modification of users if permission or replication problems exists. Why postpone an error message? That just leads to confusing bug reports (like the above tracebacks), because it hides the root cause. > permissionDenied is of course then not raised for administrator accounts with write access to e.g. the default primary group "Domain Users" and in that case the primary group would still be changed/reset. Even worse: if you're an administrator and the data in LDAP is inconsistent, you won't even get an error, you will just silently overwrite your original data. This is not how a library should work: safely. > This way the change is at least not so invasive as an complete API change. I don't see an API change, if the opening of objects with inconsistent data is prevented. To the contrary I believe that it is not part of the API that it silently overwrites data in case it finds something suspicious.
(In reply to Daniel Tröder from comment #14) > > If we don't find a primary group for the gidNumber opening a UDM module should just be possible, while `primaryGroup` is unset then. > > Why? The situation is obviously one where data in LDAP is inconsistent. IMHO > in such a case a library should not manipulate the data to a "best guess", > but deny all further operations. That is the only way to ensure that no > customer data is lost! That's the situation we currently have. A exception prevents the usage of UDM with that user. Plus we don't manipulate data anymore (in LDAP) but just leave it as `None` instead of as `"cn=Domain Users"`. > > As `primaryGroup` is required we can set it to the default value before modify() so that it still raises permissionDenied and prevents modification of users if permission or replication problems exists. > > Why postpone an error message? That just leads to confusing bug reports > (like the above tracebacks), because it hides the root cause. We already have these bug reports, that is just this bug. Postpone the behavior will cause less situations that it is reported. It enables using UDM in a read-only mode. And there is a UCR variable which wants that self-correcting behavior. > > permissionDenied is of course then not raised for administrator accounts with write access to e.g. the default primary group "Domain Users" and in that case the primary group would still be changed/reset. > > Even worse: if you're an administrator and the data in LDAP is inconsistent, > you won't even get an error, you will just silently overwrite your original > data. This is not how a library should work: safely. That was the behavior prior as well. If you open a user as administrator but the group is not yet replicated the group was reset to "Domain Users". Now the silent-overwriting is only done when you modify a user. > > This way the change is at least not so invasive as an complete API change. > > I don't see an API change, if the opening of objects with inconsistent data > is prevented. > To the contrary I believe that it is not part of the API that it silently > overwrites data in case it finds something suspicious. It's maybe not part of the API but it is the current behavior. There is a UCR variable which enables that self-correcting-behavior. I don't want to just obsolete it when I don't know which customers rely on it. The MR just keeps similar behavior but defers error conditions to a later point so that one is able to further use UDM when only reading data from it. For example comment 9 would be fixed with that as well, they don't need primaryGroup at all. So the change is not invasive, enhances the situation but of course UDM is not perfect after this. If we want UDM to be perfect we need to change a lot more in the current architecture.
(In reply to Florian Best from comment #15) > (In reply to Daniel Tröder from comment #14) > > > If we don't find a primary group for the gidNumber opening a UDM module should just be possible, while `primaryGroup` is unset then. > > > > Why? The situation is obviously one where data in LDAP is inconsistent. IMHO > > in such a case a library should not manipulate the data to a "best guess", > > but deny all further operations. That is the only way to ensure that no > > customer data is lost! > That's the situation we currently have. A exception prevents the usage of > UDM with that user. > Plus we don't manipulate data anymore (in LDAP) but just leave it as `None` > instead of as `"cn=Domain Users"`. That's not how I understand the above traceback. The situation is not that UDM stops working on purpose, because it detects a problem. The situation is that UDM tries to write data to LDAP when it should only _read_ it, and then crashes. And that behavior inconsistently depends on the permissions of the user. So UDM only behaves correctly (safely) by accident, and with a misleading error message. > > > As `primaryGroup` is required we can set it to the default value before modify() so that it still raises permissionDenied and prevents modification of users if permission or replication problems exists. > > > > Why postpone an error message? That just leads to confusing bug reports > > (like the above tracebacks), because it hides the root cause. > We already have these bug reports, that is just this bug. > Postpone the behavior will cause less situations that it is reported. > It enables using UDM in a read-only mode. > And there is a UCR variable which wants that self-correcting behavior. Ah OK. Maybe the problem is, that I can not understand the proposed solution for this bug from the above comments. A bit of text, and not just a patch, would help. From your comment I understand, that you want to catch the traceback and just ignore it. Then, when the user is only read, no traceback would occur. But when a modification would be executed, the traceback would still happen. But there is the UCRV directory/manager/user/primarygroup/update that alters the above, by automatically "correcting" the "None" primary group to "Domain Users". And that UCRV is enabled by default. Is that correct? If yes, I see problems with that solution: * Even when only read, the data is still wrong: the primary group field is empty. * The default behavior (UCRV) is unsafe, when the user has write permissions. * The error message is misleading, when the user has no write permissions. > > > permissionDenied is of course then not raised for administrator > accounts with write access to e.g. the default primary group "Domain Users" > and in that case the primary group would still be changed/reset. > > > > Even worse: if you're an administrator and the data in LDAP is inconsistent, > > you won't even get an error, you will just silently overwrite your original > > data. This is not how a library should work: safely. > That was the behavior prior as well. If you open a user as administrator but > the group is not yet replicated the group was reset to "Domain Users". Now > the silent-overwriting is only done when you modify a user. That is still an unsafe behavior and should not be the default. > > > This way the change is at least not so invasive as an complete API change. > > > > I don't see an API change, if the opening of objects with inconsistent data > > is prevented. > > To the contrary I believe that it is not part of the API that it silently > > overwrites data in case it finds something suspicious. > It's maybe not part of the API but it is the current behavior. There is a > UCR variable which enables that self-correcting-behavior. I don't want to > just obsolete it when I don't know which customers rely on it. Yes, that's right... Can we schedule the change of the default to "False" for the next patch release (5.0-3)? > The MR just keeps similar behavior but defers error conditions to a later > point so that one is able to further use UDM when only reading data from it. > For example comment 9 would be fixed with that as well, they don't need > primaryGroup at all. The problem is, that UDM cannot know what data a user requires. IMHO the UDM client (s4-c) should either wait-for-replication or use open(loadGroups=False). Oh.. that won't help. The "primaryGroup" will still be loaded and __primary_group() will still be called. And as long as directory/manager/user/primarygroup/update defaults to True that will lead to wrong data. > So the change is not invasive, enhances the situation but of course UDM is > not perfect after this. > If we want UDM to be perfect we need to change a lot more in the current > architecture. Nobody expects UDM to be perfect. But I expect a safe behavior. Safe in the way, that data is not changed unless explicitly told to. ---- This whole bug comes down to changing the default of directory/manager/user/primarygroup/update to a _safe_ value. A library must not do a write operation, when the user requests a read operation. The proposed solution does only hide the unsafe behavior. Please discuss this with the PM of UCS.
(In reply to Daniel Tröder from comment #16) > (In reply to Florian Best from comment #15) > > (In reply to Daniel Tröder from comment #14) > > > > If we don't find a primary group for the gidNumber opening a UDM module should just be possible, while `primaryGroup` is unset then. > > > > > > Why? The situation is obviously one where data in LDAP is inconsistent. IMHO > > > in such a case a library should not manipulate the data to a "best guess", > > > but deny all further operations. That is the only way to ensure that no > > > customer data is lost! > > That's the situation we currently have. A exception prevents the usage of > > UDM with that user. > > Plus we don't manipulate data anymore (in LDAP) but just leave it as `None` > > instead of as `"cn=Domain Users"`. > > That's not how I understand the above traceback. > The situation is not that UDM stops working on purpose, because it detects a > problem. > The situation is that UDM tries to write data to LDAP when it should only > _read_ it, and then crashes. yes, but with the MR it doesn't do that anymore. > And that behavior inconsistently depends on the permissions of the user. > So UDM only behaves correctly (safely) by accident, and with a misleading > error message. > > > > As `primaryGroup` is required we can set it to the default value before modify() so that it still raises permissionDenied and prevents modification of users if permission or replication problems exists. > > > > > > Why postpone an error message? That just leads to confusing bug reports > > > (like the above tracebacks), because it hides the root cause. > > We already have these bug reports, that is just this bug. > > Postpone the behavior will cause less situations that it is reported. > > It enables using UDM in a read-only mode. > > And there is a UCR variable which wants that self-correcting behavior. > > Ah OK. Maybe the problem is, that I can not understand the proposed solution > for this bug from the above comments. > A bit of text, and not just a patch, would help. Consider understanding the Code and MR instead of only the text which was meant to be for the QA contact, who does code review at the end. > From your comment I understand, that you want to catch the traceback and > just ignore it. No, the MR doesn't catch any exception but makes it impossible that it occurs during open: No modification of an object is done anymore on open. It moves the code which sets the default value of primaryGroup from `open()` into the `_ldap_pre_ready()` method, where such defaults are also usually set. > Then, when the user is only read, no traceback would occur. > But when a modification would be executed, the traceback would still happen. Yes, the default value is then still set before `modify()` happens and raises the same exception as before. Not doing this will result in other exceptions e.g. `insufficientInformation: primaryGroup is not set but required`. > But there is the UCRV directory/manager/user/primarygroup/update that alters > the above, by automatically "correcting" the "None" primary group to "Domain > Users". And that UCRV is enabled by default. > > Is that correct? The UCR variables main purpose is to update the users membership in the primary group when the user is created/moved/modified/removed. Otherwise the user has only a gidNumber but is not part of the group. > If yes, I see problems with that solution: > * Even when only read, the data is still wrong: the primary group field is > empty. Yes, but probably better than a wrong group. And this is also the behavior for other properties which aren't allowed to read. > * The default behavior (UCRV) is unsafe, when the user has write permissions. Yes, but only for `modify()` operations and not for the read-only case anymore. > * The error message is misleading, when the user has no write permissions. Well, there is no error message at all but a exception (without even a human description). And the meaning of the exception is correct. Without looking into a context of a error everything is misleading. I will add a ERROR-log message into the code when this situation occurs. > > > > permissionDenied is of course then not raised for administrator > > accounts with write access to e.g. the default primary group "Domain Users" > > and in that case the primary group would still be changed/reset. > > > > > > Even worse: if you're an administrator and the data in LDAP is inconsistent, > > > you won't even get an error, you will just silently overwrite your original > > > data. This is not how a library should work: safely. > > That was the behavior prior as well. If you open a user as administrator but > > the group is not yet replicated the group was reset to "Domain Users". Now > > the silent-overwriting is only done when you modify a user. > > That is still an unsafe behavior and should not be the default. I agree but feel a little uncomfortable about changing this now because it may have several side effects. > > > > This way the change is at least not so invasive as an complete API change. > > > > > > I don't see an API change, if the opening of objects with inconsistent data > > > is prevented. > > > To the contrary I believe that it is not part of the API that it silently > > > overwrites data in case it finds something suspicious. > > It's maybe not part of the API but it is the current behavior. There is a > > UCR variable which enables that self-correcting-behavior. I don't want to > > just obsolete it when I don't know which customers rely on it. > > Yes, that's right... > Can we schedule the change of the default to "False" for the next patch > release (5.0-3)? No, changing it to `False` will cause that the primary group is also not updated in valid cases e.g. on create,modify,remove when the primary group is set/changed as mentioned above. > > The MR just keeps similar behavior but defers error conditions to a later > > point so that one is able to further use UDM when only reading data from it. > > For example comment 9 would be fixed with that as well, they don't need > > primaryGroup at all. > > The problem is, that UDM cannot know what data a user requires. > > IMHO the UDM client (s4-c) should either wait-for-replication or use > open(loadGroups=False). > Oh.. that won't help. The "primaryGroup" will still be loaded and > __primary_group() will still be called. > And as long as directory/manager/user/primarygroup/update defaults to True > that will lead to wrong data. We can't add code in every client that waits for replication - this also only helps about the replication scenario. The missing permissions are still another scenario. Changing to False will also result in wrong data, as then the user is not anymore part of its primaryGroup but that is only indicated by the gidNumber attribute. > > So the change is not invasive, enhances the situation but of course UDM is > > not perfect after this. > > If we want UDM to be perfect we need to change a lot more in the current > > architecture. > > Nobody expects UDM to be perfect. But I expect a safe behavior. Safe in the > way, that data is not changed unless explicitly told to. If you think more safe means changing the behavior to raise an exception when opening an object where no permissions exists to read the primaryGroup or where the primaryGroup does not exists or is not replicated: * introducing a new exception which may happen during instantiating an UDM object may case several new problems in various places * It causes that the user is ignored in a lookup()/udm list - which might also cause data loss when there are other actions in parallel relying on every user is fetched. * An exception would also still prevent that a user can modify himself via UDM when it only wants to change some other properties (e.g. in the self-service). If it only means to not modify any data: The MR does it already for the situations which were reported here but not for the modify case. > > ---- > > This whole bug comes down to changing the default of > directory/manager/user/primarygroup/update to a _safe_ value. No, as mentioned above. > A library must not do a write operation, when the user requests a read > operation. And that is what the MR already solves. > The proposed solution does only hide the unsafe behavior. > Please discuss this with the PM of UCS. It doesn't hide it but it reduces it to situations where UDM is not used read-only but in write-mode. I see two options now: * I can clone this bug so that we can change the behavior to be more safe at a later point when we really need it. Because now we only need the current fix in the MR, which currently prevents using UDM in a read-only scenario. And the MR also prevents all of the above tracebacks to happen. or: * we can now additionally throw away the behavior of setting primaryGroup to a default value if we cannot detect the primaryGroup. During a modification UDM will then say "insufficienInformation: primaryGroup is missing" and denies the modification as well. We could hack something into UDM which even ignores that error and does the other modifications. I don't know which further challenges we get after that. To me it seems the old behavior once also was intended behavior but maybe it's not.
Thank you for the very detailed response and evaluating the different scenarios. I think this discussion will help future readers to understand why things are done, as they are done. That is not something that can be understood from the source code alone, let alone a patch. I am convinced now, that the proposed solution is the best for the current situation. A question remains for me: if the UCRV should stay True in every case, why do we need it at all? What is the requirement for being able to deactivate the current default behavior?
(In reply to Daniel Tröder from comment #18) > I am convinced now, that the proposed solution is the best for the current > situation. Great, thanks. > A question remains for me: if the UCRV should stay True in every case, why > do we need it at all? What is the requirement for being able to deactivate > the current default behavior? I don't know exactly. The description from the initial bug 18247 was: ``` In großen Umgebungen werden die Änderungen an der primären Standardgruppe "Domain Users" zu Flaschenhals bei der Benutzeradministration, da wir dort uniqueMember und memberUid vollständig durch einen "replace" ersetzten, wenn ein neuer Benutzer angelegt oder ein bestehender gelöscht bzw. die primäre Gruppe geändert wird. Ausserdem reagieren NSCD, Samba u.a. mit geringer Performance auf große Gruppen. Für die primäre Gruppe ist die Pflege der Attribute an der Gruppe für Dateisystemrechte, Samba und andere an Posix gebundene Dienste irrelevant. Sie sollte daher optional deaktivierbar sein. ```
The MR has been applied. The reset of primaryGroup is now postponed during modification of the object instead of during opening the object. univention-directory-manager-modules.yaml cd4333da2714 | Bug #42080: don't modify primary group for users/user during open() univention-directory-manager-modules (15.0.13-11) cd4333da2714 | Bug #42080: don't modify primary group for users/user during open()
(In reply to Florian Best from comment #19) > (In reply to Daniel Tröder from comment #18) > > A question remains for me: if the UCRV should stay True in every case, why > > do we need it at all? What is the requirement for being able to deactivate > > the current default behavior? > I don't know exactly. The description from the initial bug 18247 was: > ``` > In großen Umgebungen werden die Änderungen an der primären Standardgruppe > "Domain Users" zu Flaschenhals bei der Benutzeradministration, da wir dort > uniqueMember und memberUid vollständig durch einen "replace" ersetzten, wenn > ein neuer Benutzer angelegt oder ein bestehender gelöscht bzw. die primäre > Gruppe geändert wird. Ausserdem reagieren NSCD, Samba u.a. mit geringer > Performance auf große Gruppen. > > Für die primäre Gruppe ist die Pflege der Attribute an der Gruppe für > Dateisystemrechte, Samba und andere an Posix gebundene Dienste irrelevant. > Sie sollte daher optional deaktivierbar sein. > ``` I think that's deprecated. In UCS@school school-users are put into "Domain User $OU" groups, to prevent the performance problem. I have found only one occurrence of "directory/manager/user/primarygroup/update=false": test/ucs-test/tests/52_s4connector/516_create_delete_objects_in_ucs_check_sync.py IMHO that UCRV can be depricated.
(In reply to Daniel Tröder from comment #21) > I have found only one occurrence of > "directory/manager/user/primarygroup/update=false": > test/ucs-test/tests/52_s4connector/ > 516_create_delete_objects_in_ucs_check_sync.py > > IMHO that UCRV can be depricated. I also found Bug #51771 which wishes to have that feature.
User is not added to the primaryGroup when it is created.
(In reply to Iván.Delgado from comment #23) > User is not added to the primaryGroup when it is created. Oh yes, fixed via: univention-directory-manager-modules (15.0.13-11) 641ec2832608 | fix[udm]: mark default value for primaryGroup as changed in open() for not existing objects 4115164c3013 | refactor[udm]: move unmapping of cheap properties into _post_unmap
Verified: * All in Comment 10 * Advisory * ucs-test-udm * attachment 10994 [details] passed
A S4-Connector test is broken: https://univention-dist-jenkins.k8s.knut.univention.de/job/UCS-5.0/job/UCS-5.0-2/job/S4Connector/lastCompletedBuild/cfg=master-s4connector/testReport/52_s4connector/516_create_delete_objects_in_ucs_check_sync/test_no_leftovers_after_delete_in_ucs/
(In reply to Florian Best from comment #26) > A S4-Connector test is broken: > https://univention-dist-jenkins.k8s.knut.univention.de/job/UCS-5.0/job/UCS-5. > 0-2/job/S4Connector/lastCompletedBuild/cfg=master-s4connector/testReport/ > 52_s4connector/516_create_delete_objects_in_ucs_check_sync/ > test_no_leftovers_after_delete_in_ucs/ Seems fixed via: univention-directory-manager-modules (15.0.13-12) fe429e8f3fa7 | fixup! Bug #42080: don't modify primary group for users/user during open() There was a missing return, which caused that it was always logged that no primary group could be found. The test case is parsing log files, seems the additional log line made it un-robust. Can't explain it better as I could not reproduce it on my virtual machine.
Verified: * Advisory * ucs-test-udm
<https://errata.software-univention.de/#/?erratum=5.0x489>