Univention Bugzilla – Bug 53203
The import should not remove the admin_ou role from a teacher when it was adjusted
Last modified: 2023-04-19 18:23:54 CEST
After fixing of the roles in the course of debugging the diagnostic consistency check, the next import of teachers removes the role again. The diagnostic consistency check is complaining again.
Bug can be reproduced as follows: teachers.json (config): { "csv":{ "mapping":{ "Benutzertyp":"__role", "Schulen":"schools", "Vorname":"firstname", "Nachname":"lastname", "Geburtstag":"birthday", "EMail":"email", "Klassen":"school_classes" } }, "scheme":{ "record_uid":"<firstname>.<lastname>", "username":{ "default":"<:umlauts><firstname>.<lastname><:lower>[COUNTER2]" } }, "source_uid":"TESTID", "verbose":false, "normalize":{ "firstname":false, "lastname":false } } teachers.csv: "Benutzertyp","Schulen","Vorname","Nachname","Geburtstag","EMail","disabled","Klassen" "teacher","DEMOSCHOOL","test","teacher1","","test.teacher1@school.de","0","demoschool-1a" 1. import teacher with: /usr/share/ucs-school-import/scripts/ucs-school-user-import -c teachers.json -m -i teachers.csv 2. add admin role with: udm users/user modify --dn "uid=test.teacher1,cn=lehrer,cn=users,ou=DEMOSCHOOL,dc=school,dc=intranet" --append ucsschoolRole=school_admin:school:DEMOSCHOOL --append groups="cn=admins-demoschool,cn=ouadmins,cn=groups,dc=school,dc=intranet" 3. import teacher again -> the admin ucsschoolRole is gone, the admin-group membership stays
ucsschool.importer.models.import_user.ImportUser.make_ucsschool_roles() has to be modified. It does not take into account, that a Staff, Teacher or TeachersAndStaff can *also* be an admin. It needs something (before the return) like: if self.is_administrator(): self.ucsschool_roles.extend( [ create_ucsschool_role_string(role, school) for role in models.user.SchoolAdmin.default_roles for school in self.ou_admins_groups_I_am_a_member_of() ])
Added fix on branch troehmey/bug53203_importer_keeps_admin_role with commit ef3fe3d75 Bug #53203: fix admin role on do_modify fix was implemented in ucsschool/ucs-school-lib/python/models/user.py. It could not be implemented as suggested in comment #2, since the user_dn of the modified user is not known at that point. !! Addition to comment #1: At step 2, objectClass=ucsschoolAdministrator is also required. Set it via the UMC.
The code itself looks good to me, but I have a problem with the position of the code. In your proposal it exists in the do_modify of the user object. That has the following consequence: Whenever any user that is deemed an ucsschoolAdministrator is modified, he will get the school_admin role for any school he is part and is a member of the school admin group for. This creates the narrative that the objectType is the most important and primary factor of deciding how the user has to correctly look. In my understanding we want to move towards a role focused view on objects, so the present roles should determine which attributes are missing. The User Import is a special situation, where I would not mind such an invasive procedure to happen, since it follows its own special logic anyway. Why exactly can that not be implemented in the ImportUser class? - The DN is not present in make_ucsschool_roles - What do you need the DN for; the UDM object? - Never is present or is not present on new objects? If an existing ImportUser is modified the DN should be accessible via self.dn or not? And only on modification the handled situation could ever occur. - If there is no way at all to implement this in the make_ucsschool_role method, I would prefer this in the do_modify (or _alter_udm_obj) of the ImportUser class, not the school.lib class.
If I remember correctly, the reason it is implemented there is, that the root of the problem of the disappearing admin roles is also in the school lib. The school lib goes through the users "default_roles" attribute and adds/removes "ucsschool_roles" entries: RoleSupportMixin.update_ucsschool_roles(). But a teacher with an *additional* admin role does not have that admin role in "default_roles".
RoleSupportMixin.update_ucsschool_roles() only touches an objects roles if there are none present already for this school. So this should not be the cause. The cause for the missing role is probably def make_ucsschool_roles(self): # type: () -> List[str] in the ImportUser class. This Bug describes the problem that roles are removed during an Import of an existing User. So in this Bug we should deal with that, not with the problem of getting the role there in the first place. For that I created Bug #53485 . Your(Toni) proposed code currently does the following: Manually added roles/groups/options to make Teacher also a SchoolAdmin Reimport of said teacher - ImportUser code sets new roles and consequently removes admin role - lib code adds the role, that the import just removed, again - Teacher is correctly saved as also SchoolAdmin In a discussion with Daniel we came to the conclusion that it would probably best to separate this in two Bugs. This one here fixes the wrong removal of roles of existing ImportUsers. The new Bug #53485 deals with the fact that the roles should be set in accordance with objectClasses and group memberships. Your current code would then be a starting point for Bug #53485 and this Bug would need another solution. The necessary changes for this Bug would be the following: When an ImportUser is modified, not created, since it exists already, the Importer does not meddle with the ucsschool_roles that are already there. Daniel proposed to just empty the make_ucsschool_roles method of ImportUser Could you try this out and Verify if this fixes the Bug and has no unwanted side effects? (E.g for actually newly created users, role changes via Import [is that actually possible?], etc)
Implemented new fix (as discussed) and ucs-test with 6040ac93e Bug #53203: fixup, modify user via lo.modify 43bad9948 Bug #53203: added ucs-test import_keeps_old_roles 208c81b80 Bug #53203: keep admin and non-school roles in make_ucsschool_roles
As discussed, fix has been squashed and merged to 4.4 with 7b5898abd Bug #53203: added advisory 9646917ea Bug #53203: added changelog entries 1704f7315 Bug #53203: Merge branch 'troehmey/bug53203_importer_keeps_admin_role' into 4.4 1060c63f9 Bug #53203: added ucs-test import_keeps_old_roles 14765c560 Bug #53203: fix admin role on do_modify Successful builds: Package: ucs-school-import Version: 17.0.70A~4.4.0.202106291122 Branch: ucs_4.4-0 Scope: ucs-school-4.4 Package: ucs-test-ucsschool Version: 6.0.238A~4.4.0.202106291127 Branch: ucs_4.4-0 Scope: ucs-school-4.4
Changelog & Advisory: OK Packages install: OK Tests run: OK Manually reproduced your test scenario, which is fixed. Tried some non school roles as well Seems good to me.
Errata updates for UCS@school 4.4 v9 have been released. https://docs.software-univention.de/changelog-ucsschool-4.4v9-de.html If this error occurs again, please clone this bug.