Bug 53203 - The import should not remove the admin_ou role from a teacher when it was adjusted
The import should not remove the admin_ou role from a teacher when it was adj...
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.4
Other Linux
: P5 normal (vote)
: UCS@school 4.4 v9-errata
Assigned To: Toni Röhmeyer
Ole Schwiegert
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-30 16:47 CEST by Christina Scheinig
Modified: 2023-04-19 18:23 CEST (History)
4 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 3: Simply Wrong: The implementation doesn't match the docu
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.069
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number: 2021041521000332
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christina Scheinig univentionstaff 2021-04-30 16:47:47 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.
Comment 1 Toni Röhmeyer univentionstaff 2021-05-25 17:14:12 CEST
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
Comment 2 Daniel Tröder univentionstaff 2021-05-26 09:25:36 CEST
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()
    ])
Comment 3 Toni Röhmeyer univentionstaff 2021-05-27 11:45:40 CEST
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.
Comment 4 Ole Schwiegert univentionstaff 2021-06-21 07:28:28 CEST
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.
Comment 5 Daniel Tröder univentionstaff 2021-06-21 08:05:12 CEST
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".
Comment 6 Ole Schwiegert univentionstaff 2021-06-21 13:44:56 CEST
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)
Comment 7 Toni Röhmeyer univentionstaff 2021-06-29 09:21:35 CEST
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
Comment 8 Toni Röhmeyer univentionstaff 2021-06-29 11:39:09 CEST
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
Comment 9 Ole Schwiegert univentionstaff 2021-06-30 08:54:55 CEST
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.
Comment 10 Tobias Wenzel univentionstaff 2021-07-01 12:09:00 CEST
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.