Bug 45683 - The import crashes if the name of a class contains a backslash
The import crashes if the name of a class contains a backslash
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
unspecified
Other Mac OS X 10.1
: P5 normal (vote)
: UCS@school 4.4 v5-errata
Assigned To: Toni Röhmeyer
Tobias Wenzel
:
Depends on:
Blocks: 51190
  Show dependency treegraph
 
Reported: 2017-11-09 18:37 CET by Michel Smidt
Modified: 2020-07-30 15:10 CEST (History)
4 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 4: Minor Usability: Impairs usability in secondary scenarios
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.091
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 Michel Smidt 2017-11-09 18:37:44 CET
Got the Traceback during a customer workshop.
The error message as well wasn't that specific.
Comment 1 Daniel Tröder univentionstaff 2017-11-10 12:28:39 CET
Was this when importing users?

IMHO we should reject a class name with a backslash, as it will cause trouble later with samba/cifs (and probably break lots of shell and python code in other modules too).

Is there somewhere a definition of [dis]allowed characters for (directory + group + share) names?
Comment 2 Michel Smidt 2017-11-10 13:05:00 CET
(In reply to Daniel Tröder from comment #1)
> Was this when importing users?

Yes, during import. Unfortunately not during --dry-run

> 
> IMHO we should reject a class name with a backslash, as it will cause
> trouble later with samba/cifs (and probably break lots of shell and python
> code in other modules too).

I won't say we should reject backslashes but we should convert most of the special characters to "_" or "-". Similiar to usernames.

> 
> Is there somewhere a definition of [dis]allowed characters for (directory +
> group + share) names?

I don't know.
Comment 3 Daniel Tröder univentionstaff 2020-02-25 17:08:44 CET
user.school_classes contains group names.
Those can be checked in user.validate() against the "gid" syntax in the UDM syntax.py module.
user.validate() is executed during dry-run, so this will trigger an error in dry-run.
Comment 4 Toni Röhmeyer univentionstaff 2020-03-04 16:03:58 CET
A new field in user_import_defaults.json "school_classes_invalid_character_replacement" contains the character which replaces an invalid character in a class name (default is "-")

I added a check in ucs-school-import/checks/defaults.py which checks if that field is filled correctly.

The replacement of invalid characters was implemented in models/import_user.py

If there is still an invalid character (e.g. starts with a '-'), an error will be caught in ucs-school-lib/python/models/user.py
Invalid characters are defined in the "gid" class in syntax.py 

The new user_import_defaults field is now documented in 
doc/manual/ucsschool-import-handbuch-4.4.xml
usr/share/doc/ucs-school-import/user_import_configuration_readme.txt


The behavior was tested with 
/usr/share/ucs-school-import/scripts/ucs-school-user-import
with different test_user settings


Solution was pushed on branch troehmey/45683 with the following commits

commit 13433dac044040636ab23bd5851f75227d847f25
    Bug #45683: added checks for class name syntax

commit 4200d94c890bf40871b5b3bb6c655325e25b1110
    Bug #45683: added classname syntax check


Waiting for QA
Comment 5 Tobias Wenzel univentionstaff 2020-04-21 15:05:29 CEST
QA

[troehmey/bug45683] 13433dac0 Bug #45683: added checks for class name syntax
[troehmey/bug45683] 4200d94c8 Bug #45683: added classname syntax check

Which version did you use? Mine was 4.4.4 errata-499
Before your changes, --dry-run with faulty class names was not successful, other than in comment #2 

Code 

Is there a reason why you use assert instead of InitialisationError in defaults.py?
 assert len(self.config.get("school_classes_invalid_character_replacement", "")) in (0, 1)

Keep consistency in users.py
self.add_error('school_classes', _("{}".format(exc)))

Don't use string concat like + in import_users.py
   string.digits + string.ascii_letters + allowed_special

Use two spaces before line comment or simply loose the comment in import_users.py
 # only for debug output 

Doc

(Erlaubt sind Zahlen, Buchstaben (keine Umlaute) und die Zeichen <literal>. -_</literal>)

-> I would remove the brackets and simply append it as a common sentence.


Functionality -> all ok


After change -> --dry-run and import successful.

testkl\ass -> testkl-ass

I also did checks with /*&§!? which were also successful. Mind: testkl\ass and testkl?ass will both be renamed to testkl-ass. 

"school_classes_invalid_character_replacement": "Q" 
-> correct replacement
testkl\ass 	-> testklQass

"school_classes_invalid_character_replacement": " "
testkl\ass 	-> testkl ass

"school_classes_invalid_character_replacement": "Qq"
-> correct error
InitialisationError: school_classes_invalid_character_replacement must be one of 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 -.'

"school_classes_invalid_character_replacement": ""
-> no replacement but error gets thrown also in --dry-run
Comment 6 Toni Röhmeyer univentionstaff 2020-04-21 16:12:17 CEST
The mentioned issues have been fixed an commited with

commit 4ccf33286c6e30b7d08239b6659421bbdb4fa0d5
Bug #45683: applied minor fixes

on branch troehmey/bug45683.


I tested the behavior described in comment #2 with 4.4 v4 errata443 and also had a not successful run.
Comment 7 Tobias Wenzel univentionstaff 2020-04-22 09:41:06 CEST
QA -> all ok.

Thanks for the quick response!
Code -> looks good
Functionality -> works like in previous qa.

Please merge to 4.4, add changelog and yaml.
Comment 8 Toni Röhmeyer univentionstaff 2020-04-24 16:49:27 CEST
Feature branch was merged to 4.4 and yaml and changelog entries were added with commits

commit 49484bb9d484c6fab7326da8500b8f109d570950
Bug #45683: added yaml

commit e7f7c614f81e37901a7bee0c6d0c22caac923ba6
Bug #45683: added changelog entry

commit d79041ff910763789030cdd3f66413ab00c92197
Bug #45683: Merge branch 'troehmey/bug45683' into 4.4

Successful build: 

Package: ucs-school-lib
Version: 12.1.12A~4.4.0.202004241641
Branch: ucs_4.4-0
Scope: ucs-school-4.4

and 

Package: ucs-school-import 
Version: 17.0.31A~4.4.0.20200424163
Branch: ucs_4.4-0
Scope: ucs-school-4.4
Comment 9 Tobias Wenzel univentionstaff 2020-04-24 17:19:23 CEST
yamls -> ok
changelogs -> ok

[4.4] 37094dc7c Bug #45683: correct yamls
[4.4] 49484bb9d Bug #45683: added yaml
[4.4] e7f7c614f Bug #45683: added changelog entry
[4.4] d79041ff9 Bug #45683: Merge branch 'troehmey/bug45683' into 4.4
Comment 10 Tobias Wenzel univentionstaff 2020-07-30 15:10:37 CEST
UCS@school X.Y vZ has been released (errata update to the release).

http://docs.software-univention.de/changelog-ucsschool-4.4v5-de.html#changelog:ucsschool:2020-04-27

If this error occurs again, please clone this bug.