Bug 46970 - [4.3] ucs-school-user-import does not trim leading/trailing whitespaces
[4.3] ucs-school-user-import does not trim leading/trailing whitespaces
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.3
Other other
: P5 normal (vote)
: UCS@school 4.3 v4
Assigned To: Daniel Tröder
Ole Schwiegert
:
Depends on:
Blocks: 47092
  Show dependency treegraph
 
Reported: 2018-05-07 15:43 CEST by Michael Grandjean
Modified: 2018-07-04 18:09 CEST (History)
3 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 3: A User would likely not purchase the product
User Pain: 0.171
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
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 Michael Grandjean univentionstaff 2018-05-07 15:43:05 CEST
A customer imported some users a while ago, e.g.:

dn: uid=mary.gomme,cn=schueler,cn=users,ou=999,dc=edu,dc=example,dc=org
sn: Gommer
givenName: Mary Fairfax

For the second import run, a trailing whitespace was unintentionally added to the lastname, resulting in "Gommer " (OpenLDAP will save this in base64, then):

dn: uid=mary.gomme,cn=schueler,cn=users,ou=999,dc=edu,dc=example,dc=org
sn:: R29tbWVyIA==
givenName: Mary Fairfax

This is especially nasty if this hits the recordUID, because then a completely new user will be created and the old one will be deleted.

IMHO ucs-school-user-import should trim leading and trailing whitespaces for all (most?) attributes. Maybe we could add this as a hook, so customers can deactivate it if needed?
Comment 1 Daniel Tröder univentionstaff 2018-05-08 09:09:52 CEST
(In reply to Michael Grandjean from comment #0)
> IMHO ucs-school-user-import should trim leading and trailing whitespaces for
> all (most?) attributes. Maybe we could add this as a hook, so customers can
> deactivate it if needed?
I suggest to make trimming the default, because it almost always is wanted. If a user has a scenario in which the white space is required, a pre-* hook can re-add it.

I suggest to trim leading & trailing white space on all input fields.
Comment 2 Michel Smidt 2018-05-08 09:12:56 CEST
(In reply to Daniel Tröder from comment #1)
> (In reply to Michael Grandjean from comment #0)
> > IMHO ucs-school-user-import should trim leading and trailing whitespaces for
> > all (most?) attributes. Maybe we could add this as a hook, so customers can
> > deactivate it if needed?
> I suggest to make trimming the default, because it almost always is wanted.
> If a user has a scenario in which the white space is required, a pre-* hook
> can re-add it.
> 
> I suggest to trim leading & trailing white space on all input fields.

+1 for make trimming the default
Comment 3 Daniel Tröder univentionstaff 2018-05-16 20:49:10 CEST
Fixed in branch dtroeder/46970_strip_whitespace in commit 59f6b5c7.

The original input with whitespace is preserved, so hooks can readd it, if required.

The test 90_ucsschool/201_import-users_basic was adapted to test for stripping whitespace from CSV input.

Logline from test run (notice fields "Vor" and "Nach"):

2018-05-16 20:43:09 DEBUG base_reader.next:74  Input 5: ['teacher_and_staff', 'hcheyoatuy ', 'recordUID-pi7sxi4wz7', ' dt0loxmdkp', 'tx71qen12zw-75vc', '', 'tx71qen12zw', 'cgdngljfgp@uni.dtr'] -> {u'Gruppen': u'tx71qen12zw-75vc', u'recordUID': u'recordUID-pi7sxi4wz7', u'Nach': u'dt0loxmdkp', u'Beschreibung': u'', u'E-Mail': u'cgdngljfgp@uni.dtr', u'role': u'teacher_and_staff', u'Vor': u'hcheyoatuy', u'OUs': u'tx71qen12zw'}

The list (after "Input 5: ") with the unmodified CSV-fields will be attached to the ImportUser object as attribute "input_data" (in base_reader.next:78).
Comment 4 Sönke Schwardt-Krummrich univentionstaff 2018-05-17 15:03:28 CEST
(In reply to Daniel Tröder from comment #3)
> 2018-05-16 20:43:09 DEBUG base_reader.next:74  Input 5:
> ['teacher_and_staff', 'hcheyoatuy ', 'recordUID-pi7sxi4wz7', ' dt0loxmdkp',
> 'tx71qen12zw-75vc', '', 'tx71qen12zw', 'cgdngljfgp@uni.dtr'] -> {u'Gruppen':
> u'tx71qen12zw-75vc', u'recordUID': u'recordUID-pi7sxi4wz7', u'Nach':
> u'dt0loxmdkp', u'Beschreibung': u'', u'E-Mail': u'cgdngljfgp@uni.dtr',
> u'role': u'teacher_and_staff', u'Vor': u'hcheyoatuy', u'OUs': u'tx71qen12zw'}
> 
> The list (after "Input 5: ") with the unmodified CSV-fields will be attached
> to the ImportUser object as attribute "input_data" (in base_reader.next:78).

So hooks have to redo the trimming again, if they have to use the input_data?
Comment 5 Daniel Tröder univentionstaff 2018-05-17 16:50:55 CEST
(In reply to Sönke Schwardt-Krummrich from comment #4)
> (In reply to Daniel Tröder from comment #3)
> > The list (after "Input 5: ") with the unmodified CSV-fields will be attached
> > to the ImportUser object as attribute "input_data" (in base_reader.next:78).
> So hooks have to redo the trimming again, if they have to use the input_data?
Yes.
If a hook requires access to unmapped input data, it'll get the untouched input data.
Comment 6 Daniel Tröder univentionstaff 2018-06-04 09:11:25 CEST
Merged branch 'dtroeder/46970_strip_whitespace' into 4.3 (ed4cf3a4).

Code was added to the test 201_import-users_basic to check if whitespace is removed.

[4.3] 7921a0f6 Bug #46970: strip leading/trailing whitespace from CSV input
[4.3] 94409baa Bug #46970: add deepcopy operation support to all ucsschool.lib objects
[4.3] 11b809ba Bug #46970: use deepcopy in import
[4.3] 5b0e85fc Bug #46970: changelog
[4.3] 58d79c11 Bug #46970: advisories

ucs-school-lib (11.0.1-10)
ucs-school-import (16.0.1-30)
ucs-test-ucsschool (4.0.4-93)
Comment 7 Daniel Tröder univentionstaff 2018-06-04 09:26:00 CEST
[4.3] d3177332 Bug #46970: fix changelog
[4.3] 348ecd81 Bug #46970: add missing data to advisory

ucs-test-ucsschool (5.0.2-54)
Comment 8 Ole Schwiegert univentionstaff 2018-06-05 12:56:10 CEST
leading and trailing whitespaces were removed upon import
Comment 9 Sönke Schwardt-Krummrich univentionstaff 2018-07-04 18:09:00 CEST
UCS@school 4.3 v4 has been released.

https://docs.software-univention.de/changelog-ucsschool-4.3v4-de.html

If this error occurs again, please clone this bug.